Here's a little bit of a conspiracy theory for you. There's an old saying: "there are no coincidences on Wall Street". Not with the amounts of money that are involved. I think something similar should apply to critical security code. This reminds me of when someone tried to add the following to the Linux kernel: if ((options == (__WCLONE|__WALL)) && (current->uid = 0)) retval = -EINVAL; Oops, is that "uid == 0", or i…
How could this not happen in Python?
Because assigning in if would be a syntax error in Python.
Here's a little bit of a conspiracy theory for you. There's an old saying: "there are no coincidences on Wall Street". Not with the amounts of money that are involved. I think something similar should apply to critical security code. This reminds me of when someone tried to add the following to the Linux kernel: if ((options == (__WCLONE|__WALL)) && (current->uid = 0)) retval = -EINVAL; Oops, is that "uid == 0", or i…
How could this not happen in Python?
Python does not allow an assignment to occur within an expression. They deliberately chose that restriction, to avoid that hard-to-see bug.
Good find! But, are you sure the 10.8.5 (-55179.13) version isn't in some sense a later, patched maintenance branch compared to a 10.9 (-55471) that might have been frozen earlier? The release dates are very close (~2013-10-03 for 10.8.5; ~2013-10-22 for 10.9), and they might have already been separate branches. (Is there a 10.8.4 version to compare?)
Good point. I just checked that file ever since it was open-sourced in 10.8 and it stays exactly the same throughout all of 10.8.x (10.8 - 10.8.5) and only changes in 10.9. (You can check for yourself here: http://opensource.apple.com/ ) It doesn't seem like what you said is the case here but obviously we're still missing changesets that may have been committed between 10.8.5 (Security-55179.13) and 10.9 (Security-55…
Thanks for clarifying. I know silly errors like this can slip in, but I hope Apple does a deep x-ray on all circumstances surrounding the change.
Indeed. My first thought upon seeing this: "how in the FUCK did automated testing not catch this IMMEDIATELY!?" Absolutely terrifying that crypto safety is such a low QA priority that something like this could ever leave the building.
Because it's not a blatant regression, it's a complicate bug that requires a custom-patched TLS stack on the server to be explocited. As Adam Langley put it: > A test case could have caught this, but it's difficult because it's so deep into the handshake. One needs to write a completely separate TLS stack, with lots of options for sending invalid handshakes. In Chromium we have a patched version of TLSLite to do this…
Except: ensuring that the server is signing with the right, certificate-certified private key is the major thing that TLS is supposed to provide.
So no matter how strange or malicious the server-side stack would need to be... not having a test for such a deviation is a major oversight.
Potentially. This exploit was known to apple for some period of time, if NSA has access to the internal apple bug tracker, then they could certainly exploit the bug.
They'd also need access to Apple's private keys.
Not at all: that's the bug. It's not properly verifying that the other-end of a TLS session is the entity able to sign with the certificate-declared private-key.
Take a look at http://opensource.apple.com/source/Security/Security-55471/l... specifically check the function SSLVerifySignedServerKeyExchange I leave the joy of spotting it to you. It is obvious and if you know c you'll see it(You don't need any knowledge of crypto).
People bitch about indentation in Python. But in this case it would have prevented the bug! Of course Python doesn't even have a goto, so the code would have needed to be structured differently to start with. If I were a betting man I'd wager that this bug wasn't really an accident. It would be really interesting to check commit logs to see the history.
> People bitch about indentation in Python
Really? I did something in Python for the first time a while ago and the indentation as code block is something I find very elegant. I can't fathom why would people find something wrong with this.
Take a look at http://opensource.apple.com/source/Security/Security-55471/l... specifically check the function SSLVerifySignedServerKeyExchange I leave the joy of spotting it to you. It is obvious and if you know c you'll see it(You don't need any knowledge of crypto).
Don't C compilers warn about code that is never executed? I'm actually pretty impressed how this bug was not caught before. The compiler warning is just one thing that came to my mind but test coverage should have been the strongest hint. A linter could have also make the thing easier for a code review. Probably these parts of the code should have stricter coding guideline.
> Don't C compilers warn about code that is never executed?
Not by default. On both GCC and Clang this kind of error isn't caught using -Wall or even -Wextra - you have to explicitly opt-in using -Wunreachable-code. It'd really be nice if that changed and Clang basically had close to -Weverything on by default and required you to opt out, preferably with something like `#pragma ALLOW_SHODDY_` per file to put some pressure on C programmers not to just continue ignoring errors rather than fixing them.
People bitch about indentation in Python. But in this case it would have prevented the bug! Of course Python doesn't even have a goto, so the code would have needed to be structured differently to start with. If I were a betting man I'd wager that this bug wasn't really an accident. It would be really interesting to check commit logs to see the history.
> People bitch about indentation in Python Really? I did something in Python for the first time a while ago and the indentation as code block is something I find very elegant. I can't fathom why would people find something wrong with this.
it's relatively harder to refactor code - you can't just select between { and }. otherwise, no idea - python is my weapon of choice, so i may be biased.
Indeed. My first thought upon seeing this: "how in the FUCK did automated testing not catch this IMMEDIATELY!?" Absolutely terrifying that crypto safety is such a low QA priority that something like this could ever leave the building.
Because it's not a blatant regression, it's a complicate bug that requires a custom-patched TLS stack on the server to be explocited. As Adam Langley put it: > A test case could have caught this, but it's difficult because it's so deep into the handshake. One needs to write a completely separate TLS stack, with lots of options for sending invalid handshakes. In Chromium we have a patched version of TLSLite to do this…
This is a good example of why static analysis is so useful. Testing this in QA is a non-trivial problem but simply adding a compiler flag would report it.
Don't C compilers warn about code that is never executed? I'm actually pretty impressed how this bug was not caught before. The compiler warning is just one thing that came to my mind but test coverage should have been the strongest hint. A linter could have also make the thing easier for a code review. Probably these parts of the code should have stricter coding guideline.
> Don't C compilers warn about code that is never executed? Not by default. On both GCC and Clang this kind of error isn't caught using -Wall or even -Wextra - you have to explicitly opt-in using -Wunreachable-code. It'd really be nice if that changed and Clang basically had close to -Weverything on by default and required you to opt out, preferably with something like `#pragma ALLOW_SHODDY_ ` per file to put some pr…
In the latest GCC releases, -Wunreachable-code has been removed (the option still exists but is silently ignored for compat with existing Makefiles), as its output varied so much between releases (depending on what got optimized away).