Skip to content

LibSSH: Fix off-by-one in read_packet's payload length check - #26903

Open
Chandan3456 wants to merge 1 commit into
SerenityOS:masterfrom
Chandan3456:ssh-packet-payload-length
Open

LibSSH: Fix off-by-one in read_packet's payload length check#26903
Chandan3456 wants to merge 1 commit into
SerenityOS:masterfrom
Chandan3456:ssh-packet-payload-length

Conversation

@Chandan3456

Copy link
Copy Markdown

read_packet computes the payload as packet_length - padding_length - 1 but only rejects packet_length == padding_length, so the guard is one byte short. length 5 with padding 4 slips through and returns a zero-length payload, which breaks the size >= 1 invariant unpack_generic_message asserts on and that handle_new_keys_message relies on when it indexes payload[0]. padding_length can also just exceed packet_length: 4 with padding 255 wraps the subtraction and asks create_uninitialized for 4294967044 bytes off a nine byte packet, which asan flags as an allocation-size-too-big at Peer.cpp:60. both land before authentication since the transport is still on the identity cipher until NEWKEYS. found it reading the framing code against rfc 4253 section 6.

The payload is packet_length - padding_length - 1, but the guard only
rejected packet_length == padding_length. A packet declaring length 5
with 4 bytes of padding therefore returned a zero-length payload, and
a padding_length larger than packet_length wrapped the subtraction to
just under 4 GiB before it reached ByteBuffer::create_uninitialized.
@github-actions github-actions Bot added the 👀 pr-needs-review PR needs review from a maintainer or community member label Jul 28, 2026
Comment on lines +52 to 54
// "byte[n1] payload; n1 = packet_length - padding_length - 1"
if (packet_length <= padding_length + 1u)
return Error::from_string_literal("Packet doesn't have a payload");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for catching this and the tests!

Very small nit: do you mind moving that check just before we compute payload_length?

I think it's just a bit clearer this way (I also dropped the comment as this is literally the same thing as the code).

    // "There MUST be at least four bytes of padding."
    if (padding_length < 4)
        return Error::from_string_literal("Padding length is too short");

    if (packet_length <= padding_length + 1u)
        return Error::from_string_literal("Packet doesn't have a payload");
    auto payload_length = packet_length - padding_length - 1;

@LucasChollet LucasChollet added ⏳ pr-waiting-for-author PR is blocked by feedback / code changes from the author and removed 👀 pr-needs-review PR needs review from a maintainer or community member labels Jul 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

⏳ pr-waiting-for-author PR is blocked by feedback / code changes from the author

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants