3 ms·

Here's the patch/commit, I don't know why it's not linked form the OpenSSL changelog or heartbleed.com. A suspicious lack of transparency. http://git.openssl.o

Here's the patch/commit, I don't know why it's not linked form the OpenSSL changelog or heartbleed.com. A suspicious lack of transparency. http://git.openssl.org/gitweb/?p=openssl.git;a=commitdiff;h=96db9023b881d7cd9f379b0c154650d6c108e9a3 http://git.openssl.org/gitweb/?p=openssl.git;a=commitdiff;h=...

NelsonMinar 13y ago
I'm very curious to see the change that introduced the bug in the first place. According to the announcement it was introduced in 1.0.1. That's the version that added Heartbeat support, so maybe it was a bug from the beginning.
- syzzer 13y ago
  Yes, this has been introduced in the original heartbeat extension commit (01-01-2012): http://git.openssl.org/gitweb/?p=openssl.git;a=commit;h=bd6941cfaa31ee8a3f8661cb98227a5cbcc0f9f3 http://git.openssl.org/gitweb/?p=openssl.git;a=commit;h=bd69...
brown9-2 13y ago
if (1 + 2 + 16 > s->s3->rrec.length) I don't know C well - why write 19 like this?
- scintill76 13y ago
  Probably to make it more clear what you're referring to, and double-check yourself. There are probably components that are 1 byte, 2 bytes, and 16 bytes long. Writing it out makes it clear and eliminates a chance for human error in the sum, more than a magic 19 does. (I guess 16 is pretty magical too, though. At least it's a "round" number, and in context may be a well-known field size of something in the protocol.)
  - georgemcbay 13y ago
    Yeah, this. It is fairly common to see things like this in C-like languages when it comes to times, like when including the milliseconds in a day you might see: int timeout = 1000 * 60 * 60 * 24; (milliseconds in a second * 60 for minute, * 60 for hour, * 24 for hours in a day). Much more obvious than just putting in 86400000, and the compiler will optimize away the math and putting the math in there explicitly is arguably better than a comment that could easily become unanchored from the real value (if someone changes the value and forgets to update the comment). When it comes to byte-sizes of things, though, most code will use sizeof() to both make it more clear where these numbers are coming from and to make them automatically adjust if the structure sizes change (granted this is unlikely to happen on a mature protocol). At the very least having them be preprocessor defines would certainly make things a lot more clear here, so even for C I'd consider this a bit of a "code smell" (though the people who work on this code regularly are probably versed enough in the ssl3 record structure enough that they immediately grok this when they see it).
- jdiez17 13y ago
  Those numbers probably have some significance. `1` seems to be "heartbeat type" and `2` seems to be "heartbeat length".
  - ionforce 13y ago
    Why not use constants then?

NelsonMinar 13y ago

I'm very curious to see the change that introduced the bug in the first place. According to the announcement it was introduced in 1.0.1. That's the version that added Heartbeat support, so maybe it was a bug from the beginning.

syzzer 13y ago
Yes, this has been introduced in the original heartbeat extension commit (01-01-2012): http://git.openssl.org/gitweb/?p=openssl.git;a=commit;h=bd6941cfaa31ee8a3f8661cb98227a5cbcc0f9f3 http://git.openssl.org/gitweb/?p=openssl.git;a=commit;h=bd69...

brown9-2 13y ago

if (1 + 2 + 16 > s->s3->rrec.length) I don't know C well - why write 19 like this?

scintill76 13y ago
Probably to make it more clear what you're referring to, and double-check yourself. There are probably components that are 1 byte, 2 bytes, and 16 bytes long. Writing it out makes it clear and eliminates a chance for human error in the sum, more than a magic 19 does. (I guess 16 is pretty magical too, though. At least it's a "round" number, and in context may be a well-known field size of something in the protocol.)
- georgemcbay 13y ago
  Yeah, this. It is fairly common to see things like this in C-like languages when it comes to times, like when including the milliseconds in a day you might see: int timeout = 1000 * 60 * 60 * 24; (milliseconds in a second * 60 for minute, * 60 for hour, * 24 for hours in a day). Much more obvious than just putting in 86400000, and the compiler will optimize away the math and putting the math in there explicitly is arguably better than a comment that could easily become unanchored from the real value (if someone changes the value and forgets to update the comment). When it comes to byte-sizes of things, though, most code will use sizeof() to both make it more clear where these numbers are coming from and to make them automatically adjust if the structure sizes change (granted this is unlikely to happen on a mature protocol). At the very least having them be preprocessor defines would certainly make things a lot more clear here, so even for C I'd consider this a bit of a "code smell" (though the people who work on this code regularly are probably versed enough in the ssl3 record structure enough that they immediately grok this when they see it).
jdiez17 13y ago
Those numbers probably have some significance. `1` seems to be "heartbeat type" and `2` seems to be "heartbeat length".
- ionforce 13y ago
  Why not use constants then?