5 ms·

"Don't roll your own parsers" should really be up there with "Don't roll your own crypto". This advisory is scant on details, but this extension protocol[0] nei

by nly 13y ago

"Don't roll your own parsers" should really be up there with "Don't roll your own crypto". This advisory is scant on details, but this extension protocol[0] neither looks complex nor beyond mechanical code generation to me. Just simple enough to be dangerous. And it's pretty new, so this must be recently authored vulnerable code. [0] http://tools.ietf.org/html/draft-ietf-tls-dtls-heartbeat-04 http://tools.ietf.org/html/draft-ietf-tls-dtls-heartbeat-04

eliteraspberrie 13y ago
Looks like a good use case for Hammer: https://github.com/UpstandingHackers/hammer https://github.com/UpstandingHackers/hammer
- nly 13y ago
  Looks interesting, but static code generation along the lines of Ragel (but more oriented toward binary structures like Protocol Buffers) would be a lot better for performance.
iso8859-1 13y ago
Here's the commit for the fix: http://git.openssl.org/gitweb/?p=openssl.git;a=commitdiff;h=96db9023b881d7cd9f379b0c154650d6c108e9a3 http://git.openssl.org/gitweb/?p=openssl.git;a=commitdiff;h=...
- osivertsson 13y ago
  Ouch, pretty basic lack of bounds checking. Even though the code got better with this fix I still wouldn't accept code that looks like this in a review. Why are 1, 2, 3, 16 not defines? What's up with the code duplication between files? Where are the unit-tests? I'm starting to feel that a lot of software that has been around for 10+ years and is commonly used does not live up to current best practices regarding writing good system-level software.
  - rlpb 13y ago
    > I'm starting to feel that a lot of software that has been around for 10+ years and is commonly used does not live up to current best practices regarding writing good system-level software. I get the impression that this applies to openssl far more than other software. The code base is a mess, and it's security sensitive. So people dare not touch it. It's a shame that there isn't a better incentive for this particular code base to be fixed.
- simias 13y ago
  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.
  - dhimes 13y ago
    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.
    - saurik 13y ago
      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 https://news.ycombinator.com/item?id=7318039
      - dhimes 13y ago
        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 https://news.ycombinator.com/item?id=7282005 but the thread is a lot heftier now.
- Jare 13y ago
  I felt much better about having a fix before I looked at the actual code. It is functionally secure (looks right to me and thousands of others by now), but the way it is written would guarantee you an instant fail in an exam or interview.
jlgaddis 13y ago
2011-12-31, it looks like, so it's been there for a couple of years already (introduced between 1.0.0f and 1.0.1): http://git.openssl.org/gitweb/?p=openssl.git;a=commitdiff;h=4817504d069b4c5082161b02a22116ad75f822b1 http://git.openssl.org/gitweb/?p=openssl.git;a=commitdiff;h=...
- ge0rg 13y ago
  Wow, a code change introduced around New Year's Eve (depending on timezone), and authored by Robin Seggelmann, who also brought us DTLS-SCTP.
zurn 13y ago
> "Don't roll your own parsers" should really be up there with "Don't roll your own crypto". .. and if you do, don't do it in a highly memory-unsafe language. Espcially when it's for a security critical piece of central internet infrastructure!
- danielweber 13y ago
  How do the Ruby-YAML and Python-Pickle vulnerabilities get cataloged?
hackinthebochs 13y ago
I've been thinking along these lines for a long time now, that parsing is such a critical activity that we should treat it with far more reverence than we do. Ideally, we would define languages to describe the format of the data that we want to parse (something like a BNF perhaps), and the OS/environment would parse it and populate variables/provide a dictionary in response. Ensuring that the input to your algorithm is exactly as expected is such a critical task that no one should ever be doing it manually.

by nly 13y ago

eliteraspberrie 13y ago

Looks like a good use case for Hammer: https://github.com/UpstandingHackers/hammer https://github.com/UpstandingHackers/hammer

nly 13y ago
Looks interesting, but static code generation along the lines of Ragel (but more oriented toward binary structures like Protocol Buffers) would be a lot better for performance.

iso8859-1 13y ago

Here's the commit for the fix: http://git.openssl.org/gitweb/?p=openssl.git;a=commitdiff;h=96db9023b881d7cd9f379b0c154650d6c108e9a3 http://git.openssl.org/gitweb/?p=openssl.git;a=commitdiff;h=...

osivertsson 13y ago
Ouch, pretty basic lack of bounds checking. Even though the code got better with this fix I still wouldn't accept code that looks like this in a review. Why are 1, 2, 3, 16 not defines? What's up with the code duplication between files? Where are the unit-tests? I'm starting to feel that a lot of software that has been around for 10+ years and is commonly used does not live up to current best practices regarding writing good system-level software.
- rlpb 13y ago
  > I'm starting to feel that a lot of software that has been around for 10+ years and is commonly used does not live up to current best practices regarding writing good system-level software. I get the impression that this applies to openssl far more than other software. The code base is a mess, and it's security sensitive. So people dare not touch it. It's a shame that there isn't a better incentive for this particular code base to be fixed.
simias 13y ago
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.
- dhimes 13y ago
  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.
  - saurik 13y ago
    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 https://news.ycombinator.com/item?id=7318039
    - dhimes 13y ago
      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 https://news.ycombinator.com/item?id=7282005 but the thread is a lot heftier now.
Jare 13y ago
I felt much better about having a fix before I looked at the actual code. It is functionally secure (looks right to me and thousands of others by now), but the way it is written would guarantee you an instant fail in an exam or interview.

jlgaddis 13y ago

2011-12-31, it looks like, so it's been there for a couple of years already (introduced between 1.0.0f and 1.0.1): http://git.openssl.org/gitweb/?p=openssl.git;a=commitdiff;h=4817504d069b4c5082161b02a22116ad75f822b1 http://git.openssl.org/gitweb/?p=openssl.git;a=commitdiff;h=...

ge0rg 13y ago
Wow, a code change introduced around New Year's Eve (depending on timezone), and authored by Robin Seggelmann, who also brought us DTLS-SCTP.

zurn 13y ago

> "Don't roll your own parsers" should really be up there with "Don't roll your own crypto". .. and if you do, don't do it in a highly memory-unsafe language. Espcially when it's for a security critical piece of central internet infrastructure!

danielweber 13y ago
How do the Ruby-YAML and Python-Pickle vulnerabilities get cataloged?

hackinthebochs 13y ago

I've been thinking along these lines for a long time now, that parsing is such a critical activity that we should treat it with far more reverence than we do. Ideally, we would define languages to describe the format of the data that we want to parse (something like a BNF perhaps), and the OS/environment would parse it and populate variables/provide a dictionary in response. Ensuring that the input to your algorithm is exactly as expected is such a critical task that no one should ever be doing it manually.