How you can learn from Apple’s mistake

In late February Apple published a security update for iOS and OSX. The reason for this update will be explained at the bottom of this article.

Why Should I Care?

The reason why this specific issue warrants a post is to show you just how easy it is to fall into the trap of using a perfectly valid code syntax that can take down novices and experts a like.

You can learn something from this error and then understand why you should not use the same code syntax yourself. This is a long article but it takes you step-by-step through the whole process and you will be better for it.

Original Code

This is the offending section of code that caused all the problems. The following code is written in C, now I know many of you will not be familiar with C language but you will understand the syntax as both it and JavaScript are very similar.

if ((err = SSLFreeBuffer(&hashCtx)) != 0)
    goto fail;
if ((err = ReadyHash(&SSLHashSHA1, &hashCtx)) != 0)
    goto fail;
if ((err = SSLHashSHA1.update(&hashCtx, &clientRandom)) != 0)
    goto fail;
if ((err = SSLHashSHA1.update(&hashCtx, &serverRandom)) != 0)
    goto fail;
if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0)
    goto fail;
    goto fail;
if ((err = SSLHashSHA1.final(&hashCtx, &hashOut)) != 0)
    goto fail;

Some of you will see the error straight away and know why it failed to do the job it was meant to do and how the problem could have easily been prevented.

Did you spot the problem?

If not, do not worry – neither did Apple. I will convert the C code to a JavaScript equivalent, we can take it from there.

if ((err = SSLFreeBuffer(hashCtx)) != 0)
    fail();
if ((err = ReadyHash(SSLHashSHA1, hashCtx)) != 0)
    fail();
if ((err = SSLHashSHA1.update(hashCtx, clientRandom)) != 0)
    fail();
if ((err = SSLHashSHA1.update(hashCtx, serverRandom)) != 0)
    fail();
if ((err = SSLHashSHA1.update(hashCtx, signedParams)) != 0)
    fail();
    fail();
if ((err = SSLHashSHA1.final(hashCtx, hashOut)) != 0)
    fail();

The above code is a snippet, you do not need to know anything about this code other than what you see. In case you think this is some sort of trick and I am playing with you – I am not, it is there – look harder.

Have you seen the problem yet?

Understandable, again – neither did Apple. So how should you be looking at this code to find the answer? I will simplify the code, add a few things to make it easier to work out in your head – then take another look.

var err = 0;
var hashCtx = 1;
var clientRandom = 2;
var serverRandom = 3;
var signedParams = 4;
var hashOut = 5;
function fail() {
    Ti.API.info('failed');
}
if (err !== 0)
    fail();
if ((err === hashCtx) !== false)
    fail();
if ((err === clientRandom) !== false)
    fail();
if ((err === serverRandom) !== false)
    fail();
if ((err === signedParams) !== false)
    fail();
    fail();
if ((err === hashOut)!== false)
    fail();

So to make it easier I have included variables with values and a function that does something. From here it should make it a little easier to find. Read the code and see if you see it yet.

Spotted it yet?

Have you tried the code? Give it a go and see what happens. If you have read and understood the code you should know that all of the if statement conditions are false – so why does the console show “failed”?

Still not spotted it?

Again do not be disheartened we are very close to an answer. We will tweak the code once more to point out where the problem is.

var err = 0;
var hashCtx = 1;
var clientRandom = 2;
var serverRandom = 3;
var signedParams = 4;
var hashOut = 5;
function fail(x) {
    Ti.API.info('failed: ' + x);
}
if (err !== 0)
    fail(0);
if ((err === hashCtx) !== false)
    fail(1);
if ((err === clientRandom) !== false)
    fail(2);
if ((err === serverRandom) !== false)
    fail(3);
if ((err === signedParams) !== false)
    fail(4);
    fail(5);
if ((err === hashOut) !== false)
    fail(6);

Run the code again and you should now see the the console says “failed: 5”. But why?

So we should take a look at the “if statement” for “fail(5)” to see what happened.

// these are the values
// err = 0
// signedParams = 4
if ((err === signedParams) !== false)
// this becomes
if ((0 === 4) !== false)

We know that 0 and 4 are not the same so this part must be false. The “if statement” is checking to make sure that the answer is anything except false before it will run the code within the “if statement”. The answer is “false” though.

So why did the “if statement” actually run?

Surprise – it did not and could not have run. Just in case you are still reading (I hope you are – you are learning here) this is the break down of that “if statement” when we collapse it.

if ((err === signedParams) !== false)
if ((0 === 4) !== false)
if ((false) !== false)
if (false !== false)
if (false)

This shows that the “if statement” could not have actually executed. But we saw “failed: 5” in the console – so weird right? Hang on why did we not also see “failed: 4” as well? As both function calls are in the same “if statement” then surely both should have been shown.

Why are there no braces?

So we know that the “if statement” could not have executed but it appears to have executed, but if it did execute then why only one call to the “fail()” function?

Hopefully by now many of you are asking why we do not have any braces “{}” – this should be part of an “if statement” right? Actually no – they are optional. Here is our code again with braces brackets;

var err = 0;
var hashCtx = 1;
var clientRandom = 2;
var serverRandom = 3;
var signedParams = 4;
var hashOut = 5;
function fail(x) {
    Ti.API.info('failed: ' + x);
}
if (err !== 0) {
    fail(0);
}
if ((err === hashCtx) !== false) {
    fail(1);
}
if ((err === clientRandom) !== false) {
    fail(2);
}
if ((err === serverRandom) !== false) {
    fail(3);
}
if ((err === signedParams) !== false) {
    fail(4);
    fail(5);
}
if ((err === hashOut) !== false) {
    fail(6);
}

If you run this code you will see that nothing appears in the console – but this is the same logic – right? No.

The gotcha that you can avoid

When you have braces for your “if statements” you are saying execute any code inside when this condition is true. However if you do not include braces it actually says execute the next statement.

So if we do not use the braces then only the next statement is part of that “if statement” so in our case “fail(4);” is the next statement, whereas “fail(5);” is not part of the “if statement”. So if we add the braces into the code as they are actually interpreted under these rules they look like this;

var err = 0;
var hashCtx = 1;
var clientRandom = 2;
var serverRandom = 3;
var signedParams = 4;
var hashOut = 5;
function fail(x) {
    Ti.API.info('failed: ' + x);
}
if (err !== 0) {
    fail(0);
}
if ((err === hashCtx) !== false) {
    fail(1);
}
if ((err === clientRandom) !== false) {
    fail(2);
}
if ((err === serverRandom) !== false) {
    fail(3);
}
if ((err === signedParams) !== false) {
    fail(4);
}
fail(5);
if ((err === hashOut) !== false) {
    fail(6);
}

You should now notice that “fail(5);” is not part of the “if statement” so it actually runs every time this code block is executed as none of the “if statements” actually control that line from being executed.

The moral of the story

If you are a beginner then you should always use braces, if you are a seasoned developer then you should always use braces.

“But I am a seasoned developer I know what I am doing”. Great, you are seasoned developer – then you probably work as part of a team, or work with code that more than one developer works on. This is what happened with Apple; very smart people made a very basic mistake. The Apple example appears (but not confirmed) to be a code merge issue, this can happen to anyone including you.

So if you are a seasoned developer – then you should know it is unsafe, so unless you can guarantee this could NEVER happen to you then – play it safe. But of course there will always be some who say “I know better” – and you might do – just make sure you are always perfect :-)

If you are a beginner then use braces – better to be safe than sorry.

So why did Apple have to publish a security update?

This code was designed to check if an SSL certificate was valid or not. If the code did not have the extra “goto fail” then it would have made it down to the section that actually checked the validity of the SSL certificate. So as it did not perform that vital check which would have altered the value of “err” from zero then all certificates were deemed valid – even when they were not. So in short you are not safe unless you perform the updates.

Further reading

If you have read this far then you must be interested in being a better developer, here are a couple of places you can learn a few more tricks that will improve your skills.

Owner of Core 13 Ltd a UK based mobile app development agency. Malcolm is affectionately known by Appcelerator and the big wide world as "The Oracle". With 25 years commercial software development experience from desktop apps to web sites for single users to large enterprises.\n You may have seen some of the many contributions provided within the Titanium Q&A.


Comments

  • Richard Lustemberg

    One more reason to love curly braces ;)

  • Bryan

    This applies to ALL blocks. Never, ever, ever omit the braces. On our team, we have sanity filters on git that will fail the push if the code is missing braces.

    There is no upside to skipping the braces and plenty of downsides:

    – The code is more error-prone
    – The code is harder to read and understand (especially by others or yourself 6 months later)
    – The code is harder to extend. If you need to add a line to the block, you have to remember to add the braces also.