Skip to content

Fix oversized record length handling in TlsListener#65558

Merged
BrennanConroy merged 3 commits intomainfrom
brecon/recordlength
Mar 4, 2026
Merged

Fix oversized record length handling in TlsListener#65558
BrennanConroy merged 3 commits intomainfrom
brecon/recordlength

Conversation

@BrennanConroy
Copy link
Copy Markdown
Member

https://datatracker.ietf.org/doc/html/rfc8446#section-5.1

The length (in bytes) of the following
TLSPlaintext.fragment. The length MUST NOT exceed 2^14 bytes. An
endpoint that receives a record that exceeds this length MUST
terminate the connection with a "record_overflow" alert.

We probably shouldn't throw the "record_overflow" alert here and let SslStream handle it, but we should make sure to handle cases where the length is greater than 2^14 (16k).

I chose to still pass the client hello to the user provided callback with a max of 2^14 bytes so applications can still observe the client hello.

Copilot AI review requested due to automatic review settings February 27, 2026 21:18
@BrennanConroy BrennanConroy added feature-kestrel area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions labels Feb 27, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR hardens TLS ClientHello sniffing in TlsListener when a peer advertises an oversized TLS record length, capping the bytes forwarded to the callback to the RFC-defined maximum plaintext fragment size.

Changes:

  • Introduces MaxTlsRecordLength (16,384) and caps the parsed record length used for callback framing.
  • Adds a regression test to ensure lengths > short.MaxValue don’t cause an exception and are capped when passed to the callback.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
src/Servers/Kestrel/Core/src/Middleware/TlsListener.cs Caps parsed TLS record length to 16KB before slicing/passing bytes to the ClientHello callback.
src/Servers/Kestrel/Core/test/TlsListenerTests.cs Adds regression coverage for oversized record length values (notably > 32,767) to ensure capping behavior.

Comment thread src/Servers/Kestrel/Core/test/TlsListenerTests.cs
Comment thread src/Servers/Kestrel/Core/src/Middleware/TlsListener.cs Outdated
Comment on lines +126 to +135
// Record length (unsigned 16-bit, read as signed and convert)
if (!reader.TryReadBigEndian(out recordLength))
{
return ClientHelloParseState.NotTlsClientHello;
}

// Convert to unsigned interpretation and cap at max TLS record size
// If the record claims to be larger, we'll just pass what we can (up to 2^14 bytes)
// We'll let SslStream decide how to handle the oversized record.
recordLength = (short)Math.Min((ushort)recordLength, MaxTlsRecordLength);
Copy link

Copilot AI Feb 27, 2026

Choose a reason for hiding this comment

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

To avoid the signed/unsigned round-trip (and make the intent clearer), consider reading the record length as an unsigned 16-bit value directly (e.g., TryReadBigEndian(out ushort recordLength) if available in the target framework), then cap it and convert to the type needed for downstream slicing.

Suggested change
// Record length (unsigned 16-bit, read as signed and convert)
if (!reader.TryReadBigEndian(out recordLength))
{
return ClientHelloParseState.NotTlsClientHello;
}
// Convert to unsigned interpretation and cap at max TLS record size
// If the record claims to be larger, we'll just pass what we can (up to 2^14 bytes)
// We'll let SslStream decide how to handle the oversized record.
recordLength = (short)Math.Min((ushort)recordLength, MaxTlsRecordLength);
// Record length (unsigned 16-bit)
ushort unsignedRecordLength;
if (!reader.TryReadBigEndian(out unsignedRecordLength))
{
return ClientHelloParseState.NotTlsClientHello;
}
// Cap at max TLS record size. If the record claims to be larger, we'll just pass what we can (up to 2^14 bytes)
// and let SslStream decide how to handle the oversized record.
recordLength = (short)Math.Min(unsignedRecordLength, (ushort)MaxTlsRecordLength);

Copilot uses AI. Check for mistakes.
Comment thread src/Servers/Kestrel/Core/test/TlsListenerTests.cs Outdated
@BrennanConroy BrennanConroy merged commit 552c986 into main Mar 4, 2026
25 checks passed
@BrennanConroy BrennanConroy deleted the brecon/recordlength branch March 4, 2026 21:52
@dotnet-policy-service dotnet-policy-service Bot added this to the 11.0-preview3 milestone Mar 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions feature-kestrel

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants