4 ms·

I just can't understand why such critical components as OpenSSL just don't use Code Coverage tools like Coverity to find such things as this? Testing, coverage

I just can't understand why such critical components as OpenSSL just don't use Code Coverage tools like Coverity to find such things as this? Testing, coverage certification, static analysis: this would have been caught if these tools were being used.

peeters 13y ago
It would shock me if those tools would actually have caught this bug. That would imply that no benevolent contributor or researcher had run them against OpenSSL in the last two years, which I find incredibly hard to believe, as static analysis is a logical first step in researching a code for vulnerabilities, and OpenSSL is probably in the top 5 most valuable pieces of software to validate.
- fit2rule 13y ago
  I just ran Coverity on code that is similar to Heartbleed's bug, and it caught it. So, I think this is really just a glaring omission on a lot of peoples' part - but like you I also find it hard to believe nobody ran this code through static analysis. Its mind-boggling.
  - ajanuary 13y ago
    I don't know how Coverity works - is it possible that surrounding it with more complex code will cause it to miss the bug because it doesn't have enough information to not constantly raise false positives?
    - fournm 13y ago
      I'm not familiar with Coverity, but Sonar (for the Java world) is very similar in concept and will fail to detect things if there are enough issues before it in a file... Which I would HOPE is not the case here.
      - ajanuary 13y ago
        From my experience with static analysis tools it's very easy to write perfectly valid code that the tool doesn't like, so it wouldn't surprise me too much. Of course, the argument can often be made that if it isn't clear enough for the tool to find, it's not clear enough for a person to understand quickly.
        phazmatis 13y ago
        So you refactor until the noise goes away. Why throw out the signal with the noise? Modern software engineering theory 101.
        ajanuary 13y ago
        I absolutely agree it would be beneficial for static analysis tools to regularly be run on openssl. My stance (which I made clearer elsewhere in the thread) is more along the lines of: if it's not being done by the core maintainers, but just by concerned third parties, it's very easy to lose the signal in noise you don't have the ability to refactor away (because of time, difficulty getting it merged upstream etc.) So I'm not surprised that given the context it was missed by people running static analysis over it. That context is wrong, and it should have changed a long time ago, but under that context I can see it getting missed [1]. [1] By interpreting static analysis results, not necessarily by the code author and reviewer.
    - notacoward 13y ago
      To put it briefly: no. A little less briefly, then. ;) It is absolutely possible to create code that these tools in general, and Coverity in particular, will have difficulty analyzing . . . but you really have to work at it. Seriously, these guys are good. It's a bit like the "arms race" in building vs. breaking crypto. These guys have been there, they've seen all the moves, they know all the countermoves. Sure, if you load up your code with runtime-assigned function pointers and code that only executes if the last five iterations of a loop each went through specific code paths themselves, then that's going to cause some problems, but most programmers are unlikely to "win" that battle. However, this particular bug looks like it's in the absolute easiest category. Any static analyzer should have caught it. As others have pointed out, the real problem is false positives. If it was caught, but the report was buried in hundreds or thousands of crappy reports about things that actually aren't problems, then it might as well not have been caught. That's why the pros at this spend as much time writing code to eliminate false positives as they do writing code to find new things. In every project I've worked on that used static analysis, the weak link in the chain has been between reporting and remedy, not in the analysis itself.
