6 ms·

The Apple goto fail vulnerability: lessons learned (2014)

102 points by MrXOR 6y ago

netsharc 6y ago
One of the ideas to make this error visible is to forbid misleading indentation (section 3.5). It would be cool if the IDE can show this goof straight away..
- deleted 6y ago
  [deleted]
- comex 6y ago
  These days, both GCC and Clang have a compiler warning about it: https://developers.redhat.com/blog/2016/02/26/gcc-6-wmisleading-indentation-vs-goto-fail/ https://developers.redhat.com/blog/2016/02/26/gcc-6-wmislead... However, for some reason, while GCC puts it under -Wall, Clang (which Apple uses) doesn't enable it even with -Wall -Wextra; you have to manually add -Wmisleading-indentation. It's also not enabled by the default Xcode template.
- Forge36 6y ago
  Code Formating tools can help avoid this too.
  - nitrogen 6y ago
    At some point we should juat switch to editing abstract syntax trees instead of raw text. Then formatting is irrelevant, the structure is always clear, and each developer can choose their own way to view the code.
    - madeofpalk 6y ago
      Well, when you edit with and require a code formatting tool as a part of your development lifecycle, you're basically there.
      - nitrogen 6y ago
        True, though you'd have to run the formatting tool with different rules on both checkout and commit for developers to be able to use their own preferred formats. Also keeping the syntax tree would make non-textual displays and visualizations easier.
    - ByteJockey 6y ago
      That's basically the idea behind lisp (until you get into macros).
bobbylarrybobby 6y ago
For as long as I live, I’ll never understand style guides that permit omitting brackets around a single line following an if statement (or for, while, etc), nor code formatters that dont automatically insert them.
- lucasyvas 6y ago
  I agree - it should be a compile or runtime error.
  - umvi 6y ago
    Well if you compile with `-Werror -Wall` (and you should), it would throw in error in GCC 6+: https://developers.redhat.com/blog/2016/02/26/gcc-6-wmisleading-indentation-vs-goto-fail/ https://developers.redhat.com/blog/2016/02/26/gcc-6-wmislead...
- jimbob45 6y ago
  It can make code bloated and harder to follow. TBH it would be nice if someone made C with indent scoping instead of block scoping but I don’t think that’s truly practical with the preprocessor.
  - efwfwef 6y ago
    harder to follow? In what sense? Because in my book the fact that you can write both the line with braces and without make it much harder to think about, whereas in most other languages I don't need to think about it because there's only one way to write that.
  - int_19h 6y ago
    What's the practical difference between: if (foo) bar; else baz; and: if (foo) { bar; } else { baz; } The latter has one extra line, which is hardly bloat. It also has the braces, of course, but the upside of this style is that it's clear where the compound statement begins (it's the line that doesn't start with a brace!), and where it continues.
CoolGuySteve 6y ago
Meh, I worked on C at Apple and that programming style was idiomatic there. There’s nothing malicious about it, it was almost certainly a diff tool or merge error that resulted in the duplicated line. One thing that was weird about our merges was that we always had way too many branches in flight for the current OS X, the last OS X, the next OS X beta, iOS, the next iOS, and the Windows iTunes stuff. Even though we had a small team inside our division dedicated to releases, it's almost guaranteed there will be merge issues when managing that many forks of the same SVN or git repo.
tester756 6y ago
>The problem with ‘goto’ is that it allows developers to create ‘flow’ in code that is unnatural... and that’s bad because developers should always strive to make code easy to read. Those aren't mutually exclusive
dwheeler 6y ago
Hi, I'm the author of the referenced article. Thanks for pointing to it! However, can you change HN thread to the article title, which is: "The Apple goto fail vulnerability: lessons learned"? I never used the term "backdoor" in the entire article, and I certainly never claimed that this was an intentional backdoor or that it looked just like a backdoor. I said, "The Apple goto fail vulnerability was a dangerous vulnerability that should have been found by Apple." - but I never said it was intentional. I personally doubt it was intentional (it's possible, but I have no specific evidence suggesting it). While I'm here... ask me anything (AMA)!
- gigatexal 6y ago
  Flagged in hopes the title gets changed
- pansa2 6y ago
  The OP has re-used the title of an unrelated article posted a few days ago, for some reason: https://news.ycombinator.com/item?id=25726068 https://news.ycombinator.com/item?id=25726068
