

Add heartbeat extension bounds check - whadar
https://github.com/openssl/openssl/commit/731f431497f463f3a2a97236fe0187b11c44aead

======
whadar
This blog post explains the code: [http://blog.existentialize.com/diagnosis-
of-the-openssl-hear...](http://blog.existentialize.com/diagnosis-of-the-
openssl-heartbleed-bug.html)

~~~
userbinator
This line made me go between it and inspecting the ssl3_record_st structure
carefully, since the way it was written looked like a flexible array member:

    
    
        unsigned char *p = &s->s3->rrec.data[0], *pl;
    

I think it would be better written as:

    
    
        unsigned char *p = s->s3->rrec.data, *pl;
    

to make it clearer that data is a pointer within the structure.

------
cmbaus
I'm sort of surprised an allocation occurs every time the heartbeat is sent.
That is a lot of trips to the heap.

I'm not very familiar with how TLS heartbeats are implemented, but I wonder if
the buffer could have just been alloc'd once when the connection was created.

~~~
icebraining
I wouldn't know, but in any case that would require allocating the whole 64KiB
for each connection, when the actual heartbeats can be much smaller. Wouldn't
the memory waste be worse than the allocations?

~~~
bad_user
Depends on implementation. Memory allocations can lead to fragmentation and
thus to waste and decreased performance. That's what's great about using a
compacting GC btw.

------
Cthulhu_
I like how those bounds checks (the ifs) have no curly braces, as if that
Apple security bug didn't wake people up about such trivial opportunities for
bugs.

~~~
wreegab
I don't get it either. I am an old programmer full of habits too I suppose,
but I had no problem to adopt mandatory curly braces after reading the
rationale for mandatory curly braces in Golang years ago. I would think the
Apple bug was an even stronger rationale to adopt mandatory curly braces
habit...

------
Flow
I wonder if any of the existing static code analyzers would have found this?

PVS-Studio checks some open source projects and posts part of the results on
their blog. I did a search and found that they did take a look at OpenSSL in
2012.

[http://www.viva64.com/en/b/0183/](http://www.viva64.com/en/b/0183/)

And Coverity:
[https://scan.coverity.com/projects/294](https://scan.coverity.com/projects/294)

~~~
Shish2k
I wonder how "unvalidated user input passed to memcpy()" didn't trigger _all_
of the static analysers...

I guess that the anaylser doesn't know that it's untrusted - perhaps it's
worth having separate "trusted int" and "untrusted int" data types, so this
would've been a compile-time error?

~~~
Flow
Or create a standard of annotations for C, like those used by Java and C#?
NotNull, CanBeNull etc, and let the static analyzers use those hints to help
even more.

------
kzrdude
1 + 3 + padding and 1 + 3 + 16 are repeated. I suspect the magic 16 is
actually just the padding too.

~~~
nate_martin
I'm curious as to why they are not using constants or preprocessor macros for
these values. It seems weird for it to be repeated in multiple places.

~~~
wyager
Probably just to make it easier to understand what's going on. If they just
had "20+..." it wouldn't give you any hints where the 20 came from.

~~~
kzrdude
you'd have a define with an understandable name instead of just the magic
constant 20.

~~~
mikeash
You could even make a constant for 1, 3, and 16 separately, then still add
them together in the code. It may seem excessive for such a small case, but in
my experience it really helps readability to have kWhateverFieldSize instead
of 1.

------
IvyMike

        /* Read type and payload length first */
    

And now this is actually the second thing the code does, not the first.

~~~
nodata
Did you submit a pull request?

~~~
renang
Not him, but someone else did:
[https://github.com/openssl/openssl/pull/58](https://github.com/openssl/openssl/pull/58)

------
voltagex_
I wonder if OpenSSL will get some code clean up courtesy of the extra eyes
that are now on the code?

------
yiedyie
HN front page is heartbleeding, I counted at least 8 stories.

~~~
Beltiras
It's the biggest security vulnerability in the modern crypto age. Sysadmins
everywhere are scrambling to renew keys and certs. It warrants at _least_ 8
stories on the frontpage.

~~~
mikeash
Complaining about the coverage of heartbleed (if grandparent is indeed
complaining and not just remarking) is sort of like complaining that the NYT
had excessive coverage of Pearl Harbor in their issue on December 8th, 1941.

~~~
yiedyie
Its just awe remark, and just your comment that starts with "complaining" got
me a few down-votes, you involuntarily framed me (if indeed was involuntary
;))

~~~
mikeash
Well, have an apologetic upvote from me. I tried to mitigate it with my
parenthetical!

