Skip to content

bound sessionID read in sniffer ProcessServerHello#10631

Open
netliomax25-code wants to merge 2 commits into
wolfSSL:masterfrom
netliomax25-code:sniffer-serverhello-sessionid-bounds
Open

bound sessionID read in sniffer ProcessServerHello#10631
netliomax25-code wants to merge 2 commits into
wolfSSL:masterfrom
netliomax25-code:sniffer-serverhello-sessionid-bounds

Conversation

@netliomax25-code

Copy link
Copy Markdown

ProcessServerHello copies a fixed ID_LEN bytes of session id out of the captured ServerHello when the session-id length byte is non-zero, but the bounds check before it only ensures that length-byte value plus the cipher suite and compression fields are present. A ServerHello whose session id is shorter than 32 bytes, in a record with no trailing data, makes that copy read past the end of the frame buffer. ProcessClientHello already guards the identical copy with an ID_LEN check against the remaining bytes, so add the same check on the server side before the copy.

@wolfSSL-Bot

Copy link
Copy Markdown

Can one of the admins verify this patch?

@parasol-aser

Copy link
Copy Markdown

src/sniffer.c:ProcessServerHello
The new ID_LEN > *sslBytes check rejects complete ServerHello messages that advertise a short nonzero session ID and have no or few extension bytes. A server or pcap using a short nonzero session ID can now make the sniffer mark the flow fatal and remove the session, so affected sniffer deployments lose visibility for that TLS flow; this is flow-level availability/evasion, not a crash or secret disclosure.
Fix: Validate the encoded length (b <= ID_LEN) and copy/set the session ID size from b; if the sniffer intentionally supports only 32-byte nonzero IDs, reject that invariant based on b and add focused pcap coverage for short IDs.

The previous ID_LEN > *sslBytes check rejected a ServerHello with a
valid short non-zero session id and no trailing data, marking the flow
fatal. Copy the advertised length b instead of a fixed ID_LEN, guarded
by b > ID_LEN so the fixed-size buffer can't overflow; the read of b
bytes is already bounded by the existing length check. Add a sniffer
unit test that drives a short-session-id ServerHello through
ssl_DecodePacket.
@netliomax25-code

Copy link
Copy Markdown
Author

Good catch, you're right that the first version rejected legitimate short session IDs.

  1. Issue: the ID_LEN > *sslBytes check fataled any ServerHello advertising a non-zero session id shorter than 29 bytes with no trailing data, which removes the flow and drops sniffer visibility for it.
  2. Fix: copy the advertised length b and set sessionIDSz = b, guarded by b > ID_LEN so the fixed-size sessionID buffer can't overflow. Reading b bytes is already covered by the existing (b + SUITE_LEN + ENUM_LEN) > *sslBytes check, so the original out-of-bounds read stays closed.
  3. Resume path is unchanged: the match still requires sessionIDSz == ID_LEN on both sides, and for the usual 32-byte id b == ID_LEN, so it still resumes.

Added test_sniffer_serverhello_short_sessionid in tests/api.c that drives a 1-byte-session-id ServerHello through ssl_DecodePacket; it fails against the old check and passes with this change. The existing sniffer pcap tests (static-rsa, tls13-ecc, tls13-ecc-resume) still pass.

@dgarske

dgarske commented Jun 10, 2026

Copy link
Copy Markdown
Member

Hi @netliomax25-code , can you tell us more about your project and use of the sniffer tool? Is this commercial or accedemic? What tools did you use to find these issues? We do require a signed contributor agreement. If that's something that you'd like to request please email support at wolfssl dot com and include details like your region and your organizational affiliation. We encourage reports like this and thank you for submitting your two PR's. Are you planning to submit further work? Thanks, David Garske, wolfSSL

@netliomax25-code

netliomax25-code commented Jun 11, 2026

Copy link
Copy Markdown
Author

Hi David, thanks for taking a look.
I'm with Digiscrypt Technologies (digiscrypt.com), a software engineering and open-source security studio based in Bengaluru, India. Part of our work is auditing and hardening widely-used open-source libraries, which is how we came across the sniffer issues — so commercial affiliation, though the contributions themselves are just us trying to get fixes upstream.
On tooling — these were found with an in-house analysis platform we've built. It combines deep static analysis of parsing and memory-handling paths with targeted validation of the findings, and we manually review and reproduce every issue before reporting, so what reaches you is confirmed, not scanner noise. The sniffer's ServerHello/ClientHello handling came up during a sweep of packet-processing code in widely-deployed TLS implementations. The platform is currently internal-only — we plan to bring it to market down the line, but until then we're not sharing internals. Happy to walk through how any specific finding was confirmed, though.
Yes, we're planning to submit further work as we find things worth fixing. I'll email support@wolfssl.com about the contributor agreement with our details.
Thanks again for the quick review on PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants