Permalink
Browse files

Add heartbeat extension bounds check.

A missing bounds check in the handling of the TLS heartbeat extension
can be used to reveal up to 64k of memory to a connected client or
server.

Thanks for Neel Mehta of Google Security for discovering this bug and to
Adam Langley <agl@chromium.org> and Bodo Moeller <bmoeller@acm.org> for
preparing the fix (CVE-2014-0160)
(cherry picked from commit 96db902)
  • Loading branch information...
snhenson committed Apr 5, 2014
1 parent 4e6c12f commit 731f431497f463f3a2a97236fe0187b11c44aead
Showing with 36 additions and 13 deletions.
  1. +9 −0 CHANGES
  2. +18 −8 ssl/d1_both.c
  3. +9 −5 ssl/t1_lib.c
View
@@ -4,6 +4,15 @@
Changes between 1.0.2 and 1.1.0 [xx XXX xxxx]
*) A missing bounds check in the handling of the TLS heartbeat extension
can be used to reveal up to 64k of memory to a connected client or
server.
Thanks for Neel Mehta of Google Security for discovering this bug and to
Adam Langley <agl@chromium.org> and Bodo Moeller <bmoeller@acm.org> for
preparing the fix (CVE-2014-0160)
[Adam Langley, Bodo Moeller]
*) Fix for the attack described in the paper "Recovering OpenSSL
ECDSA Nonces Using the FLUSH+RELOAD Cache Side-channel Attack"
by Yuval Yarom and Naomi Benger. Details can be obtained from:
View
@@ -1330,26 +1330,36 @@ dtls1_process_heartbeat(SSL *s)
unsigned int payload;
unsigned int padding = 16; /* Use minimum padding */
/* Read type and payload length first */
hbtype = *p++;
n2s(p, payload);
pl = p;
if (s->msg_callback)
s->msg_callback(0, s->version, TLS1_RT_HEARTBEAT,
&s->s3->rrec.data[0], s->s3->rrec.length,
s, s->msg_callback_arg);
/* Read type and payload length first */

This comment has been minimized.

Show comment
Hide comment
@johanatan

johanatan Apr 9, 2014

The following block of code is way too large to be repeated in both d1_both.c and tl_lib.c. I'm sure there are many other common blocks of code between these files (and the project at large given its reputation) as well.

@johanatan

johanatan Apr 9, 2014

The following block of code is way too large to be repeated in both d1_both.c and tl_lib.c. I'm sure there are many other common blocks of code between these files (and the project at large given its reputation) as well.

if (1 + 2 + 16 > s->s3->rrec.length)

This comment has been minimized.

Show comment
Hide comment
@mcvsama

mcvsama Apr 9, 2014

Wouldn't be better if you replaced those numbers with some named constants?

@mcvsama

mcvsama Apr 9, 2014

Wouldn't be better if you replaced those numbers with some named constants?

This comment has been minimized.

Show comment
Hide comment
@rtepowers

rtepowers Apr 9, 2014

It seems there is a bit of differing style on the '1+2+x' part as below you see '3+x'. I think named constants would help clarify what is actually happening here, otherwise why not just roll them up into 3?

@rtepowers

rtepowers Apr 9, 2014

It seems there is a bit of differing style on the '1+2+x' part as below you see '3+x'. I think named constants would help clarify what is actually happening here, otherwise why not just roll them up into 3?

This comment has been minimized.

Show comment
Hide comment
@jvz

jvz Apr 9, 2014

1, 2, and 16 are named constants for the binary numbers 1b, 10b, and 10000b.

@jvz

jvz Apr 9, 2014

1, 2, and 16 are named constants for the binary numbers 1b, 10b, and 10000b.

This comment has been minimized.

Show comment
Hide comment
@babelshift

babelshift Apr 9, 2014

@jvz you know exactly what they meant. Your answer is counter-productive to improving the readability and documentation of the code.

@babelshift

