Re: tls1_process_heartbeat/dtls1_process_heartbeat don't check RAND_pseudo_bytes return value

[Kylo Ginsberg]

On Thu, Apr 10, 2014 at 12:04 PM, Joseph Birr-Pixton jpix...@gmail.comwrote:

 On 10 April 2014 18:54, Kylo Ginsberg k...@kylo.net wrote:
  Looking at the heartbeat code, I notice that neither of the process
  heartbeat functions check whether RAND_pseudo_bytes returned success
 when it
  is generating the heartbeat padding.
 
  I don't know if there are real-world scenarios where this could happen

 Failed memory allocation, typically.


Alterntely, in rand_lib.c, if RAND_get_rand_method returns null or its
pseudorand method is null, it will return -1. So there may be multiple
failure modes that expose this hole.


  A patch might look like this:
 
  diff --git a/ssl/d1_both.c b/ssl/d1_both.c
  +   if (RAND_pseudo_bytes(bp, padding)  0)

 RAND_pseudo_bytes returns -1 or 0 if it fails[1]. This expression
 should be RAND_pseudo_bytes(...) != 1, which basically equivalent to
 RAND_bytes(...) != 1.

 This isn't your fault; the documentation doesn't have any relationship
 to the actual behaviour, and the many other callers in the library are
 sloppy like this.


I did find the docs here:
https://www.openssl.org/docs/crypto/RAND_bytes.html

I chose the ' 0' comparison because:

a) I thought either 0 or 1 would be okay in this case (I don't think the
random bytes have to be cryptographically sound for the heartbeat padding).

b) I was trying to follow the style of the code base, although it's not
consistent. Most of the sites in the code that check the return from
RAND_pseudo_bytes use a = 0 or a  0 (though some do direct comparisons as
you suggest). So I went with what seemed to be most typical practice for
this code base.

All that said, I mostly wanted confirmation that this is a bug. I'd be fine
with tweaking the comparison to be != 1.

Thanks for the comments - much appreciated!

Kylo

tls1_process_heartbeat/dtls1_process_heartbeat don't check RAND_pseudo_bytes return value

[Kylo Ginsberg]

Looking at the heartbeat code, I notice that neither of the process
heartbeat functions check whether RAND_pseudo_bytes returned success when
it is generating the heartbeat padding.

I don't know if there are real-world scenarios where this could happen, but
if so, I believe this would bleed 16 bytes of heap memory in the
heartbeat response.

A patch might look like this:

diff --git a/ssl/d1_both.c b/ssl/d1_both.c
index d8bcd58..a607d8c 100644
--- a/ssl/d1_both.c
+++ b/ssl/d1_both.c
@@ -1367,8 +1367,13 @@ dtls1_process_heartbeat(SSL *s)
s2n(payload, bp);
memcpy(bp, pl, payload);
bp += payload;
+
/* Random padding */
-   RAND_pseudo_bytes(bp, padding);
+   if (RAND_pseudo_bytes(bp, padding)  0)
+   {
+   OPENSSL_free(buffer);
+   return 0;
+   }

r = dtls1_write_bytes(s, TLS1_RT_HEARTBEAT, buffer,
write_length);

diff --git a/ssl/t1_lib.c b/ssl/t1_lib.c
index bcb99b8..f1a53e6 100644
--- a/ssl/t1_lib.c
+++ b/ssl/t1_lib.c
@@ -4000,8 +4000,13 @@ tls1_process_heartbeat(SSL *s)
s2n(payload, bp);
memcpy(bp, pl, payload);
bp += payload;
+
/* Random padding */
-   RAND_pseudo_bytes(bp, padding);
+   if (RAND_pseudo_bytes(bp, padding)  0)
+   {
+   OPENSSL_free(buffer);
+   return 0;
+   }

r = ssl3_write_bytes(s, TLS1_RT_HEARTBEAT, buffer, 3 +
payload + padding);

Comments?

Thanks,
Kylo