Apache HTTP Server: 'RewriteCond expr' always evaluates to true
70 comments
·July 24, 2025kevincox
ralferoo
Yeah, the refactor looks somewhat iffy to me, although to be fair, the error here was no recognising that the `rc` was modified in the preceding call back to an it. This means that any code that was explicitly checking against the enum values would fail.
COND_RC_MATCH comes from a newly introduced enum `cond_return_type` but `rc` is still declared as int (`int rc = COND_RC_NOMATCH;`).
At least the `rc` from the call to `ap_expr_exec_re` in line 4270 should be an intermediate variable so that `rc` can be defined correctly as the enum type, so that similar mistakes would be flagged as a warning at compile time.
Cthulhu_
How come this wasn't covered by one or more automated tests that failed?
oaiey
You mean Apache httpd ... the thing which is called a-patchy-httpd server? (that is not a joke!).
Apache httpd existed many years before junit was invented in 1997. Long before TDD became a thing and our rigorous modern understanding set in. For a second, I even thought the Apache Foundation (founded because of httpd) later hosted junit, but I was wrong, is the Eclipse Foundation.
PuercoPop
junit was not invented, it was a port from Smalltalk's SUnit, which was created on 1989, ~6 years before the first release of Apache. Yes, the extreme programming (XP) craze hadn't popularized TDD, but united testing as a practice already existed, even if only some communities.
Though I agree, that although not a technical justification, an explanation as to why there are no tests is because Apache HTTP is from the 90's. Not writing unit tests was par for course back then. Most FLOSS code bases in the 90s didn't have unit tests, let a alone a CI to run the test suite for each change. Adding tests later is hard. Though there are some tests under the test folder.
sidewndr46
What does this have to do with Junit?
testplzignore
Based on https://github.com/apache/httpd/commits?author=covener, either the committer never writes tests, or this project just doesn't do testing at all. Nothing here would pass a code review at my company. Totally insane.
homebrewer
Your company, and others like it, are of course entitled to a refund. These infrastructural projects never get any funding when everything goes well, but when an overworked maintainer screws up in good faith, everybody piles on them.
The attacks on OpenSSL maintainers ten years ago were disgusting, and I think we've learned nothing since then.
evantbyrne
Developers can only be as good at their jobs as their environment allows them to be. Based on the commit log for all authors, my takeaway here is that this is a legacy software project that needs better test coverage and to establish standards around adding tests when merging in contributions.
throwaway2037
Yeah, crazy. Also, the bugfix does not include a test case.
dlachausse
Feel free to contribute one. I’m sure the maintainers would welcome it…
> If you want to participate in actively developing Apache please subscribe to the dev@httpd.apache.org mailing list as described at https://httpd.apache.org/lists.html#http-dev
csmpltn
Welcome to open source software, the "year of the linux desktop", etc...
teddyh
> 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
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]
captn3m0
https://repology.org/project/apache/versions
Main ones: FreeBSD, Alpine, Fedora 42, OpenSUSE Tumbleweed
iforgotpassword
[flagged]
kruffalon
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
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.
loloquwowndueo
Nothing a quick googling of jia Tan wouldn’t fix. It was a whole thing last year.
efitz
[flagged]
dxxvi
[flagged]
db48x
Lol. Although funny, it might be a little too soon to be making that joke.
nneonneo
Some FAQs:
- This only affects rewrite conditions which literally start with “RewriteCond expr”; this is a special form that causes the condition pattern to be treated as an Apache expression. See the documentation on that feature here: https://httpd.apache.org/docs/trunk/mod/mod_rewrite.html#:~:...
- Yes, there are tests. They’re stored in a separate repository. Here are the regression tests added for this bug: https://github.com/apache/httpd-tests/commit/48a85e34051959c.... As for why testing didn’t find this bug in the first place, you can see that they have tests for RewriteCond, but just not for expression conditions, likely due to the relative rarity of that subfeature.
ameliaquining
This still suggests that nobody's checking for adequate test coverage, if a feature that has its own dedicated syntax didn't have any. (Also, I can't find any documentation on how to measure coverage while running the test suite, which suggests that nobody is doing this routinely.) But admittedly this is a lesser sin than not having tests at all.
philipwhiuk
If the tests aren't run on check-in you may as well not have them.
abanana
The apparent lack of testing is shocking for something this big. If I'm understanding correctly, this update will have caused issues across huge swathes of the web, including all Wordpress installations running on this version of Apache, as they include this block of code in their .htaccess file:
RewriteEngine On
RewriteBase /
RewriteRule ^index\.php$ - [L]
RewriteCond %{REQUEST_FILENAME} !-f
RewriteCond %{REQUEST_FILENAME} !-d
RewriteRule . /index.php [L]
agwa
This bug only affects a special form of RewriteCond where the first argument is literally "expr", so the rules that you quoted are unaffected.
I have to assume that this form of RewriteCond is pretty rare or the bug would have been caught much sooner.
abanana
Ah I see, thank you for the explanation.
Regarding how quickly it was caught, bugs like this are a clear argument against the idea of "always update immediately" that's pushed down everyone's throat these days, and parroted by so many who don't realise it's part of marketing. Luckily updates do tend to be a lot slower when they're components of managed packages (in this case, WHM and the like).
liveoneggs
Since a few years ago the recommendation from apache is to use https://httpd.apache.org/docs/trunk/mod/mod_dir.html#fallbac... instead of mod_rewrite for this pattern
binaryturtle
That feels like something some form of automatic build/feature testing should have caught.
firesteelrain
I'm not going to be all high and mighty then say that tests should have caught this since Apache has tests. I am not sure why RewriteCond expr is even valid at all since expr typically needs an expression to come after it like RewriteCond expr "'cache/%{md5:%{REQUEST_URI}?%{QUERY_STRING}}.html' =~ /(.+)/".
The bug appeared to be introduced on July 7 with a backport from trunk:
https://github.com/apache/httpd/blame/ed99ef021de902363c36af...
It took me a second but the fix addresses the case where rc==0. If statement is less than 0. Therefore, rc==0 should indicate no match. https://github.com/apache/httpd/commit/8abb3d06b23975705ebcf...
I suppose because there is also the conditional for err?
elric
Can someone elaborate on how this is a security issue?
mrspuratic
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.
mrspuratic
Anti-"image theft" example from Apache httpd documentation that would break with this bug:
RewriteCond expr "! %{HTTP_REFERER} -strmatch '*://%{HTTP_HOST}/*'"
RewriteRule "^/images" "-" [F]dspillett
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
Interesting. I've never used rewrite rules conditionally, and if a rewritten request is your only defense you've probably got bigger problems.
mrspuratic
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
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
No test added/changed in this commit? Is there any test for this area?
nneonneo
Looks like tests were added: https://github.com/apache/httpd-tests/commit/48a85e34051959c...
Shame it wasn’t caught by any existing test though.
lozenge
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.
0x457
Well, that should be the first thing you? If it doesn't catch it, then what are you testing?
kstrauser
The tests include the comment:
# Seems to have a side affect for any subsequent GET's
julik
Somehow this is not the first infrastructure OSS project written in C/C++ where I see two things being the case:
* There is some crucial counting/reference/condition code that contains a bug
* There is exactly 0 tests for that code
* A fix gets done, but no tests
Coming from dynlangs this does strike me as irresponsible. I believe the previous case I saw was the S3-compatibility header change in ceph, and similar with CORS configuration there too.
Is it so that experienced C developers assume the compiler will flag any bugs that matter?..
0cf8612b2e1e
This kind of logic error could happen in any language.
jofla_net
mod_rewrite, again, man if i had a nickel for every...
brunooliv
Damn, this is the type of code that probably makes sense to the original writer but good luck maintaining it. Even AI would struggle with this type of convoluted conditional logic without a test harness. None of these things should be merged without tests. Even basic unit tests would catch these. Code quality is an illusion.
mgaunard
That's not a security issue, it's a correctness issue.
The whole feature simply does not work.
0x457
Is if I use OpenSSL to generate random number, but someone accidentally made it return 4 and nothing else. Is it a correctness issues or security issue? The whole feature simply does not work.
pytness
if a gpg signature check fails, is it a correctness issue? a security issue? or both?
amiga386
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.
inopinatus
If upgrade/reframe that last point more strongly. Any configuration of software that is accepted by its own parser is in product scope.
mgaunard
RewriteCond is a mechanism to redirect under certain conditions.
Security becomes irrelevant if the whole Apache module is broken.
falcor84
I don't see how it becomes irrelevant. It's as if I have a door in the entrance to my building that serves multiple purposes, such as holding the company logo and keeping the A/C-controlled air in, and then someone smashes the door with a sledgehammer. The fact that all of the door's functionality stopped working doesn't make the security aspect of not having a door irrelevant.
It seems like the bug was originally introduced here: https://github.com/apache/httpd/commit/dd98030cb399e962aa605...
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?