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...
- +9 −0 CHANGES
- +18 −8 ssl/d1_both.c
- +9 −5 ssl/t1_lib.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) | ||
| + 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
|
||
| + 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 */ | ||
| @@ -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); | ||
| @@ -2588,16 +2588,20 @@ tls1_process_heartbeat(SSL *s) | ||
| unsigned int payload; | ||
| unsigned int padding = 16; /* Use minimum padding */ | ||
FenilKavathia
|
||
| - /* 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) | ||
davidgoli
|
||
| + 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
replied
Apr 10, 2014
|
what's up with all the magic numbers? Right click, Refactor -> Introduce Variable |
chrisgriffis
replied
Apr 11, 2014
|
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
replied
Apr 12, 2014
|
Extract Method |
Could probably usepaddinghere rather than the hard coded16?Edit: According to rfc6520 s4, the
16is the minimum padding allowed thus having no relation topaddingwhich should be a random number greater than 16