4 ms·

There is a great comment linked at the bottom of the article that explains what exactly caused the authentication bug (it was strncmp!). Is there somehow a law

by Cyph0n 10y ago

There is a great comment linked at the bottom of the article that explains what exactly caused the authentication bug (it was strncmp!). Is there somehow a law that states that the higher the severity of a software bug, the simpler the reason behind it?

mirimir 10y ago
Yes, that is crazy. Checking for match based on the length of the input. If input is "", that matches anything.
chmaynard 10y ago
From the comment: What the programmer should have done is check if the hash coming from the browser has the correct length, 32 characters, before attempting to compare the two strings. Or even better, the programmer should have used the proper string comparing function, strcmp, that already does that for you...
- benchaney 10y ago
  strcmp has its own issues. What they actually should have done is use a cryptography library with properly implemented comparison, because it is surprisingly easy to get wrong. Also, fixed length hashes aren't strings. They shouldn't use the same functions as strings. Edit: Using a proper cryptography library also might have saved them from using MD5. At the very least, most modern libraries have a warning that is is deprecated and should not be used for new projects.
  - dom0 10y ago
    Yep, even if they had used memcmp/strncmp correctly the AMT would still be vulnerable to a timing attack here, which is probably even easy, because it runs on a low-power system with, I assume, not much background activity.
    - benchaney 10y ago
      In addition to timing attacks, if the attacker supplied a non null terminated string, it could potentially cause a memory overrun. This may not be possible depending on how the string is handled on ingress, but if it is, it would likely have serious consequences.
      - qb45 10y ago
        The attacker supplies ""-enclosed string and the HTTP parser is supposed to verify this. Maybe they used some in-place HTTP parser and that's why they didn't have zero-termination to use strcmp. And btw, it's not clear if the code was C to begin with. I believe large chunk of ME firmware is said to be written in Java, which kinda makes sense for a network facing system considering how shitty string.h is. It would be interesting if somebody found that they are using some COTS webserver, C/Java - doesn't matter, and see if there are some CVEs for it.
        benchaney 10y ago
        I doubt using Java would really help in this case, because in order to use Java they would have to port the entire JVM to whatever platform they are using. Doing this without introducing any security bugs is about as difficult as just doing everything in C to begin with. By the way, the fact that the issue was with strncmp very strongly suggests that the issue was in C code.
        dom0 10y ago
        Some security paper showed ARM disassembly of the issue, although I'm not sure if that was this specific issue or just another "strncmp-induced" issue. (Btw. there are many smaller Java VMs for stuff like Java Card and JEFF)
        qb45 10y ago
        They use ARC architecture and some embedded JVM. Of course this doesn't guarantee that the AMT web server runs on this JVM. Few years ago somebody published analysis of the ME firmware for servers (desktop version was compressed with unknown algorithm). I'm sure you can google this for details. > the fact that the issue was with strncmp But we don't know for sure if it was strncmp. It's a logic bug, which can be implemented in any language. But again, maybe this server runs outside JVM and is written in C.
  - qb45 10y ago
    > Also, fixed length hashes aren't strings. In HTTP everything is a string. > Using a proper cryptography library also might have saved them from using MD5. HTTP, again. Browser sends MD5 whether you like it or not.
    - benchaney 10y ago
      > In HTTP everything is a string. Once you parse it out of HTTP it is no longer as string (or at least it should no longer be a string). >HTTP, again. Browser sends MD5 whether you like it or not. They are quite clearly doing verification themselves. They can use whatever algorithm they want.
      - qb45 10y ago
        It has to be MD5. They use HTTP digest auth, which uses MD5. You send a nonce, the browser throws a popup asking for password and returns md5(nonce+pass). No way around it.
        coldtea 10y ago
        Very easy way around it: do NOT use digest auth.
        qb45 10y ago
        Sure, which brings us to the second point: HTTP's MD5 digest auth is still good enough to keep passive eavesdroppers from learning the password when TLS isn't used, which is its only purpose.
  - rusanu 10y ago
    They were implementing HTTP Digest[0]. This leaves no room to choose something other than MD5. And I'm not sure what other HTTP authentication scheme they could had chose for firmware (ie. cannot take dependency on anything in infrastructure, like PKI or Kerb) [0] https://en.wikipedia.org/wiki/Digest_access_authentication https://en.wikipedia.org/wiki/Digest_access_authentication
