8 ms·

Memcmp being wrongly stripped like strcmp

250 points by luu 6y ago

ishitatsuyuki 6y ago
Related: https://news.ycombinator.com/item?id=24636326 https://news.ycombinator.com/item?id=24636326
progre 6y ago
This is a really good example of how to report, and respond to a bug.
- papaf 6y ago
  The tooling is also interesting -- bugzilla and CVS. Edit: my mistake its actually git underneath.
  - ylyn 6y ago
    Why is Bugzilla interesting?
    - papaf 6y ago
      I wanted to make the point that old tools do not get in the way of professional results -- the point kind of fell flat because they are not actually using CVS and its just the label that Bugzilla is giving to Git.
mrtnmcc 6y ago
> We hit this bug in a test case in secp256k1, the cryptography library used in Bitcoin Core. That smells fishy. Is it possible the compiler introduces an intentional bug?
- FandangoRanger 6y ago
  Reflections on trusting trust?
  - mrtnmcc 6y ago
    OK my getting downvoted shouldn't be a surprise. But it sure seems suspicious that the author introducing the bug made commits that end with this bug - and then silence. https://gcc.gnu.org/git/?p=gcc.git;a=search;h=d01b568a78351beb6b693b613e6b48bcb2475eae;s=Bernd+Edlinger;st=author https://gcc.gnu.org/git/?p=gcc.git;a=search;h=d01b568a78351b... His name and hotmail email is associated in git with various cryptography projects including openssl.
    - admax88q 6y ago
      Your search is broken, you are searching up to the specific commit. Look now the history "ends" at a different commit: https://gcc.gnu.org/git/?p=gcc.git;a=search;h=5828c09abe00cce5ddff7eefef274c48021c72c9;s=Bernd+Edlinger;st=author https://gcc.gnu.org/git/?p=gcc.git;a=search;h=5828c09abe00cc...
    - DarthGhandi 6y ago
      thought I'd heard that name before, he's quite active, no idea how you ended up with "silence" https://www.theregister.com/2020/04/23/gcc_openssl_vulnerability/ https://www.theregister.com/2020/04/23/gcc_openssl_vulnerabi...
    - deleted 6y ago
      [deleted]
    - lwansbrough 6y ago
      That’s a very interesting discovery.
      - segfaultbuserr 6y ago
        And it's wrong, as the comment pointed out. Bernd Edlinger has been active since 2013, his most recent work was on GCC 10's static analyzer, which led to the discovery of an exploitable OpenSSL bug, and was seen in the press in April 2020 [0]. The initial bug report was posted on March, this can hardly be qualified as "going silent". [0] https://www.theregister.com/2020/04/23/gcc_openssl_vulnerability/ https://www.theregister.com/2020/04/23/gcc_openssl_vulnerabi...
- segfaultbuserr 6y ago
  The original secp256k1 developer Russell O’Connor who was hit by the bug (was on HN [0]) [1] said he rebuilt every package in his system after the bug was identified, and found only 10 lines of code that were miscompiled. Also, note that the bug was found during an attempt to add more test cases, pre-existing code in secp256k1 was not affected. > Three lines are tests. All of the lines could be rewritten as a comparison to 0. None of the lines looked that serious. I am not sure which one is the worse: the reduced message integrity code(?) from some ARCFOUR implementation or the something something from an ATM driver? Two unusual conditions are needed to trigger it... > * at least one of the compared byte arrays is constant and has a zero byte in position 0, 1, 2, or 3, and > * the result of the memcmp isn't immediately used in a "== 0" or "!= 0" test (or equivalently "if(memcmp(...))" or "if(!memcmp(...))"). > In particular the second condition makes this bug pretty rare and explains why it's mostly hit in non-inlined memcmp wrappers. (But in our case we hit it with a "<" comparison. ) Looks like a critical bug, but doesn't look like a conspiracy. [0] https://news.ycombinator.com/item?id=24636326 https://news.ycombinator.com/item?id=24636326 [1] http://r6.ca/blog/20200929T023701Z.html http://r6.ca/blog/20200929T023701Z.html
  - moonchild 6y ago
    Or, alternately, it's a conspiracy which is intended to have very tight coverage; it's very difficult to detect, if most code slips it by.
    - segfaultbuserr 6y ago
      > it's very difficult to detect, if most code slips it by. What would be the point if it has missed nearly every target? Based on the field survey by Russell O’Connor, no major crypto project or server project has been affected. The only security victim is a RC4-HMAC implementation for Kerberos 5, which has very limited uses - for Windows 2000, and deprecated since a long time ago. The only possible explanation would be either, 1. A long-term time bomb, intended to hit a big fish in the future, in a random, non-specific way. 2. Targeted attack (which doesn't make much sense, if you have enough exploitation capabilities to launch many types of targeted attacks already, why bother to leave a public record in GCC?).
