12 ms·

A brief history of one line fixes

264 points by coconutrandom 13y ago

claudius 13y ago
Regarding the Debian OpenSSL disaster, the question was brought up on the OpenSSL mailing list (and read there); regarding the Tarsnap bug, it is not an unbraced if but a non-incremented nonce. If two of the five comments in an article are obviously wrong, how much trust should I possibly put into the remaining 60%?
- tokenrove 13y ago
  It's sarcasm. You've missed the rhetorical point of the article. (To be more clear, each comment mimics the comments made about the "goto fail" bug, and the crucial point is not pure mockery or a defence of Apple, but rather the final paragraph.)
  - thezilch 13y ago
    The point is that because there are always mistakes in everything, that the NSA is never involved? Strange, because I'm sure they've been implicated more than zero times. NOTE: I have no opinion on their involvement in the goto fail;
    - adamnemecek 13y ago
      > The point is that because there are always mistakes in everything, that the NSA is never involved? I'm pretty sure he's being sarcastic.
    - mpyne 13y ago
      The logical negation of "there are rarely these easy mistakes in something, so the NSA must always be involved", is not "because there are always mistakes in everything, that the NSA is never involved".
  - TwoBit 13y ago
    Probably a lot of people missed this point of the article. It certainly seemed odd to me.
  - claudius 13y ago
    Oh. Okay. Sorry.
  - chengiz 13y ago
    If there is sarcasm or rhetorical point to the article, it is not made clear at all. This is just poor writing.
    - PhantomGremlin 13y ago
      I agree with you, poor writing. You were being voted down earlier, but not any more. So there are others who agree with you.
mpetrov 13y ago
The commentary on last example from Tarsnap seems wrong. The error was that the nonce stopped being incremented (see the linked Tarsnap blog post), but the author suggests the issue there is the unbraced if. The unbraced if is still not great style, but it wasn't the cause of the security blunder.
- wging 13y ago
  The commentary does fit the recent Apple 'goto fail' bug. I think this post may've originally included it. Perhaps the wrong commentary got deleted.
- clarry 13y ago
  It is kind of on purpose, that's the whole point of the writeup. It's taking a little jab at everyone who proclaims a "simple and obvious" way any given error could've been avoided. Fact is, making the programmer type more (braces or other stuff) doesn't save him from himself; the goto fail error could've been made with braces just as well as without. The other examples are make the same point. There are ways to help catch specific bugs. But there is no magic "fix it" button. Consider static analysis for example: you actually have to run it and inspect the output. You have to interpret it right. You have to fix it right. And so on. Human error can ruin each step, as has happened.
- mst 13y ago
  Yes. That was the joke.
adamlj 13y ago
"This is an important lesson to learn: Mistakes can happen any time a piece of code is modified." From http://www.daemonology.net/blog/2011-01-18-tarsnap-critical-security-bug.html http://www.daemonology.net/blog/2011-01-18-tarsnap-critical-... regarding the Tarsnap bug. Solution: write more unit tests.
- peterhunt 13y ago
  Or use better abstractions / static analysis.
jodrellblank 13y ago
Another thing they have in common is trying to reason about security of SSL certificate chains and other high abstraction concepts in terms of C pointers and function return values.
fjarlq 13y ago
A favorite of mine, in C: ASSERT(apples = 1); VERIFY(oranges = 2); (Note the '=' means assignment, not comparison.) Both are assertion-testers resulting in a core dump if the test fails, but ASSERT is only defined for DEBUG builds. Since the assignments are to nonzero (true) values, buggy values of 'apples' are silently corrected -- but only during test runs. Buggy values of 'oranges' are always silently corrected. Hilarity ensued. Afterwards, 'gcc -Wparentheses' became mandatory.
- fooyc 13y ago
  One way to avoid this kind of mistakes is to use Yoda conditions [0]: ASSERTS(1 == apples); If you forget one `=`, it doesn't compile. [0]: https://en.wikipedia.org/wiki/Yoda_conditions https://en.wikipedia.org/wiki/Yoda_conditions
  - deletes 13y ago
    I don't think learning this is worth it. Using this will not always warn you( complicated expression or variables ) and you still might miss something. +EDIT: In those occasions when you actually compare to a integer, the expression is so simple that you immediately understand the whole line. In case of an actual typo the compiler will warn you, and the inversion is not needed. Nowadays, there is simply no excuse for a ignored or missed( bad compiler ) warning. Assignment in an If statement must produce a warning!
    - hibbelig 13y ago
      Don't understand? If the constant is on the left, then forgetting one of the equals signs will always be an error.
      - rguldener 13y ago
        Yes but I think he is saying that if the left/what you compare to is not a constant then it will not warn you (if the type agrees, as it should anyway for a comparison).
chimeracoder 13y ago
> What do all these earlier mistakes have in common, apart from the obvious: being exemplars of “catastrophic loss of structural integrity”? They all date from before 2013. That’s how we know the NSA wasn’t involved. I get that this is trying to make fun of the response to Apple's "goto fail;", but this logic ("These similar errors predate 2013 → Apple's similar error was not an NSA backdoor") seems rather faulty. There are a number of flaws with this line of reasoning. To name a few: * The NSA could have been involved with backdoors before 2013 (unlikely in Debian, but mentioning 2013 is a bit of a red herring) * Apple could have been encouraged to insert a backdoor and did so in a way that gave them plausible deniability (either because the NSA specified that, or because they wanted to cover their behinds) Whether or not this incident was the result of the NSA's prompting is something we'll probably never know[0], but this article doesn't do much to argue one way or the other. [0] The only way we could know is if someone literally came out and admitted it (or someone like Snowden were to leak it). It's possible to prove the existence of something (ie, a backdoor attempt), but impossible to prove the absence of something.
- lstamour 13y ago
  My takeaway was that the NSA could have been involved in all the above, we just weren't paying as much attention before ;-) Of course, whether the NSA was involved or not isn't the point of this article. The point is that high severity one-liner bugs have been made before. I feel partly to blame. Until my comment on HackerNews revealing where the source code was, no one seemed willing to post more details about the bug (based on Twitter posts). But I think it will end well, as more people will pay attention to the code in future. Sadly, Apple has yet to share revised source code for the 10.9.2-shipping security library that I'm aware of.
- yeukhon 13y ago
  It's pointless to discuss whether this is an intentional flaw or a mistake because the problem lies in code review and tool. As someone has said in the past to me that compiler can catch these errors, well, if so, what flags do they use and should we have a standard list of flags to enable to test these problems? Do we have tools to show these errors instead of going through a 1000 pages compiled log?
mindslight 13y ago
Cough, there's one major thing all of these bugs have in common - they're all in C code. This is mostly due to the volume and prominence of C. But a language with the same fundamental semantics of C but lacking the cleverbait syntax and weakened types would have prevented over half of these mistakes.
- dror 13y ago
  And C has this thing called lint that's only been around since 1979 that can detect many of these issues.
  - mindslight 13y ago
    Yes, many tools have been built to address the problems with C, and one is remiss if they ignore the greater development ecosystem. But, that they're not integrated into the language itself means those tools can only ever make suggestions in a separate context, and so cannot function cohesively to permanently rule out classes of bugs in a way that can simply be taken for granted. In fact, the Debian OpenSSL bug was actually caused [1] by a message from one of these tools being uncritically acted upon. [1] the job of failure analysis is to find all contributing factors, not just pin everything on one.
    - stusmall 13y ago
      Static code analysis tools exist in many languages even very high level, safe ones. A lot of them are even named after lint or as a tip of the hat to it. Their presence isn't a sign of flaws of the language. They are signs of flaws in programmers. They give the designers the ability to extend the power of the compiler's warnings to help catch common mistakes and tune the automated feedback to fit the needs of the project.
      - mindslight 13y ago
        Sure, but static analysis can only add so much, as the programmer is still working in the base language and can't convey higher level concepts to the static analysis tool, so it's forced to backfit them by guessing intent. And to the extent you can make annotations for this, you're effectively working in a new formal language without the benefit of a compiler.
_pmf_ 13y ago
> How is this possible? Does nobody use a compiler that warns about comparisons always being false? External symbols are resolved at link time; as far as the compiler is concerned, setuid might be a symbol declared as __weak. I don't particularly like the snide remarks towards the maintainers of these libraries.
- GhotiFish 13y ago
  I suspect the author is parodying the reaction towards apples mistake, and bares these maintainers no ill will.
- spc476 13y ago
  I ran the following code through GCC: #include <stdio.h> #include <unistd.h> #include <sys/types.h> int main(void) { if (getuid() != 0 && geteuid == 0) { printf("only root\n"); return 1; } printf("okay\n"); return 0; } With just a plain GCC (no options), the code compiled and exhibited the bug (it always printed "okay"). That's to be expected. What was not expected what when I compiled the code with "gcc -ansi -Wall -Wextra -pedantic". NOT ONE ERROR OR WARNING! How is that happening? Well, C allows function pointers. Also, in C, a value of 0 used in a pointer context is treated as a NULL pointer. So that one line is seen as: if (getuid() != 0 && geteuid == NULL) Now, let's play compiler. geteuid() is a function and it's defined. So the compiler can generate: if (getuid() != 0 && false) The result of this expression can never be true. But because the call to getuid() could cause side-effects (the prototype for getuid() does not include the GCC extension marking it as a "pure" function) the compiler has to emit a call to the function, but otherwise, it ignores the result since it doesn't matter what the result is. The resultant code is: int main(void) { getuid(); printf("okay\n"); return 0; } And thus, it's possible, even with the commonly used "-Wall -Wextra" options to GCC. To detect this, you will also need to add "-Wunreachable-code" (why that isn't included with '-Wall' is a good question).
  - CUViper 13y ago
    > "gcc -ansi -Wall -Wextra -pedantic". NOT ONE ERROR OR WARNING! $ rpm -q gcc gcc-4.8.2-7.fc20.x86_64 $ gcc -ansi -Wall -Wextra -pedantic test.c test.c: In function ‘main’: test.c:7:36: warning: the comparison will always evaluate as ‘false’ for the address of ‘geteuid’ will never be NULL [-Waddress] if (getuid() != 0 && geteuid == 0) ^ If fact, just -Wall is enough to enable that warning. Maybe the coverage of -Waddress has changed since your version?
