Announcement

**dlevere** · 02-24-2014, 04:48:44 PM

I TOLD You Gotos Are Dangerous!

By Mike James

A recent security flaw in iOS is down to an error involving a spurious goto statement - but when you look a little more closely there is a bigger lesson to learn from the incident - and not just "goto considered harmful".

Apple relased an update to patch a cryptography flaw a few days ago but didn't say what it was patching.

Adam Langley, a web encryption expert at Google, did some digging and discovered that it was all about a single extra goto.

We all know that goto is harmful but surely there is more too it than this?

[img]/images/stories/News/2014/Feb/B/gotocartoon.gif[/img]
Click on this Joy of Tech cartoon to see the remaining nine frames..

Don't misunderstand me; the goto IS harmful, but because it is one of the clearest examples of unstructured coding - the opposite of structured coding.

A well structured program makes use of statement blocks that fit together like Lego and cannot be misused. OK, in reality that is "less likely to be misused".

Take a look at the code that caused the problem:

Code:

	if((err=SSLHashSHA1.update(&amp;hashCtx,
                      &amp;serverRandom)) != 0)
     goto fail;

	if ((err=SSLHashSHA1.update(&amp;hashCtx, 
                      &amp;signedParams)) != 0)

     goto fail;

     goto fail;

	if ((err=SSLHashSHA1.final(&amp;hashCtx, 
                      &amp;hashOut)) != 0)

		goto fail;

	[i]...[/i]

Code:

fail: deal with error

Do you see the silly mistake?

The second if statement is only associated with the first goto fail, despite the indentation (if only this was Python!).

The correct indentation makes the mistake perfectly clear:

Code:

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

The problem is compounded by the fact that err contains a successful value, even though it has been passed to the error handler to process, and this means the signature verification never fails.

How did this happen!

My guess is that the programmer was copy and pasting "goto fail" into the code and just pressed the keys twice.

What would have stopped this from happening?

You might at this point want to jump in with "ban the goto" - I know I would, but this isn't really the problem.

A simple auto-formatting editor would have drawn the problem to the attention of the programmer, even if it didn't protect against it.

Having the compiler flag dead code also would have detected the problem - neither GCC or Clang in Xcode flag dead code by default.

Insisting that curly braces are used to delineate code blocks, even if they are only one line long, would have also stopped the problem.

Perhaps the most wide reaching solution is to simply not use cascaded Ifs with goto used as an exit. The big problem is how to implement this in C/C++ as a structured statement. You can't use a switch because C/C++ only allows constant expressions in each test and hence you can't make the function calls. Not to mention the fact that the C/C++ switch has problems of its own with the need to include the break.

The best you can do is use nested ifs:

Code:

if((err=SSLHashSHA1.update(&amp;hashCtx,
                     &amp;serverRandom))!= 0){
  [i]call error function[/i]

Code:

else if ((err=SSLHashSHA1.update(&amp;hashCtx, 
                     &amp;signedParams)) != 0 {
 [i]call error function[/i]

Code:

} 
else if ((err=SSLHashSHA1.final(&amp;hashCtx,

Code:

&amp;hashOut)) != 0){

Code:

 [i]call error function[/i]

Code:

}
else {
 [i]process success[/i]
}

Is this as clear as it can be?

I think not.

The problem is that you want to call the error function just once if any one of the conditions is true and the success code only if all three are false.

There is also the small matter that the error function has to be a function and can't be part of the same scope as the success code - unless you use a goto of course.

So is there a better way in C/C++?

It makes you realize that when it comes to expressing conditional execution we are still primitive.

Source: joyof tech.com

Who would have thought a single extra goto could cause so much trouble!

More Information

Details Of The Bug

Related Articles

Edsger Dijkstra - The Poetry Of Programming

The Goto, Spaghetti and the Velociraptor

Android Security Hole More Stupid Error Than Defect

Announcement

Apple slays scary SSL snooping bug in iOS

Apple slays scary SSL snooping bug in iOS

Comment