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?
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.
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.
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.
Junit was one of the first libraries to popularise modern patterns of unit testing.
Some people were writing their own ad-hoc scaffolds before that but it wasn't a widespread practice. Testing meant manual testers clicking on things in the UI, and sophisticated testing was if you had checklists of things to test manually.
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.
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.
Depends on what, exactly, the developers are doing with them projects! If those are in the "maintenance mode" (no new features, just fixing bugs and making sure it still builds with newer toolchains/ecosystems), then this amount of humanpower may be quite reasonable.
It's still a drop in the ocean, heard of that KISS-CAM CEO that got busted for cheating? His company literally seems to be built on selling hosted Apache Airflow services and just took in a series D round of 93 MUSD.
Open source is open, so naturally people can use it but the ecosystem has also been at a breaking point for years and bad actors has caught the scent of that.
The vast majority of ASF projects are in maintenance mode. This isn’t a bad thing, but compared to a project like the Linux kernel for example, Apache httpd is mature software that doesn’t require many full time developers to maintain.
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.
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
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]
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.
- 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.
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.
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:
If the request doesn't exist as a file or directory, rewrite it to index.php in the root, so Wordpress can handle it. This kind of rewriting is very common of course, I'm just taking Wordpress as an example because of its popularity.
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).
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:
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.
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).
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.
That's what I mean by "not a good way" and "at least not on its own". It _can_ be a good optimisation to make some easy decisions quickly on the web server before pushing anything less definite to your more expensive logic processing, if well implemented. If badly implemented it leads to security holes and actually worse performance.
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.
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?..
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.
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.
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.
A better analogy is that you decided to replace your door with a new one, and before installation you notice that it is smashed to pieces and can't be used.
I'll take that. But in this case it's even worse, as apparently they never bothered to check if the door is in one piece and just screwed the smashed pieces onto the hinges regardless. So now it's not working as a door neither functionally nor security-wise, but it took someone visiting from outside to see that the emperor has no door.
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.
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?
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.
How come this wasn't covered by one or more automated tests that failed?
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.
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.
Now I learned something. Seems like I am not old enough
What does this have to do with Junit?
Junit was one of the first libraries to popularise modern patterns of unit testing.
Some people were writing their own ad-hoc scaffolds before that but it wasn't a widespread practice. Testing meant manual testers clicking on things in the UI, and sophisticated testing was if you had checklists of things to test manually.
Perl has had a very pervasive testing culture that predates Junit by a decade…
https://www.perl.org/about/whitepapers/perl-testing.html
My apologies for that. I always forget I am old ;). What Macha said.
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.
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.
No, we've learned to stop using OpenSSL.
And ASF does receive funding, by the way, even if not much (slightly less than two and a half million USD in 2024).
So let's pretend this only paid for employees. 5-15 employees depending on where they live. Let's assume 1-2 admin and 13 devs.
A quick eyeball of the projects list looks like about 100 projects: https://projects.apache.org/projects.html
So each project gets funded enough for 10% of a developer. That's not enough to provide infrastructure to commercial users satisfaction.
Depends on what, exactly, the developers are doing with them projects! If those are in the "maintenance mode" (no new features, just fixing bugs and making sure it still builds with newer toolchains/ecosystems), then this amount of humanpower may be quite reasonable.
It's still a drop in the ocean, heard of that KISS-CAM CEO that got busted for cheating? His company literally seems to be built on selling hosted Apache Airflow services and just took in a series D round of 93 MUSD.
Apache HTTPD still seems to run about 17% of all sites, plenty of those probably make money using the software. https://www.netcraft.com/blog/january-2025-web-server-survey
Open source is open, so naturally people can use it but the ecosystem has also been at a breaking point for years and bad actors has caught the scent of that.
The vast majority of ASF projects are in maintenance mode. This isn’t a bad thing, but compared to a project like the Linux kernel for example, Apache httpd is mature software that doesn’t require many full time developers to maintain.
> The attacks on OpenSSL maintainers ten years ago were disgusting
I didn't register the attacks, but I'm sure there were some when you say it.
I summarized the blame on that incident xkcd's wording:
"some random person has been thanklessly maintaining since 2003"
https://xkcd.com/2347/
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.
Yeah, crazy. Also, the bugfix does not include a test case.
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
There are tests?
It is "CrowdTested".
You already have a registered trademark? Will use that ;)
Welcome to open source software, the "year of the linux desktop", etc...
> 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?
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]
[1] https://archlinux.org/packages/extra/x86_64/apache/
https://repology.org/project/apache/versions
Main ones: FreeBSD, Alpine, Fedora 42, OpenSUSE Tumbleweed
[flagged]
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.
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.
https://en.wikipedia.org/wiki/XZ_Utils_backdoor
Thank you! <3
Nothing a quick googling of jia Tan wouldn’t fix. It was a whole thing last year.
[flagged]
[flagged]
Lol. Although funny, it might be a little too soon to be making that joke.
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.
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.
If the tests aren't run on check-in you may as well not have them.
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:
If the request doesn't exist as a file or directory, rewrite it to index.php in the root, so Wordpress can handle it. This kind of rewriting is very common of course, I'm just taking Wordpress as an example because of its popularity.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.
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).
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
That feels like something some form of automatic build/feature testing should have caught.
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?
Can someone elaborate on how this is a security issue?
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.
Anti-"image theft" example from Apache httpd documentation that would break with this bug:
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).
Interesting. I've never used rewrite rules conditionally, and if a rewritten request is your only defense you've probably got bigger problems.
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.
That's what I mean by "not a good way" and "at least not on its own". It _can_ be a good optimisation to make some easy decisions quickly on the web server before pushing anything less definite to your more expensive logic processing, if well implemented. If badly implemented it leads to security holes and actually worse performance.
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.
No test added/changed in this commit? Is there any test for this area?
Looks like tests were added: https://github.com/apache/httpd-tests/commit/48a85e34051959c...
Shame it wasn’t caught by any existing test though.
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.
Well, that should be the first thing you? If it doesn't catch it, then what are you testing?
The tests include the comment:
Well… does it? And if so, is that fixable?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?..
This kind of logic error could happen in any language.
Undoubtedly! It's just that usually I see unit tests for such fixes, but not on projects in C/C++
mod_rewrite, again, man if i had a nickel for every...
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.
That's not a security issue, it's a correctness issue.
The whole feature simply does not work.
if a gpg signature check fails, is it a correctness issue? a security issue? or both?
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.
If upgrade/reframe that last point more strongly. Any configuration of software that is accepted by its own parser is in product scope.
RewriteCond is a mechanism to redirect under certain conditions.
Security becomes irrelevant if the whole Apache module is broken.
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.
A better analogy is that you decided to replace your door with a new one, and before installation you notice that it is smashed to pieces and can't be used.
I'll take that. But in this case it's even worse, as apparently they never bothered to check if the door is in one piece and just screwed the smashed pieces onto the hinges regardless. So now it's not working as a door neither functionally nor security-wise, but it took someone visiting from outside to see that the emperor has no door.
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.