6 ms·

Apache HTTP Server: 'RewriteCond expr' always evaluates to true

136 points by Bogdanp 1y ago

binaryturtle 1y ago
That feels like something some form of automatic build/feature testing should have caught.
Hexcles 1y ago
No test added/changed in this commit? Is there any test for this area?
- nneonneo 1y ago
  Looks like tests were added: https://github.com/apache/httpd-tests/commit/48a85e34051959c5fa3ee6e0deb35a9acd4e8d59 https://github.com/apache/httpd-tests/commit/48a85e34051959c... Shame it wasn’t caught by any existing test though.
  - lozenge 1y 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.
    - 0x457 1y ago
      Well, that should be the first thing you? If it doesn't catch it, then what are you testing?
  - kstrauser 1y ago
    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 1y ago
It seems like the bug was originally introduced here: https://github.com/apache/httpd/commit/dd98030cb399e962aa6058c35893491a9ef7d147#diff-c79653a5ec3ccb8725b180419680b328e34ab9a0a7f453208d79570448860a6aR4287 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/dd98030cb399e962aa6058c35893491a9ef7d147#diff-c79653a5ec3ccb8725b180419680b328e34ab9a0a7f453208d79570448860a6aR4308 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 1y ago
  [flagged]
  - efitz 1y ago
    [flagged]
  - kruffalon 1y 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 1y 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. https://en.wikipedia.org/wiki/XZ_Utils_backdoor https://en.wikipedia.org/wiki/XZ_Utils_backdoor
elric 1y ago
Can someone elaborate on how this is a security issue?
- francislavoie 1y 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.
- dspillett 1y 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 1y 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 1y 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.
    - dspillett 1y ago
      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.
mgaunard 1y ago
That's not a security issue, it's a correctness issue. The whole feature simply does not work.
- pytness 1y ago
  if a gpg signature check fails, is it a correctness issue? a security issue? or both?
  - mgaunard 1y ago
    RewriteCond is a mechanism to redirect under certain conditions. Security becomes irrelevant if the whole Apache module is broken.
    - falcor84 1y ago
      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 1y ago
        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 1y ago
        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.
abanana 1y ago
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 1y ago
  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 1y ago
    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 1y ago
  Since a few years ago the recommendation from apache is to use https://httpd.apache.org/docs/trunk/mod/mod_dir.html#fallbackresource https://httpd.apache.org/docs/trunk/mod/mod_dir.html#fallbac... instead of mod_rewrite for this pattern
brunooliv 1y ago
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 1y ago
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#:~:text=RewriteCond%20expr,-%22!%20%25%7BHTTP_REFERER%7D%20%2Dstrmatch%20'*://%25%7BHTTP_HOST 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/48a85e34051959c5fa3ee6e0deb35a9acd4e8d59 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 1y ago
  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 1y ago
  If the tests aren't run on check-in you may as well not have them.
julik 1y ago
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 1y ago
  This kind of logic error could happen in any language.
  - julik 1y ago
    Undoubtedly! It's just that usually I see unit tests for such fixes, but not on projects in C/C++
firesteelrain 1y ago
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/ed99ef021de902363c36afa5786f54163ffae2db/modules/mappers/mod_rewrite.c 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/8abb3d06b23975705ebcf4bf4476464fd0b9bd0b https://github.com/apache/httpd/commit/8abb3d06b23975705ebcf... I suppose because there is also the conditional for err?
jofla_net 1y ago
mod_rewrite, again, man if i had a nickel for every...

136 points by Bogdanp 1y ago

binaryturtle 1y ago

That feels like something some form of automatic build/feature testing should have caught.

Hexcles 1y ago

No test added/changed in this commit? Is there any test for this area?

nneonneo 1y ago
Looks like tests were added: https://github.com/apache/httpd-tests/commit/48a85e34051959c5fa3ee6e0deb35a9acd4e8d59 https://github.com/apache/httpd-tests/commit/48a85e34051959c... Shame it wasn’t caught by any existing test though.
- lozenge 1y 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.
  - 0x457 1y ago
    Well, that should be the first thing you? If it doesn't catch it, then what are you testing?
- kstrauser 1y ago
  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 1y ago

It seems like the bug was originally introduced here: https://github.com/apache/httpd/commit/dd98030cb399e962aa6058c35893491a9ef7d147#diff-c79653a5ec3ccb8725b180419680b328e34ab9a0a7f453208d79570448860a6aR4287 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/dd98030cb399e962aa6058c35893491a9ef7d147#diff-c79653a5ec3ccb8725b180419680b328e34ab9a0a7f453208d79570448860a6aR4308 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 1y ago
[flagged]
- efitz 1y ago
  [flagged]
- kruffalon 1y 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 1y 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. https://en.wikipedia.org/wiki/XZ_Utils_backdoor https://en.wikipedia.org/wiki/XZ_Utils_backdoor

elric 1y ago

Can someone elaborate on how this is a security issue?

francislavoie 1y 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.
dspillett 1y 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 1y 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 1y 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.
  - dspillett 1y ago
    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.

mgaunard 1y ago

That's not a security issue, it's a correctness issue. The whole feature simply does not work.

pytness 1y ago
if a gpg signature check fails, is it a correctness issue? a security issue? or both?
- mgaunard 1y ago
  RewriteCond is a mechanism to redirect under certain conditions. Security becomes irrelevant if the whole Apache module is broken.
  - falcor84 1y ago
    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 1y ago
      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 1y ago
        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.

abanana 1y ago

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 1y ago
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 1y ago
  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 1y ago
Since a few years ago the recommendation from apache is to use https://httpd.apache.org/docs/trunk/mod/mod_dir.html#fallbackresource https://httpd.apache.org/docs/trunk/mod/mod_dir.html#fallbac... instead of mod_rewrite for this pattern

brunooliv 1y ago

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 1y ago

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#:~:text=RewriteCond%20expr,-%22!%20%25%7BHTTP_REFERER%7D%20%2Dstrmatch%20'*://%25%7BHTTP_HOST 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/48a85e34051959c5fa3ee6e0deb35a9acd4e8d59 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 1y ago
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 1y ago
If the tests aren't run on check-in you may as well not have them.

julik 1y ago

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 1y ago
This kind of logic error could happen in any language.
- julik 1y ago
  Undoubtedly! It's just that usually I see unit tests for such fixes, but not on projects in C/C++

firesteelrain 1y ago

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/ed99ef021de902363c36afa5786f54163ffae2db/modules/mappers/mod_rewrite.c 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/8abb3d06b23975705ebcf4bf4476464fd0b9bd0b https://github.com/apache/httpd/commit/8abb3d06b23975705ebcf... I suppose because there is also the conditional for err?

jofla_net 1y ago

mod_rewrite, again, man if i had a nickel for every...