Fix ALPN protocol list size field type and add boundary tests by rzikm · Pull Request #124590 · dotnet/runtime

rzikm · 2026-02-19T12:28:42Z

Summary

Fix Sec_Application_Protocols.ProtocolListSize field type from short to ushort to match the native Windows SEC_APPLICATION_PROTOCOL_LIST struct (USHORT), and add tests for ALPN list size validation.

Changes

Bug fix

Interop.Sec_Application_Protocols.cs: Changed ProtocolListSize from short to ushort and updated the aggregate size limit from short.MaxValue (32,767) to ushort.MaxValue (65,535), aligning with both the native Windows API and the RFC 7301 TLS wire format.

Tests

SslStreamAlpnTests.cs: Added SslStream_StreamToStream_AlpnListTotalSizeExceedsLimit_Throws — verifies that exceeding the 65,535-byte aggregate ALPN list limit throws:
- ArgumentException on Windows (managed validation in GetProtocolLength())
- AuthenticationException on Linux/FreeBSD (OpenSSL fails during ClientHello construction)
SslApplicationProtocolTests.cs: Added boundary tests for individual protocol sizes (0, 1, 254, 255, 256, 512 bytes) via both byte[] and string constructors.

- Change Sec_Application_Protocols.ProtocolListSize from short to ushort to match the native Windows SEC_APPLICATION_PROTOCOL_LIST struct (USHORT) - Update aggregate size limit from short.MaxValue (32,767) to ushort.MaxValue (65,535), aligning with RFC 7301 wire format - Add functional test for oversized ALPN list (Windows and Unix paths) - Add unit tests for individual protocol size boundaries Co-authored-by: Copilot <[email protected]>

dotnet-policy-service · 2026-02-19T12:30:17Z

Tagging subscribers to this area: @dotnet/ncl, @bartonjs, @vcsjones
See info in area-owners.md if you want to be subscribed.

Copilot

Pull request overview

This PR fixes a type mismatch in the Windows SChannel interop layer where Sec_Application_Protocols.ProtocolListSize was incorrectly defined as short instead of ushort, aligning it with the native Windows SEC_APPLICATION_PROTOCOL_LIST structure and RFC 7301 specifications. The fix increases the maximum aggregate ALPN list size from 32,767 to 65,535 bytes and adds comprehensive tests to validate both individual protocol size limits and aggregate list size limits.

Changes:

Fixed ProtocolListSize field type from short to ushort in the Windows SChannel interop struct
Updated aggregate ALPN list size validation limit from short.MaxValue (32,767) to ushort.MaxValue (65,535)
Added unit tests for individual protocol size boundaries (0, 1, 254, 255, 256, 512 bytes)
Added functional test verifying aggregate ALPN list size limit enforcement with platform-specific exception handling

Reviewed changes

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

File	Description
`Interop.Sec_Application_Protocols.cs`	Changed `ProtocolListSize` field from `short` to `ushort` and updated all casts and comparisons to use `ushort.MaxValue` instead of `short.MaxValue`
`SslApplicationProtocolTests.cs`	Added two theory-based boundary tests for individual protocol size validation via both byte array and string constructors
`SslStreamAlpnTests.cs`	Added functional test for aggregate ALPN list size limit, verifying ArgumentException on Windows and AuthenticationException on Linux/FreeBSD

- Add ushort.MaxValue aggregate size check to Unix (OpenSSL) path - Add ushort.MaxValue aggregate size check to macOS (Network.framework and SecureTransport) paths - Add ushort.MaxValue aggregate size check to Android path - All platforms now consistently throw ArgumentException for oversized ALPN lists, matching the RFC 7301 wire format limit - Simplify test to assert ArgumentException on all platforms Co-authored-by: Copilot <[email protected]>

Copilot

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (2)

src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamAlpnTests.cs:285

The explicit server.Dispose() on line 285 will be followed by an implicit Dispose when the using statement exits, resulting in double disposal. While Dispose is idempotent and should be safe, this is redundant and could mask issues.

Since the Dispose is being used to force the server task to complete (by closing the underlying stream), consider restructuring the test to avoid the double disposal. One approach is to move the Dispose call outside the using block, or to rely on the using statement's implicit disposal after awaiting the server task. Alternatively, document why the explicit Dispose is necessary (to unblock the server that's waiting for client data that will never arrive).

                server.Dispose();

src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamAlpnTests.cs:284

The PR description states that Linux/FreeBSD will throw AuthenticationException when OpenSSL fails during ClientHello construction. However, the code changes add managed validation to all platforms (Unix in Interop.Ssl.cs lines 256-259, macOS in both SafeDeleteSslContext.cs and SafeDeleteNwContext.cs, and Android in SafeDeleteSslContext.cs lines 326-337). All platforms now throw ArgumentException before calling native APIs, making the behavior consistent across all platforms.

