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

[Frank Li]

Hi,

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.

Cheers,

Frank Li

Proof of concept: I think the below code is clear enough. I have tested
using openSSL's s_client to send a heartbeat request where the payload
length field is 1 larger than the payload, and I preset the padding. I see
my first bye of padding return.

I explain the error in comments //*** comment **//

In gnutls's lib/ext/heartbeat.c:

int _gnutls_heartbeat_handle(gnutls_session_t session, mbuffer_st * bufel)
{
  int ret;
  unsigned type;
  unsigned pos;
  uint8_t *msg = _mbuffer_get_udata_ptr(bufel);
  size_t hb_len, len = _mbuffer_get_udata_size(bufel);

  if (gnutls_heartbeat_allowed(session, GNUTLS_HB_PEER_ALLOWED_TO_SEND) ==
0)
    return gnutls_assert_val(GNUTLS_E_UNEXPECTED_PACKET);

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

  pos = 0;
  type = msg[pos++];

////**** hb_len is simply the length field (2 bytes) from heartbeat packet
***////
  hb_len = _gnutls_read_uint16(&msg[pos]);

////**** hb_len can be as big as len(padding)+len(payload). So not
vulnerable to heartbleed***////
if (hb_len > len - 3)
    return
        gnutls_assert_val(GNUTLS_E_UNEXPECTED_PACKET_LENGTH);

  pos += 2;

  switch (type) {
  case HEARTBEAT_REQUEST:
    _gnutls_buffer_reset(&session->internals.hb_remote_data);

////**** buffer sized to that of hb_len, which could be wrong (like in
heartbleed)***////
    ret =
        _gnutls_buffer_resize(&session->internals.
            hb_remote_data, hb_len);
    if (ret < 0)
      return gnutls_assert_val(ret);

////**** copying hb_len of data, which can reading bytes in padding (no
further). Hence this is not vulnerable, but still is an unchecked bound and
not RFC compliant ***////
    if (hb_len > 0)
      memcpy(session->internals.hb_remote_data.data,
             &msg[pos], hb_len);
    session->internals.hb_remote_data.length = hb_len;

    return gnutls_assert_val(GNUTLS_E_HEARTBEAT_PING_RECEIVED);

  case HEARTBEAT_RESPONSE:
.... (end of important stuff)
-------------- next part --------------
An HTML attachment was scrubbed...
URL: </pipermail/attachments/20140410/4e616063/attachment.html>