Skip to content

Commit

Permalink
PR: 2658
Browse files Browse the repository at this point in the history
Submitted by: Robin Seggelmann <seggelmann@fh-muenster.de>
Reviewed by: steve

Support for TLS/DTLS heartbeats.
  • Loading branch information
snhenson committed Dec 31, 2011
1 parent 84b6e27 commit 4817504
Show file tree
Hide file tree
Showing 20 changed files with 561 additions and 4 deletions.
3 changes: 3 additions & 0 deletions CHANGES
Original file line number Diff line number Diff line change
Expand Up @@ -255,6 +255,9 @@

Changes between 1.0.0f and 1.0.1 [xx XXX xxxx]

*) Add support for TLS/DTLS heartbeats.
[Robin Seggelmann <seggelmann@fh-muenster.de>]

*) Improved PRNG seeding for VOS.
[Paul Green <Paul.Green@stratus.com>]

Expand Down
20 changes: 20 additions & 0 deletions apps/s_cb.c
Original file line number Diff line number Diff line change
Expand Up @@ -603,6 +603,26 @@ void MS_CALLBACK msg_cb(int write_p, int version, int content_type, const void *
}
}
}

#ifndef OPENSSL_NO_HEARTBEATS
if (content_type == 24) /* Heartbeat */
{
str_details1 = ", Heartbeat";

if (len > 0)
{
switch (((const unsigned char*)buf)[0])
{
case 1:
str_details1 = ", HeartbeatRequest";
break;
case 2:
str_details1 = ", HeartbeatResponse";
break;
}
}
}
#endif
}