babelshift Apr 9, 2014

@jvz you know exactly what they meant. Your answer is counter-productive to improving the readability and documentation of the code.

This comment has been minimized.

Show comment
Hide comment
@jvz

jvz Apr 9, 2014

I know, the code as is barely makes sense as to why anything is done the way it is. Some named constants would go a long way toward improving readability.

@jvz

jvz Apr 9, 2014

I know, the code as is barely makes sense as to why anything is done the way it is. Some named constants would go a long way toward improving readability.

This comment has been minimized.

Show comment
Hide comment
@rtepowers

rtepowers Apr 9, 2014

I think below on lines 1350 and 1351 specify the values. 1 = heartbeat type, and 2 = heartbeat length. It is difficult to know though as @jvz mentions.

@rtepowers

rtepowers Apr 9, 2014

I think below on lines 1350 and 1351 specify the values. 1 = heartbeat type, and 2 = heartbeat length. It is difficult to know though as @jvz mentions.

This comment has been minimized.

Show comment
Hide comment
@borisvidolov

borisvidolov Apr 14, 2014

If 16 is the padding, the local variable above should be used. This makes the code more obvious and updatable.

@borisvidolov

borisvidolov Apr 14, 2014

If 16 is the padding, the local variable above should be used. This makes the code more obvious and updatable.

return 0; /* silently discard */
hbtype = *p++;
n2s(p, payload);
if (1 + 2 + payload + 16 > s->s3->rrec.length)

This comment has been minimized.

Show comment
Hide comment
@soheil

soheil Apr 9, 2014

How about sticking this in an unsigned int for reuse below?

@soheil

soheil Apr 9, 2014

How about sticking this in an unsigned int for reuse below?