- nullc 6y ago
  There is no security concern about it in libsecp256k1: The compilation was only happening in a newly written test case and didn't impact the production code. The miscompilation of the implementation of draft-brezak-win2k-krb-rc4-hmac-04.txt seemed interesting to me and potentially of a security concern. They looked to me like they were potentially significantly reducing the length of the message integrity check in an authentication protocol. https://github.com/heimdal/heimdal/blob/7ae2dfd853c87f9cbecb6f399de4dad09bad4606/lib/gssapi/krb5/arcfour.c#L390 https://github.com/heimdal/heimdal/blob/7ae2dfd853c87f9cbecb... https://github.com/heimdal/heimdal/blob/7ae2dfd853c87f9cbecb6f399de4dad09bad4606/lib/gssapi/krb5/arcfour.c#L661 https://github.com/heimdal/heimdal/blob/7ae2dfd853c87f9cbecb... https://github.com/heimdal/heimdal/blob/7ae2dfd853c87f9cbecb6f399de4dad09bad4606/lib/gssapi/krb5/arcfour.c#L1279 https://github.com/heimdal/heimdal/blob/7ae2dfd853c87f9cbecb... Though its hard to say for sure without even a passing familiarity with that protocol or its implementation. If you wanted to go vulnerability hunting, however, that is where I'd start. That said, even if there is a bug that is no evidence that it would have been intentional. Sadly, an intentionally introduced bug could be absurdly subtle. One of the downsides of software bugs being relatively common is that you can't be sure if a bug was intentional or not... and at the same time it's silly to assume one was intentional because we know that real bugs are so common. It's long been my view that if you're not occasionally finding compiler bugs -- even in somewhat older compilers-- you're just not testing hard enough. They've become more rare but are not so rare that a single developer comprehensively testing a small library won't eventually find one (or more). (Libsecp256k1 is ~9,964 lines of code and ~8,235 lines of test code). Many other cases found by Russell's system wide recompilation were also in test cases or were otherwise pretty obviously not that big of a deal.
  - segfaultbuserr 6y ago
    > The miscompilation of the implementation of draft-brezak-win2k-krb-rc4-hmac-04.txt seemed interesting to me and potentially of a security concern. Fortunately, this ancient and deprecated [0] authentication protocol from Windows 2000 is uncommon today. AES-HMAC is used since Windows Vista. [0] https://web.mit.edu/kerberos/krb5-devel/doc/admin/enctypes.html#enctype-compatibility https://web.mit.edu/kerberos/krb5-devel/doc/admin/enctypes.h...
- rurban 6y ago
  Unlikely. We were hit by a series of similar string builtin related bugs in the tree optimizer since gcc 9.0. And the bugfest is still ongoing. The most likely explanation is an incompetent developer touching these areas.
  - segfaultbuserr 6y ago
    Not sure why has this comment been downvoted. It simply describes the bug in context ("there has been a series of similar string builtin related bugs in the tree optimizer since gcc 9.0") and offers a plausible explanation of the bug ("an incompetent developer touching these areas") based on Hanlon's Razor. Even this is not welcomed on HN nowadays?
    - joosters 6y ago
      Perhaps because it is unfairly assuming incompetence? Everyone makes mistakes.
      - naniwaduni 6y ago
        It's attributing the error to incompetence instead of malice. It's not particularly unfair, it's providing an alternate explanation, if a bit bluntly.
        rbanffy 6y ago
        Even the most competent among us makes mistakes from time to time. I make them almost daily. Sometimes you just have a problem that's hard to wrap your head around and you think you found a way to make code faster that just isn't so. Now the issue is better understood and we may end up with a couple good tests to prevent future regressions.
        Dylan16807 6y ago
        A bunch of mistakes all in the same area mean you're doing something wrong, moreso than "everyone makes mistakes". And if it's an accident, and not caused by someone overworking you, then it falls under the umbrella of "incompetence". That doesn't mean you're a failure in general, it means you weren't up to doing this particular work at this moment in your life.
- kstenerud 6y ago
  Everything smells fishy when you want it to be.
- beowulfey 6y ago
  It was a fairly recent regression (this past March?) and based on developer comments in the source it really looks more like a mistake than anything. Personally I’m gonna go with Occam’s razor on this one.
userbinator 6y ago
Wow. This failure case of a compiler almost looks like the same sort of nonsense that AI/neural networks can generate, no doubt caused similarly by their immense complexity. The common theme is that no sane human would even think of generating such output.
- lmm 6y ago
  It seems a pretty straightforward (and all-too-common) kind of misunderstanding between humans: one person gave a function used by both memcmp and strcmp a name to do with strcmp, then another person wrote a comment that talked about strcmp, and eventually a third person made a change that would have been valid if the function was used only by strcmp. No individual would make such a mistake, true, but an organisation of several people could easily fail in the same way.
  - jmgao 6y ago
    > No individual would make such a mistake, true, but an organisation of several people could easily fail in the same way. You might be able to claim that it's unlikely for someone to make this mistake if the changes were in one single change (I doubt even this!), but the same person revisiting code at multiple points in time might as well be different people.
    - asddubs 6y ago
      yeah I've definitely made equivalent mistakes like that, where I gave something a misleading name because I didn't spend enough time thinking about it, and then eventually confused myself into introducing a bug
- nullc 6y ago
  Actually this is a common sort of bug I've seen programmers implement, e.g. using strcpy where memcpy should be used or vice versa. Most common on interface boundaries with other languages, particular "everything is a string" languages. So it's not that shocking to see a similar sort of bug made inside the compiler.
deleted 6y ago
[deleted]
nurettin 6y ago
This thread is the unfortunate side effect of a highly skilled developer making a simple mistake which can be understood and thus scrutinized by your average web guy. People thinking it is a compiler bug, people who want to milk controversy because some test in some bitcoin library dependency is affected. You are not getting a discussion worth a spit because the topic is extremely trivial and there is no genuine depth in it.
- gnud 6y ago
  What do you mean? This is from the first patch message on the mailing list: > An enhancement to GCC 10 to improve the expansion of strncmp calls with strings with embedded nuls introduced a regression in similar calls to memcmp. That patch was approved and merged. Sounds like a compiler bug to me.
  - nurettin 6y ago
    It was posted in the correct place, but you can avoid it entirely using the same compiler by skipping builtins. So it is a library bug rather than platform lexer parser or optimizer.
    - hvdijk 6y ago
      Builtins are functions that are built into the compiler rather than (or in addition to) being provided as part of any library. That is why they are called builtins. A bug in their implementation is a compiler bug, not a library bug.
      - nurettin 6y ago
        It is a bug in a library which you can avoid using with a flag. Take what you want from that. I won't reply to this nonsense.
  - abollaert 6y ago
    Thing is that people try to look for conspiracies where this is probably just an honest mistake.

250 points by luu 6y ago

ishitatsuyuki 6y ago

Related: https://news.ycombinator.com/item?id=24636326 https://news.ycombinator.com/item?id=24636326

progre 6y ago

This is a really good example of how to report, and respond to a bug.

papaf 6y ago
The tooling is also interesting -- bugzilla and CVS. Edit: my mistake its actually git underneath.
- ylyn 6y ago
  Why is Bugzilla interesting?
  - papaf 6y ago
    I wanted to make the point that old tools do not get in the way of professional results -- the point kind of fell flat because they are not actually using CVS and its just the label that Bugzilla is giving to Git.

mrtnmcc 6y ago

> We hit this bug in a test case in secp256k1, the cryptography library used in Bitcoin Core. That smells fishy. Is it possible the compiler introduces an intentional bug?

FandangoRanger 6y ago
Reflections on trusting trust?
- mrtnmcc 6y ago
  OK my getting downvoted shouldn't be a surprise. But it sure seems suspicious that the author introducing the bug made commits that end with this bug - and then silence. https://gcc.gnu.org/git/?p=gcc.git;a=search;h=d01b568a78351beb6b693b613e6b48bcb2475eae;s=Bernd+Edlinger;st=author https://gcc.gnu.org/git/?p=gcc.git;a=search;h=d01b568a78351b... His name and hotmail email is associated in git with various cryptography projects including openssl.
  - admax88q 6y ago
    Your search is broken, you are searching up to the specific commit. Look now the history "ends" at a different commit: https://gcc.gnu.org/git/?p=gcc.git;a=search;h=5828c09abe00cce5ddff7eefef274c48021c72c9;s=Bernd+Edlinger;st=author https://gcc.gnu.org/git/?p=gcc.git;a=search;h=5828c09abe00cc...
  - DarthGhandi 6y ago
    thought I'd heard that name before, he's quite active, no idea how you ended up with "silence" https://www.theregister.com/2020/04/23/gcc_openssl_vulnerability/ https://www.theregister.com/2020/04/23/gcc_openssl_vulnerabi...
  - deleted 6y ago
    [deleted]
  - lwansbrough 6y ago
    That’s a very interesting discovery.
    - segfaultbuserr 6y ago
      And it's wrong, as the comment pointed out. Bernd Edlinger has been active since 2013, his most recent work was on GCC 10's static analyzer, which led to the discovery of an exploitable OpenSSL bug, and was seen in the press in April 2020 [0]. The initial bug report was posted on March, this can hardly be qualified as "going silent". [0] https://www.theregister.com/2020/04/23/gcc_openssl_vulnerability/ https://www.theregister.com/2020/04/23/gcc_openssl_vulnerabi...
segfaultbuserr 6y ago
The original secp256k1 developer Russell O’Connor who was hit by the bug (was on HN [0]) [1] said he rebuilt every package in his system after the bug was identified, and found only 10 lines of code that were miscompiled. Also, note that the bug was found during an attempt to add more test cases, pre-existing code in secp256k1 was not affected. > Three lines are tests. All of the lines could be rewritten as a comparison to 0. None of the lines looked that serious. I am not sure which one is the worse: the reduced message integrity code(?) from some ARCFOUR implementation or the something something from an ATM driver? Two unusual conditions are needed to trigger it... > * at least one of the compared byte arrays is constant and has a zero byte in position 0, 1, 2, or 3, and > * the result of the memcmp isn't immediately used in a "== 0" or "!= 0" test (or equivalently "if(memcmp(...))" or "if(!memcmp(...))"). > In particular the second condition makes this bug pretty rare and explains why it's mostly hit in non-inlined memcmp wrappers. (But in our case we hit it with a "<" comparison. ) Looks like a critical bug, but doesn't look like a conspiracy. [0] https://news.ycombinator.com/item?id=24636326 https://news.ycombinator.com/item?id=24636326 [1] http://r6.ca/blog/20200929T023701Z.html http://r6.ca/blog/20200929T023701Z.html
- moonchild 6y ago
  Or, alternately, it's a conspiracy which is intended to have very tight coverage; it's very difficult to detect, if most code slips it by.
  - segfaultbuserr 6y ago
    > it's very difficult to detect, if most code slips it by. What would be the point if it has missed nearly every target? Based on the field survey by Russell O’Connor, no major crypto project or server project has been affected. The only security victim is a RC4-HMAC implementation for Kerberos 5, which has very limited uses - for Windows 2000, and deprecated since a long time ago. The only possible explanation would be either, 1. A long-term time bomb, intended to hit a big fish in the future, in a random, non-specific way. 2. Targeted attack (which doesn't make much sense, if you have enough exploitation capabilities to launch many types of targeted attacks already, why bother to leave a public record in GCC?).
