4 ms·

There have been a bunch of major security vulnerabilities due to mistakes involving GOTO in C, being used as suggested by the article. Here’s a memorable one: h

by jitl 4y ago

There have been a bunch of major security vulnerabilities due to mistakes involving GOTO in C, being used as suggested by the article. Here’s a memorable one: https://www.imperialviolet.org/2014/02/22/applebug.html https://www.imperialviolet.org/2014/02/22/applebug.html

ouid 4y ago
I wondered then if this couldn't have been a merge error.
- olliej 4y ago
  That’s been my assumption as well.
RicardoLuis0 4y ago
that one in particular was not really caused by goto, but rather by braceless if statements, it'd be a vulnerability all the same if the line was a "fail" function that was called instead of a goto.
Swenrekcah 4y ago
I’d say this was more due to the case not being properly enclosed in brackets. Many people seem to really dislike using the brackets but this is what that gets you.
- kevin_thibedeau 4y ago
  This is detected by -Wmisleading-indentation now.
  - anthomtb 4y ago
    For the curious this flag has been available since GCC 6.1 (2016) and Clang 10 (2020). It is enabled by -Wall for both compilers. Interestingly (or perhaps coming full circle), the bug reference by the GGP comment is also called out in the GCC 6.1 release notes: > -Wmisleading-indentation warns about places where the indentation of the code gives a misleading idea of the block structure of the code to a human reader. For example, given CVE-2014-1266 sslKeyExchange.c: In function 'SSLVerifySignedServerKeyExchange': sslKeyExchange.c:629:3: warning: this 'if' clause does not guard... [- Wmisleading-indentation] if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0) ^~ sslKeyExchange.c:631:5: note: ...this statement, but the latter is misleadingly indented as if it is guarded by the 'if' goto fail; ^~~~
- arp242 4y ago
  Lack of code review and testing always seemed like the most pressing problems with that; both of which should have caught this error. Yes, it probably also should have had braces, but that seems like the lesser issue.
- Gibbon1 4y ago
  More do to the case that they obviously didn't have unit tests for that piece of code. Which is most frightening to me.
chclt 4y ago
This seems to be more an issue with if statements without curly braces.
camel-cdr 4y ago
if considered harmful
- eckza 4y ago
  Expressions that aren't required to return a value for all possible branches of execution that can also mutate state considered harmful.
bazoom42 4y ago
That error is not due to goto, it was just a goto which was errously executed because of badly formatted code. (It looked like the statement was inside an if-block due to the indent.) Pyhon would have prevented this bug, but so would a formatter. Rust also requires braces for if-blocks to prevent this kind of error.
- bitwize 4y ago
  "The problem isn't C, it's that they weren't using C properly..."
  - Karellen 4y ago
    That's not really a fair complaint against C, when many safer languages whose syntax derives from C (e.g. javascript) would have the same problem.
    - benj111 4y ago
      It's a fair complaint against C and JavaScript.
      - bazoom42 4y ago
        And Java, C#, C++ etc. But whitespace-sensitive languages dot not have the issue.
  - lolcatuser 4y ago
    But that is a legitimate problem. Using a language feature that is a known footgun (`if ...` instead of `if {...}`) without being cautious enough to avoid shooting yourself in the foot is not the fault of the footgun, it's the fault of the programmer. Additionally, in the above linked case, the problem isn't a misused `goto`, it's a misused `if ...`. It would be just as problematic if they typed `cleanup_context();` instead of `goto fail;`, but nobody complains about cleaning up state, do they?
    - 4y ago
olliej 4y ago
“Goto fail” would have been wrong in a non-goto language that use RAII. The problem was not goto, it was that the else path was also “give up”. In a non goto/RAII/defer language it would have been something like If (error) Return Return My assumption has been this was some kind of merge error rather being wrong off the bay. Interestingly mandatory indenting or mandatory braces might have stopped this, but then i would have thought -Werror with the dead code warnings would have as well :-/ But again the error was not the goto, and believing it was is the exact problem the article is talking about: people are so opposed to goto they are unable to see real issues (I have seen C code that tries to avoid goto completely and error handling code becomes horrific, far more complex, and far more error prone that just using goto). The problem with goto is that it is very easy to use it unnecessarily, in ways that complicated control flow but don’t actually make things better.