The test correctly expects ArgumentException, but the PR description should be updated to reflect that all platforms now throw ArgumentException due to managed validation, not just Windows.

        [ConditionalFact(nameof(BackendSupportsAlpn))]
        public async Task SslStream_StreamToStream_AlpnListTotalSizeExceedsLimit_Throws()
        {
            // Each protocol is 255 bytes, serialized with a 1-byte length prefix = 256 bytes each.
            // Per RFC 7301, TLS wire format limits ProtocolNameList to 2^16-1 (65,535) bytes.
            // All platforms enforce this via managed validation before calling native APIs.
            // 256 * 256 = 65,536 > 65,535
            const int protocolCount = 256;
            List<SslApplicationProtocol> oversizedProtocols = new List<SslApplicationProtocol>();
            for (int i = 0; i < protocolCount; i++)
            {
                byte[] proto = new byte[255];
                proto.AsSpan().Fill((byte)'a');
                proto[0] = (byte)((i >> 8) + 1);
                proto[1] = (byte)(i & 0xFF);
                oversizedProtocols.Add(new SslApplicationProtocol(proto));
            }

            using X509Certificate2 certificate = Configuration.Certificates.GetServerCertificate();

            SslClientAuthenticationOptions clientOptions = new SslClientAuthenticationOptions
            {
                ApplicationProtocols = oversizedProtocols,
                RemoteCertificateValidationCallback = delegate { return true; },
                TargetHost = Guid.NewGuid().ToString("N"),
            };

            SslServerAuthenticationOptions serverOptions = new SslServerAuthenticationOptions
            {
                ApplicationProtocols = new List<SslApplicationProtocol> { SslApplicationProtocol.Http2 },
                ServerCertificateContext = SslStreamCertificateContext.Create(certificate, null)
            };

            (Stream clientStream, Stream serverStream) = TestHelper.GetConnectedStreams();
            using (clientStream)
            using (serverStream)
            using (var client = new SslStream(clientStream, false))
            using (var server = new SslStream(serverStream, false))
            {
                Task serverTask = server.AuthenticateAsServerAsync(TestAuthenticateAsync, serverOptions);
                await Assert.ThrowsAsync<ArgumentException>(() => client.AuthenticateAsClientAsync(TestAuthenticateAsync, clientOptions));

On non-Windows platforms, ArgumentException from ALPN validation was caught by the generic catch(Exception) block in HandshakeInternal and wrapped in AuthenticationException. Add an explicit catch for ArgumentException that re-throws, matching Windows behavior. Co-authored-by: Copilot <[email protected]>

On non-Windows platforms, ArgumentException from ALPN validation was caught by the generic catch(Exception) block in HandshakeInternal and wrapped in AuthenticationException. Use exception filter to skip ArgumentException, matching Windows behavior. Co-authored-by: Copilot <[email protected]>

Copilot

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (5)

src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs:413

ValidateAlpnProtocolListSize only checks the aggregate size; it should also validate each protocol’s length is within 1..255 (consistent with RFC 7301 and other platform implementations). Without this, a list containing default(SslApplicationProtocol) (length 0) will pass validation and potentially fail later in native code with a less clear error.

        private static void ValidateAlpnProtocolListSize(List<SslApplicationProtocol> applicationProtocols)
        {
            int protocolListSize = 0;
            foreach (SslApplicationProtocol protocol in applicationProtocols)
            {
                protocolListSize += protocol.Protocol.Length + 1;
                if (protocolListSize > ushort.MaxValue)
                {
                    throw new ArgumentException(SR.net_ssl_app_protocols_invalid, nameof(applicationProtocols));
                }
            }

src/libraries/System.Net.Security/src/System/Net/Security/Pal.Android/SafeDeleteSslContext.cs:336

ValidateAlpnProtocolListSize should also validate each protocol’s length is within 1..255, not just the aggregate. Since SslApplicationProtocol is a struct, callers can pass default entries (0 length) which currently won’t be rejected here, leading to platform-specific behavior differences vs Windows/OpenSSL where invalid entries throw ArgumentException earlier.

        private static void ValidateAlpnProtocolListSize(List<SslApplicationProtocol> applicationProtocols)
        {
            int protocolListSize = 0;
            foreach (SslApplicationProtocol protocol in applicationProtocols)
            {
                protocolListSize += protocol.Protocol.Length + 1;
                if (protocolListSize > ushort.MaxValue)
                {
                    throw new ArgumentException(SR.net_ssl_app_protocols_invalid, nameof(applicationProtocols));
                }
            }

src/libraries/System.Net.Security/src/System/Net/Security/SslStreamPal.Unix.cs:236

The catch (ArgumentException) { throw; } block is only here to prevent the subsequent catch (Exception) from wrapping validation failures into an InternalError token. Consider simplifying by changing the general catch to catch (Exception exc) when (exc is not ArgumentException) and removing this dedicated catch block, which reduces noise while preserving the behavior of surfacing ArgumentException to the caller.

            catch (Exception exc) when (exc is not ArgumentException)
            {
                token.Status = new SecurityStatusPal(SecurityStatusPalErrorCode.InternalError, exc);
            }

src/libraries/System.Net.Security/src/System/Net/Security/SslStreamPal.OSX.cs:365

Same pattern as Unix: catch (ArgumentException) { throw; } can be replaced with an exception filter on the broad catch (e.g., catch (Exception exc) when (exc is not ArgumentException)) to avoid an otherwise redundant catch block while still letting argument validation failures propagate to callers.

            catch (Exception exc) when (exc is not ArgumentException)
            {
                token.Status = new SecurityStatusPal(SecurityStatusPalErrorCode.InternalError, exc);
                return token;

src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeDeleteNwContext.cs:456

GetAlpnProtocolListSerializedLength validates the aggregate ALPN wire size, but it doesn’t validate each protocol’s length (must be 1..255). Because SslApplicationProtocol is a struct, callers can include default values (length 0), and SerializeAlpnProtocolList will silently emit a zero-length entry (and would truncate >255 via the (byte) cast). Add per-protocol validation here (matching Windows/OpenSSL paths) before computing sizes.

                int protocolSize = 0;
                int wireSize = 0;

                foreach (SslApplicationProtocol protocol in applicationProtocols)
                {
                    protocolSize += protocol.Protocol.Length + 2;

                    wireSize += protocol.Protocol.Length + 1;
                    if (wireSize > ushort.MaxValue)
                    {
                        throw new ArgumentException(SR.net_ssl_app_protocols_invalid, nameof(applicationProtocols));
                    }
                }

Copilot AI review requested due to automatic review settings February 19, 2026 12:28

dotnet-policy-service bot assigned rzikm Feb 19, 2026

github-actions bot added the area-System.Net.Security label Feb 19, 2026

rzikm marked this pull request as draft February 19, 2026 12:29

Copilot started reviewing on behalf of rzikm February 19, 2026 12:29 View session

Copilot AI reviewed Feb 19, 2026

View reviewed changes

This was referenced Feb 19, 2026

[Wasm] The request was canceled due to the configured HttpClient.Timeout of 100 seconds elapsing #123572

Open

[android][clr] No peer certificates when executing System.Net.Http.Functional.Tests on Android emulator #124526

Open

rzikm and others added 2 commits February 20, 2026 10:41

Fix build

5ea6891

Copilot AI review requested due to automatic review settings February 20, 2026 11:15

Copilot started reviewing on behalf of rzikm February 20, 2026 11:15 View session

Copilot AI reviewed Feb 20, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings February 23, 2026 17:13

Copilot started reviewing on behalf of rzikm February 23, 2026 17:14 View session

rzikm force-pushed the fix/sslstream-alpn-size-validation branch from b0f4d97 to da8b85f Compare February 23, 2026 17:18

Copilot AI reviewed Feb 23, 2026

View reviewed changes

build-analysis bot mentioned this pull request Feb 23, 2026

[android] Android.Device_Emulator.JIT.Test failing on emulators with CoreCLR #112633

Open

rzikm marked this pull request as ready for review February 24, 2026 13:51

rzikm requested a review from a team February 24, 2026 13:51

ManickaP approved these changes Feb 25, 2026

View reviewed changes

rzikm merged commit 0f12574 into dotnet:main Feb 25, 2026
87 of 90 checks passed

dotnet-maestro bot mentioned this pull request Feb 25, 2026

[main] Source code updates from dotnet/runtime dotnet/dotnet#5088

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix ALPN protocol list size field type and add boundary tests#124590

Fix ALPN protocol list size field type and add boundary tests#124590
rzikm merged 5 commits intodotnet:mainfrom
rzikm:fix/sslstream-alpn-size-validation

rzikm commented Feb 19, 2026

Uh oh!

dotnet-policy-service bot commented Feb 19, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

rzikm commented Feb 19, 2026

Summary

Changes

Bug fix

Tests

Uh oh!

dotnet-policy-service bot commented Feb 19, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants