v2: make sure ModSecurity runs after mod_maxminddb (GeoIP) in phase 1

[dune73]

Very simple patch adding mod_maxminddb.c to postread_beforeme_list.

This allows to use mod_maxminddb GeoIP information in phase 1.

[marcstern]

Pay attention, this breaks some current implementations.
mod_maxminddb allows to set the environent variable MMDB_ADDR to the IP to analyze; a rule in phase 1 can do that (and we actually use that heavily). This typically allows to retrieve the IP from a header (X-Forwarded-For, ...).
This should be a compilation option in case it's added.

[dune73]

I might be misunderstanding your use case, but is not this something you would typically do with mod_remoteip?

I see how this breaks your use case, yet I think doing the lookup before phase 1 is the more natural behavior. Using ModSecurity to work around the X-Forwarded-For header in phase 1, so you can then do GeoIP lookup and run your GeoIP rules in phase 2 obviously works, but I think it's a hack that only works since ModSecurity happens to run after mod_maxminddb by hazard. Neither of the two modules defines the priority with regards to the other module. So either ModSecurity or mod_maxminddb could change their behaviour anytime and break your setup.

[marcstern]

I introduced that feature in mod_maxminddb to allow ModSecurity to set the variable in phase 1, so it's really wanted.
mod_remoteip can be used also, providing the header is fully compliant with the ususal X-Forwarded-For syntax; if you have another syntax, you're stuck parsing it with ModSecurity.
So, why not, but as a compile-time option please.

[dune73]

Fine with me, it's @martinhsv's call anyways.

[martinhsv]

@marcstern ,

Could you explain in more detail that earlier feature of yours and why this PR would break it?

[marcstern]

If you don't want to use mod_remoteip, or if you need to parse a HTTP header with a syntax that mod_remoteip doesn't support, you can do it with a phase 1 rule. If ModSecurity runs first, you set an environment variable with the IP to be analyzed by mod_maxminddb.
If mod_maxminddb runs before ModSecurity, there's no way to parse yourself a header to drive mod_maxminddb.

[dune73]

I see a lack of flexibility on the side of ModSec and on the side of mod_maxminddb.

• We have the desire to direct mod_maxminddb by setting an env variable in ModSec's phase 1.
• Looking up the GeoIP information and reacting via ModSec as early as possible in the lifecycle, thus before the request body is parsed, is also very useful and IMHO a natural desire.

Right now, the two use cases contradict each other.

Maybe we need to bring in the Maxmind developers into the conversation.

I see that @horgh and @oschwald are frequent committers with a maxmind email address.

[marcstern]

@oschwald interacted with me at the time and approved my PR On 21-10-2022 09:00, Christian Folini wrote: CAUTION: this email originated from outside of Approach. Do not click links or open attachments unless you trust the sender and know the content is safe. I see a lack of flexibility on the side of ModSec and on the side of mod_maxminddb. We have the desire to direct mod_maxminddb by setting an env variable in ModSec's phase 1. Looking up the GeoIP information and reacting via ModSec as early as possible in the lifecycle, thus before the request body is parsed, is also very useful and IMHO a natural desire. Right now, the two use cases contradict each other. Maybe we need to bring in the Maxmind developers into the conversation. I see that @horgh and @oschwald are frequent committers with a maxmind email address. — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: ***@***.***> [ { ***@***.***": "http://schema.org", ***@***.***": "EmailMessage", "potentialAction": { ***@***.***": "ViewAction", "target": "#2812 (comment)", "url": "#2812 (comment)", "name": "View Pull Request" }, "description": "View this Pull Request on GitHub", "publisher": { ***@***.***": "Organization", "name": "GitHub", "url": "https://github.com" } } ]

[oschwald]

I'd suspect the second use case is more common, but ideally there would not be breakage for the first use case. If there is something that can be done on the mod_maxminddb side, we would likely accept pull requests that introduce non-breaking changes. I suspect both of you are much more familiar with Apache modules and internals than either @horgh or me.

[dune73]

Thank you very much for chiming in @oschwald.

[dune73]

For the record, my latest blog post about GeoIP including the breaking change to mod_maxminddb is here: https://www.netnea.com/cms/2022/10/12/using-geoip-information-together-with-modsecurity/

[oschwald]

If the changes in 1.2.0 was breaking, I apologize. We strive to only introduce such changes when there is a major version bump. That said, it has been over 2.5 years since that release and I assume many people now depend on the new behavior it introduced.

[dune73]

No, I see no breakage.

Most ModSecurity users worked with the legacy DB format since ModSecurity 2.9 supported that natively. When legacy disappeared (thank you for keeping it alive for so long), a few people I know started to transform the new format into the legacy format in order to keep it working on ModSec 2.9.

I was not aware @marcstern is a user of mod_maxminddb. As far as I can see I was the first one to write a detailed blog post how to integrate GeoIP information and also mod_maxminddb with ModSecurity.

So the whole coordination problem between the two modules is rather new, or let's say it only surfaced now to a somewhat wider audience.

@marcstern: How would you solve it on the MaxMind side so that both use cases could continue?

[marcstern]

With the current behaviour (MS in phase 1 before mod_maxminddb), we have these possibilities:

• drive IP address with MS (in phase 1)
• check GeoIP in phase 2
• check GeoIP in phase 1, when MS is compiled with default settings (as "real" phase 1 need a special compile flag)

If we switch the order, we have these possibilities:

• no way to drive IP address with MS
• check GeoIP in phase 1 and 2 (including "real" phase 1)

The only advantage of the second case is to allow GeoIP in "real" phase 1. Usually, people from SpidereLabs tell me "you're one for the very few using that, we should focus on the average user" ;-)
Seriously, the first solution allows all use cases, with one in a sub-optimal way (phase 2 instead of 1). The second solution really discards a feature.

Btw, one goal of MS is also to protect internals. In the current situation, in case there's a problem (with mod_remoteip or another similar mechanism), MS can check, in phase 1, that the IP has the right format and maybe blocking an attack on mod_maxminddb (buffer overflow, ...). That would be impossible if mod_maxminddb runs first. I guess this is nice argument.