BIO_printf(bio, "%s %s%s [length %04lx]%s%s\n", str_write_p, str_version, str_content_type, (unsigned long)len, str_details1, str_details2);
Expand Down
8 changes: 8 additions & 0 deletions apps/s_client.c
Original file line number Diff line number Diff line change
Expand Up @@ -1862,6 +1862,14 @@ printf("read=%d pending=%d peek=%d\n",k,SSL_pending(con),SSL_peek(con,zbuf,10240
SSL_renegotiate(con);
cbuf_len=0;
}
#ifndef OPENSSL_NO_HEARTBEATS
else if ((!c_ign_eof) && (cbuf[0] == 'B'))
{
BIO_printf(bio_err,"HEARTBEATING\n");
SSL_heartbeat(con);
cbuf_len=0;
}
#endif
else
{
cbuf_len=i;
Expand Down
10 changes: 10 additions & 0 deletions apps/s_server.c
Original file line number Diff line number Diff line change
Expand Up @@ -2222,6 +2222,16 @@ static int sv_body(char *hostname, int s, unsigned char *context)
goto err;
}

#ifndef OPENSSL_NO_HEARTBEATS
if ((buf[0] == 'B') &&
((buf[1] == '\n') || (buf[1] == '\r')))
{
BIO_printf(bio_err,"HEARTBEATING\n");
SSL_heartbeat(con);
i=0;
continue;
}
#endif
if ((buf[0] == 'r') &&
((buf[1] == '\n') || (buf[1] == '\r')))
{
Expand Down
152 changes: 151 additions & 1 deletion ssl/d1_both.c
Original file line number Diff line number Diff line change
Expand Up @@ -1084,7 +1084,11 @@ int dtls1_read_failed(SSL *s, int code)
return code;
}

if ( ! SSL_in_init(s)) /* done, no need to send a retransmit */
#ifndef OPENSSL_NO_HEARTBEATS
if (!SSL_in_init(s) && !s->tlsext_hb_pending) /* done, no need to send a retransmit */
#else
if (!SSL_in_init(s)) /* done, no need to send a retransmit */
#endif
{
BIO_set_flags(SSL_get_rbio(s), BIO_FLAGS_READ);
return code;
Expand Down Expand Up @@ -1438,3 +1442,149 @@ int dtls1_shutdown(SSL *s)
#endif
return ret;
}

#ifndef OPENSSL_NO_HEARTBEATS
int
dtls1_process_heartbeat(SSL *s)
{
unsigned char *p = &s->s3->rrec.data[0], *pl;
unsigned short hbtype;
unsigned int payload;
unsigned int padding = 16; /* Use minimum padding */

/* Read type and payload length first */
hbtype = *p++;
n2s(p, payload);
pl = p;

if (s->msg_callback)
s->msg_callback(0, s->version, TLS1_RT_HEARTBEAT,
&s->s3->rrec.data[0], s->s3->rrec.length,
s, s->msg_callback_arg);

if (hbtype == TLS1_HB_REQUEST)
{
unsigned char *buffer, *bp;
int r;

/* Allocate memory for the response, size is 1 byte
* message type, plus 2 bytes payload length, plus
* payload, plus padding
*/
buffer = OPENSSL_malloc(1 + 2 + payload + padding);
bp = buffer;

/* Enter response type, length and copy payload */
*bp++ = TLS1_HB_RESPONSE;
s2n(payload, bp);
memcpy(bp, pl, payload);

This comment has been minimized.

Copy link
@kallus

kallus Apr 10, 2014

Heartbleed explanation: This line copies payload bytes from pl to bp. But payload contains the length that the client says that pl has, it has not been checked that the client doesn't lie about the length of pl. So if the client sent less data than it said it would, whatever lies after pl in memory also gets copied to bp. A few lines below, buffer (which is bp and its header) gets sent back to the client. Thanks to http://blog.existentialize.com/diagnosis-of-the-openssl-heartbleed-bug.html for explaining this.

In defense of the person who committed this code, it can be argued that a bad practice in another place in the code base made this bug much harder to find. http://article.gmane.org/gmane.os.openbsd.misc/211963

What about the very similar code in ssl/t1_lib.c below, is something subtle making it safe or is that also part of the bug?

This comment has been minimized.

Copy link
@derekm

derekm Apr 11, 2014

Notice a difference in the Internet-Draft and the final RFC:

https://tools.ietf.org/html/draft-seggelmann-tls-dtls-heartbeat-01
vs.
https://tools.ietf.org/html/rfc6520

The draft leaves out the length, as if implementations will read in the payload as a null-terminated string. Implementation of the draft under those assumptions would not have led to a security compromise, and I have to believe the authors of the draft had a working test implementation of their draft specification.

IMHO, the RFC and the OpenSSL reference implementation were designed to intentionally introduce this security vulnerability.

Prior drafts and RFCs from R. Seggelmann attribute his employment to T-Systems, a subsidiary of T-Mobile, a subsidiary of Deutsche Telekom, formerly a state-owned monopoly which continues to maintain a close relationship with BND (Federal Intelligence Service).

This comment has been minimized.

Copy link
@kallus

kallus Apr 11, 2014

No, a lot of protocols specify the payload length in the header, so that is not a reason to believe that this bug was intentionally introduced by anyone.

This comment has been minimized.

Copy link
@derekm

derekm Apr 11, 2014

kallus,

Re: "What about the very similar code in ssl/t1_lib.c below, is something subtle making it safe or is that also part of the bug?"

They both contain the same bug with the same malicious result. The code here is for UDP packets, the code below is for TCP packets. The exploits seen in-the-wild against major Internet companies would have followed the code path below, since HTTP sits atop TCP.

This comment has been minimized.

Copy link
@colinmollenhour

colinmollenhour Apr 11, 2014

@kallus What could possibly be the purpose of having a variable length in a heartbeat packet other than to introduce this gaping security hole? Either someone was forced to introduce this bug, turned, or paid. Accidentally introducing this bug would be incredibly careless for an intermediate programmer, and just downright stupid for anyone who has commit access on OpenSSL. This is a classic buffer overflow that anyone with 300-level Computer Science knowledge should know better than to introduce. Of course this will never be investigated because we can all make a very good guess as to who was probably behind it..

This comment has been minimized.

Copy link
@derekm

derekm Apr 11, 2014

colinmollenhour,

The variable length was to support re-using this extension for Path MTU Discovery on the UDP side of things (where you need to send messages of increasing length up to the max MTU until you see a packet drop, then you know the smallest MTU along the entire path). The addition of a separate random padding presumably keeps the length field from becoming a vector for a known-plaintext attack. Not sure the purpose of a padding before the addition of yet another length header (beyond {,D}TLSPlaintext.length), except maybe to keep the reply to your heartbeat small while continually increasing the size of the packets while discovering Path MTU in UDP.

However, the length field may remain a vector for known-plaintext attacks since the default padding length is not randomized, but is set to 16 bytes in both request and response when not doing Path MTU Discovery. So the length field can become known by taking the length of the ciphertext and subtracting 19 (3 bytes for header, 16 bytes for padding).

This comment has been minimized.

Copy link
@kallus

kallus Apr 11, 2014

@colinmollenhour some reasons for a variable length heartbeat packet are listed here https://news.ycombinator.com/item?id=7572343

Most bugs look simple after they have been found.

This comment has been minimized.

Copy link
@colinmollenhour

colinmollenhour Apr 11, 2014

How would a fixed-length payload preclude these additional uses, and do they really belong in a security-layer?

Any time one uses memcpy, strcpy, strncpy, etc. it is a given that there is buffer-over/underflow risk and the cardinal rule should be observed: never trust user input. I'm sure the first place hackers look for holes is uses of memcpy. This one is not buried in a complicated wrapper or anything, it is just right there!

/* Random padding */
RAND_pseudo_bytes(p, padding);

r = dtls1_write_bytes(s, TLS1_RT_HEARTBEAT, buffer, 3 + payload + padding);

if (r >= 0 && s->msg_callback)
s->msg_callback(1, s->version, TLS1_RT_HEARTBEAT,
buffer, 3 + payload + padding,
s, s->msg_callback_arg);

OPENSSL_free(buffer);

if (r < 0)
return r;
}
else if (hbtype == TLS1_HB_RESPONSE)
{
unsigned int seq;

/* We only send sequence numbers (2 bytes unsigned int),
* and 16 random bytes, so we just try to read the
* sequence number */
n2s(pl, seq);

if (payload == 18 && seq == s->tlsext_hb_seq)
{
dtls1_stop_timer(s);
s->tlsext_hb_seq++;
s->tlsext_hb_pending = 0;
}
}

return 0;
}

int
dtls1_heartbeat(SSL *s)
{
unsigned char *buf, *p;
int ret;
unsigned int payload = 18; /* Sequence number + random bytes */
unsigned int padding = 16; /* Use minimum padding */

/* Only send if peer supports and accepts HB requests... */
if (!(s->tlsext_heartbeat & SSL_TLSEXT_HB_ENABLED) ||
s->tlsext_heartbeat & SSL_TLSEXT_HB_DONT_SEND_REQUESTS)
{
SSLerr(SSL_F_DTLS1_HEARTBEAT,SSL_R_TLS_HEARTBEAT_PEER_DOESNT_ACCEPT);
return -1;
}

/* ...and there is none in flight yet... */
if (s->tlsext_hb_pending)
{
SSLerr(SSL_F_DTLS1_HEARTBEAT,SSL_R_TLS_HEARTBEAT_PENDING);
return -1;
}

/* ...and no handshake in progress. */
if (SSL_in_init(s) || s->in_handshake)
{
SSLerr(SSL_F_DTLS1_HEARTBEAT,SSL_R_UNEXPECTED_MESSAGE);
return -1;
}

/* Check if padding is too long, payload and padding
* must not exceed 2^14 - 3 = 16381 bytes in total.
*/
OPENSSL_assert(payload + padding <= 16381);

/* Create HeartBeat message, we just use a sequence number
* as payload to distuingish different messages and add
* some random stuff.
* - Message Type, 1 byte
* - Payload Length, 2 bytes (unsigned int)
* - Payload, the sequence number (2 bytes uint)
* - Payload, random bytes (16 bytes uint)
* - Padding
*/
buf = OPENSSL_malloc(1 + 2 + payload + padding);
p = buf;
/* Message Type */
*p++ = TLS1_HB_REQUEST;
/* Payload length (18 bytes here) */
s2n(payload, p);
/* Sequence number */
s2n(s->tlsext_hb_seq, p);
/* 16 random bytes */
RAND_pseudo_bytes(p, 16);
p += 16;
/* Random padding */
RAND_pseudo_bytes(p, padding);

ret = dtls1_write_bytes(s, TLS1_RT_HEARTBEAT, buf, 3 + payload + padding);
if (ret >= 0)
{
if (s->msg_callback)
s->msg_callback(1, s->version, TLS1_RT_HEARTBEAT,
buf, 3 + payload + padding,
s, s->msg_callback_arg);

dtls1_start_timer(s);
s->tlsext_hb_pending = 1;
}

OPENSSL_free(buf);

return ret;
}
#endif
13 changes: 13 additions & 0 deletions ssl/d1_clnt.c
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,19 @@ int dtls1_connect(SSL *s)
BIO_ctrl(SSL_get_wbio(s), BIO_CTRL_DGRAM_SCTP_SET_IN_HANDSHAKE, s->in_handshake, NULL);
#endif

#ifndef OPENSSL_NO_HEARTBEATS
/* If we're awaiting a HeartbeatResponse, pretend we
* already got and don't await it anymore, because
* Heartbeats don't make sense during handshakes anyway.
*/
if (s->tlsext_hb_pending)
{
dtls1_stop_timer(s);
s->tlsext_hb_pending = 0;
s->tlsext_hb_seq++;
}
#endif

for (;;)
{
state=s->state;
Expand Down
8 changes: 8 additions & 0 deletions ssl/d1_lib.c
Original file line number Diff line number Diff line change
Expand Up @@ -424,6 +424,14 @@ int dtls1_handle_timeout(SSL *s)
state->timeout.read_timeouts = 1;
}

#ifndef OPENSSL_NO_HEARTBEATS
if (s->tlsext_hb_pending)
{
s->tlsext_hb_pending = 0;
return dtls1_heartbeat(s);
}
#endif

dtls1_start_timer(s);
return dtls1_retransmit_buffered_messages(s);
}
Expand Down
13 changes: 13 additions & 0 deletions ssl/d1_pkt.c
Original file line number Diff line number Diff line change
Expand Up @@ -937,6 +937,19 @@ int dtls1_read_bytes(SSL *s, int type, unsigned char *buf, int len, int peek)
dest = s->d1->alert_fragment;
dest_len = &s->d1->alert_fragment_len;
}
#ifndef OPENSSL_NO_HEARTBEATS
else if (rr->type == TLS1_RT_HEARTBEAT)
{
dtls1_process_heartbeat(s);

/* Exit and notify application to read again */
rr->length = 0;
s->rwstate=SSL_READING;
BIO_clear_retry_flags(SSL_get_rbio(s));
BIO_set_retry_read(SSL_get_rbio(s));
return(-1);
}
#endif
/* else it's a CCS message, or application data or wrong */
else if (rr->type != SSL3_RT_CHANGE_CIPHER_SPEC)
{
Expand Down
13 changes: 13 additions & 0 deletions ssl/d1_srvr.c
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,19 @@ int dtls1_accept(SSL *s)
return(-1);
}

