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 a489632 commit 7e840163c06c7692b796a93e3fa85a93136adbb2
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.1f and 1.0.2 [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 */
if (1 + 2 + 16 > s->s3->rrec.length)

This comment has been minimized.

Show comment
Hide comment
@AndriiBorysov

AndriiBorysov Apr 8, 2014

What is the magic number "16"? It seems it should be the "padding" variable, which is initialized to "16". Both of them ("padding" and "16") are used in the code below, that may lead to a problem in future, if the "padding" variable will be initialized with another value.

@AndriiBorysov

AndriiBorysov Apr 8, 2014

What is the magic number "16"? It seems it should be the "padding" variable, which is initialized to "16". Both of them ("padding" and "16") are used in the code below, that may lead to a problem in future, if the "padding" variable will be initialized with another value.

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;
unsigned int write_length = 1 /* heartbeat type */ +
2 /* heartbeat length */ +
payload + padding;
int r;
if (write_length > SSL3_RT_MAX_PLAIN_LENGTH)
return 0;
/* Allocate memory for the response, size is 1 byte
* 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
@@ -3801,16 +3801,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;

4 comments on commit 7e84016

@mritun

This comment has been minimized.

Show comment
Hide comment
@mritun

mritun Apr 8, 2014

The "payload" variable should have been named "payload_length" or equivalent. Did anyone really review this patch?

mritun replied Apr 8, 2014

The "payload" variable should have been named "payload_length" or equivalent. Did anyone really review this patch?

@weskerfoot

This comment has been minimized.

Show comment
Hide comment
@weskerfoot

weskerfoot Apr 9, 2014

I agree, the code is confusing. I would assume "payload" means the actual payload...not the length of the payload. This may have been why it went undetected for so long. OpenSSL is badly in need of better style guidelines at least (and some serious quickcheck style tests as well).

weskerfoot replied Apr 9, 2014

I agree, the code is confusing. I would assume "payload" means the actual payload...not the length of the payload. This may have been why it went undetected for so long. OpenSSL is badly in need of better style guidelines at least (and some serious quickcheck style tests as well).

@weskerfoot

This comment has been minimized.

Show comment
Hide comment
@weskerfoot

weskerfoot Apr 9, 2014

Oh also, for the love of god start using braces for every conditional, we don't want another gotofail style bug. This isn't code golf.

weskerfoot replied Apr 9, 2014

Oh also, for the love of god start using braces for every conditional, we don't want another gotofail style bug. This isn't code golf.

@okaiji

This comment has been minimized.

Show comment
Hide comment
@okaiji

okaiji Apr 12, 2014

payload_length would be a good idea.

okaiji replied Apr 12, 2014

payload_length would be a good idea.

Please sign in to comment.