Skip to content

Comment on OpenSSL Security Advisory: TLS heartbeat read overrunparent

Comments

    if (1 + 2 + 16 > s->s3->rrec.length)
    if (1 + 2 + payload + 16 > s->s3->rrec.length)
Come on. At least use a macro or something.

This always makes me cringe during code reviews.

Yep, and no braces after the 'if' statement in the patch. Even after the previous ssl vuln (I thought it was gnutls- struggling to find the relevant hn discussion) was caused by an omission of braces after the 'if' statement.

You are thinking of "goto fail", a bug in Apple's Security framework. I would not claim that was "caused" by a lack of braces: even having the braces, that bug--in addition to a wide class of similar bugs--is still quite possible, even if in a few models of how the bug was caused it becomes slightly less likely. The best place to lay blame for that kind of error is a stubborn insistence that error handling should involve boilerplate return value checks strewn throughout the code, with no attempt at abstraction or structure: it leads to numerous potential mistakes. Please read the various discussions attached to this article that made this claim:

https://news.ycombinator.com/item?id=7318039

Yes, that is exactly what I was thinking of. Thanks for the link. The discussion I was recalling is here

https://news.ycombinator.com/item?id=7282005

but the thread is a lot heftier now.

AboutSource Built by g1lg1l

Hackerly is an independent reader for Hacker News, built on the public HN API. Not affiliated with Y Combinator.