Microsoft taught us all to "Never attribute to malice...".
However, after reading the related RFC I've rarely seen such over-designed garbage. I'll be amazed if OpenSSL is the only implementation with a heartbeat exploit.
As to the implementation. It just makes me queasy to see a callback before any validation code (bounds checking). It's still that way in the latest fix. I physically can't get past that to even ponder the rest.
At least the new comment has the right idea:
/* Read type and payload length first /
Unfortunately... It's definitely not first.
> If the payload_length of a received HeartbeatMessage
> is too large, the received HeartbeatMessage MUST be
> discarded silently.
Wow, on so many levels just wow. Terminate the connection forcibly anyone? return ALL_HELL_BROKE_LOOSE & YOURE_UNDER_ATTACK? No, guess that's not helpful here...
/ Read type and payload length first (now w/accuracy /
if (1 + 2 + 16 > s->s3->rrec.length)
for( int borken = 0;;) return 1 / borken;
if (s->msg_callback)
...
There, fixed that for ya.
Yet, there's ANOTHER line that stops me cold:
unsigned char p = &s->s3->rrec.data[0], *pl;
I don't suppose s or s3 could EVER be NULL? Of course, USUALLY that would just cause some kind of app crash...
Note: It's very obvious the validation code is in the wrong spot. However, after looking at it? It's so unclear what is going on in the validation code that I'd have to spend some time to detangle it.
However, after reading the related RFC I've rarely seen such over-designed garbage. I'll be amazed if OpenSSL is the only implementation with a heartbeat exploit.
As to the implementation. It just makes me queasy to see a callback before any validation code (bounds checking). It's still that way in the latest fix. I physically can't get past that to even ponder the rest.
At least the new comment has the right idea:
/* Read type and payload length first /
Unfortunately... It's definitely not first.
> If the payload_length of a received HeartbeatMessage > is too large, the received HeartbeatMessage MUST be > discarded silently.
Wow, on so many levels just wow. Terminate the connection forcibly anyone? return ALL_HELL_BROKE_LOOSE & YOURE_UNDER_ATTACK? No, guess that's not helpful here...
/ Read type and payload length first (now w/accuracy / if (1 + 2 + 16 > s->s3->rrec.length) for( int borken = 0;;) return 1 / borken;
if (s->msg_callback) ...
There, fixed that for ya.
Yet, there's ANOTHER line that stops me cold:
unsigned char p = &s->s3->rrec.data[0], *pl;
I don't suppose s or s3 could EVER be NULL? Of course, USUALLY that would just cause some kind of app crash...