- dang 6y ago
  Fixed now. Thanks! (Submitted title was "The Most Backdoor-Looking Bug I’ve Ever Seen: Apple's goto fail bug (2014)". Submitters: please don't do that—it's against the site guidelines, which ask: "Please use the original title, unless it is misleading or linkbait; don't editorialize." We eventually take submission privileges away for breaking that rule, so please follow it.)
  - dwheeler 6y ago
    Thanks! And while I'm here, THANK YOU for all the thankless work you do. MANY people, including me, appreciate it!
  - MrXOR 6y ago
    Excuse me.
Jerry2 6y ago
Title of this story is heavily editorialized. The article mentions nothing about the backdoor.
alisausaaaa 6y ago
Wanna have hot-lovin' conversations? You’re on the right way! - https://adultlove.life https://adultlove.life
seanwilson 6y ago
> Arie van Deursen argues that “code formatting is a security feature” and that indentation “white space is a security concern. The correct indentation immediately shows something fishy is going on...” [vanDeursen2014]. Arie van Deursen also argues that code formatting should be done by tools, not by hand, and shows that clearly this code was not routinely formatted automatically. Praise then for languages with significant white space? Using a formatter tool to add the white space and a lint rule in your compiler to catch when it's not done is a bandaid for something that should be encoded into the language in my opinion. Leaving this stuff optional to enforce only eats up productivity for no decent upside.
fmntf 6y ago
The code written in my company must follow the MISRA rules. Among the others, there is a rules that requires branches to have code blocks, and another one that prohibits the use of goto. Yes, compilers have flags, coding standards can achieve the same results. The point is that this stuff is non standard, not everybody uses GCC or Clang. Stating "this code is MISRA compliant" is stronger that "this code does not produce compile warnings with the flags x, y, z on compiler W version a.b.c".
- mungoLarry 6y ago
  But what tool verifies that the code is MISRA compliant? Then you’re just stating “my code is MISRA compliant according to tool W version a.b.c” ;)
viktorcode 6y ago
I'm afraid the only working solution to prevent this type of errors is to switch to a programming language that requires you to use braces around if statement body. Static analyser is an optional tool, with output that sometimes points out to perfectly correct code, thus making it more a nuisance then a helping tool in the eyes of a time-constrained programmer.

102 points by MrXOR 6y ago

One of the ideas to make this error visible is to forbid misleading indentation (section 3.5). It would be cool if the IDE can show this goof straight away..

deleted 6y ago
[deleted]
comex 6y ago
These days, both GCC and Clang have a compiler warning about it: https://developers.redhat.com/blog/2016/02/26/gcc-6-wmisleading-indentation-vs-goto-fail/ https://developers.redhat.com/blog/2016/02/26/gcc-6-wmislead... However, for some reason, while GCC puts it under -Wall, Clang (which Apple uses) doesn't enable it even with -Wall -Wextra; you have to manually add -Wmisleading-indentation. It's also not enabled by the default Xcode template.
Forge36 6y ago
Code Formating tools can help avoid this too.
- nitrogen 6y ago
  At some point we should juat switch to editing abstract syntax trees instead of raw text. Then formatting is irrelevant, the structure is always clear, and each developer can choose their own way to view the code.
  - madeofpalk 6y ago
    Well, when you edit with and require a code formatting tool as a part of your development lifecycle, you're basically there.
    - nitrogen 6y ago
      True, though you'd have to run the formatting tool with different rules on both checkout and commit for developers to be able to use their own preferred formats. Also keeping the syntax tree would make non-textual displays and visualizations easier.
  - ByteJockey 6y ago
    That's basically the idea behind lisp (until you get into macros).

bobbylarrybobby 6y ago

For as long as I live, I’ll never understand style guides that permit omitting brackets around a single line following an if statement (or for, while, etc), nor code formatters that dont automatically insert them.

lucasyvas 6y ago
I agree - it should be a compile or runtime error.
- umvi 6y ago
  Well if you compile with `-Werror -Wall` (and you should), it would throw in error in GCC 6+: https://developers.redhat.com/blog/2016/02/26/gcc-6-wmisleading-indentation-vs-goto-fail/ https://developers.redhat.com/blog/2016/02/26/gcc-6-wmislead...
