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

[Frank Li]

Hi Nikos and Peter,

Thanks for the quick confirmation and reply. The heartbeat RFC specs are
indeed a bit odd. My suggestions would be to update whenever convenient the
if (len < 4) check to if (len <19), and the if (hb_len > len - 3) to if
(hb_len > len - 19).

As for client compatibility, I'm inclined to believe the above changes
should not be a serious issue. The RFC specs are quite clear (for unclear
reasons) on the message structure, and other TLS implementations I've seen
have not made a mistake about sending a 16 byte padding. In fact, openSSL's
bounds check protecting against Heartbleed checks that (hb_len == len -
19), enforcing an exactly 16 byte padding. I'd imagine if gnuTLS included
those bounds, any reasonable client should decide to follow the RFC for
sake of working correctly with both openSSL and gnuTLS :P

Anyhow, this is not a priority bug, just wanted to inform you guys (noted
this when investigating Heartbleed).

Cheers!

Frank





On Fri, Apr 11, 2014 at 4:46 AM, Nikos Mavrogiannopoulos <nmav at gnutls.org>wrote:

> 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: </pipermail/attachments/20140411/b5b1bbf1/attachment-0001.html>