- ajanuary 13y ago
  Someone commented (I can't find it now) that post disclosure they ran 5 or 6 coverage tools and one of them flagged it. They didn't say how much else got flagged, and how many false positives there were. It's very easy to retrospectively look through the list and say "oh yeah, there it is, amongst this big pile of nonsense".
svaha1728 13y ago
Does someone know the actual line of source where the error appears? The goto fail vulnerability was easy to catch with static analysis, but I haven't seen the actual lines of code for this one yet.
ThursdayNext 13y ago
This thread here: http://openssl.6102.n7.nabble.com/Coverity-coverage-of-OpenSSL-td42651.html http://openssl.6102.n7.nabble.com/Coverity-coverage-of-OpenS... mentions that one OpenSSL developer used to see defect reports from Coverity (probably through Coverity's scan project). He states: "Coverity used to, and perhaps still do, run scans of OpenSSL, which we had (have?) access to. I used to look at them and fix relevant ones, but got irritated with the false positive level in the end. If Coverity were interested in fixing their bugs, I might get interested in looking at their reports again." Of course this doesn't demonstrate that Coverity found this particular problem, and since he doesn't state what the false positive rate was it's difficult to know how reasonable his statement is.
- shagie 13y ago
  According to Andy Chou of Coverity on Twitter https://twitter.com/_achou/status/454287263917043712 https://twitter.com/_achou/status/454287263917043712 - "We looked at the code and it turns out the bug is caused by something we don't currently detect." and then goes on to say "But we're going to look into it further. In cases like this it is not only about finding the defect it's also about not flagging hundreds/thousands of false positives at the same time" So no, Coverity wouldn't find it.
wglb 13y ago
The problem with most static analysis processes is the typically vast number of false positives.

peeters 13y ago

It would shock me if those tools would actually have caught this bug. That would imply that no benevolent contributor or researcher had run them against OpenSSL in the last two years, which I find incredibly hard to believe, as static analysis is a logical first step in researching a code for vulnerabilities, and OpenSSL is probably in the top 5 most valuable pieces of software to validate.

fit2rule 13y ago
I just ran Coverity on code that is similar to Heartbleed's bug, and it caught it. So, I think this is really just a glaring omission on a lot of peoples' part - but like you I also find it hard to believe nobody ran this code through static analysis. Its mind-boggling.
- ajanuary 13y ago
  I don't know how Coverity works - is it possible that surrounding it with more complex code will cause it to miss the bug because it doesn't have enough information to not constantly raise false positives?
  - fournm 13y ago
    I'm not familiar with Coverity, but Sonar (for the Java world) is very similar in concept and will fail to detect things if there are enough issues before it in a file... Which I would HOPE is not the case here.
    - ajanuary 13y ago
      From my experience with static analysis tools it's very easy to write perfectly valid code that the tool doesn't like, so it wouldn't surprise me too much. Of course, the argument can often be made that if it isn't clear enough for the tool to find, it's not clear enough for a person to understand quickly.
      - phazmatis 13y ago
        So you refactor until the noise goes away. Why throw out the signal with the noise? Modern software engineering theory 101.
        ajanuary 13y ago
        I absolutely agree it would be beneficial for static analysis tools to regularly be run on openssl. My stance (which I made clearer elsewhere in the thread) is more along the lines of: if it's not being done by the core maintainers, but just by concerned third parties, it's very easy to lose the signal in noise you don't have the ability to refactor away (because of time, difficulty getting it merged upstream etc.) So I'm not surprised that given the context it was missed by people running static analysis over it. That context is wrong, and it should have changed a long time ago, but under that context I can see it getting missed [1]. [1] By interpreting static analysis results, not necessarily by the code author and reviewer.
  - notacoward 13y ago
    To put it briefly: no. A little less briefly, then. ;) It is absolutely possible to create code that these tools in general, and Coverity in particular, will have difficulty analyzing . . . but you really have to work at it. Seriously, these guys are good. It's a bit like the "arms race" in building vs. breaking crypto. These guys have been there, they've seen all the moves, they know all the countermoves. Sure, if you load up your code with runtime-assigned function pointers and code that only executes if the last five iterations of a loop each went through specific code paths themselves, then that's going to cause some problems, but most programmers are unlikely to "win" that battle. However, this particular bug looks like it's in the absolute easiest category. Any static analyzer should have caught it. As others have pointed out, the real problem is false positives. If it was caught, but the report was buried in hundreds or thousands of crappy reports about things that actually aren't problems, then it might as well not have been caught. That's why the pros at this spend as much time writing code to eliminate false positives as they do writing code to find new things. In every project I've worked on that used static analysis, the weak link in the chain has been between reporting and remedy, not in the analysis itself.
ajanuary 13y ago
Someone commented (I can't find it now) that post disclosure they ran 5 or 6 coverage tools and one of them flagged it. They didn't say how much else got flagged, and how many false positives there were. It's very easy to retrospectively look through the list and say "oh yeah, there it is, amongst this big pile of nonsense".

svaha1728 13y ago

Does someone know the actual line of source where the error appears? The goto fail vulnerability was easy to catch with static analysis, but I haven't seen the actual lines of code for this one yet.

ThursdayNext 13y ago

This thread here: http://openssl.6102.n7.nabble.com/Coverity-coverage-of-OpenSSL-td42651.html http://openssl.6102.n7.nabble.com/Coverity-coverage-of-OpenS... mentions that one OpenSSL developer used to see defect reports from Coverity (probably through Coverity's scan project). He states: "Coverity used to, and perhaps still do, run scans of OpenSSL, which we had (have?) access to. I used to look at them and fix relevant ones, but got irritated with the false positive level in the end. If Coverity were interested in fixing their bugs, I might get interested in looking at their reports again." Of course this doesn't demonstrate that Coverity found this particular problem, and since he doesn't state what the false positive rate was it's difficult to know how reasonable his statement is.

shagie 13y ago
According to Andy Chou of Coverity on Twitter https://twitter.com/_achou/status/454287263917043712 https://twitter.com/_achou/status/454287263917043712 - "We looked at the code and it turns out the bug is caused by something we don't currently detect." and then goes on to say "But we're going to look into it further. In cases like this it is not only about finding the defect it's also about not flagging hundreds/thousands of false positives at the same time" So no, Coverity wouldn't find it.

wglb 13y ago

The problem with most static analysis processes is the typically vast number of false positives.