jimbob45 6y ago
It can make code bloated and harder to follow. TBH it would be nice if someone made C with indent scoping instead of block scoping but I don’t think that’s truly practical with the preprocessor.
- efwfwef 6y ago
  harder to follow? In what sense? Because in my book the fact that you can write both the line with braces and without make it much harder to think about, whereas in most other languages I don't need to think about it because there's only one way to write that.
- int_19h 6y ago
  What's the practical difference between: if (foo) bar; else baz; and: if (foo) { bar; } else { baz; } The latter has one extra line, which is hardly bloat. It also has the braces, of course, but the upside of this style is that it's clear where the compound statement begins (it's the line that doesn't start with a brace!), and where it continues.

CoolGuySteve 6y ago

Meh, I worked on C at Apple and that programming style was idiomatic there. There’s nothing malicious about it, it was almost certainly a diff tool or merge error that resulted in the duplicated line. One thing that was weird about our merges was that we always had way too many branches in flight for the current OS X, the last OS X, the next OS X beta, iOS, the next iOS, and the Windows iTunes stuff. Even though we had a small team inside our division dedicated to releases, it's almost guaranteed there will be merge issues when managing that many forks of the same SVN or git repo.

tester756 6y ago

>The problem with ‘goto’ is that it allows developers to create ‘flow’ in code that is unnatural... and that’s bad because developers should always strive to make code easy to read. Those aren't mutually exclusive

dwheeler 6y ago

Hi, I'm the author of the referenced article. Thanks for pointing to it! However, can you change HN thread to the article title, which is: "The Apple goto fail vulnerability: lessons learned"? I never used the term "backdoor" in the entire article, and I certainly never claimed that this was an intentional backdoor or that it looked just like a backdoor. I said, "The Apple goto fail vulnerability was a dangerous vulnerability that should have been found by Apple." - but I never said it was intentional. I personally doubt it was intentional (it's possible, but I have no specific evidence suggesting it). While I'm here... ask me anything (AMA)!

gigatexal 6y ago
Flagged in hopes the title gets changed
pansa2 6y ago
The OP has re-used the title of an unrelated article posted a few days ago, for some reason: https://news.ycombinator.com/item?id=25726068 https://news.ycombinator.com/item?id=25726068
dang 6y ago
Fixed now. Thanks! (Submitted title was "The Most Backdoor-Looking Bug I’ve Ever Seen: Apple's goto fail bug (2014)". Submitters: please don't do that—it's against the site guidelines, which ask: "Please use the original title, unless it is misleading or linkbait; don't editorialize." We eventually take submission privileges away for breaking that rule, so please follow it.)
- dwheeler 6y ago
  Thanks! And while I'm here, THANK YOU for all the thankless work you do. MANY people, including me, appreciate it!
- MrXOR 6y ago
  Excuse me.

Jerry2 6y ago

Title of this story is heavily editorialized. The article mentions nothing about the backdoor.

alisausaaaa 6y ago

Wanna have hot-lovin' conversations? You’re on the right way! - https://adultlove.life https://adultlove.life

seanwilson 6y ago

> Arie van Deursen argues that “code formatting is a security feature” and that indentation “white space is a security concern. The correct indentation immediately shows something fishy is going on...” [vanDeursen2014]. Arie van Deursen also argues that code formatting should be done by tools, not by hand, and shows that clearly this code was not routinely formatted automatically. Praise then for languages with significant white space? Using a formatter tool to add the white space and a lint rule in your compiler to catch when it's not done is a bandaid for something that should be encoded into the language in my opinion. Leaving this stuff optional to enforce only eats up productivity for no decent upside.

fmntf 6y ago

The code written in my company must follow the MISRA rules. Among the others, there is a rules that requires branches to have code blocks, and another one that prohibits the use of goto. Yes, compilers have flags, coding standards can achieve the same results. The point is that this stuff is non standard, not everybody uses GCC or Clang. Stating "this code is MISRA compliant" is stronger that "this code does not produce compile warnings with the flags x, y, z on compiler W version a.b.c".

mungoLarry 6y ago
But what tool verifies that the code is MISRA compliant? Then you’re just stating “my code is MISRA compliant according to tool W version a.b.c” ;)

viktorcode 6y ago

I'm afraid the only working solution to prevent this type of errors is to switch to a programming language that requires you to use braces around if statement body. Static analyser is an optional tool, with output that sometimes points out to perfectly correct code, thus making it more a nuisance then a helping tool in the eyes of a time-constrained programmer.