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

[Peter Williams]

general comment.


Dont think TOO much in terms of per-layer countermeasures (i.e. dont assume that the attack vector is via just one layer) Dont forget to think like those who have designed packet-based  exploits for decades now - and assume that folks deliver malformed packets in order to alter the behaviour of the driver STACK-  an ability enabled because of their packet-handlers coding style and pre-placement by the OS/networking vendor of code so as to accept the exploit (or become an exploit typically by inducing a timing based covert channel). Assume the very coding style of drivers and packet handlers is what allows for the class of exploit, that is; and that the exploitability is a function of the COMBINATION of handlers.


For 40+ years, folks have designed C structures like this, for messaging and packet handling. And , if one looks back over 40 years of telco-linked computing at HOW exploits were delivered or effected via networking packet based methods of altering the packet handler OR the protocol handler, its always the same story.


be careful. Even as late as 1990, you’d be taken out and shot for addressing these topics publicly. There is a lot of still secret long-term vendor/govt  politics around this topic, and NOT all of it/was badly motivated.








From: Peter Dettman
Sent: ‎Friday‎, ‎April‎ ‎11‎, ‎2014 ‎2‎:‎55‎ ‎AM
To: gnutls-devel at lists.gnutls.org





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.


_______________________________________________
Gnutls-devel mailing list
Gnutls-devel at lists.gnutls.org
http://lists.gnupg.org/mailman/listinfo/gnutls-devel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: </pipermail/attachments/20140411/472da2fd/attachment.html>