by jitl 4y ago

ouid 4y ago

I wondered then if this couldn't have been a merge error.

olliej 4y ago
That’s been my assumption as well.

RicardoLuis0 4y ago

that one in particular was not really caused by goto, but rather by braceless if statements, it'd be a vulnerability all the same if the line was a "fail" function that was called instead of a goto.

Swenrekcah 4y ago

I’d say this was more due to the case not being properly enclosed in brackets. Many people seem to really dislike using the brackets but this is what that gets you.

kevin_thibedeau 4y ago
This is detected by -Wmisleading-indentation now.
- anthomtb 4y ago
  For the curious this flag has been available since GCC 6.1 (2016) and Clang 10 (2020). It is enabled by -Wall for both compilers. Interestingly (or perhaps coming full circle), the bug reference by the GGP comment is also called out in the GCC 6.1 release notes: > -Wmisleading-indentation warns about places where the indentation of the code gives a misleading idea of the block structure of the code to a human reader. For example, given CVE-2014-1266 sslKeyExchange.c: In function 'SSLVerifySignedServerKeyExchange': sslKeyExchange.c:629:3: warning: this 'if' clause does not guard... [- Wmisleading-indentation] if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0) ^~ sslKeyExchange.c:631:5: note: ...this statement, but the latter is misleadingly indented as if it is guarded by the 'if' goto fail; ^~~~
arp242 4y ago
Lack of code review and testing always seemed like the most pressing problems with that; both of which should have caught this error. Yes, it probably also should have had braces, but that seems like the lesser issue.
Gibbon1 4y ago
More do to the case that they obviously didn't have unit tests for that piece of code. Which is most frightening to me.

chclt 4y ago

This seems to be more an issue with if statements without curly braces.

camel-cdr 4y ago

if considered harmful

eckza 4y ago
Expressions that aren't required to return a value for all possible branches of execution that can also mutate state considered harmful.

bazoom42 4y ago

That error is not due to goto, it was just a goto which was errously executed because of badly formatted code. (It looked like the statement was inside an if-block due to the indent.) Pyhon would have prevented this bug, but so would a formatter. Rust also requires braces for if-blocks to prevent this kind of error.

bitwize 4y ago
"The problem isn't C, it's that they weren't using C properly..."
- Karellen 4y ago
  That's not really a fair complaint against C, when many safer languages whose syntax derives from C (e.g. javascript) would have the same problem.
  - benj111 4y ago
    It's a fair complaint against C and JavaScript.
    - bazoom42 4y ago
      And Java, C#, C++ etc. But whitespace-sensitive languages dot not have the issue.
- lolcatuser 4y ago
  But that is a legitimate problem. Using a language feature that is a known footgun (`if ...` instead of `if {...}`) without being cautious enough to avoid shooting yourself in the foot is not the fault of the footgun, it's the fault of the programmer. Additionally, in the above linked case, the problem isn't a misused `goto`, it's a misused `if ...`. It would be just as problematic if they typed `cleanup_context();` instead of `goto fail;`, but nobody complains about cleaning up state, do they?
  - 4y ago

olliej 4y ago

“Goto fail” would have been wrong in a non-goto language that use RAII. The problem was not goto, it was that the else path was also “give up”. In a non goto/RAII/defer language it would have been something like If (error) Return Return My assumption has been this was some kind of merge error rather being wrong off the bay. Interestingly mandatory indenting or mandatory braces might have stopped this, but then i would have thought -Werror with the dead code warnings would have as well :-/ But again the error was not the goto, and believing it was is the exact problem the article is talking about: people are so opposed to goto they are unable to see real issues (I have seen C code that tries to avoid goto completely and error handling code becomes horrific, far more complex, and far more error prone that just using goto). The problem with goto is that it is very easy to use it unnecessarily, in ways that complicated control flow but don’t actually make things better.