[HN Gopher] Apache HTTP Server: 'RewriteCond expr' always evalua...
___________________________________________________________________
Apache HTTP Server: 'RewriteCond expr' always evaluates to true
Author : Bogdanp
Score : 124 points
Date : 2025-07-24 04:20 UTC (18 hours ago)
(HTM) web link (github.com)
(TXT) w3m dump (github.com)
| binaryturtle wrote:
| That feels like something some form of automatic build/feature
| testing should have caught.
| Hexcles wrote:
| No test added/changed in this commit? Is there any test for this
| area?
| nneonneo wrote:
| Looks like tests were added: https://github.com/apache/httpd-
| tests/commit/48a85e34051959c...
|
| Shame it wasn't caught by any existing test though.
| lozenge wrote:
| 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 wrote:
| Well, that should be the first thing you? If it doesn't
| catch it, then what are you testing?
| kstrauser wrote:
| The tests include the comment: # Seems to
| have a side affect for any subsequent GET's
|
| Well... does it? And if so, is that fixable?
| kevincox wrote:
| 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?
| iforgotpassword wrote:
| [flagged]
| kruffalon wrote:
| 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 wrote:
| 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
| kruffalon wrote:
| Thank you! <3
| loloquwowndueo wrote:
| Nothing a quick googling of jia Tan wouldn't fix. It was a
| whole thing last year.
| db48x wrote:
| Lol. Although funny, it might be a little too soon to be
| making that joke.
| teddyh wrote:
| > _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 wrote:
| 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/
| captn3m0 wrote:
| https://repology.org/project/apache/versions
|
| Main ones: FreeBSD, Alpine, Fedora 42, OpenSUSE Tumbleweed
| Cthulhu_ wrote:
| How come this wasn't covered by one or more automated tests
| that failed?
| throwaway2037 wrote:
| Yeah, crazy. Also, the bugfix does not include a test case.
| dlachausse wrote:
| 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
| whatevaa wrote:
| There are tests?
| Hilift wrote:
| It is "CrowdTested".
| oaiey wrote:
| You already have a registered trademark? Will use that ;)
| testplzignore wrote:
| 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 wrote:
| 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.
| sshine wrote:
| > _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/
| Joker_vD wrote:
| 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).
| Macha wrote:
| 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.
| Joker_vD wrote:
| 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.
| whizzter wrote:
| 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.
| dlachausse wrote:
| 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.
| evantbyrne wrote:
| 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.
| oaiey wrote:
| 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.
| sidewndr46 wrote:
| What does this have to do with Junit?
| Macha wrote:
| 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.
| dlachausse wrote:
| Perl has had a very pervasive testing culture that
| predates Junit by a decade...
|
| https://www.perl.org/about/whitepapers/perl-testing.html
| oaiey wrote:
| My apologies for that. I always forget I am old ;). What
| Macha said.
| PuercoPop wrote:
| 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.
| csmpltn wrote:
| Welcome to open source software, the "year of the linux
| desktop", etc...
| ralferoo wrote:
| 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.
| elric wrote:
| Can someone elaborate on how this is a security issue?
| francislavoie wrote:
| 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.
| dspillett wrote:
| 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 wrote:
| Interesting. I've never used rewrite rules conditionally, and
| if a rewritten request is your only defense you've probably
| got bigger problems.
| mrspuratic wrote:
| 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.
| mrspuratic wrote:
| 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 wrote:
| Anti-"image theft" example from Apache httpd documentation
| that would break with this bug: RewriteCond
| expr "! %{HTTP_REFERER} -strmatch '*://%{HTTP_HOST}/*'"
| RewriteRule "^/images" "-" [F]
| mgaunard wrote:
| That's not a security issue, it's a correctness issue.
|
| The whole feature simply does not work.
| pytness wrote:
| if a gpg signature check fails, is it a correctness issue? a
| security issue? or both?
| mgaunard wrote:
| RewriteCond is a mechanism to redirect under certain
| conditions.
|
| Security becomes irrelevant if the whole Apache module is
| broken.
| falcor84 wrote:
| 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.
| mgaunard wrote:
| 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.
| falcor84 wrote:
| 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.
| amiga386 wrote:
| 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 wrote:
| If upgrade/reframe that last point more strongly. Any
| configuration of software that is accepted by its own
| parser is in product scope.
| 0x457 wrote:
| 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.
| abanana wrote:
| 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]
|
| 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.
| agwa wrote:
| 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 wrote:
| 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 wrote:
| 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
| brunooliv wrote:
| 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.
| nneonneo wrote:
| 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 wrote:
| 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 wrote:
| If the tests aren't run on check-in you may as well not have
| them.
| julik wrote:
| 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 wrote:
| This kind of logic error could happen in any language.
| firesteelrain wrote:
| 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?
| jofla_net wrote:
| mod_rewrite, again, man if i had a nickel for every...
___________________________________________________________________
(page generated 2025-07-24 23:01 UTC)