Tuesday, February 25, 2014

goto fail, do not pass go, do not collect your next paycheck

by now you've probably heard about the rather widely reported SSL bug that Apple quietly dropped on friday afternoon, teasing security researchers into finding out what was up. if not, the gist of it is that the C code Apple used for verifying signatures used in SSL had what appears to have been a copy&paste error that broke the security and allowed people to read your supposedly secure traffic. literally there were 2 lines that said "goto fail;" when there should only have been one. now i'm not about to make a big deal about copy&paste errors because that can legitimately happen to anyone, but i am going to make a big deal about the content of that copy&paste error.

the overall lack of acknowledgement (and in some cases denial) that the use of goto represents a deeper problem in Apple's application security is itself suggestive of a failure to recognize a fundamental principle: in software, quality is the foundation upon which security must stand.

goto is representative of the kind of spaghetti code we had before the introduction of structured programming approximately 50 years ago. no, that's not a typo. goto has been falling out of favour for about half a century and when i saw how much it was used in Apple's code it raised a red flag for me. every programmer i broached the subject with similarly found it concerning - including my boss who admitted he hasn't coded in C in over 30 years. he wondered, as do i, if the programmer responsible for the code in question still has a job at Apple.

you may think me rehashing a decades old debate, and perhaps i am - i wouldn't know, i never read any of that stuff - Edsger Dijkstra's letter "Go To Statement Considered Harmful" was published about 7 years before i was born. what i'm not doing, however, is mindlessly repeating dogma. we're interested today in application security and, as we have already covered, that requires software quality. structured programming produces software that is higher quality, easier to read, easier to model, easier to audit, easier to prove, etc. than unstructured programming.

why is this so? to answer that we need to think about what "structured programming" means. it is the nature of structure (all structure) to serve as a kind of constraint. your bones, for example, provide structure for your body and in so doing limit the way your body can move and bend. the support pillars for a bridge provide structure for that bridge and limit the extent to which the bridge can move and bend (yes, they bend and flex, but if the structure is doing it's job they only flex a little). likewise, code that follows the structured programming paradigm is constrained such that program control flows in a more limited number of ways. reducing the number of possibilities for program control flow makes it easier to predict (almost by definition) how a block of code's control will flow with just a quick glance. fewer possibilities mean it's easier to know what to expect. i'm sure you've seen the same effect with the written word. just like it's easier to read and understand sentences made with familiar words and phrases and following a few familiar construction patterns, the same is true for reading and understanding code as well. it's just another language, after all.

that reduction of possibilities also reduces the complexity of the code, which makes building a mental model of an arbitrary block of code easier. the constructs that make structured programming what it is lend themselves more naturally to abstraction since random lines of code within them are unlikely to cause program control to jump to random other places in the code. making it easier to build a mental model of the code makes it easier to formally prove the code's correctness because it's easier to describe the algorithm you're trying to prove. less formally, greater ease in building accurate mental models of the code means that it's easier to anticipate outcomes, especially unwanted ones that you want to eliminate, before they happen because it becomes easier to run through those possibilities in your head.

finally, both the greater ease of reading/understanding and the greater ease of modeling benefit efforts of others to review or audit the code. they're really only doing the same thing the programmer him/herself would have done by reading and understanding the code, creating a mental model of it, and trying to anticipate unwanted outcomes.

i work as a programmer professionally in a small company with not a lot of resources. we fly by the seat of our pants in some ways, but if someone asked me to review code that relied as much on goto as this source file from Apple, i wouldn't accept it. i'm surprised that code like that is able to survive for so long in a company with as many resources as Apple has. it makes me wonder about the programming culture within the company and it reminds me of a talk Jacob Appelbaum gave not too long ago where he accused them of writing shitty software. sure code reviews and more rigorous testing might have found the copy&paste error that sparked this off, but those processes don't add quality, they subtract problems. it's still a garbage-in/garbage-out sort of scenario so there's only so much they can do to affect the quality of Apple's software. quality has to go into those filters before you can get quality out.

i've often heard it said that regular programmers typically don't understand the nuances involved in writing secure code, especially when it comes to crypto, and having seen programmers more senior than myself flub crypto code i can certainly agree with that sentiment. that being said, i think it's probably also true that regular security people typically don't understand the nuances involved in writing quality code. since quality is a prerequisite for security, it's just as important for a programmer responsible for security-related code to have mastered the coding practices and techniques that lead to quality software as it is for them to understand secure software development.

i'll concede that it may well be possible for a master programmer to produce high quality, highly readable code that relies as heavily on goto as Apple's programmers appear to; but, as "The Tao Of Programming" satirically points out, you are not a master programmer, almost none of you are, so stop pretending and learn the lessons they've been trying to teach for the past 50 years.

(and now i'll go read Dijkstra's letter. maybe this is a rehash, but that wouldn't make it wrong even if it were)

(updated to fix the spelling of Jacob Appelbaum's name. thanks to Martijn Grooten)


Phil said...

This strikes me as an incredibly dogmatic post. There is nothing wrong with using "goto" in C code, as long as it's used properly. In this case, it looks like it was used for cleanup code, which is a fairly standard practice in C programming. The exact same error could have occurred if the author had written:

if ((err = foo()) != 0)
return err;
return err;

More or less the exact same error without the gotos. I only briefly skimmed the file, and in each case it looked like it was used to jump to common cleanup code. IMO this is much safer than the following:

if (err == ERROR1) {
return err;
/* Do stuff */
if (err == ERROR2) {
/* ... */
if (err == ERRORN) {
/* ... */

Gotos were not used to jump around mindlessly in a function; this wasn't spaghetti code by any stretch of the imagination. Yes, goto is mostly harmless, but it can be used safely, and in some cases it's the best option.

Phil said...

Err... that last line should be "mostly harmful" not "mostly harmless."

kurt wismer said...

"There is nothing wrong with using "goto" in C code, as long as it's used properly."

please refer to the final paragraph of the post. alternatively, i could rephrase this claim as "there's nothing wrong with goto as long as you use common sense" and then opine about how uncommon common sense is.

actually, refer to the discussion about the reduction in the possibility space for control flow. that is a complexity reduction, even when measured against code where goto is used sensibly. less complex code is easier to ensure is correct, while more complex code is easier to get wrong. as such i axiomatically reject the notion that there is "nothing" wrong with using goto.

as for the example of the same error happening with a double early return, yes that's true. not once in my post did i say that the use of goto was what caused the bug that Apple fixed. instead it was the bug that Apple fixed that highlighted a deeper (but not necessarily related) problem.

as for goto not being used mindlessly, look at the code again. specifically, look at the final goto in the problem function. do you see any code between that goto and the label it references? no, there is none, because goto WAS being used mindlessly.