6 ms·

It seems like the bug was originally introduced here: https://github.com/apache/httpd/commit/dd98030cb399e962aa6058c35893491a9ef7d147#diff-c79653a5ec3ccb8725b18

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
    - kruffalon 1y ago
      Thank you! <3
  - loloquwowndueo 1y ago
    Nothing a quick googling of jia Tan wouldn’t fix. It was a whole thing last year.
- dxxvi 1y ago
  [flagged]
- db48x 1y ago
  Lol. Although funny, it might be a little too soon to be making that joke.
teddyh 1y 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 1y 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] [1] https://archlinux.org/packages/extra/x86_64/apache/ https://archlinux.org/packages/extra/x86_64/apache/
  - captn3m0 1y ago
    https://repology.org/project/apache/versions https://repology.org/project/apache/versions Main ones: FreeBSD, Alpine, Fedora 42, OpenSUSE Tumbleweed
Cthulhu_ 1y ago
How come this wasn't covered by one or more automated tests that failed?
- throwaway2037 1y ago
  Yeah, crazy. Also, the bugfix does not include a test case.
  - _mlbt 1y ago
    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 https://httpd.apache.org/lists.html#http-dev
- whatevaa 1y ago
  There are tests?
  - Hilift 1y ago
    It is "CrowdTested".
    - oaiey 1y ago
      You already have a registered trademark? Will use that ;)
- testplzignore 1y ago
  Based on https://github.com/apache/httpd/commits?author=covener 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 1y ago
ralferoo 1y ago
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.

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
  - kruffalon 1y ago
    Thank you! <3
- loloquwowndueo 1y ago
  Nothing a quick googling of jia Tan wouldn’t fix. It was a whole thing last year.
dxxvi 1y ago
[flagged]
db48x 1y ago
Lol. Although funny, it might be a little too soon to be making that joke.

teddyh 1y 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 1y 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] [1] https://archlinux.org/packages/extra/x86_64/apache/ https://archlinux.org/packages/extra/x86_64/apache/
- captn3m0 1y ago
  https://repology.org/project/apache/versions https://repology.org/project/apache/versions Main ones: FreeBSD, Alpine, Fedora 42, OpenSUSE Tumbleweed

Cthulhu_ 1y ago

How come this wasn't covered by one or more automated tests that failed?

throwaway2037 1y ago
Yeah, crazy. Also, the bugfix does not include a test case.
- _mlbt 1y ago
  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 https://httpd.apache.org/lists.html#http-dev
whatevaa 1y ago
There are tests?
- Hilift 1y ago
  It is "CrowdTested".
  - oaiey 1y ago
    You already have a registered trademark? Will use that ;)
testplzignore 1y ago
Based on https://github.com/apache/httpd/commits?author=covener 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 1y ago

ralferoo 1y ago

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.