This is seemingly a well-intentioned cleanup that misunderstood the branching logic. The original code was normalizing `rc < 0` to COND_RC_NOMATCH (0), but leaving `rc >= 0` as the original value. However the new code accidentally normalizes `rc >= 0` to COND_RC_MATCH (1) while it should be normalizing `rc > 0` to COND_RC_MATCH (leaving 0 as COND_RC_NOMATCH).
There are a few other logic changes in this patch which should likely be reviewed carefully. For example https://github.com/apache/httpd/commit/dd98030cb399e962aa605... does `rc <= COND_RC_MATCH` which matches both match and no match. Presumably it is checking for COND_RC_STATUS_SET but it seems like and odd way to write `rc == COND_RC_STATUS_SET`. Maybe the intention is to match future special values?
Cthulhu_ · 2h ago
How come this wasn't covered by one or more automated tests that failed?
throwaway2037 · 10m ago
Yeah, crazy. Also, the bugfix does not include a test case.
whatevaa · 2m ago
There are tests?
teddyh · 2h ago
> It seems like the bug was originally introduced here: […]
So, two weeks ago? Meaning, everybody running a version of Apache older than two weeks is safe?
st_goliath · 2h ago
Sure looks like it, the commit that introduced this is from July 7th, the affected version (Apache 2.4.64) was released on July 10th. Today (as of writing this in CEST) is the 24th.
It looks like not even Arch Linux had that version in their repo yet (currently 2.4.63-3) [1]
Main ones: FreeBSD, Alpine, Fedora 42, OpenSUSE Tumbleweed
iforgotpassword · 3h ago
Signed-off-by: Jia Tan?
;)
kruffalon · 3h ago
This reads to me like a comment that skipped a bunch of context that would add value for the non-initiated (like me).
If I'm wrong I apologise for reading too much into it but if I'm right please add context.
bauruine · 3h ago
Jia Tan was the alias of someone that added a backdoor to xz that could be used to allow remote code execution on OpenSSH servers using the backdoored xz version.
Lol. Although funny, it might be a little too soon to be making that joke.
binaryturtle · 4h ago
That feels like something some form of automatic build/feature testing should have caught.
elric · 2h ago
Can someone elaborate on how this is a security issue?
mrspuratic · 2h ago
Commonly used in access control to check IP addresses, usernames, cookies, query params, URI paths, environment variables ...
Also filtering REQUEST_METHOD to allowed verbs is good practice.
dspillett · 2h ago
Off the top of my head, all that springs to mind is: If someone is using rewrite rules to direct users depending on cookies and other request values, it could permit access to things the current user should not see, or should need to re-auth to see.
Though this doesn't seem to be a good way of doing that anyway, certainly not on its own (perhaps as a low resource initial test it is valid, in a bloom filter sort of way it could cover some "definitely shouldn't be here" cases efficiently).
elric · 2h ago
Interesting. I've never used rewrite rules conditionally, and if a rewritten request is your only defense you've probably got bigger problems.
mrspuratic · 1h ago
For better or worse, mod_rewrite's flexibility meant it got used to add logic, primitive flow control and conditional behaviours. You don't actually need to rewrite a URL path. More recently, "Require expr" can do some of this.
francislavoie · 2h ago
Typically a boolean issue like this is a cause for escalation if you use it in combination with some auth handler, like "if has session cookie then serve protected files" and since the condition always passes then it could bypass auth. For example.
Hexcles · 4h ago
No test added/changed in this commit? Is there any test for this area?
Shame it wasn’t caught by any existing test though.
lozenge · 3h ago
Did they check the tests passed on the old code and would that have caught the issue? That's an extra step I often do.
mgaunard · 2h ago
That's not a security issue, it's a correctness issue.
The whole feature simply does not work.
pytness · 57m ago
if a gpg signature check fails, is it a correctness issue? a security issue? or both?
amiga386 · 18m ago
If RewriteCond (or any other Apache directive) doesn't behave as documented, that's a correctness issue.
If you use RewriteCond as the basis of securing your website, that's a security issue for you.
If it's a security issue for a significant number of users, or if the documentation recommends using the directive for a security role, then it's also a security issue for the product itself.
mgaunard · 36m ago
RewriteCond is a mechanism to redirect under certain conditions.
Security becomes irrelevant if the whole Apache module is broken.
This is seemingly a well-intentioned cleanup that misunderstood the branching logic. The original code was normalizing `rc < 0` to COND_RC_NOMATCH (0), but leaving `rc >= 0` as the original value. However the new code accidentally normalizes `rc >= 0` to COND_RC_MATCH (1) while it should be normalizing `rc > 0` to COND_RC_MATCH (leaving 0 as COND_RC_NOMATCH).
There are a few other logic changes in this patch which should likely be reviewed carefully. For example https://github.com/apache/httpd/commit/dd98030cb399e962aa605... does `rc <= COND_RC_MATCH` which matches both match and no match. Presumably it is checking for COND_RC_STATUS_SET but it seems like and odd way to write `rc == COND_RC_STATUS_SET`. Maybe the intention is to match future special values?
So, two weeks ago? Meaning, everybody running a version of Apache older than two weeks is safe?
It looks like not even Arch Linux had that version in their repo yet (currently 2.4.63-3) [1]
[1] https://archlinux.org/packages/extra/x86_64/apache/
Main ones: FreeBSD, Alpine, Fedora 42, OpenSUSE Tumbleweed
;)
If I'm wrong I apologise for reading too much into it but if I'm right please add context.
https://en.wikipedia.org/wiki/XZ_Utils_backdoor
Though this doesn't seem to be a good way of doing that anyway, certainly not on its own (perhaps as a low resource initial test it is valid, in a bloom filter sort of way it could cover some "definitely shouldn't be here" cases efficiently).
Shame it wasn’t caught by any existing test though.
The whole feature simply does not work.
If you use RewriteCond as the basis of securing your website, that's a security issue for you.
If it's a security issue for a significant number of users, or if the documentation recommends using the directive for a security role, then it's also a security issue for the product itself.
Security becomes irrelevant if the whole Apache module is broken.