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

[Nikos Mavrogiannopoulos]

On Fri, Apr 11, 2014 at 11:02 AM, Peter Dettman
<peter.dettman at bouncycastle.org> wrote:
> 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.

Thank you Frank and Peter for investigating the issue.

If I understand correctly what you say Frank, I don't think we can do
much about it. The payload is set by the sender, so if he sets it
incorrectly we cannot do anything than believe him (given the fact
that the given structure allows unaccountable data). Being more strict
as Peter says and ensuring that padding is at least 16 bytes would
help.

>       if (len < 4)
>         return  gnutls_assert_val(GNUTLS_E_UNEXPECTED_PACKET_LENGTH);
> The minimum packet length is 19, so that should be "len < 19".

The checks are check as you go, so the code only checks whether the
data that are going to be read next actually exist. It's a nice idea
to be updated to ensure that padding_length is at least 16 bytes, but
I'm afraid that given the inexplicable reason for padding, there may
be implementations that don't follow that requirement correctly. In
any case, I'll try to update it to be more precise.

> 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?

Yes, that's a pretty weird decision. Unfortunately I do not remember
much of the TLS heartbeat extension discussion in TLS working group.
The distinction between payload and padding (and the requirement to
have a padding of at least 16), is there for undocumented reasons. I
cannot guess the reason it is there.

 and 2) suspicious in the
> light of DUAL_EC_DRBG and other RNG-related revelations. I would advise to

I remember that this was not the choice of the authors. That change
was forced by the IESG reviewers.
http://www.ietf.org/mail-archive/web/tls/current/msg08311.html

> 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).

I'd be afraid to introduce more complexity by an rng only for that
code (which is really rarely enabled/used). In 3.3.0 I've separated
the rng to generate keys from the rng that generates nonces as in that
case, and I believe that should be sufficient.

regards,
Nikos