Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

'Heartbleed' contributor denies he inserted it deliberately

smh.com.au

Post reply on HN
Print view View original

71–80 of 86 posts

Re: 'Heartbleed' contributor denies he inserted it deliberately

#71

Post by notacoward »

ajanuary wrote: post #38
fit2rule wrote: post #31

Earlier quoted context omitted.

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.

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?

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.

Re: 'Heartbleed' contributor denies he inserted it deliberately

#72

Post by ajanuary »

phazmatis wrote: post #65
ajanuary wrote: post #56

Earlier quoted context omitted.

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.

So you refactor until the noise goes away. Why throw out the signal with the noise? Modern software engineering theory 101.

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.

Re: 'Heartbleed' contributor denies he inserted it deliberately

#73

Post by tfinniga »

notacoward wrote: post #29

Maybe the silver lining here is that it puts the final nail in the coffin for "many eyes make all bugs shallow" - which was always total BS from the day it was uttered. There's so much code out there, much of it highly specialized and even project-specific, that there are very few eyes looking at any particular piece of code, and not all eyes are connected to the greatest of brains. Most static code analyzers could h…

A lot of what Linus says only really applies to linux kernel development.

And some of that only applies when you're Linus.

Re: 'Heartbleed' contributor denies he inserted it deliberately

#74

Post by wglb »

fit2rule wrote: post #22

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.

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

Re: 'Heartbleed' contributor denies he inserted it deliberately

#75

Post by bunderbunder »

chiph wrote: post #16

Any C programmers who have never failed to check their bounds? Anyone? Anyone? Because everyone has done it. Now would be a nice time for one of the Lint vendors to donate copies of their product to some of the OpenSSL team members, and for them to dedicate some resources into fixing the more important stuff it finds.

Or better yet, start seriously talking about moving to a language that doesn't make it so stupendously easy for trivial bugs like these to get so far out of hand.

Bounds checking incurs such a trivial amount of overhead, and the potential cost of straying out of bounds is so high. I understand we've got decades' worth of C culture built up around coding idioms that are incompatible with type safety and bounds checking. I understand that means that developing and migrating to a dialect of C or C++ that offers something in the way of a safety net is anything but trivial. But we've also got decades' worth of painful examples of just how catastrophic - and difficult to prevent - these kinds of errors can be, and that alone should justify the cost.

At the very least, consider the example set by C#: Be type safe and do bounds checks by default, but provide an `unsafe` keyword for those cases where it really is necessary to disable them.

Re: 'Heartbleed' contributor denies he inserted it deliberately

#76

Post by sleevi »

tptacek wrote: post #40
dangayle wrote: post #36

Earlier quoted context omitted.

>> Maybe next time people will spend a little less time designing a logo and a little more time doing things that actually help (though that's a wish and what I expect is the exact opposite). So you want the graphic designers contributing to crypto code? As someone who is a mid level programmer but a decent/good designer, the only way I'm able to contribute to some projects is through things like logos. Don't use thi…

It sure would be nice if some of those designers would take a whack at the Mozilla certificate configuration UI. It hasn't meaningfully changed in a decade, it's one of the Internet's biggest security flaws, and it's something any UX person could safely propose changes to.

"It's one of the Internet's biggest security flaws" - aww, you're killing me here :)

We have looked at it. Repeatedly. First when porting Chrome to Linux (and trying to decide what sort of UI there should be for NSS cert management). Then for ChromeOS. And then again, revisiting it in light of the OpenSSL migration.

The reality is, the certificate management/configuration UIs, of all platforms (Windows, OS X, Linux-via-Firefox-and-Chrome-which-just-copies-Firefoxes-PSM), is hard. It's hard to express the decisions. It's hard to explain how the branching and cross-certifications of CAs can affect trust decisions (eg: recall DigiNotar's cross-certifications).

And in the end, it's not where users are spending their time/being tripped up. Getting the lock icon into a more meaningful state is a far, far greater investment, with far, far more actionable returns in users security. The work of the Chrome Security Enamel team on this - especially Adrienne Porter Felt - has been focused on trying to improve this.

Re: 'Heartbleed' contributor denies he inserted it deliberately

#77

Post by notacoward »

notacoward wrote: post #69
nuncanada wrote: post #61

Earlier quoted context omitted.

OpenSSL's code uses particularly bad MACRO's ifdefs that defeat static analyzer's capabilities to analyze the code...

I'm not sure how much those macros affect anything here. For one thing, most static analyzers are actually very good at cutting through that kind of crap, running its analyses on code that has already been pre-processed using exactly the same command-line flags as the real compiler saw. Even if that weren't the case, I think this particular bug would still qualify as low-hanging fruit. It doesn't involve a lot of mac…

Meant to say "false positives" but it's too late to edit. IRTE

Re: 'Heartbleed' contributor denies he inserted it deliberately

#78

Post by notacoward »

DanBC wrote: post #70
orblivion wrote: post #59

Earlier quoted context omitted.

> Most static code analyzers could have caught this particular bug Does that mean nobody out there in the crypto world ran a static analyzer on OpenSSL in the last 2 years?

... And wasn't the Debian bug caused by someone running a tool against the code and trusting the tool?

Yeah, and I've heard that one guy died because he got tangled up in his climbing rope too. Those things are dangerous, nobody should use them.

Re: 'Heartbleed' contributor denies he inserted it deliberately

#79

Post by tptacek »

sleevi wrote: post #76
tptacek wrote: post #40

Earlier quoted context omitted.

It sure would be nice if some of those designers would take a whack at the Mozilla certificate configuration UI. It hasn't meaningfully changed in a decade, it's one of the Internet's biggest security flaws, and it's something any UX person could safely propose changes to.

"It's one of the Internet's biggest security flaws" - aww, you're killing me here :) We have looked at it. Repeatedly. First when porting Chrome to Linux (and trying to decide what sort of UI there should be for NSS cert management). Then for ChromeOS. And then again, revisiting it in light of the OpenSSL migration. The reality is, the certificate management/configuration UIs, of all platforms (Windows, OS X, Linux-v…

I understand why you're writing this. You think I'm saying, "the UI that users use to modify the root certificate store is so bad that it's the Internet's worst problem".

I'm not saying that. I'm saying that the fact that there's only a UI to configure your root store, and not a UI to:

* set policy for which CAs you trust with which domains

* subscribe to policies published by trusted third policies

* monitor which sites bind you to which CAs

* easily opt in and out of trusting different CAs

... is the big problem. It's a UX problem, not a simple UI problem. There are additional features that need to be added, and that can be added, without litigating the whole CA system.

Re: 'Heartbleed' contributor denies he inserted it deliberately

#80

Post by shagie »

ThursdayNext wrote: post #67
fit2rule wrote: post #22

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.

This thread here: 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 Co…

According to Andy Chou of Coverity on Twitter 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.

Post reply on HN