← Back to context

Comment by userbinator

12 years ago

Agreed. Simple code is easy to understand and just as easy to find any bugs in. After looking at the heartbeat spec and the code, I can already see a simplification that, had it been written this way, would've likely avoided introducing this bug. Instead of allocating memory of a new length, how about just validating the existing message fields as per the spec:

> The total length of a HeartbeatMessage MUST NOT exceed 2^14 or max_fragment_length when negotiated as defined in [RFC6066].

> The padding_length MUST be at least 16.

> The sender of a HeartbeatMessage MUST use a random padding of at least 16 bytes.

> If the payload_length of a received HeartbeatMessage is too large, the received HeartbeatMessage MUST be discarded silently.

Then if it's all good, modify the buffer to change its type to heartbeat_response, fill the padding with new random bytes, and send this response. No need to copy the payload (which is where the bug was), no need to allocate more memory.

(Now I'm sure someone will try to find a flaw in this approach...)