nullc 6y ago
There is no security concern about it in libsecp256k1: The compilation was only happening in a newly written test case and didn't impact the production code. The miscompilation of the implementation of draft-brezak-win2k-krb-rc4-hmac-04.txt seemed interesting to me and potentially of a security concern. They looked to me like they were potentially significantly reducing the length of the message integrity check in an authentication protocol. https://github.com/heimdal/heimdal/blob/7ae2dfd853c87f9cbecb6f399de4dad09bad4606/lib/gssapi/krb5/arcfour.c#L390 https://github.com/heimdal/heimdal/blob/7ae2dfd853c87f9cbecb... https://github.com/heimdal/heimdal/blob/7ae2dfd853c87f9cbecb6f399de4dad09bad4606/lib/gssapi/krb5/arcfour.c#L661 https://github.com/heimdal/heimdal/blob/7ae2dfd853c87f9cbecb... https://github.com/heimdal/heimdal/blob/7ae2dfd853c87f9cbecb6f399de4dad09bad4606/lib/gssapi/krb5/arcfour.c#L1279 https://github.com/heimdal/heimdal/blob/7ae2dfd853c87f9cbecb... Though its hard to say for sure without even a passing familiarity with that protocol or its implementation. If you wanted to go vulnerability hunting, however, that is where I'd start. That said, even if there is a bug that is no evidence that it would have been intentional. Sadly, an intentionally introduced bug could be absurdly subtle. One of the downsides of software bugs being relatively common is that you can't be sure if a bug was intentional or not... and at the same time it's silly to assume one was intentional because we know that real bugs are so common. It's long been my view that if you're not occasionally finding compiler bugs -- even in somewhat older compilers-- you're just not testing hard enough. They've become more rare but are not so rare that a single developer comprehensively testing a small library won't eventually find one (or more). (Libsecp256k1 is ~9,964 lines of code and ~8,235 lines of test code). Many other cases found by Russell's system wide recompilation were also in test cases or were otherwise pretty obviously not that big of a deal.
- segfaultbuserr 6y ago
  > The miscompilation of the implementation of draft-brezak-win2k-krb-rc4-hmac-04.txt seemed interesting to me and potentially of a security concern. Fortunately, this ancient and deprecated [0] authentication protocol from Windows 2000 is uncommon today. AES-HMAC is used since Windows Vista. [0] https://web.mit.edu/kerberos/krb5-devel/doc/admin/enctypes.html#enctype-compatibility https://web.mit.edu/kerberos/krb5-devel/doc/admin/enctypes.h...