#ifndef OPENSSL_NO_HEARTBEATS
/* If we're awaiting a HeartbeatResponse, pretend we
* already got and don't await it anymore, because
* Heartbeats don't make sense during handshakes anyway.
*/
if (s->tlsext_hb_pending)
{
dtls1_stop_timer(s);
s->tlsext_hb_pending = 0;
s->tlsext_hb_seq++;
}
#endif

for (;;)
{
state=s->state;
Expand Down
2 changes: 1 addition & 1 deletion ssl/dtls1.h
Original file line number Diff line number Diff line change
Expand Up @@ -236,7 +236,7 @@ typedef struct dtls1_state_st

struct dtls1_timeout_st timeout;

/* Indicates when the last handshake msg sent will timeout */
/* Indicates when the last handshake msg or heartbeat sent will timeout */
struct timeval next_timeout;

/* Timeout duration */
Expand Down
12 changes: 12 additions & 0 deletions ssl/s3_clnt.c
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,18 @@ int ssl3_connect(SSL *s)
s->in_handshake++;
if (!SSL_in_init(s) || SSL_in_before(s)) SSL_clear(s);

#ifndef OPENSSL_NO_HEARTBEATS
/* If we're awaiting a HeartbeatResponse, pretend we
* already got and don't await it anymore, because
* Heartbeats don't make sense during handshakes anyway.
*/
if (s->tlsext_hb_pending)
{
s->tlsext_hb_pending = 0;
s->tlsext_hb_seq++;
}
#endif

for (;;)
{
state=s->state;
Expand Down
21 changes: 21 additions & 0 deletions ssl/s3_lib.c
Original file line number Diff line number Diff line change
Expand Up @@ -3328,6 +3328,27 @@ long ssl3_ctrl(SSL *s, int cmd, long larg, void *parg)
ret = 1;
break;

#ifndef OPENSSL_NO_HEARTBEATS
case SSL_CTRL_TLS_EXT_SEND_HEARTBEAT:
if (SSL_version(s) == DTLS1_VERSION || SSL_version(s) == DTLS1_BAD_VER)
ret = dtls1_heartbeat(s);
else
ret = tls1_heartbeat(s);
break;

case SSL_CTRL_GET_TLS_EXT_HEARTBEAT_PENDING:
ret = s->tlsext_hb_pending;
break;

case SSL_CTRL_SET_TLS_EXT_HEARTBEAT_NO_REQUESTS:
if (larg)
s->tlsext_heartbeat |= SSL_TLSEXT_HB_DONT_RECV_REQUESTS;
else
s->tlsext_heartbeat &= ~SSL_TLSEXT_HB_DONT_RECV_REQUESTS;
ret = 1;
break;
#endif

#endif /* !OPENSSL_NO_TLSEXT */
default:
break;
Expand Down
13 changes: 13 additions & 0 deletions ssl/s3_pkt.c
Original file line number Diff line number Diff line change
Expand Up @@ -1070,6 +1070,19 @@ int ssl3_read_bytes(SSL *s, int type, unsigned char *buf, int len, int peek)
dest = s->s3->alert_fragment;
dest_len = &s->s3->alert_fragment_len;
}
#ifndef OPENSSL_NO_HEARTBEATS
else if (rr->type == TLS1_RT_HEARTBEAT)
{
tls1_process_heartbeat(s);

/* Exit and notify application to read again */
rr->length = 0;
s->rwstate=SSL_READING;
BIO_clear_retry_flags(SSL_get_rbio(s));
BIO_set_retry_read(SSL_get_rbio(s));
return(-1);
}
#endif

if (dest_maxlen > 0)
{
Expand Down
Loading

31 comments on commit 4817504

@CounterPillow
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job, "steve".

@jof
Copy link

@jof jof commented on 4817504 Apr 8, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

@kadafax
Copy link

@kadafax kadafax commented on 4817504 Apr 8, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

llacby8

@neur0manc
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The exact lines of code responsible for Heartbleed would be interesting. Has anyone spotted them already?

@hypertexthero
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@naxIO
Copy link

@naxIO naxIO commented on 4817504 Apr 8, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kudos anyway for contributing to open source.

@nadimkobeissi
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😿

@mark-kubacki
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And here is a PoC for the exploit: https://gist.github.com/takeshixx/10107280

@garu
Copy link

@garu garu commented on 4817504 Apr 8, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heartbeat support in OpenSSL has a critical bug and what do we do? Oh, I know! Let's harass the committer and reviewer on Github! Real mature, people.

How about, instead, congratulating them for continuously supporting and improving one of the most critical OPEN SOURCE security libraries out there (otherwise you wouldn't be here whining, amirite?), and letting them know that you support their hard work? Shit happens, and issues like this shouldn't put anyone down but instead improve processes and motivate committers on how important and valuable their work is so they keep on delivering great software for us.

And hey, if you know your C, how about engaging and helping out the OpenSSL project yourself, too?

Robin, Steve, your work matters. Thank you, and keep it up!

@nadimkobeissi
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I second @garu. Everyone who works on and contributes to OpenSSL is a hero. 👍

Huge kudos to the OpenSSL team. I'm sure they are doing their best and this bug shouldn't shake our support for the world-supporting work they're doing.

@CounterPillow
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@garu: My comment was to point out how simply writing "steve" is not satisfactory to declare who reviewed the commit, as it's not clear what "steve" that is, especially two years later.

I'm having no part in the GitHub tradition of a stale image macro circlejerk done by web-plumber hacks.

@amtal
Copy link

@amtal amtal commented on 4817504 Apr 8, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@CounterPillow If you had to maintain critical legacy code like this, you wouldn't want your full name and email on it either. :)

@daira
Copy link

@daira daira commented on 4817504 Apr 9, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was a huge patch with too much duplicate code (which is still there in the fixed version [731f431]), and no references to the spec it was implementing. Hint: for every "MUST" statement in an RFC, there has to be some code that explicitly handles that case.

@sebadoom
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sigh. Why aren't we switching to a formally verified development model for critical software such as this already?

@garu
Copy link

@garu garu commented on 4817504 Apr 9, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, @daira, and I'm pretty sure they would appreciate patches (or even just more reviewers available to help the development process).

@sebadoom do you know how the development model works for openssl? OpenSSL is volunteer-driven, and if you have suggestions to improve the current model, I'm pretty sure the project leads would welcome constructive input on this. Why don't you join the mailing list at openssl-dev@openssl.org to discuss it?

@CounterPillow, thanks for the explanation. "steve", in this case, is the well-known handle for Dr. Stephen Henson (steve@openssl.org), one of the 4 members of the current OpenSSL core team.

It's probably worth noticing that this is just a mirror repository provided by github. The actual development is done at git.openssl.org. Sadly, github doesn't let you disable issue reporting, so I don't know whether issues reported here are seen at all by the project leads, as bugs are supposed to go to http://www.openssl.org/support/rt.html.

@sebadoom
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@garu Thanks for your response. Yes, I know OpenSSL is community driven, and I congratulate the team so far. However I do think critical code should be written differently (using stricter guidelines, static analysis, formal verification, etc). I do realize it is probably too late for OpenSSL to go that way (tons of complicated and already tested code, no point in destabilizing it), I just wonder if we can do better.

@amtal
Copy link

@amtal amtal commented on 4817504 Apr 9, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sebadoom "We"? Do you want to send a pull request to show them how it's done?

@jvz
Copy link

@jvz jvz commented on 4817504 Apr 9, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Guys, "steve" is the committer of the pull request: @snhenson

@sebadoom
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amtal Calm down. It's the proverbial 'we' I'm using above (i.e. the software community as a whole). I am arguing in favor of better software development practices in general.

In any case, you make a good point about being part of the solution: I'll see about diving into OpenSSL if time is on my side. We should all give back to the community whenever possible.

Thanks for commenting.

@Spacefish
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see how they failed there.... But Kudos to the OpenSSL Devs anyway, for providing the library and patching such bugs!

@StefanScherer
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we write unit tests for this?

@rhuijben
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As noted before: This is a read only GIT mirror of the actual OpenSSL repository.

The discussion should move to a platform that involves the actual OpenSSL developers. We can blame/troll, whatever we want here but it doesn't help the actual OpenSSL progress. It would be nice if instead of wasting everybody's time here, we actually improve OpenSSL in the right way.

[Unsubscribing from this thread now (if possible) as this is going nowhere]

@markcheret
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everyone who works on and contributes to OpenSSL is a hero. Cheers for fixing the bug as fast as you guys did. For those who wonder how these bugs find their way into such crucial parts of software, I can only say: Review, review, review. Reviewing code is just as important as creating concepts, planning the hacking, writing the actual code, checking the progress, and distributing it. I'm only beginning to understand the importance of code review and try to become better and better at reviewing code.

@mortenegelund
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heartbleed explained for dummies: http://xkcd.com/1354/
Have a nice day! :-)

@torgeir
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@daira
Copy link

@daira daira commented on 4817504 Apr 12, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@derekm said: "The draft leaves out the length, as if implementations will use zero-padding and the payload will be read in as a null-terminated string."

This is a false premise. The "<0..2^14-5>" notation implies a length field. The difference between the draft and the RFC is that the padding does not have a length field in the RFC, whereas it did in the draft. It could be argued that implementations might have been more likely to check consistency of the record length with the payload + padding length in the draft design, but that's debateable.

@justingoldberg
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

He submitted the code on December 31st, 11:59pm, which may be why it wasn't reviewed:

http://www.businessinsider.com/robin-seggelmann-regrets-introducing-heartbleed-error-2014-4

@HermanSchoenfeld
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Who uses memcpy's without range checks? I range check everything even in managed languages like C# and scripting languages like Javascript. How on Earth does someone not do such an elementary check in C within critical security code?

@derekm
Copy link

@derekm derekm commented on 4817504 Apr 14, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@daira -- I don't see that implication. Length prefixes vs. sentinel values are a common point of argumentation in protocol design. While length prefixes are considered preferable, they are not always preferable.

In this case, length headers are bad design, and here is why: There remains a known-plaintext exploit in the supposedly "fixed" Heartbeat code. The length is always determinable by anyone intercepting traffic, as implemented in OpenSSL, or as to-be-implemented by most people given a presumption that most will always go with a minimum padding of 16 bytes rather than a randomized padding length (when not doing PMTU discovery). Since the length can always be determined as implemented, so too is the area of the ciphertext that violates the cardinal cryptographic sin of encoding the same message twice with the same key (a fortunate or unfortunate [depending on perspective] side-effect of the heartbeat also being an echo service).

Any string has a length, but the fact of its length does not imply PASCAL-style strings (length prefixed) vs. C-style strings (sentinel terminated).

@justingoldberg -- The timing of the commit likely coincided with a desire to avoid code reviews, knowing most reasonable hackers would be partying that night and hung over the next day.

Someone needs to notify the press about the remaining known-plaintext vector. Nevermind, I'll just email Bruce Schneier...

@daira
Copy link

@daira daira commented on 4817504 Apr 14, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@derekm
a) The argument you made originally was based on a misreading of the draft. You're now making an entirely different argument.
b) Known-plaintext attacks are not a problem, period. With the exception of RC4 (and possibly some ciphersuites that were only standardized for political reasons and that no-one uses), TLS doesn't use algorithms that are vulnerable to known-plaintext attack. (For the export algorithms with too-short key lengths, known plaintext is useful for confirming a key but there is already plenty of it for that purpose; the design of extensions won't make any difference. In the case of RC4, its weaknesses are correctly considered to be a choice-of-algorithm issue rather than a protocol design issue.) Making design decisions on the basis of avoiding known plaintext is placing entirely useless constraints on a design space that is arguably already overconstrained, which is a really bad idea. Also, if encrypting the plaintext twice actually mattered (it doesn't, for the modes used by TLS), the heartbeat extension would have to be totally redesigned to not be an echo service; arguing about its use of length fields would be beside the point.
c) TLS doesn't use zero-byte-terminated strings anywhere. It would be a bad idea to use them only in one place.