- mynameisbahaa 10y ago
  I think the programmer should have supplied the length of the "computed_hash" not the "response" which as I understood supplied by the user. Like this : strncmp(computed_hash, response, computed_hash_length)
  - jjnoakes 10y ago
    You don't want to use strncmp() for this; aside from the timing attacks it opens up, using strncmp() for these kinds of comparisons implies that the operation you are performing is "string a is a prefix of string b" (or vice-versa). Even though your example ends up being ok-ish (if the computed hash is a prefix of the response, perhaps it is ok to ignore any trailing junk in the response), intent is important for code quality and maintainability. In this instance, the operation desired is "string a matches string b", which means strcmp() would be the right solution (ignoring timing attacks). Of course, since we're talking about sensitive crypto operations here, neither is really the right answer. But in non-crypto contexts, if you want to know if two (valid) strings are the same, just use strcmp(). The "n" and the length argument doesn't automatically make strncmp() "safer" somehow; it is a totally different operation.
  - angry_octet 10y ago
    Yes. It was hilarious that the poster thought strcmp was the solution... Though perhaps memcmp with fixed size buffers would be better still, no worrying about null terminated strings.
- chillingeffect 10y ago
  It almost smells like an obfuscated tactic to evade review. In a rush a reviewer might see strncmp and think "Not using strcmp. Check." without going all the way to consider the strlen as argument, which really should have been a constant and the smoking gun.
ComodoHacker 10y ago
That comment id great, but where is the source? I can't find any technical details following links in the article. Moreover, Embedi team who found the vulnerability says[0] on their site: "Intel representatives have asked Embedi to hold off on disclosing any technical details regarding this issue until further notice." Or is the commenter just guessing? 0. https://www.embedi.com/news/mythbusters-cve-2017-5689 https://www.embedi.com/news/mythbusters-cve-2017-5689

by Cyph0n 10y ago

mirimir 10y ago

Yes, that is crazy. Checking for match based on the length of the input. If input is "", that matches anything.

chmaynard 10y ago

From the comment: What the programmer should have done is check if the hash coming from the browser has the correct length, 32 characters, before attempting to compare the two strings. Or even better, the programmer should have used the proper string comparing function, strcmp, that already does that for you...

benchaney 10y ago
strcmp has its own issues. What they actually should have done is use a cryptography library with properly implemented comparison, because it is surprisingly easy to get wrong. Also, fixed length hashes aren't strings. They shouldn't use the same functions as strings. Edit: Using a proper cryptography library also might have saved them from using MD5. At the very least, most modern libraries have a warning that is is deprecated and should not be used for new projects.
- dom0 10y ago
  Yep, even if they had used memcmp/strncmp correctly the AMT would still be vulnerable to a timing attack here, which is probably even easy, because it runs on a low-power system with, I assume, not much background activity.
  - benchaney 10y ago
    In addition to timing attacks, if the attacker supplied a non null terminated string, it could potentially cause a memory overrun. This may not be possible depending on how the string is handled on ingress, but if it is, it would likely have serious consequences.
    - qb45 10y ago
      The attacker supplies ""-enclosed string and the HTTP parser is supposed to verify this. Maybe they used some in-place HTTP parser and that's why they didn't have zero-termination to use strcmp. And btw, it's not clear if the code was C to begin with. I believe large chunk of ME firmware is said to be written in Java, which kinda makes sense for a network facing system considering how shitty string.h is. It would be interesting if somebody found that they are using some COTS webserver, C/Java - doesn't matter, and see if there are some CVEs for it.
      - benchaney 10y ago
        I doubt using Java would really help in this case, because in order to use Java they would have to port the entire JVM to whatever platform they are using. Doing this without introducing any security bugs is about as difficult as just doing everything in C to begin with. By the way, the fact that the issue was with strncmp very strongly suggests that the issue was in C code.
        dom0 10y ago
        Some security paper showed ARM disassembly of the issue, although I'm not sure if that was this specific issue or just another "strncmp-induced" issue. (Btw. there are many smaller Java VMs for stuff like Java Card and JEFF)
        qb45 10y ago
        They use ARC architecture and some embedded JVM. Of course this doesn't guarantee that the AMT web server runs on this JVM. Few years ago somebody published analysis of the ME firmware for servers (desktop version was compressed with unknown algorithm). I'm sure you can google this for details. > the fact that the issue was with strncmp But we don't know for sure if it was strncmp. It's a logic bug, which can be implemented in any language. But again, maybe this server runs outside JVM and is written in C.