rurban 6y ago
Unlikely. We were hit by a series of similar string builtin related bugs in the tree optimizer since gcc 9.0. And the bugfest is still ongoing. The most likely explanation is an incompetent developer touching these areas.
- segfaultbuserr 6y ago
  Not sure why has this comment been downvoted. It simply describes the bug in context ("there has been a series of similar string builtin related bugs in the tree optimizer since gcc 9.0") and offers a plausible explanation of the bug ("an incompetent developer touching these areas") based on Hanlon's Razor. Even this is not welcomed on HN nowadays?
  - joosters 6y ago
    Perhaps because it is unfairly assuming incompetence? Everyone makes mistakes.
    - naniwaduni 6y ago
      It's attributing the error to incompetence instead of malice. It's not particularly unfair, it's providing an alternate explanation, if a bit bluntly.
      - rbanffy 6y ago
        Even the most competent among us makes mistakes from time to time. I make them almost daily. Sometimes you just have a problem that's hard to wrap your head around and you think you found a way to make code faster that just isn't so. Now the issue is better understood and we may end up with a couple good tests to prevent future regressions.
        Dylan16807 6y ago
        A bunch of mistakes all in the same area mean you're doing something wrong, moreso than "everyone makes mistakes". And if it's an accident, and not caused by someone overworking you, then it falls under the umbrella of "incompetence". That doesn't mean you're a failure in general, it means you weren't up to doing this particular work at this moment in your life.
kstenerud 6y ago
Everything smells fishy when you want it to be.
beowulfey 6y ago
It was a fairly recent regression (this past March?) and based on developer comments in the source it really looks more like a mistake than anything. Personally I’m gonna go with Occam’s razor on this one.

userbinator 6y ago

Wow. This failure case of a compiler almost looks like the same sort of nonsense that AI/neural networks can generate, no doubt caused similarly by their immense complexity. The common theme is that no sane human would even think of generating such output.

lmm 6y ago
It seems a pretty straightforward (and all-too-common) kind of misunderstanding between humans: one person gave a function used by both memcmp and strcmp a name to do with strcmp, then another person wrote a comment that talked about strcmp, and eventually a third person made a change that would have been valid if the function was used only by strcmp. No individual would make such a mistake, true, but an organisation of several people could easily fail in the same way.
- jmgao 6y ago
  > No individual would make such a mistake, true, but an organisation of several people could easily fail in the same way. You might be able to claim that it's unlikely for someone to make this mistake if the changes were in one single change (I doubt even this!), but the same person revisiting code at multiple points in time might as well be different people.
  - asddubs 6y ago
    yeah I've definitely made equivalent mistakes like that, where I gave something a misleading name because I didn't spend enough time thinking about it, and then eventually confused myself into introducing a bug
nullc 6y ago
Actually this is a common sort of bug I've seen programmers implement, e.g. using strcpy where memcpy should be used or vice versa. Most common on interface boundaries with other languages, particular "everything is a string" languages. So it's not that shocking to see a similar sort of bug made inside the compiler.

deleted 6y ago

[deleted]

nurettin 6y ago

This thread is the unfortunate side effect of a highly skilled developer making a simple mistake which can be understood and thus scrutinized by your average web guy. People thinking it is a compiler bug, people who want to milk controversy because some test in some bitcoin library dependency is affected. You are not getting a discussion worth a spit because the topic is extremely trivial and there is no genuine depth in it.

gnud 6y ago
What do you mean? This is from the first patch message on the mailing list: > An enhancement to GCC 10 to improve the expansion of strncmp calls with strings with embedded nuls introduced a regression in similar calls to memcmp. That patch was approved and merged. Sounds like a compiler bug to me.
- nurettin 6y ago
  It was posted in the correct place, but you can avoid it entirely using the same compiler by skipping builtins. So it is a library bug rather than platform lexer parser or optimizer.
  - hvdijk 6y ago
    Builtins are functions that are built into the compiler rather than (or in addition to) being provided as part of any library. That is why they are called builtins. A bug in their implementation is a compiler bug, not a library bug.
    - nurettin 6y ago
      It is a bug in a library which you can avoid using with a flag. Take what you want from that. I won't reply to this nonsense.
- abollaert 6y ago
  Thing is that people try to look for conspiracies where this is probably just an honest mistake.