[gnutls-devel] Incorrect SSL heartbeat bounds checking (not exploitable)

[Peter Dettman]

Hi All,

I concur with Frank, in that this code is not enforcing a minimum length 
for the padding of 16 bytes as specified in RFC 6520.

       if (len < 4)
         return  gnutls_assert_val(GNUTLS_E_UNEXPECTED_PACKET_LENGTH);

The minimum packet length is 19, so that should be "len < 19".

       if (hb_len > len - 3)
         return gnutls_assert_val(GNUTLS_E_UNEXPECTED_PACKET_LENGTH);

The payload is bounded by the minimum 19 bytes for non-payload data, so 
this should be "hb_len > len - 19". I would wonder also whether that is 
the most appropriate error code for a malformed message, but I don't 
know enough about the available error codes to judge.

A further comment on the message format:

    struct {
       HeartbeatMessageType type;
       uint16 payload_length;
       opaque payload[HeartbeatMessage.payload_length];
       opaque padding[padding_length];
    } HeartbeatMessage;
  

The "padding_length" is defined implicitly here i.e. "whatever is left 
in the record following the previous fields" (subject to being at least 
16 bytes). That's quite a strange choice; the only other example of such 
a thing I can think of is ClientHello.extensions, where there is a more 
apparent justification. Why not explicitly include the padding_length?

Perhaps more seriously, requiring that it be filled with "random padding 
of at least 16 bytes" is 1) semantically pretty meaningless (later it 
says that the receiver MUST ignore the contents anyway!), and 2) 
suspicious in the light of DUAL_EC_DRBG and other RNG-related 
revelations. I would advise to review what randomness is used by GnuTLS 
here, and I suggest a deterministic construction of the TLS PRF keyed on 
a hash of the payload  (by analogy to RFC 6979) should be considered 
instead of actual random data (especially if the same source is used for 
key material).

Regards
Pete Dettman

On 11/04/2014 8:34 AM, Frank Li wrote:
> This may have already been discovered due to Heartbleed. gnutls's TLS 
> heartbeat implementation is not vulnerable to heartbleed but also does 
> not check the heartbeat length field matches the payload length. A 
> heartbeat response may be larger than the heartbeat request. I bring 
> this up for correctness and RFC6520-adherence sake.