5 ms·
Add heartbeat extension bounds check
- whadar 13y agoThis blog post explains the code: http://blog.existentialize.com/diagnosis-of-the-openssl-heartbleed-bug.html http://blog.existentialize.com/diagnosis-of-the-openssl-hear...
- gargarplex 13y agoThanks; fascinating. The "length" of the payload is useful for error correcting, but the implementer didn't take advantage of this property, so it was usable against them. A double edged sword.
- userbinator 13y agoThis 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.
- IvyMike 13y ago/* Read type and payload length first */ And now this is actually the second thing the code does, not the first.
- kzrdude 13y ago1 + 3 + padding and 1 + 3 + 16 are repeated. I suspect the magic 16 is actually just the padding too.
- whadar 13y agoYeah, it just that you wanna make sure the padding variable is not being overridden by some malloc ;)
- nate_martin 13y agoI'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 13y agoProbably 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 13y agoyou'd have a define with an understandable name instead of just the magic constant 20.
- mikeash 13y agoYou 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.
- voltagex_ 13y agoI wonder if OpenSSL will get some code clean up courtesy of the extra eyes that are now on the code?
- yiedyie 13y agoHN front page is heartbleeding, I counted at least 8 stories.
- Beltiras 13y agoIt'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 13y agoComplaining 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.
- deleted 13y ago[deleted]
- cmbaus 13y agoI'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 13y agoI 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 13y agoDepends 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.
- userbinator 13y agoI believe there's another one that occurs every time a heartbeat request is received, and was just as surprised that they had to allocate a new buffer, copy the payload over (which is where the bug was), basically construct a new message, when they could've just validated the incoming message, changed type + padding, and sent it back out using the same buffer. Heartbeat extension is only slightly more complex than an echo protocol. It's probably much harder to remove the one on reception due to how the rest of OpenSSL is written, but at least from a glance at the code, a lot easier to rid the sending one. In terms of design, the simplest implementation would be malloc-on-receive + modify-and-send; a little better is an expanding buffer that's allocated once but reallocated if necessary, and to me, the way it's currently being done is the most complex, inefficient, and error-prone. Having many unnecessary dynamic allocations tends to be a trend I've noticed most often in C/C++ code written by programmers with a Java background. Not saying that this necessarily applies to heartbleed's culprit, but the general trend of excessive complexity is there.
- Flow 13y agoI 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 13y agoI 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 13y agoOr 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.
- Cthulhu_ 13y agoI 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.
- mkempe 13y agoIndeed, seeing that coding style should raise further questions -- if the fix to a huge security hole is somewhat sloppy, what are the chances that everything else is sloppy?
- wreegab 13y agoI 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...
- userbinator 13y agoExcept in this case, an extra 'return 0' would just mean ignoring the heartbeat message completely. It's a slightly different situation from Apple's bug.