return 0; /* silently discard per RFC 6520 sec. 4 */
pl = p;
if (hbtype == TLS1_HB_REQUEST)
{
unsigned char *buffer, *bp;
unsigned int write_length = 1 /* heartbeat type */ +
2 /* heartbeat length */ +
payload + padding;

This comment has been minimized.

Show comment
Hide comment
@mozjag

mozjag Apr 11, 2014

Why not hoist this up to the first use at line 1343? Should make this code a bit easier to follow and gets rid of the magical 16. Maybe split out 1 + 2 + padding into overhead and also use that at line 1339?

@mozjag

mozjag Apr 11, 2014

Why not hoist this up to the first use at line 1343? Should make this code a bit easier to follow and gets rid of the magical 16. Maybe split out 1 + 2 + padding into overhead and also use that at line 1339?

This comment has been minimized.

Show comment
Hide comment
@rtepowers

rtepowers Apr 11, 2014

@mozjag, I agree. That would make this clearer.

@rtepowers

rtepowers Apr 11, 2014

@mozjag, I agree. That would make this clearer.

int r;
if (write_length > SSL3_RT_MAX_PLAIN_LENGTH)
return 0;

This comment has been minimized.

Show comment
Hide comment
@soheil

soheil Apr 9, 2014

silently discard

@soheil

soheil Apr 9, 2014

silently discard

/* Allocate memory for the response, size is 1 byte

This comment has been minimized.

Show comment
Hide comment
@ahmetb

ahmetb Apr 9, 2014

This comment probably does not belong here anymore since logic is moved above.

@ahmetb

ahmetb Apr 9, 2014

This comment probably does not belong here anymore since logic is moved above.

This comment has been minimized.

Show comment
Hide comment
@rtepowers

rtepowers Apr 9, 2014

@ahmetalpbalkan, you're right. The line 1350 describes this more succinctly.

@rtepowers

rtepowers Apr 9, 2014

@ahmetalpbalkan, you're right. The line 1350 describes this more succinctly.

* message type, plus 2 bytes payload length, plus
* payload, plus padding
*/
buffer = OPENSSL_malloc(1 + 2 + payload + padding);
buffer = OPENSSL_malloc(write_length);
bp = buffer;
/* Enter response type, length and copy payload */
@@ -1360,11 +1370,11 @@ dtls1_process_heartbeat(SSL *s)
/* Random padding */
RAND_pseudo_bytes(bp, padding);
r = dtls1_write_bytes(s, TLS1_RT_HEARTBEAT, buffer, 3 + payload + padding);
r = dtls1_write_bytes(s, TLS1_RT_HEARTBEAT, buffer, write_length);
if (r >= 0 && s->msg_callback)
s->msg_callback(1, s->version, TLS1_RT_HEARTBEAT,
buffer, 3 + payload + padding,
buffer, write_length,
s, s->msg_callback_arg);
OPENSSL_free(buffer);
View
@@ -3969,16 +3969,20 @@ tls1_process_heartbeat(SSL *s)
unsigned int payload;
unsigned int padding = 16; /* Use minimum padding */
/* Read type and payload length first */
hbtype = *p++;
n2s(p, payload);
pl = p;
if (s->msg_callback)
s->msg_callback(0, s->version, TLS1_RT_HEARTBEAT,
&s->s3->rrec.data[0], s->s3->rrec.length,
s, s->msg_callback_arg);
/* Read type and payload length first */
if (1 + 2 + 16 > s->s3->rrec.length)
return 0; /* silently discard */
hbtype = *p++;
n2s(p, payload);
if (1 + 2 + payload + 16 > s->s3->rrec.length)
return 0; /* silently discard per RFC 6520 sec. 4 */
pl = p;
if (hbtype == TLS1_HB_REQUEST)
{
unsigned char *buffer, *bp;

25 comments on commit 731f431

@daira

This comment has been minimized.

Show comment
Hide comment
@daira

daira Apr 9, 2014

I notice that the recommendation on http://heartbleed.com to disable support for the heartbeat extension has not been followed.

"Should heartbeat be removed to aid in detection of vulnerable services?

Recovery from this bug could benefit if the new version of the OpenSSL would both fix the bug and disable heartbeat temporarily until some future version. It appears that majority if not almost all TLS implementations that respond to the heartbeat request today are vulnerable versions of OpenSSL. If only vulnerable versions of OpenSSL would continue to respond to the heartbeat for next few months then large scale coordinated response to reach owners of vulnerable services would become more feasible."

daira replied Apr 9, 2014

I notice that the recommendation on http://heartbleed.com to disable support for the heartbeat extension has not been followed.

"Should heartbeat be removed to aid in detection of vulnerable services?

Recovery from this bug could benefit if the new version of the OpenSSL would both fix the bug and disable heartbeat temporarily until some future version. It appears that majority if not almost all TLS implementations that respond to the heartbeat request today are vulnerable versions of OpenSSL. If only vulnerable versions of OpenSSL would continue to respond to the heartbeat for next few months then large scale coordinated response to reach owners of vulnerable services would become more feasible."

@lwneal

This comment has been minimized.

Show comment
Hide comment
@lwneal

lwneal Apr 9, 2014

It is simple to tell the difference between a TLS implementation that handles heartbeat requests correctly, and an implementation that is vulnerable to Heartbleed.

To do so, replace s2n(payload, p); on line 4086 with s2n(LARGE_NUMBER, p);. Replace the "silently discard" logic with any desired logging. Then compile and send heartbeat requests with:

./openssl s_client -connect www.serverundertest.com:443

It is simple to tell the difference between a TLS implementation that handles heartbeat requests correctly, and an implementation that is vulnerable to Heartbleed.

To do so, replace s2n(payload, p); on line 4086 with s2n(LARGE_NUMBER, p);. Replace the "silently discard" logic with any desired logging. Then compile and send heartbeat requests with:

./openssl s_client -connect www.serverundertest.com:443
@rdewolff

This comment has been minimized.

Show comment
Hide comment
@rdewolff

rdewolff Apr 9, 2014

This has to be shared!

This has to be shared!

@lumaxis

This comment has been minimized.

Show comment
Hide comment
@lumaxis

lumaxis Apr 9, 2014

Where are the braces?

Where are the braces?

@filinger

This comment has been minimized.

Show comment
Hide comment
@filinger

filinger Apr 9, 2014

They did it in hustle so no time to put braces!!11

They did it in hustle so no time to put braces!!11

@douglasheld

This comment has been minimized.

Show comment
Hide comment
@douglasheld

douglasheld Apr 9, 2014

lwneal, I've been using the simple python tester published here: https://github.com/musalbas/heartbleed-masstest
It's been one full day and I've found a handful of sites in my web history that are still vulnerable...

lwneal, I've been using the simple python tester published here: https://github.com/musalbas/heartbleed-masstest
It's been one full day and I've found a handful of sites in my web history that are still vulnerable...

@jlmalone

This comment has been minimized.

Show comment
Hide comment
@jlmalone

jlmalone Apr 9, 2014

Thank you douglasheld for giving hackers a tool to find all the exposed sites on the Internet.

http://vimeo.com/44182757

Perhaps if the code were a bit more readable in not in the format, 1 + 2 + payload + 16, this bug would have been caught earlier. It's ok though, it's not like the world's data security relies on this or anything.

Thank you douglasheld for giving hackers a tool to find all the exposed sites on the Internet.

http://vimeo.com/44182757

Perhaps if the code were a bit more readable in not in the format, 1 + 2 + payload + 16, this bug would have been caught earlier. It's ok though, it's not like the world's data security relies on this or anything.

@pierrevalade

This comment has been minimized.

Show comment
Hide comment
@pierrevalade

pierrevalade Apr 9, 2014

Could we imagine that writing tests could help make sure regressions are not introduced? Thanks.

Could we imagine that writing tests could help make sure regressions are not introduced? Thanks.

@pcai

This comment has been minimized.

Show comment
Hide comment
@pcai

pcai Apr 10, 2014

@jlmalone perhaps if you were making more pull requests instead of snarky comments, the code would be more readable. It's OK though, it's not like you're the only one to whine about FOSS while benefiting from it without contributing anything in return.

pcai replied Apr 10, 2014

@jlmalone perhaps if you were making more pull requests instead of snarky comments, the code would be more readable. It's OK though, it's not like you're the only one to whine about FOSS while benefiting from it without contributing anything in return.

@mkotelba

This comment has been minimized.

Show comment
Hide comment
@mkotelba

mkotelba Apr 10, 2014

Could we imagine that writing tests could help make sure regressions are not introduced? Thanks.

Writing tests is for those mediocre developers who spend their lives writing Java (ugh). Its much better to just have a handful of printf-esque statements in the code (commented out, of course).

Also, defining constants, especially those with human-readable names, is a massive affront to performance! If another developer wants to know that "payload" actually describes the size of the buffer, they should go read the RFC.

Could we imagine that writing tests could help make sure regressions are not introduced? Thanks.

Writing tests is for those mediocre developers who spend their lives writing Java (ugh). Its much better to just have a handful of printf-esque statements in the code (commented out, of course).

Also, defining constants, especially those with human-readable names, is a massive affront to performance! If another developer wants to know that "payload" actually describes the size of the buffer, they should go read the RFC.

@billymoon

This comment has been minimized.

Show comment
Hide comment
@billymoon

billymoon Apr 10, 2014

@mkotelba if named constants are such a performance hit, maybe the code could be transpiled with constants replaced before compiling, so source is easier to understand, and performance is not hit.

It's late, I am going to go read some RFC specs to put myself to sleep.

@mkotelba if named constants are such a performance hit, maybe the code could be transpiled with constants replaced before compiling, so source is easier to understand, and performance is not hit.

It's late, I am going to go read some RFC specs to put myself to sleep.

@johanatan

This comment has been minimized.

Show comment
Hide comment
@johanatan

johanatan Apr 10, 2014

Or, you know, #define.

Or, you know, #define.

@jameswangz

This comment has been minimized.

Show comment
Hide comment
@jameswangz

jameswangz Apr 10, 2014

Writing tests is for those mediocre developers who spend their lives writing Java (ugh). Its much better to just have a handful of printf-esque statements in the code (commented out, of course).

@mkotelba You don't write test, so your users do the testing for you, they find a stupid bug and they ask : who wrote the stupid code?

Writing tests is for those mediocre developers who spend their lives writing Java (ugh). Its much better to just have a handful of printf-esque statements in the code (commented out, of course).

@mkotelba You don't write test, so your users do the testing for you, they find a stupid bug and they ask : who wrote the stupid code?

@vczh

This comment has been minimized.

Show comment
Hide comment
@vczh

vczh Apr 10, 2014

@mkotelba I cannot find any important softwares which don't need testing. If you managed to do that, please let me know. So that I can learn from you about how to write correct and efficient code in such a brand new way.

vczh replied Apr 10, 2014

@mkotelba I cannot find any important softwares which don't need testing. If you managed to do that, please let me know. So that I can learn from you about how to write correct and efficient code in such a brand new way.

@johanatan

This comment has been minimized.

Show comment
Hide comment
@johanatan

johanatan Apr 10, 2014

I read a sarcastic tone in @mkotelba's post. If it wasn't intended to be sarcastic, then this thread is becoming even more hilarious!

I read a sarcastic tone in @mkotelba's post. If it wasn't intended to be sarcastic, then this thread is becoming even more hilarious!

@brupm

This comment has been minimized.

Show comment
Hide comment
@brupm

brupm Apr 11, 2014

Is it only me or does anyone else agree that such a high profile lib could benefit for a robust test suite?

Is it only me or does anyone else agree that such a high profile lib could benefit for a robust test suite?

@rtepowers

This comment has been minimized.

Show comment
Hide comment
@rtepowers

rtepowers Apr 11, 2014

@brupm, I agree wholeheartedly.

@brupm, I agree wholeheartedly.

@douglasheld

This comment has been minimized.

Show comment
Hide comment
@douglasheld

douglasheld Apr 11, 2014

I think a functional/unit test would be really helpful, except that those typically don't identify security vulnerabilities (because they are not designed to).

Think about it however: some simple unit tests would have found the Apple "goto fail" -- which was in the wild for what, years? Pretty damning.

As for security assurance of infrastructure critical code, the path has to be:

  • declutter the code. Nobody can read OpenSSL, this has been documented.
  • Periodic security architecture reviews.
  • My own field is static analysis. The available static analysis tools should be coming up clean for the core library, plus sample code and reference implementations.

These steps are not impossible to achieve, but the list has correctly identified that the "many eyes" are not bringing this about automatically. "Many eyes" are really good for catching a broken build, but not design flaws such as this.

I think a functional/unit test would be really helpful, except that those typically don't identify security vulnerabilities (because they are not designed to).

Think about it however: some simple unit tests would have found the Apple "goto fail" -- which was in the wild for what, years? Pretty damning.

As for security assurance of infrastructure critical code, the path has to be:

  • declutter the code. Nobody can read OpenSSL, this has been documented.
  • Periodic security architecture reviews.
  • My own field is static analysis. The available static analysis tools should be coming up clean for the core library, plus sample code and reference implementations.

These steps are not impossible to achieve, but the list has correctly identified that the "many eyes" are not bringing this about automatically. "Many eyes" are really good for catching a broken build, but not design flaws such as this.

@vrpratt

This comment has been minimized.

Show comment
Hide comment
@vrpratt

vrpratt Apr 14, 2014

"Higher level language" was my first reaction to this bug. But which one? There's a catch-22 here: the more universal the HLL the more clutter is likely to result in implementing any given algorithm. But the more targeted an HLL is to any domain, the more compilers you need, one for each HLL, each with its own opportunities for bugs in the compiler.

For lowering the incidence of heartbleed-type bugs, library coordination between servers might be a more effective and more immediately implementable solution than higher level languages. Why is it so hard to provide sendstring and receivestring functions that are provably unspoofable? That's not a rhetorical question.

"Higher level language" was my first reaction to this bug. But which one? There's a catch-22 here: the more universal the HLL the more clutter is likely to result in implementing any given algorithm. But the more targeted an HLL is to any domain, the more compilers you need, one for each HLL, each with its own opportunities for bugs in the compiler.

For lowering the incidence of heartbleed-type bugs, library coordination between servers might be a more effective and more immediately implementable solution than higher level languages. Why is it so hard to provide sendstring and receivestring functions that are provably unspoofable? That's not a rhetorical question.

@MetroWind

This comment has been minimized.

Show comment
Hide comment
@MetroWind

MetroWind Apr 15, 2014

Wait… No one noticed @mkotelba was just joking? @mkotelba, you were joking by saying things in the opposite way, right? Right? … I’m sure he was joking. Come on guys.

Wait… No one noticed @mkotelba was just joking? @mkotelba, you were joking by saying things in the opposite way, right? Right? … I’m sure he was joking. Come on guys.

@mcvsama

This comment has been minimized.

Show comment
Hide comment
@mcvsama

mcvsama Apr 15, 2014

@corsair It was so obvious that no one needed to write about it.

@corsair It was so obvious that no one needed to write about it.

@johanatan

This comment has been minimized.

Show comment
Hide comment
@johanatan

johanatan Apr 15, 2014

Have you read the thread? It wasn't obvious to the people who responded seriously.

Have you read the thread? It wasn't obvious to the people who responded seriously.

@mkotelba

This comment has been minimized.

Show comment
Hide comment
@mkotelba

mkotelba Apr 19, 2014

Heh, I'm glad I forgot that I posted in this thread for a week.

Yes, of course I was being sarcastic :) I tend to work in the land of Java most of the time, and often on projects that are heavily security-focused (fancy S/MIME processing, etc).

Call me a fanboy if you must, but even the most complicated, massive, and poorly-documented Java libs/APIs out there (relevant ex., BounceyCastle) still manage to have a ~passable amount of test coverage. The fact that throwing a single JUnit or TestNG dependency into your project makes even complex testing a breeze certainly helps.

In regards to OpenSSL, @douglasheld is dead-on in his ideas: hell, even just deciding on a given code-style configuration and then letting a code formatter apply it to the entire codebase would help tremendously.

Heh, I'm glad I forgot that I posted in this thread for a week.

Yes, of course I was being sarcastic :) I tend to work in the land of Java most of the time, and often on projects that are heavily security-focused (fancy S/MIME processing, etc).

Call me a fanboy if you must, but even the most complicated, massive, and poorly-documented Java libs/APIs out there (relevant ex., BounceyCastle) still manage to have a ~passable amount of test coverage. The fact that throwing a single JUnit or TestNG dependency into your project makes even complex testing a breeze certainly helps.

In regards to OpenSSL, @douglasheld is dead-on in his ideas: hell, even just deciding on a given code-style configuration and then letting a code formatter apply it to the entire codebase would help tremendously.

@MetroWind

This comment has been minimized.

Show comment
Hide comment
@MetroWind

MetroWind Apr 22, 2014

@hamradiojava Haha that's a good one~~

@hamradiojava Haha that's a good one~~

@mbland

This comment has been minimized.

Show comment
Hide comment
@mbland

mbland Apr 26, 2014

Contributor

I've posted #81 to add a unit test to prevent regression of the Heartbleed bug.

Contributor

mbland replied Apr 26, 2014

I've posted #81 to add a unit test to prevent regression of the Heartbleed bug.

Please sign in to comment.