yeukhon 13y ago
I have done similar thing to myself in Python, sabotaging my own little project. The following is not a rare thing. Imagine this were initializing random key in TLS... But since Python syntax is arguably cleaner and more readable, it's easier to catch. But that's a bet we have to put up with... >>> def f(a,b=[]): ... b.append(a) ... return b ... >>> print f(1) [1] >>> print f(1) [1, 1] >>> print f(1) [1, 1, 1] >>> import random >>> def key(): ... return random.randint(1,10000) ... >>> def f2(a, b=key): ... return b() ... >>> def f3(a, b=key()): .... return b >>> f2(1) 3974 >>> f2(1) 8684 >>> f3(1) 2867 >>> f3(1) 2867
- GuiA 13y ago
  Well, any Python developer worth his salt knows that initializing default parameters that way is a big no-no. I actually use code with those exact mistakes in interviews, as it is a good way to tell Python beginners from more enlightened users.
  - raverbashing 13y ago
    Correct Unfortunately a lot of people still use it (and it's actually correct, IF you don't modify it) It's a risk not worth taking (to me, at least)
    - klibertp 13y ago
      That's the only way of creating function with state without introducing closure or making an object with __call__. It's very handy for accumulators and other bits of state which are inherent to the function you write. It's essentially a `static` variable declared inside a function in C and believe it or not it has it's uses. Depending on a problem it can be worthwhile to use other techniques of creating stateful callables, but that doesn't mean you should never create a function with mutable default argument. It's there in the language - learn about it, understand how it works and why it works like this, then understand where to use it and use it where it makes sense. That's a pretty generic advice, valid for almost any language feature.
rjzzleep 13y ago
i'm actually a little surprised about the openssl one. I always assumed it would be good practice to number error codes negatively. OTOH that's also kinda expected behavior for reverse engineers. if the return value is negative you can be almost certain it's an error code i guess whoever wrote that line thought the same.
matt__rose 13y ago
with the X bug, building X has always triggered tons of warnings, it's easy to see why they're overlooked http://keithp.com/blogs/xserver-warnings/ http://keithp.com/blogs/xserver-warnings/
IvyMike 13y ago
Maybe that will be my new way to measure the effectiveness of my test suite. "Imagine the NSA snuck a one line 'fix' into your software overnight. Do your tests quickly and accurately detect the problem and point to the code that is broken? If not, your unit tests are broken."
- ghayes 13y ago
  If, per se, the NSA has your codebase, surely they would be able to find something you haven't tested. Your test cases should be extensive, but it's impossible to make them exhaustive.
  - mpyne 13y ago
    And even if you do, now you have a large and probably-buggy test base.
- hughes 13y ago
  But what if they "fixed" your unit tests? Gotta test the tests!
