Eyebleed. A technical analysis of the fix (not the bug!) for the …

Heartbleed (CVE-2014-0160) is a bug found in OpenSSL due to a misimplementation of the Heartbeat functionality, and found by Neel Mehta (Google Security Team)

A lot of people (Existentialize, IOActive, etc.) already provided an analysis of the CVE-2014-0160 bug, widely known under the name “Heartbleed”, but nobody has analyzed the fix yet.

I had a look at both the code and the RFC, and it raised few questions to me.

1. The fix (and the original code)

We can see below a copy of the fixed-version of the dtls1_process_heartbeat() function.

1. int

2. dtls1_process_heartbeat(SSL *s)

4. unsigned char *p = &s->s3->rrec.data[0], *pl;

5. unsigned short hbtype;

6. unsigned int payload;

7. unsigned int padding = 16; /* Use minimum padding */

9. if (s->msg_callback)

10. s->msg_callback(0, s->version, TLS1_RT_HEARTBEAT,

11. &s->s3->rrec.data[0], s->s3->rrec.length,

12. s, s->msg_callback_arg);

14. /* Read type and payload length first */

15. if (1 + 2 + 16 > s->s3->rrec.length)

16. return 0; /* silently discard */

17. hbtype = *p++;

18. n2s(p, payload);

19. if (1 + 2 + payload + 16 > s->s3->rrec.length)

20. return 0; /* silently discard per RFC 6520 sec. 4 */

21. pl = p;

23. if (hbtype == TLS1_HB_REQUEST)

25. unsigned char *buffer, *bp;

26. unsigned int write_length = 1 /* heartbeat type */ +

27. 2 /* heartbeat length */ +

28. payload + padding;

29. int r;

31. if (write_length > SSL3_RT_MAX_PLAIN_LENGTH)

32. return 0;

34. /* Allocate memory for the response, size is 1 byte

35. * message type, plus 2 bytes payload length, plus

36. * payload, plus padding

37. */

38. buffer = OPENSSL_malloc(write_length);

39. bp = buffer;

41. /* Enter response type, length and copy payload */

42. *bp++ = TLS1_HB_RESPONSE;

43. s2n(payload, bp);

44. memcpy(bp, pl, payload); < ==== The initial bug was here, because it was copying “too-much” data from the source buffer.

45. bp += payload;

46. /* Random padding */

47. RAND_pseudo_bytes(bp, padding);

49. r = dtls1_write_bytes(s, TLS1_RT_HEARTBEAT, buffer, write_length);

51. if (r >= 0 && s->msg_callback)

52. s->msg_callback(1, s->version, TLS1_RT_HEARTBEAT,

53. buffer, write_length,

54. s, s->msg_callback_arg);

56. OPENSSL_free(buffer);

58. if (r < 0)

59. return r;

1.1 Misleading comments & undefined constants

I found the function so hard to read that it looked like it was obfuscated JavaScript code, especially with all those undefined constants everywhere and misleading comments. For instance, what is described as “heartbeat length” is in fact the “payload length of the heartbeat message” but what intrigued me the most was the static constant for “padding length” (mentioned as “padding” grrr.)

1.2 Padding Length

After a look at the RFC 6520 “Transport Layer Security (TLS) and Datagram Transport Layer Security (DTLS) Heartbeat Extension”, I didn’t see any point mentioning that padding length was supposed to be a have static size (16) as done in the code. On the contrary, it is quite confusing to understand who is supposed to take care of it.

1.2.1 RFC 6520. Heartbeat messages

RFC 6520: Therefore, the padding_length is TLSPlaintext.length – payload_length – 3 for TLS and
DTLSPlaintext.length – payload_length – 3 for DTLS. The padding_length MUST be at least 16.
The sender of a HeartbeatMessage MUST use a random padding of at
least 16 bytes. The padding of a received HeartbeatMessage message
MUST be ignored.

The Heartbeat protocol messages consist of their type and an arbitrary payload and padding.

Read the full RFC 6520.

1.2.2 My understanding

My understanding is that there are two types of messages:

HeartbeatResponse
HeartbeatRequest

And that *both* have a padding, including the padding of a received HeartbeatMessage message MUST be ignored. But by that, I understand that the RFC it means the content and not the field itself.
This is my I concluded that OpenSSL should not be the one deciding of the size of the padding buffer, which therefore would have prevented to allocate a buffer and to use memcpy().

Below is my pseudo-code interpretation of the RFC 6520 and what the code should be:

1. typedef struct heartbeat_msg_st

3. /*

4. RFC 6520: The total length of a HeartbeatMessage MUST NOT exceed 2^14 or

5. max_fragment_length when negotiated as defined in [RFC6066].

6. */

7. char type;

8. unsigned short payload_length;

9. unsigned char payload[1]; /* Any size array. RFC 6520 only mentions “arbitrary content” which is (probably?) assumed to be non-empty. */

10. unsigned char padding[16]; /* The padding_length MUST be at least 16. */

11. HEARTBEAT_MSG;

13. int

14. dtls1_process_heartbeat(SSL *s)

16. HEARTBEAT_MSG *hb_msg = &s->s3->rrec.data[0];

17. char hb_type;

18. unsigned short payload_length;

19. unsigned int padding_length;

20. unsigned int hb_msg_length = s->s3->rrec.length;

22. if (s->msg_callback)

23. s->msg_callback(0, s->version, TLS1_RT_HEARTBEAT,

24. hb_msg, hb_msg_length,

25. s, s->msg_callback_arg);

27. if (hb_msg_length < sizeof(HEARTBEAT_MSG)) return 0; // silently discard.

28. padding_length = hb_msg_length – hb_msg->payload_length – sizeof(payload_length) – sizeof(hb_type);

29. if (padding_length < 16) return 0; // silently discard.

31. hb_type = hb_msg->type;

32. payload_length = hb_msg->payload_length;

34. if (hb_type == TLS1_HB_REQUEST)

36. unsigned char *padding = (unsigned char *)&hb_msg;

37. padding = &padding[hb_msg_length – padding_length];

38. if (hb_msg_length > SSL3_RT_MAX_PLAIN_LENGTH) return 0;

40. hb_msg->type = TLS1_HB_RESPONSE;

42. /* Random padding */

43. RAND_pseudo_bytes(padding, padding_length);

45. r = dtls1_write_bytes(s, TLS1_RT_HEARTBEAT, hb_msg, hb_msg_length);

47. if (r >= 0 && s->msg_callback)

48. s->msg_callback(1, s->version, TLS1_RT_HEARTBEAT,

49. hb_msg, hb_msg_length,

50. s, s->msg_callback_arg);

52. if (r < 0) return r;

2. Any thoughts ?

Reading a RFC can be hard, including widely deployed code such as OpenSSL. I’d be curious to hear (@msuiche) your interpretation of the RFC.

Source:  

Eyebleed. A technical analysis of the fix (not the bug!) for the …

See which stocks are being affected by Social Media