[openssl-dev] [openssl.org #3908] Patch fixing some heartbeat issues (vs latest git master)
Peter Dettman via RT
rt at openssl.org
Sat Jun 13 13:13:26 UTC 2015
Hi,
Please find attached a patch against the current git master that fixes
some problems in TLS (and DTLS) heartbeats. This patch supercedes the
original pull request I made closer to the time of heartbleed
(https://github.com/openssl/openssl/pull/66), which I neglected to
report to rt.
The following 2 issues are addressed by the patch:
1. The current implementation includes 16 bytes of random data in the
payload (after the sequence number). To clarify, RFC 6520 requires 16
bytes minimum of random _padding_. There's no such requirement on the
payload, which may be arbitrary - but this is essentially pointless (or
worse). Furthermore, the response processor is not verifying that the
response payload actually echoes this data correctly. The patch removes
this extra random data from heartbeat requests, also making the
verification issue moot.
2. For a long-running connection, tlsext_hb_seq could grow beyond 16
bits (where sizeof(unsigned int) > 2), so the response processor should
only compare that the bottom 16 bits match. The patch changes that
comparison (alternatively tlsext_hb_seq++ could explicitly wrap to 0).
Regards,
Pete Dettman
-------------- next part --------------
diff --git a/ssl/d1_both.c b/ssl/d1_both.c
index 155b8bf..a227b7e 100644
--- a/ssl/d1_both.c
+++ b/ssl/d1_both.c
@@ -1404,15 +1404,16 @@ int dtls1_process_heartbeat(SSL *s, unsigned char *p, unsigned int length)
unsigned int seq;
/*
- * We only send sequence numbers (2 bytes unsigned int), and 16
- * random bytes, so we just try to read the sequence number
+ * We only send sequence numbers (2 bytes unsigned int),
+ * so we confirm the response payload matches that.
*/
- n2s(pl, seq);
-
- if (payload == 18 && seq == s->tlsext_hb_seq) {
- dtls1_stop_timer(s);
- s->tlsext_hb_seq++;
- s->tlsext_hb_pending = 0;
+ if (2 == payload) {
+ n2s(pl, seq);
+ if (seq == (s->tlsext_hb_seq & 0xffff)) {
+ dtls1_stop_timer(s);
+ s->tlsext_hb_seq++;
+ s->tlsext_hb_pending = 0;
+ }
}
}
@@ -1423,7 +1424,7 @@ int dtls1_heartbeat(SSL *s)
{
unsigned char *buf, *p;
int ret = -1;
- unsigned int payload = 18; /* Sequence number + random bytes */
+ unsigned int payload = 2; /* Sequence number */
unsigned int padding = 16; /* Use minimum padding */
/* Only send if peer supports and accepts HB requests... */
@@ -1453,13 +1454,11 @@ int dtls1_heartbeat(SSL *s)
/*-
* Create HeartBeat message, we just use a sequence number
- * as payload to distuingish different messages and add
- * some random stuff.
+ * as payload to distinguish different messages.
* - Message Type, 1 byte
* - Payload Length, 2 bytes (unsigned int)
* - Payload, the sequence number (2 bytes uint)
- * - Payload, random bytes (16 bytes uint)
- * - Padding
+ * - Padding (16 random bytes)
*/
buf = OPENSSL_malloc(1 + 2 + payload + padding);
if (buf == NULL) {
@@ -1469,16 +1468,10 @@ int dtls1_heartbeat(SSL *s)
p = buf;
/* Message Type */
*p++ = TLS1_HB_REQUEST;
- /* Payload length (18 bytes here) */
+ /* Payload length */
s2n(payload, p);
- /* Sequence number */
+ /* Sequence number (low 16 bits) */
s2n(s->tlsext_hb_seq, p);
- /* 16 random bytes */
- if (RAND_bytes(p, 16) <= 0) {
- SSLerr(SSL_F_DTLS1_HEARTBEAT, ERR_R_INTERNAL_ERROR);
- goto err;
- }
- p += 16;
/* Random padding */
if (RAND_bytes(p, padding) <= 0) {
SSLerr(SSL_F_DTLS1_HEARTBEAT, ERR_R_INTERNAL_ERROR);
diff --git a/ssl/t1_lib.c b/ssl/t1_lib.c
index 402047a..d7938f5 100644
--- a/ssl/t1_lib.c
+++ b/ssl/t1_lib.c
@@ -3636,14 +3636,15 @@ int tls1_process_heartbeat(SSL *s, unsigned char *p, unsigned int length)
unsigned int seq;
/*
- * We only send sequence numbers (2 bytes unsigned int), and 16
- * random bytes, so we just try to read the sequence number
+ * We only send sequence numbers (2 bytes unsigned int),
+ * so we confirm the response payload matches that.
*/
- n2s(pl, seq);
-
- if (payload == 18 && seq == s->tlsext_hb_seq) {
- s->tlsext_hb_seq++;
- s->tlsext_hb_pending = 0;
+ if (2 == payload) {
+ n2s(pl, seq);
+ if (seq == (s->tlsext_hb_seq & 0xffff)) {
+ s->tlsext_hb_seq++;
+ s->tlsext_hb_pending = 0;
+ }
}
}
@@ -3654,7 +3655,7 @@ int tls1_heartbeat(SSL *s)
{
unsigned char *buf, *p;
int ret = -1;
- unsigned int payload = 18; /* Sequence number + random bytes */
+ unsigned int payload = 2; /* Sequence number */
unsigned int padding = 16; /* Use minimum padding */
/* Only send if peer supports and accepts HB requests... */
@@ -3684,13 +3685,11 @@ int tls1_heartbeat(SSL *s)
/*-
* Create HeartBeat message, we just use a sequence number
- * as payload to distuingish different messages and add
- * some random stuff.
+ * as payload to distinguish different messages.
* - Message Type, 1 byte
* - Payload Length, 2 bytes (unsigned int)
* - Payload, the sequence number (2 bytes uint)
- * - Payload, random bytes (16 bytes uint)
- * - Padding
+ * - Padding (16 random bytes)
*/
buf = OPENSSL_malloc(1 + 2 + payload + padding);
if (buf == NULL) {
@@ -3700,16 +3699,10 @@ int tls1_heartbeat(SSL *s)
p = buf;
/* Message Type */
*p++ = TLS1_HB_REQUEST;
- /* Payload length (18 bytes here) */
+ /* Payload length */
s2n(payload, p);
- /* Sequence number */
+ /* Sequence number (low 16 bits) */
s2n(s->tlsext_hb_seq, p);
- /* 16 random bytes */
- if (RAND_bytes(p, 16) <= 0) {
- SSLerr(SSL_F_TLS1_HEARTBEAT, ERR_R_INTERNAL_ERROR);
- goto err;
- }
- p += 16;
/* Random padding */
if (RAND_bytes(p, padding) <= 0) {
SSLerr(SSL_F_TLS1_HEARTBEAT, ERR_R_INTERNAL_ERROR);
-------------- next part --------------
_______________________________________________
openssl-bugs-mod mailing list
openssl-bugs-mod at openssl.org
https://mta.openssl.org/mailman/listinfo/openssl-bugs-mod
More information about the openssl-dev
mailing list