Rygu 13y ago
1. Start writing tests. Tests will save your ass when you're merging a feature branch. Cause are you sure everything still behaves correctly after those ingenious refactorings? 2. Use an IDE and/or compiler that actually warns you about common human errors like unused variables, unreachable statements, and dumb mistakes like a case-block with silent fallthrough to the next case-block. 3. Acknowledge responsibility when writing security related code. Know that there's no room for mistakes.
chetanahuja 13y ago
To those learning C programming "by reading other's code", following is an example of what not to do (either the subtracted or the added line). [Now I wait for a deluge or replies about how it's "idiomatic" C. All I have to say to that is, s/ma// ] --- libc-a/memset.c +++ libc-b/memset.c @@ -1,6 +1,6 @@ void *memset(void *_p, unsigned v, unsigned count) { unsigned char *p = _p; - while(count-- > 0) *p++ = 0; + while(count-- > 0) *p++ = v; return _p; } [Edited later to use the code mode]
- EdwardDiego 13y ago
  What is particularly bad? How would you have done it differently? I've not done much C, so I can't see the issue, although is it perhaps the types? Would count be modified by side-effect in the calling context also?
  - chetanahuja 13y ago
    "Would count be modified by side-effect in the calling context also?" Questions like this and others (eg., does the pointer deref operation applies to incremented or non-incremented value of p?) that you have to answer before you can make sense of the code. The cognitive overload on the reader's brain is at least part of the reason that a bug like that went past code-review in memset function on a major platform. MEMSET !! Was the needless cleverness and terseness of the code worth it? P.S. The answer to your question is No. The variable count as used in the body of this function is local to this function (it's passed by value... so it's a copy of what the caller passed).
    - EdwardDiego 13y ago
      Cheers mate, ah, so unless the function declares that it takes a pointer then it's pass-by-val?
      - ggreer 13y ago
        Everything is pass by value in C, but pointers are values. Functions can't modify the value of a pointer, but they can modify whatever's in the memory pointed to by the pointer. To change the pointer itself, you'd have to pass a pointer to a pointer. Functions like asprintf do this. C++ added pass by reference (&), which can be a handy bit of syntactic sugar.
MBlume 13y ago
Lint tools exist. My number one take away from this is that we should all be using them more.
- azernik 13y ago
  And ignoring compiler warnings less.
  - girvo 13y ago
    I only recently learnt C, and -Wall -Wextra -Werror was drummed into me as being "the right way". It certainly does catch a lot of my idiotic mistakes.
    - azernik 13y ago
      Unfortunately, if you're using anyone else's code (especially if it's multi-platform code), -Werror is sometimes not feasible. For one, lots of third-party code is full of warning-level mistakes, but most of them are not errors, and fixing them up would be a job for a whole separate company. For another, sometimes warnings are there because the code generated by compiler macros isn't available to the compiler for the purpose of e.g. checking for unused variables/parameters or type range checking. For example, there are some Linux kernel macros that will ignore certain parameters on a lot of architectures; this will give unused-variable warnings on those architectures if the variable/param isn't used elsewhere. In other places, data types change between signed and unsigned based on architecture, meaning that an in-general-meaningful test for >= zero will give you a warning on platforms where the variable is unsigned. Long story short, the preprocessor, while super-useful, is not good for static checking.
    - bhaak 13y ago
      Although be aware that -Wall and -Wextra activate different warnings on different compilers and even on different versions of the same compiler. E.g. gcc added lots of warnings to them in the last versions. So your code might not compile anymore on newer compiler versions. This is not a problem as such but for example in combination with updates to your continuous integration environment, this might cause build fails.
jonalmeida 13y ago
I've come across a bug that was probably caused by a developer that held the Ctrl key while pressing the up arrow which caused the selected line to move above another. It luckily only took me a full day to find within the codebase..
nemothekid 13y ago
A lot of arguments center around "well, the compiler should have done X!" I don't write a lot of C code, but when I did, and even more relevant - when I compile packages, the compiler tends to spit out chapters worth of warnings that I just ignore. How accepted is compiling C code in practice with Werror? Could it be the compiler did warn about goto fail, and the OP's error, but it was 1 warning out of a million? Really interested in an answer from those who work in C shops.
- acomjean 13y ago
  In my past experience writing and compiling c and c++ on Radar software: The project was big and fair amount of legacy code, there were a fair amount of warnings in the c code. A lot of code was in Ada, which I grew to really appreciate, but for some things we needed code in c. I started work on a new from the ground up simulator in c++. The 12+ coders got used to the warning = errors (Werror). I think it made us better coders. We were able to do that because the project was from the ground up new. Most projects in the Radar field use a lot of previously tested legacy code that does issue warnings.
- Gracana 13y ago
  I write my code with -Wall, -Werror, -pedantic, and a standard specified. If my code does something questionable, I need to instruct the compiler that I really mean for it to do that. Why tolerate unsafe and ambiguous behavior?
  - bsder 13y ago
    One problem is that -Wall doesn't actually enable all warnings. And that's nuts.
    - Gracana 13y ago
      You can enable -Wextra as well, but there are still more warnings you have to enable manually. At that point though, you're looking at weird not-necessarily-bad and language-specific things.
- 13y ago
julie1 13y ago
openBSD credo: security bugs are just bugs. Mostly human, and avoidable but always present because people don't take the time to do it correctly. Quality, taking the time to make the things right, slowly, correctly, all that matters more than genius. Quality code at all level is the key to secure development. Every little things count. I know a few openBSD developers, and their song about doing the right things, taking our times to do it correctly - simpler, with quality because it is the shortest path to correctness thus speed (what is the use of an incorrect program: none). This song is appealing. The fact they are strangely psychopathic paranoid nerds is not ;) I think computer industry would be far better if we would slow down and focus on doing less software, but better built. Less functionalities maybe, but the one that maybe more powerful. The stable foundation for sustainable progress. And, if you like them -even though they are repulsing antipathic nerds- you can support them :)
- nayden 13y ago
  careful there. we're not all the same -- some are MUCH more paranoid than others and I am yet to meet an antipathetic developer. ;) But thanks to those who support what we do!
ybaumes 13y ago
In the example of the Apple bug, I've read it took eighteen months (!!) to discover it after it has been committed in the source code base. As a consequence, I would be interested in knowing, for each bug, the time it took to discover them. Also, I assume that once discovered, the time to fix it is usually negligible. Usually. And as long as you don't count the time for full deployment in production...
chris_wot 13y ago
Actually, with the Debian SSL "fix" that removed entropy, the dev who made the change did ask about it on IRC. Though a bad mistake, it's understandable how it happened.
dhugiaskmak 13y ago
"Never write your own security code, because you'll get it wrong. Leave it to the smart people."
tbmatuka 13y ago
This one may not be as bad as the ones in the article, but it still gave us a few weeks of work upgrading kernels on a couple of thousand production servers: http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=8176cced706b5e5d15887584150764894e94e02f http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.g...

264 points by coconutrandom 13y ago

Regarding the Debian OpenSSL disaster, the question was brought up on the OpenSSL mailing list (and read there); regarding the Tarsnap bug, it is not an unbraced if but a non-incremented nonce. If two of the five comments in an article are obviously wrong, how much trust should I possibly put into the remaining 60%?

tokenrove 13y ago
It's sarcasm. You've missed the rhetorical point of the article. (To be more clear, each comment mimics the comments made about the "goto fail" bug, and the crucial point is not pure mockery or a defence of Apple, but rather the final paragraph.)
- thezilch 13y ago
  The point is that because there are always mistakes in everything, that the NSA is never involved? Strange, because I'm sure they've been implicated more than zero times. NOTE: I have no opinion on their involvement in the goto fail;
  - adamnemecek 13y ago
    > The point is that because there are always mistakes in everything, that the NSA is never involved? I'm pretty sure he's being sarcastic.
  - mpyne 13y ago
    The logical negation of "there are rarely these easy mistakes in something, so the NSA must always be involved", is not "because there are always mistakes in everything, that the NSA is never involved".
- TwoBit 13y ago
  Probably a lot of people missed this point of the article. It certainly seemed odd to me.
- claudius 13y ago
  Oh. Okay. Sorry.
- chengiz 13y ago
  If there is sarcasm or rhetorical point to the article, it is not made clear at all. This is just poor writing.
  - PhantomGremlin 13y ago
    I agree with you, poor writing. You were being voted down earlier, but not any more. So there are others who agree with you.

mpetrov 13y ago

The commentary on last example from Tarsnap seems wrong. The error was that the nonce stopped being incremented (see the linked Tarsnap blog post), but the author suggests the issue there is the unbraced if. The unbraced if is still not great style, but it wasn't the cause of the security blunder.

wging 13y ago
The commentary does fit the recent Apple 'goto fail' bug. I think this post may've originally included it. Perhaps the wrong commentary got deleted.
clarry 13y ago
It is kind of on purpose, that's the whole point of the writeup. It's taking a little jab at everyone who proclaims a "simple and obvious" way any given error could've been avoided. Fact is, making the programmer type more (braces or other stuff) doesn't save him from himself; the goto fail error could've been made with braces just as well as without. The other examples are make the same point. There are ways to help catch specific bugs. But there is no magic "fix it" button. Consider static analysis for example: you actually have to run it and inspect the output. You have to interpret it right. You have to fix it right. And so on. Human error can ruin each step, as has happened.
mst 13y ago
Yes. That was the joke.

adamlj 13y ago

"This is an important lesson to learn: Mistakes can happen any time a piece of code is modified." From http://www.daemonology.net/blog/2011-01-18-tarsnap-critical-security-bug.html http://www.daemonology.net/blog/2011-01-18-tarsnap-critical-... regarding the Tarsnap bug. Solution: write more unit tests.

peterhunt 13y ago
Or use better abstractions / static analysis.

jodrellblank 13y ago

Another thing they have in common is trying to reason about security of SSL certificate chains and other high abstraction concepts in terms of C pointers and function return values.

fjarlq 13y ago

A favorite of mine, in C: ASSERT(apples = 1); VERIFY(oranges = 2); (Note the '=' means assignment, not comparison.) Both are assertion-testers resulting in a core dump if the test fails, but ASSERT is only defined for DEBUG builds. Since the assignments are to nonzero (true) values, buggy values of 'apples' are silently corrected -- but only during test runs. Buggy values of 'oranges' are always silently corrected. Hilarity ensued. Afterwards, 'gcc -Wparentheses' became mandatory.

fooyc 13y ago
One way to avoid this kind of mistakes is to use Yoda conditions [0]: ASSERTS(1 == apples); If you forget one `=`, it doesn't compile. [0]: https://en.wikipedia.org/wiki/Yoda_conditions https://en.wikipedia.org/wiki/Yoda_conditions
- deletes 13y ago
  I don't think learning this is worth it. Using this will not always warn you( complicated expression or variables ) and you still might miss something. +EDIT: In those occasions when you actually compare to a integer, the expression is so simple that you immediately understand the whole line. In case of an actual typo the compiler will warn you, and the inversion is not needed. Nowadays, there is simply no excuse for a ignored or missed( bad compiler ) warning. Assignment in an If statement must produce a warning!
  - hibbelig 13y ago
    Don't understand? If the constant is on the left, then forgetting one of the equals signs will always be an error.
    - rguldener 13y ago
      Yes but I think he is saying that if the left/what you compare to is not a constant then it will not warn you (if the type agrees, as it should anyway for a comparison).

chimeracoder 13y ago

> What do all these earlier mistakes have in common, apart from the obvious: being exemplars of “catastrophic loss of structural integrity”? They all date from before 2013. That’s how we know the NSA wasn’t involved. I get that this is trying to make fun of the response to Apple's "goto fail;", but this logic ("These similar errors predate 2013 → Apple's similar error was not an NSA backdoor") seems rather faulty. There are a number of flaws with this line of reasoning. To name a few: * The NSA could have been involved with backdoors before 2013 (unlikely in Debian, but mentioning 2013 is a bit of a red herring) * Apple could have been encouraged to insert a backdoor and did so in a way that gave them plausible deniability (either because the NSA specified that, or because they wanted to cover their behinds) Whether or not this incident was the result of the NSA's prompting is something we'll probably never know[0], but this article doesn't do much to argue one way or the other. [0] The only way we could know is if someone literally came out and admitted it (or someone like Snowden were to leak it). It's possible to prove the existence of something (ie, a backdoor attempt), but impossible to prove the absence of something.

lstamour 13y ago
My takeaway was that the NSA could have been involved in all the above, we just weren't paying as much attention before ;-) Of course, whether the NSA was involved or not isn't the point of this article. The point is that high severity one-liner bugs have been made before. I feel partly to blame. Until my comment on HackerNews revealing where the source code was, no one seemed willing to post more details about the bug (based on Twitter posts). But I think it will end well, as more people will pay attention to the code in future. Sadly, Apple has yet to share revised source code for the 10.9.2-shipping security library that I'm aware of.
yeukhon 13y ago
It's pointless to discuss whether this is an intentional flaw or a mistake because the problem lies in code review and tool. As someone has said in the past to me that compiler can catch these errors, well, if so, what flags do they use and should we have a standard list of flags to enable to test these problems? Do we have tools to show these errors instead of going through a 1000 pages compiled log?

mindslight 13y ago

Cough, there's one major thing all of these bugs have in common - they're all in C code. This is mostly due to the volume and prominence of C. But a language with the same fundamental semantics of C but lacking the cleverbait syntax and weakened types would have prevented over half of these mistakes.

dror 13y ago
And C has this thing called lint that's only been around since 1979 that can detect many of these issues.
- mindslight 13y ago
  Yes, many tools have been built to address the problems with C, and one is remiss if they ignore the greater development ecosystem. But, that they're not integrated into the language itself means those tools can only ever make suggestions in a separate context, and so cannot function cohesively to permanently rule out classes of bugs in a way that can simply be taken for granted. In fact, the Debian OpenSSL bug was actually caused [1] by a message from one of these tools being uncritically acted upon. [1] the job of failure analysis is to find all contributing factors, not just pin everything on one.
  - stusmall 13y ago
    Static code analysis tools exist in many languages even very high level, safe ones. A lot of them are even named after lint or as a tip of the hat to it. Their presence isn't a sign of flaws of the language. They are signs of flaws in programmers. They give the designers the ability to extend the power of the compiler's warnings to help catch common mistakes and tune the automated feedback to fit the needs of the project.
    - mindslight 13y ago
      Sure, but static analysis can only add so much, as the programmer is still working in the base language and can't convey higher level concepts to the static analysis tool, so it's forced to backfit them by guessing intent. And to the extent you can make annotations for this, you're effectively working in a new formal language without the benefit of a compiler.

_pmf_ 13y ago

> How is this possible? Does nobody use a compiler that warns about comparisons always being false? External symbols are resolved at link time; as far as the compiler is concerned, setuid might be a symbol declared as __weak. I don't particularly like the snide remarks towards the maintainers of these libraries.

GhotiFish 13y ago
I suspect the author is parodying the reaction towards apples mistake, and bares these maintainers no ill will.
spc476 13y ago
I ran the following code through GCC: #include <stdio.h> #include <unistd.h> #include <sys/types.h> int main(void) { if (getuid() != 0 && geteuid == 0) { printf("only root\n"); return 1; } printf("okay\n"); return 0; } With just a plain GCC (no options), the code compiled and exhibited the bug (it always printed "okay"). That's to be expected. What was not expected what when I compiled the code with "gcc -ansi -Wall -Wextra -pedantic". NOT ONE ERROR OR WARNING! How is that happening? Well, C allows function pointers. Also, in C, a value of 0 used in a pointer context is treated as a NULL pointer. So that one line is seen as: if (getuid() != 0 && geteuid == NULL) Now, let's play compiler. geteuid() is a function and it's defined. So the compiler can generate: if (getuid() != 0 && false) The result of this expression can never be true. But because the call to getuid() could cause side-effects (the prototype for getuid() does not include the GCC extension marking it as a "pure" function) the compiler has to emit a call to the function, but otherwise, it ignores the result since it doesn't matter what the result is. The resultant code is: int main(void) { getuid(); printf("okay\n"); return 0; } And thus, it's possible, even with the commonly used "-Wall -Wextra" options to GCC. To detect this, you will also need to add "-Wunreachable-code" (why that isn't included with '-Wall' is a good question).
- CUViper 13y ago
  > "gcc -ansi -Wall -Wextra -pedantic". NOT ONE ERROR OR WARNING! $ rpm -q gcc gcc-4.8.2-7.fc20.x86_64 $ gcc -ansi -Wall -Wextra -pedantic test.c test.c: In function ‘main’: test.c:7:36: warning: the comparison will always evaluate as ‘false’ for the address of ‘geteuid’ will never be NULL [-Waddress] if (getuid() != 0 && geteuid == 0) ^ If fact, just -Wall is enough to enable that warning. Maybe the coverage of -Waddress has changed since your version?

yeukhon 13y ago

I have done similar thing to myself in Python, sabotaging my own little project. The following is not a rare thing. Imagine this were initializing random key in TLS... But since Python syntax is arguably cleaner and more readable, it's easier to catch. But that's a bet we have to put up with... >>> def f(a,b=[]): ... b.append(a) ... return b ... >>> print f(1) [1] >>> print f(1) [1, 1] >>> print f(1) [1, 1, 1] >>> import random >>> def key(): ... return random.randint(1,10000) ... >>> def f2(a, b=key): ... return b() ... >>> def f3(a, b=key()): .... return b >>> f2(1) 3974 >>> f2(1) 8684 >>> f3(1) 2867 >>> f3(1) 2867

GuiA 13y ago
Well, any Python developer worth his salt knows that initializing default parameters that way is a big no-no. I actually use code with those exact mistakes in interviews, as it is a good way to tell Python beginners from more enlightened users.
- raverbashing 13y ago
  Correct Unfortunately a lot of people still use it (and it's actually correct, IF you don't modify it) It's a risk not worth taking (to me, at least)
  - klibertp 13y ago
    That's the only way of creating function with state without introducing closure or making an object with __call__. It's very handy for accumulators and other bits of state which are inherent to the function you write. It's essentially a `static` variable declared inside a function in C and believe it or not it has it's uses. Depending on a problem it can be worthwhile to use other techniques of creating stateful callables, but that doesn't mean you should never create a function with mutable default argument. It's there in the language - learn about it, understand how it works and why it works like this, then understand where to use it and use it where it makes sense. That's a pretty generic advice, valid for almost any language feature.

rjzzleep 13y ago

i'm actually a little surprised about the openssl one. I always assumed it would be good practice to number error codes negatively. OTOH that's also kinda expected behavior for reverse engineers. if the return value is negative you can be almost certain it's an error code i guess whoever wrote that line thought the same.

matt__rose 13y ago

with the X bug, building X has always triggered tons of warnings, it's easy to see why they're overlooked http://keithp.com/blogs/xserver-warnings/ http://keithp.com/blogs/xserver-warnings/

IvyMike 13y ago

Maybe that will be my new way to measure the effectiveness of my test suite. "Imagine the NSA snuck a one line 'fix' into your software overnight. Do your tests quickly and accurately detect the problem and point to the code that is broken? If not, your unit tests are broken."

ghayes 13y ago
If, per se, the NSA has your codebase, surely they would be able to find something you haven't tested. Your test cases should be extensive, but it's impossible to make them exhaustive.
- mpyne 13y ago
  And even if you do, now you have a large and probably-buggy test base.
hughes 13y ago
But what if they "fixed" your unit tests? Gotta test the tests!

Rygu 13y ago

1. Start writing tests. Tests will save your ass when you're merging a feature branch. Cause are you sure everything still behaves correctly after those ingenious refactorings? 2. Use an IDE and/or compiler that actually warns you about common human errors like unused variables, unreachable statements, and dumb mistakes like a case-block with silent fallthrough to the next case-block. 3. Acknowledge responsibility when writing security related code. Know that there's no room for mistakes.

chetanahuja 13y ago

To those learning C programming "by reading other's code", following is an example of what not to do (either the subtracted or the added line). [Now I wait for a deluge or replies about how it's "idiomatic" C. All I have to say to that is, s/ma// ] --- libc-a/memset.c +++ libc-b/memset.c @@ -1,6 +1,6 @@ void *memset(void *_p, unsigned v, unsigned count) { unsigned char *p = _p; - while(count-- > 0) *p++ = 0; + while(count-- > 0) *p++ = v; return _p; } [Edited later to use the code mode]

EdwardDiego 13y ago
What is particularly bad? How would you have done it differently? I've not done much C, so I can't see the issue, although is it perhaps the types? Would count be modified by side-effect in the calling context also?
- chetanahuja 13y ago
  "Would count be modified by side-effect in the calling context also?" Questions like this and others (eg., does the pointer deref operation applies to incremented or non-incremented value of p?) that you have to answer before you can make sense of the code. The cognitive overload on the reader's brain is at least part of the reason that a bug like that went past code-review in memset function on a major platform. MEMSET !! Was the needless cleverness and terseness of the code worth it? P.S. The answer to your question is No. The variable count as used in the body of this function is local to this function (it's passed by value... so it's a copy of what the caller passed).
  - EdwardDiego 13y ago
    Cheers mate, ah, so unless the function declares that it takes a pointer then it's pass-by-val?
    - ggreer 13y ago
      Everything is pass by value in C, but pointers are values. Functions can't modify the value of a pointer, but they can modify whatever's in the memory pointed to by the pointer. To change the pointer itself, you'd have to pass a pointer to a pointer. Functions like asprintf do this. C++ added pass by reference (&), which can be a handy bit of syntactic sugar.

MBlume 13y ago

Lint tools exist. My number one take away from this is that we should all be using them more.

azernik 13y ago
And ignoring compiler warnings less.
- girvo 13y ago
  I only recently learnt C, and -Wall -Wextra -Werror was drummed into me as being "the right way". It certainly does catch a lot of my idiotic mistakes.
  - azernik 13y ago
    Unfortunately, if you're using anyone else's code (especially if it's multi-platform code), -Werror is sometimes not feasible. For one, lots of third-party code is full of warning-level mistakes, but most of them are not errors, and fixing them up would be a job for a whole separate company. For another, sometimes warnings are there because the code generated by compiler macros isn't available to the compiler for the purpose of e.g. checking for unused variables/parameters or type range checking. For example, there are some Linux kernel macros that will ignore certain parameters on a lot of architectures; this will give unused-variable warnings on those architectures if the variable/param isn't used elsewhere. In other places, data types change between signed and unsigned based on architecture, meaning that an in-general-meaningful test for >= zero will give you a warning on platforms where the variable is unsigned. Long story short, the preprocessor, while super-useful, is not good for static checking.
  - bhaak 13y ago
    Although be aware that -Wall and -Wextra activate different warnings on different compilers and even on different versions of the same compiler. E.g. gcc added lots of warnings to them in the last versions. So your code might not compile anymore on newer compiler versions. This is not a problem as such but for example in combination with updates to your continuous integration environment, this might cause build fails.

jonalmeida 13y ago

I've come across a bug that was probably caused by a developer that held the Ctrl key while pressing the up arrow which caused the selected line to move above another. It luckily only took me a full day to find within the codebase..

nemothekid 13y ago

A lot of arguments center around "well, the compiler should have done X!" I don't write a lot of C code, but when I did, and even more relevant - when I compile packages, the compiler tends to spit out chapters worth of warnings that I just ignore. How accepted is compiling C code in practice with Werror? Could it be the compiler did warn about goto fail, and the OP's error, but it was 1 warning out of a million? Really interested in an answer from those who work in C shops.

acomjean 13y ago
In my past experience writing and compiling c and c++ on Radar software: The project was big and fair amount of legacy code, there were a fair amount of warnings in the c code. A lot of code was in Ada, which I grew to really appreciate, but for some things we needed code in c. I started work on a new from the ground up simulator in c++. The 12+ coders got used to the warning = errors (Werror). I think it made us better coders. We were able to do that because the project was from the ground up new. Most projects in the Radar field use a lot of previously tested legacy code that does issue warnings.
Gracana 13y ago
I write my code with -Wall, -Werror, -pedantic, and a standard specified. If my code does something questionable, I need to instruct the compiler that I really mean for it to do that. Why tolerate unsafe and ambiguous behavior?
- bsder 13y ago
  One problem is that -Wall doesn't actually enable all warnings. And that's nuts.
  - Gracana 13y ago
    You can enable -Wextra as well, but there are still more warnings you have to enable manually. At that point though, you're looking at weird not-necessarily-bad and language-specific things.
13y ago

julie1 13y ago

openBSD credo: security bugs are just bugs. Mostly human, and avoidable but always present because people don't take the time to do it correctly. Quality, taking the time to make the things right, slowly, correctly, all that matters more than genius. Quality code at all level is the key to secure development. Every little things count. I know a few openBSD developers, and their song about doing the right things, taking our times to do it correctly - simpler, with quality because it is the shortest path to correctness thus speed (what is the use of an incorrect program: none). This song is appealing. The fact they are strangely psychopathic paranoid nerds is not ;) I think computer industry would be far better if we would slow down and focus on doing less software, but better built. Less functionalities maybe, but the one that maybe more powerful. The stable foundation for sustainable progress. And, if you like them -even though they are repulsing antipathic nerds- you can support them :)

nayden 13y ago
careful there. we're not all the same -- some are MUCH more paranoid than others and I am yet to meet an antipathetic developer. ;) But thanks to those who support what we do!

ybaumes 13y ago

In the example of the Apple bug, I've read it took eighteen months (!!) to discover it after it has been committed in the source code base. As a consequence, I would be interested in knowing, for each bug, the time it took to discover them. Also, I assume that once discovered, the time to fix it is usually negligible. Usually. And as long as you don't count the time for full deployment in production...

chris_wot 13y ago

Actually, with the Debian SSL "fix" that removed entropy, the dev who made the change did ask about it on IRC. Though a bad mistake, it's understandable how it happened.

dhugiaskmak 13y ago

"Never write your own security code, because you'll get it wrong. Leave it to the smart people."

tbmatuka 13y ago

This one may not be as bad as the ones in the article, but it still gave us a few weeks of work upgrading kernels on a couple of thousand production servers: http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=8176cced706b5e5d15887584150764894e94e02f http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.g...