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)
  • Loading branch information...
1 parent 0d7717f commit 96db9023b881d7cd9f379b0c154650d6c108e9a3 @snhenson snhenson committed Apr 5, 2014
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
9 CHANGES
@@ -4,6 +4,15 @@
Changes between 1.0.1f and 1.0.1g [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
26 ssl/d1_both.c
@@ -1459,26 +1459,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)
+ return 0; /* silently discard */
+ hbtype = *p++;
+ n2s(p, payload);
+ if (1 + 2 + payload + 16 > s->s3->rrec.length)
@James-TR
James-TR Apr 9, 2014

Could probably use padding here rather than the hard coded 16?
Edit: According to rfc6520 s4, the 16 is the minimum padding allowed thus having no relation to padding which should be a random number greater than 16

+ 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 */ +
@davidgoli
davidgoli Apr 11, 2014

This would be a great place to define some constants for 1 and 2 which are used all over the place without regard to why or where they come from

@bradvido
bradvido Apr 15, 2014

+1. It seems like 1 (byte) is for message type and 2 (bytes) is for payload length. Naming the constants instead of continuously commenting when they are used would make for more readable code.

+ payload + padding;
int r;
+ if (write_length > SSL3_RT_MAX_PLAIN_LENGTH)
+ return 0;
+
/* Allocate memory for the response, size is 1 byte
@gpakosz
gpakosz May 11, 2014

How long before comment goes out of sync with actual value of write_length?

* 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 */
@@ -1489,11 +1499,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
14 ssl/t1_lib.c
@@ -2588,16 +2588,20 @@ tls1_process_heartbeat(SSL *s)
unsigned int payload;
unsigned int padding = 16; /* Use minimum padding */
@FenilKavathia
FenilKavathia Apr 17, 2014

Using minimum padding is justifiable (saves bandwidth at small scale), but we should have at-least constant defined instead of using just magic number (16). (can create confusion)

- /* 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)
@aaronzirbes
aaronzirbes Apr 8, 2014

Could probably use padding here rather than the hard coded 16?

@davidgoli
davidgoli Apr 11, 2014

Can we get some constants defined here instead of the magic numbers? Scary stuff.

+ 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;

3 comments on commit 96db902

@fatso83

what's up with all the magic numbers? Right click, Refactor -> Introduce Variable

@chrisgriffis

Ha, I thought the same thing. Actually, I thought "god I can't stand seeing that in code reviews but I guess it's how they do it in the real world"

@okaiji

Extract Method

Please sign in to comment.