- qb45 10y ago
  > Also, fixed length hashes aren't strings. In HTTP everything is a string. > Using a proper cryptography library also might have saved them from using MD5. HTTP, again. Browser sends MD5 whether you like it or not.
  - benchaney 10y ago
    > In HTTP everything is a string. Once you parse it out of HTTP it is no longer as string (or at least it should no longer be a string). >HTTP, again. Browser sends MD5 whether you like it or not. They are quite clearly doing verification themselves. They can use whatever algorithm they want.
    - qb45 10y ago
      It has to be MD5. They use HTTP digest auth, which uses MD5. You send a nonce, the browser throws a popup asking for password and returns md5(nonce+pass). No way around it.
      - coldtea 10y ago
        Very easy way around it: do NOT use digest auth.
        qb45 10y ago
        Sure, which brings us to the second point: HTTP's MD5 digest auth is still good enough to keep passive eavesdroppers from learning the password when TLS isn't used, which is its only purpose.
- rusanu 10y ago
  They were implementing HTTP Digest[0]. This leaves no room to choose something other than MD5. And I'm not sure what other HTTP authentication scheme they could had chose for firmware (ie. cannot take dependency on anything in infrastructure, like PKI or Kerb) [0] https://en.wikipedia.org/wiki/Digest_access_authentication https://en.wikipedia.org/wiki/Digest_access_authentication
mynameisbahaa 10y ago
I think the programmer should have supplied the length of the "computed_hash" not the "response" which as I understood supplied by the user. Like this : strncmp(computed_hash, response, computed_hash_length)
- jjnoakes 10y ago
  You don't want to use strncmp() for this; aside from the timing attacks it opens up, using strncmp() for these kinds of comparisons implies that the operation you are performing is "string a is a prefix of string b" (or vice-versa). Even though your example ends up being ok-ish (if the computed hash is a prefix of the response, perhaps it is ok to ignore any trailing junk in the response), intent is important for code quality and maintainability. In this instance, the operation desired is "string a matches string b", which means strcmp() would be the right solution (ignoring timing attacks). Of course, since we're talking about sensitive crypto operations here, neither is really the right answer. But in non-crypto contexts, if you want to know if two (valid) strings are the same, just use strcmp(). The "n" and the length argument doesn't automatically make strncmp() "safer" somehow; it is a totally different operation.
- angry_octet 10y ago
  Yes. It was hilarious that the poster thought strcmp was the solution... Though perhaps memcmp with fixed size buffers would be better still, no worrying about null terminated strings.
chillingeffect 10y ago
It almost smells like an obfuscated tactic to evade review. In a rush a reviewer might see strncmp and think "Not using strcmp. Check." without going all the way to consider the strlen as argument, which really should have been a constant and the smoking gun.

ComodoHacker 10y ago

That comment id great, but where is the source? I can't find any technical details following links in the article. Moreover, Embedi team who found the vulnerability says[0] on their site: "Intel representatives have asked Embedi to hold off on disclosing any technical details regarding this issue until further notice." Or is the commenter just guessing? 0. https://www.embedi.com/news/mythbusters-cve-2017-5689 https://www.embedi.com/news/mythbusters-cve-2017-5689