A bug in Apple’s code for exchaning SSL keys has been found and published. Even when you only take a quick look the issue here is pretty clear.
The code is implementing a bunch of if-statements followed by only a sinlge action each time. In C -as well as PHP- you don’t have to wrap a lonely action into curly brackets – as you would have to when you’re dealing with more than one.
Though it’s a known best practice to always wrap your code into such curly brackets, no matter how silly you think this is, you can see this kind of laziness all the time.
The problem is that as soon as you’re adding more code you might not see what still belongs to if-statement and what doesn’t.
And this is what happend here: At some point the SSL code always jumps to a sub routine and doesn’t even execute any code below that.
if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0)
goto fail;
goto fail;
What should have been written instead was either
if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0) {
goto fail;
goto fail;
}
in which case everything would have been just fine. Or
if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0) {
goto fail;
}
goto fail;
in which case the bug would still be there but would have been found much easier since it’s clear that the second goto fail running on its own.
To me it looks like this has been a simple copy/paste mistake. It’s not Apple specific by any means but I think it’s great if you can argue by pointing to such a prominent reference!
Again and to sum it up: Always wrap your conditionals into curly brackets!
3LpC 