Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

About the security content of iOS 7.0.6

support.apple.com

Post reply on HN
Print view View original

141–150 of 155 posts

Re: About the security content of iOS 7.0.6

#141

Post by spoint »

chaitanya wrote: post #84
pencilo wrote: post #61

Earlier quoted context omitted.

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).

(Spoiler) This is another reason why you should never use if statements without curly braces. Thanks for the hint. It was amusing to spot the actual bug.

I partially agree. Curly braces won't fix a typo or a rushed code review.

Re: About the security content of iOS 7.0.6

#142

Post by [deleted] »

alex1 wrote: post #78
pencilo wrote: post #61

Earlier quoted context omitted.

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).

Here's a diff of that file from OS X 10.8.5 (Security-55179.13) to 10.9 (Security-55471): https://gist.github.com/alexyakoubian/9151610/revisions Check line 631. Appears seemingly out of nowhere.

[deleted]

Re: About the security content of iOS 7.0.6

#143

Post by cripto »

alex1 wrote: post #78
pencilo wrote: post #61

Earlier quoted context omitted.

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).

Here's a diff of that file from OS X 10.8.5 (Security-55179.13) to 10.9 (Security-55471): https://gist.github.com/alexyakoubian/9151610/revisions Check line 631. Appears seemingly out of nowhere.

So, what does that mean, the goto fail portion of the code. It seems like it will go to that no matter what. What is the end outcome?

Thanks in advanced

Re: About the security content of iOS 7.0.6

#144

Post by peternlewis »

subdigital wrote: post #140
kplaxmaster wrote: post #135

Earlier quoted context omitted.

Haha love the whitespace comment. This also makes you think of the static source-code analysis done at Apple. Surely static tools would have picked this up, no...?

Clang currently does not warn about this, but I'd wager that Xcode will boast a feature in the next version that detects dead code due to early returns/gotos.

Clang may not warn about white space issues, but it certainly does warn about unreachable code if -Wunreachable is on. This code would not compile in my project because the unconditional goto leaves the following code unreachable.

Re: About the security content of iOS 7.0.6

#145

Post by chengg11 »

nikster wrote: post #133
pencilo wrote: post #61

Earlier quoted context omitted.

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).

Conspiracy theories aside, every programmer has probably made that mistake. The better ones learn to just avoid if statements without blocks ;) But one thing that pisses me off is that they go and implement SSL and don't have any automated tests for it !! For a company of Apple's size, that can only be called grossly negligent. There is no excuse. And they probably don't have any automated testing for most of their o…

Of all the comments, I think yours is most to the point: we cannot rely on developers not making mistakes, but some kind of processes (tools, QA testing) should have caught an error like this one.

Re: About the security content of iOS 7.0.6

#146

Post by juliok »

nikster wrote: post #133
pencilo wrote: post #61

Earlier quoted context omitted.

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).

Conspiracy theories aside, every programmer has probably made that mistake. The better ones learn to just avoid if statements without blocks ;) But one thing that pisses me off is that they go and implement SSL and don't have any automated tests for it !! For a company of Apple's size, that can only be called grossly negligent. There is no excuse. And they probably don't have any automated testing for most of their o…

My sentiments exactly. For them to have a piece of code that returns OK/notOK, in SSL of all things, and not have tests for both branches is inexcusable. Not only is this code untested, obviously, but the calling code (which should have two branches based on the result of this call) must be untested too. Bleagh!

Re: About the security content of iOS 7.0.6

#147

Post by WaltSellers »

stigi wrote: post #131
lectrick wrote: post #129

Earlier quoted context omitted.

Goto considered harmful, indeed!

Not much a problem of `goto` per se. Rather a problem with if conditions used without code blocks. Others might say it's a problem of whitespace insensitive languages ;)

Even if there were braces, the bug would still exist if the extra goto was outside the braces.

It might be more noticeable, but then, the original bug existed because no one noticed.

Re: About the security content of iOS 7.0.6

#148

Post by WaltSellers »

cripto wrote: post #143
alex1 wrote: post #78

Earlier quoted context omitted.

Here's a diff of that file from OS X 10.8.5 (Security-55179.13) to 10.9 (Security-55471): https://gist.github.com/alexyakoubian/9151610/revisions Check line 631. Appears seemingly out of nowhere.

So, what does that mean, the goto fail portion of the code. It seems like it will go to that no matter what. What is the end outcome? Thanks in advanced

At that point the variable 'err' is still zero (noErr). So the function returns noErr, the caller thinks everything is good, and the communication is allowed.

Re: About the security content of iOS 7.0.6

#149

Post by WaltSellers »

rennison wrote: post #139
pencilo wrote: post #61

Earlier quoted context omitted.

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).

Damn what kind of half-assed developers do they have at Apple these days? They broke 2 of my (admittedly many) cardinal rules of C development: 1 - There is never ever a valid reason for using goto. It should not be part of the language. 2 - Always enclose script blocks in {} even if it's only a single line.

What about these?

  - don't leave local variables uninitialized.
  - don't try to hand-optimize lines unnecessarily. 
  - don't mix assignments into boolean expressions. (Which is really an unneeded optimization.)

Re: About the security content of iOS 7.0.6

#150

Post by WaltSellers »

stigi wrote: post #131
lectrick wrote: post #129

Earlier quoted context omitted.

Goto considered harmful, indeed!

Not much a problem of `goto` per se. Rather a problem with if conditions used without code blocks. Others might say it's a problem of whitespace insensitive languages ;)

From a different point of view, part of the problem is the undefined state of the code at the "fail" label.

Execution will arrive there somehow, but the 'how' is unclear. The word "fail" implies you should reach that point only if there was an error, but that is a bad assumption in this case.

If the real answer to 'how did we get here?' was checked, then the bug could not hide in the undefined behavior. This would not allow a dangling goto to result in a false positive. A false negative will get someone's attention when their web page doesn't load.

Something like this could remove the undefined state:

      goto pass;
  fail:
    if ( err == 0 ) {
    	assert( err != 0 ); // BUG! variable must contain an error number
    	err = kSomeAppropriateErrorNumber;
    }
  pass:
    SSLFreeBuffer(&signedHashes);
    SSLFreeBuffer(&hashCtx);
    return err;
Post reply on HN