@derekm
Copy link

@derekm derekm commented on 4817504 Apr 14, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@daira -- For what it's worth, I liked what you said on twitter:
'Re #heartbleed: original RFC3546 TLS ext'ns with variable length fields all had explicit Security Considerations text 1/3
'...pointing out buffer overflow potential http://tools.ietf.org/html/rfc3546#section-6 … But RFC6520 incorrectly says... 2/3
' "This document does not introduce any new security considerations." https://tools.ietf.org/html/rfc6520#section-7 … 3/3
'No excuses, people! Write proper Security Considerations sections for any protocols you define. #heartbleed #heartbeatbug #security
'And don't include variable-length fields without any design rationale. #heartbleed #heartbeatbug #security #langsec'

Variable length field input validation & handling combined with "encrypt and MAC" famously led to a slew of SSH exploits. Not saying the same issues exist here also, but it makes me suspicious.

Although it says the two strings can't be larger than 2^14-5 individually or combined, maybe this is an Internet-Draft convention I'm not familiar with, with the implication coming strongest from the "-5" part of the formula ("minus 5" being 1 byte for message type plus 4 bytes being 2 bytes for each of 2 different lengths, payload and padding [notice a change from "-3" to "-5" between versions 00 and 01 of the draft). But, to your point, some new security considerations here might have been "2-byte lengths allow for 64k of data, but beyond 16k of data is not permissible, heartbeats with erroneous length fields should be discarded silently," etc.

I tried to find their original patches going back to 2009 here: http://sctp.fh-muenster.de/dtls-patches.html
...but I couldn't find their oldest patches that would have corresponded to this presentation: http://www.ietf.org/proceedings/75/slides/tls-3.pdf ... All available patches seem to relate to the final RFC and were uploaded in the month of Dec. 2011.

I guess what you're saying is these specs were very poorly written from the get-go. I can agree to that!

If this was intentional, it was BND's backdoor, not NSA's.