3 ms·

To everyone claiming "it looks like a merge gone bad": That's a bit hard to believe. There's no other changes around the extra goto, so it's pretty weird for a

by 0x0 13y ago

To everyone claiming "it looks like a merge gone bad": That's a bit hard to believe. There's no other changes around the extra goto, so it's pretty weird for a duplicate line to appear like that out of nowhere. It's also incredibly visible in any diff viewer, so how such a checkin could have gone in without being noticed is just unfathomable.

thrownaway2424 13y ago
Where's the commit that introduced the extra goto? I haven't seen it yet.
- 0x0 13y ago
  As outsiders we can't see commits, but the next best thing is a diff between the public source code releases: https://gist.github.com/alexyakoubian/9151610/revisions https://gist.github.com/alexyakoubian/9151610/revisions
  - thirsteh 13y ago
    Line 631 in the second column for anyone wondering.
  - MBCook 13y ago
    It could have been merge between public releases. My money would be on a copy & paste error, but there is no more evidence for that than the merge error theory.
  - thrownaway2424 13y ago
    Pretty yak-shavy. Looks like some symbols were renamed and many of the APIs were (probably pointlessly) mutated. In total, unreviewable. But as you say, this may not have been the internal diff seen by someone at Apple. They may have seen individual, focussed patches that did single things like changing from the 2-argument to the 1-argument SSLFreeBuffer, another patch for changing selectedCipherSpec to selectedCipherSpecParams (hello, yak shaving), renaming noErr to errSecSuccess, and so forth. The question is, in any of those changes, the extra "goto fail;" line would have stuck out as irrelevant. In a gigantic 500-line delta the reviewer's eyes may have been glazed over by the time they got to that one. Assuming anyone reviews code at Apple, of course.
    - 0x0 13y ago
      I'm shocked and find that hard to believe; even without separate commits, it's not a huge single diff and it should really not be too much to ask for a proper review. You'd think one would be a little less sloppy when hacking on the very core of the SSL libraries being deployed on hundreds of million of devices.
      - tptacek 13y ago
        You'd think one would be a little less sloppy when hacking on the very core of the SSL libraries being deployed on hundreds of million of devices Hahahahahaha.
  - dandelany 13y ago
    > As outsiders we can't see commits Exactly. I was going to refute your point but you did it for me.
tptacek 13y ago
It's hard to believe if you pay no attention to bugs caused by code refactorings, but easy to believe if you, for instance, saw the OpenBSD IPSEC team refactor authentication checking on IPSEC packets out of the kernel by accident, in almost the exact same manner as this bug.
- lawnchair_larry 13y ago
  No, because the OpenBSD case was NSA as well (via NETSEC, who worked on the OpenBSD IPSEC stack, and have since admitted to sneaking backdoors in). Exact same MO, 15 years later! (I don't actually believe this, but it was too convenient and amusing not to call out.)
  - tptacek 13y ago
    I thought the NETSEC conspiracy theory was Angelos Keromytis working for the FBI.

by 0x0 13y ago

thrownaway2424 13y ago

Where's the commit that introduced the extra goto? I haven't seen it yet.

0x0 13y ago
As outsiders we can't see commits, but the next best thing is a diff between the public source code releases: https://gist.github.com/alexyakoubian/9151610/revisions https://gist.github.com/alexyakoubian/9151610/revisions
- thirsteh 13y ago
  Line 631 in the second column for anyone wondering.
- MBCook 13y ago
  It could have been merge between public releases. My money would be on a copy & paste error, but there is no more evidence for that than the merge error theory.
- thrownaway2424 13y ago
  Pretty yak-shavy. Looks like some symbols were renamed and many of the APIs were (probably pointlessly) mutated. In total, unreviewable. But as you say, this may not have been the internal diff seen by someone at Apple. They may have seen individual, focussed patches that did single things like changing from the 2-argument to the 1-argument SSLFreeBuffer, another patch for changing selectedCipherSpec to selectedCipherSpecParams (hello, yak shaving), renaming noErr to errSecSuccess, and so forth. The question is, in any of those changes, the extra "goto fail;" line would have stuck out as irrelevant. In a gigantic 500-line delta the reviewer's eyes may have been glazed over by the time they got to that one. Assuming anyone reviews code at Apple, of course.
  - 0x0 13y ago
    I'm shocked and find that hard to believe; even without separate commits, it's not a huge single diff and it should really not be too much to ask for a proper review. You'd think one would be a little less sloppy when hacking on the very core of the SSL libraries being deployed on hundreds of million of devices.
    - tptacek 13y ago
      You'd think one would be a little less sloppy when hacking on the very core of the SSL libraries being deployed on hundreds of million of devices Hahahahahaha.
- dandelany 13y ago
  > As outsiders we can't see commits Exactly. I was going to refute your point but you did it for me.

tptacek 13y ago

It's hard to believe if you pay no attention to bugs caused by code refactorings, but easy to believe if you, for instance, saw the OpenBSD IPSEC team refactor authentication checking on IPSEC packets out of the kernel by accident, in almost the exact same manner as this bug.

lawnchair_larry 13y ago
No, because the OpenBSD case was NSA as well (via NETSEC, who worked on the OpenBSD IPSEC stack, and have since admitted to sneaking backdoors in). Exact same MO, 15 years later! (I don't actually believe this, but it was too convenient and amusing not to call out.)
- tptacek 13y ago
  I thought the NETSEC conspiracy theory was Angelos Keromytis working for the FBI.