Fix ALPN protocol list size field type and add boundary tests#124590
Draft
rzikm wants to merge 2 commits intodotnet:mainfrom
Draft
Fix ALPN protocol list size field type and add boundary tests#124590rzikm wants to merge 2 commits intodotnet:mainfrom
rzikm wants to merge 2 commits intodotnet:mainfrom
Conversation
- 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 <223556219+Copilot@users.noreply.github.com>
Contributor
|
Tagging subscribers to this area: @dotnet/ncl, @bartonjs, @vcsjones |
Contributor
There was a problem hiding this comment.
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
ProtocolListSizefield type fromshorttoushortin the Windows SChannel interop struct - Updated aggregate ALPN list size validation limit from
short.MaxValue(32,767) toushort.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 <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fix
Sec_Application_Protocols.ProtocolListSizefield type fromshorttoushortto match the native WindowsSEC_APPLICATION_PROTOCOL_LISTstruct (USHORT), and add tests for ALPN list size validation.Changes
Bug fix
Interop.Sec_Application_Protocols.cs: ChangedProtocolListSizefromshorttoushortand updated the aggregate size limit fromshort.MaxValue(32,767) toushort.MaxValue(65,535), aligning with both the native Windows API and the RFC 7301 TLS wire format.Tests
SslStreamAlpnTests.cs: AddedSslStream_StreamToStream_AlpnListTotalSizeExceedsLimit_Throws— verifies that exceeding the 65,535-byte aggregate ALPN list limit throws:ArgumentExceptionon Windows (managed validation inGetProtocolLength())AuthenticationExceptionon Linux/FreeBSD (OpenSSL fails during ClientHello construction)SslApplicationProtocolTests.cs: Added boundary tests for individual protocol sizes (0, 1, 254, 255, 256, 512 bytes) via bothbyte[]andstringconstructors.