Fix logging format string

[Devstellar]

Fix issue #85 using a fixed format string in the calls to log

[fzipi]

This should be merged.

[airween]

This should be merged.

I'm not sure about that. I remember once I ran into a strange problem with Apache 2.4. I made a fix but haven't sent any PR yet.

Here is the commit:

airween@e4cbbac

I suggest to add this, instead of replace the variable.

[fzipi]

The correct fix is always use "%s", msg. Reading it again, looks like it is removing the r->status, but adding it back should make it work.

Yours does a costly parsing that doesn't make sense in this scenario.

Another extra check that could be done is adding limits to the size you are printing out.

[airween]

The correct fix is always use "%s", msg. Reading it again, looks like it is removing the r->status, but adding it back should make it work.

In my mentioned commit I also used msg.

Yours does a costly parsing that doesn't make sense in this scenario.

I'm not sure about that.

Another extra check that could be done is adding limits to the size you are printing out.

The point is not the size limit. The point is this:

        // add % escape to avoid the '%' chars placeholder mark in logmsg

If the message contains a % sign (in msg, as you suggested), Apache crashes.

[fzipi]

If the message contains a % sign (in msg, as you suggested), Apache crashes.

@airween Of course. That's the exact definition of a format string vulnerability. Please read the attack and the solution. The way of solving it is always the same: use "%s", msg. That way the message cannot contain formatting, the format is already the "%s". That would properly output everything as intended, and cannot be exploited.

[fzipi]

Here we can see the function signature.

The macros will fill the proper fields, and the PR here is a good solution. If you are still not convinced, you can do a grep -R ap_log_rerror in the apache httpd source code.

[Devstellar]

Hi, I'm the person who proposed this patch. I've been reading your posts these past few days and I'm not sure if you're aware that this project isn't being maintained. Look comment about it here

[airween]

Ah, I didn't remember that ap_log_error() allows formatted strings... Let me check this soon.

[fzipi]

Hi @Devstellar. Well, @airween is the new project leader for ModSecurity. I guess if you start pushing changes now, he might take a look. Every change is welcome.

[fzipi]

@Devstellar See https://modsecurity.org/developers/. You are welcome to get onboard. 😄

[airween]

Hi, I'm the person who proposed this patch.

Yep, thank you!

I've been reading your posts these past few days and I'm not sure if you're aware that this project isn't being maintained. Look comment about it here

I think I ran into the same problem in 2019 (see this), but as I wrote above, I didn't realized then that ap_log_error() can handle formatted strings.

I'm not sure if you're aware that this project isn't being maintained

We focus to other issues which appear in the library and in Nginx connector. We don't want to ignore this connector, but we lack of resources.