Fix handling of errSSLClosedGraceful from SSLWrite on macOS#123987
Fix handling of errSSLClosedGraceful from SSLWrite on macOS#123987
Conversation
Co-authored-by: rzikm <32671551+rzikm@users.noreply.github.com>
Co-authored-by: rzikm <32671551+rzikm@users.noreply.github.com>
src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamAlertsTest.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamAlertsTest.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamAlertsTest.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamAlertsTest.cs
Outdated
Show resolved
Hide resolved
…outs, simplify test Co-authored-by: rzikm <32671551+rzikm@users.noreply.github.com>
|
Tagging subscribers to this area: @dotnet/ncl, @bartonjs, @vcsjones |
|
@copilot The new test seems to be failing on all platforms, can you take a look? |
Co-authored-by: rzikm <32671551+rzikm@users.noreply.github.com>
Fixed in 306d7c3. The test was failing on Linux/Windows because those platforms don't throw an exception on the first write after close_notify (they disregard the alert and attempt to send data). The fix in SslStreamPal.OSX.cs only affects macOS with Secure Transport, so I've restored the PlatformSpecific(TestPlatforms.OSX) attribute to make the test run only where the behavior applies. |
src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamAlertsTest.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamAlertsTest.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: rzikm <32671551+rzikm@users.noreply.github.com>
src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamAlertsTest.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamAlertsTest.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: rzikm <32671551+rzikm@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Fixes macOS Secure Transport behavior when SSLWrite returns errSSLClosedGraceful (mapped to PAL_TlsIo.ClosedGracefully) by treating it as a graceful TLS close instead of an unknown/internal error, and adds a regression test for write-after-close_notify.
Changes:
- Handle
PAL_TlsIo.ClosedGracefullyinSslStreamPal.OSX.EncryptMessageby returningSecurityStatusPalErrorCode.ContextExpired. - Add a functional test covering client write after server
close_notify.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/libraries/System.Net.Security/src/System/Net/Security/SslStreamPal.OSX.cs |
Adds ClosedGracefully handling during encryption to avoid falling into the “unknown status” path. |
src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamAlertsTest.cs |
Adds a regression test for writing after receiving close_notify, with platform-conditional expectations. |
src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamAlertsTest.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamAlertsTest.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamAlertsTest.cs
Show resolved
Hide resolved
src/libraries/System.Net.Security/src/System/Net/Security/SslStreamPal.OSX.cs
Show resolved
Hide resolved
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
src/libraries/System.Net.Security/src/System/Net/Security/SslStreamPal.OSX.cs
Show resolved
Hide resolved
stephentoub
left a comment
There was a problem hiding this comment.
🤖 Copilot Code Review — PR #123987
Holistic Assessment
Motivation: This PR addresses a valid bug where writing to an SslStream after the remote side sends a TLS close_notify alert causes a Debug.Fail crash in debug builds or throws a misleading IOException with "Bad address" message in release builds on macOS with Secure Transport. The problem is well-documented in issue #121272 with clear root cause analysis.
Approach: The fix correctly adds handling for PAL_TlsIo.ClosedGracefully in EncryptMessage, mapping it to SecurityStatusPalErrorCode.ContextExpired. This mirrors the existing pattern in DecryptMessage and is the correct semantic representation for a closed TLS session.
Net positive: ✅ This is a net positive change — it fixes a crash/bug on macOS and improves error reporting consistency with a minimal 3-line production code change.
Detailed Findings
✅ Production Code — Correct fix pattern
The addition of the case PAL_TlsIo.ClosedGracefully: block in SslStreamPal.OSX.cs correctly mirrors the existing pattern in DecryptMessage (lines 240-241 in the original file):
case PAL_TlsIo.ClosedGracefully:
return new SecurityStatusPal(SecurityStatusPalErrorCode.ContextExpired);Mapping this status to ContextExpired is semantically correct — it signals that the TLS context is no longer valid for operations, which the upper SslStream layer translates into an appropriate IOException. This eliminates the Debug.Fail crash path.
✅ Consistency — Matches sibling DecryptMessage implementation
The fix maintains symmetry between EncryptMessage and DecryptMessage — both now handle ClosedGracefully identically. This is the expected pattern for this codebase.
✅ Test Coverage — Adequate regression test
The new test SslStream_WriteAfterRemoteCloseNotify_MayThrowIOException correctly exercises the fix:
- Establishes a TLS 1.2 connection between client and server
- Server initiates shutdown (sending
close_notify) - Client reads the closure (receives 0 bytes)
- Client attempts to write after receiving
close_notify - Verifies that macOS throws
IOExceptionwhile other platforms may succeed
The test uses proper timeouts (TestConfiguration.PassingTestTimeout) to prevent hangs in CI, and the platform-conditional assertion accurately reflects the behavioral difference between Secure Transport and OpenSSL/Schannel.
💡 Test Design — Platform divergence codified
The test's structure with Assert.False(PlatformDetection.IsOSX, ...) inside the try block explicitly codifies that macOS behaves more strictly (throwing immediately on write after close) than Windows/Linux (which may allow the first write to succeed). This is acceptable as it reflects the reality of the underlying SSL implementations, but note this commits the test to maintaining this documented divergence.
Cross-Cutting Analysis
Sibling platform implementations: I verified that SslStreamPal.Windows.cs, SslStreamPal.Unix.cs, and SslStreamPal.Android.cs exist. The fix is appropriately scoped to the OSX implementation since this bug is specific to how macOS Secure Transport returns errSSLClosedGraceful from SSLWrite.
Network Framework path: The PR description notes this issue doesn't apply to the new Network.framework-based implementation (SafeDeleteNwContext). The fix correctly targets only the SafeDeleteSslContext (Secure Transport) code path.
Issue #121272 mentions a second bug: The issue notes that GetException produces misleading error messages when SecurityStatusPalErrorCode.InternalError (value 14) is passed to Win32Exception, which interprets it as Unix errno EFAULT. This PR doesn't address that secondary issue, but that's acceptable as the fix prevents reaching that code path for this particular scenario.
Summary
LGTM. The fix is correct, minimal, consistent with existing patterns, and verified by an appropriate regression test. The PR has already been approved by @wfurt.
Review performed with: Claude Sonnet 4, Gemini 3 Pro. (GPT-5.1-Codex timed out)
|
/ba-g unrelated failure |
|
/ba-g Test failure is unrelated |
Description
On macOS with Secure Transport, writing to an
SslStreamafter the remote side sendsclose_notifytriggersDebug.Fail(debug builds) or throwsIOExceptionwith misleading "Bad address" message (release builds). Root cause:EncryptMessagedoesn't handlePAL_TlsIo.ClosedGracefullystatus fromSSLWrite.Changes
Production code (
SslStreamPal.OSX.cs):PAL_TlsIo.ClosedGracefullycase inEncryptMessageswitch statementSecurityStatusPal(SecurityStatusPalErrorCode.ContextExpired)DecryptMessagepatternTest (
SslStreamAlertsTest.cs):SslStream_WriteAfterRemoteCloseNotify_ThrowsIOExceptiontestPlatformDetection.IsOSX) with try-catch pattern to handle different behaviors across platforms:IOExceptionmust be thrown when writing after close_notifyIOExceptionto accommodate different SSL implementation behaviors (OpenSSL/Schannel allow the first write to succeed)Task.WhenAllfor concurrent client/server handshake to prevent deadlockImpact
Debug.Failcrash in debug builds on macOSIOExceptionderived fromContextExpiredon macOSOriginal prompt
This section details on the original issue you should resolve
<issue_title>Improper handling of
errSSLClosedGracefulreturned fromSSLWriteon macOS</issue_title><issue_description>### Description
This issue applies to the original Secure Transport-based SSL implementation on macOS. It does not apply to the new Network.framework-based implementation which seems slated to replace it, but for now Secure Transport is still the default implementation.
If you try to write to a
System.Net.Security.SslStreamafter the other side has sent a TLS close_notify alert (which is a standard way to close a connection), you'll get an unexpected exception:Cause
There are actually two bugs here.
1. No check for
ClosedGracefullyIn this case, the native function
SSLWriteis returningerrSSLClosedGraceful, whichInterop.AppleCrypto.SslWritetranslates toPAL_TlsIo.ClosedGracefully. But the caller,SslStreamPal.EncryptMessage(inSslStreamPal.OSX.cs), doesn't have a case for this error code.DecryptMessagedoes have a case for it:But
EncryptMessagedoesn’t, so we fall into:Note that
Debug.Failcrashes the program on debug builds ofSystem.Net.Security(but does nothing on release builds). Clearly we don’t want to fall into this case in any legitimate circumstance.2. Misinterpreted error code
If we’re on a release build, then we proceed past the
Debug.Failtotoken.Status = new SecurityStatusPal(SecurityStatusPalErrorCode.InternalError);However, this is later translated to an exception with the wrong message: “Win32Exception (14): Bad address”. There is no actual bad address here. Instead, this function inSslStreamPal.OSX.csis extracting the numeric value ofSecurityStatusPalErrorCode.InternalError, 14, and passing it tonew Win32Exception:Which picks this constructor:
But
GetPInvokeErrorMessageinterprets the integer as a Unix errno value and calls out tostrerrorto get a string description. "Bad address" is the description ofEFAULT, which just happens to be the errno value with numeric value 14 on macOS.This should be fixed in addition to the first bug, since it could still apply to other error cases.
Test case
To reproduce, compile and run Program.cs.
(On macOS this will use Secure Transport by default. To use Network Framework, change the
SslProtocolsargument to eitherSslProtocols.Tls13 | SslProtocols.Tls12orSslProtocols.Tls13, and passDOTNET_SYSTEM_NET_SECURITY_USENETWORKFRAMEWORK=1. If you only passSslProtocols.Tls12then it will fall back to Secure Transport regardless of the environment variable, so to test Network.framework with TLS 1.2 you need to passSslProtocols.Tls13 | SslProtocols.Tls12and connect to a server that doesn’t support TLS 1.3. For convenience, I’m running such a server on port 444 at the same host from the test case.)Here are some pcaps taken from running the program on various systems. They have the TLS secrets injected, so they should be viewable in Wireshark without any additional files. However, to view these properly, you should go to Preferences -> Protocols -> TLS and uncheck “Reassemble TLS Application Data spanning multiple TLS records”. Otherwise, some of the close_notify alerts will not be decoded properly. This appears to be a Wireshark bug.
What do we expect to happen?
This section is merely informative.
For practical compatibility, it would be nice if the specific exception thrown was the same on all platforms, but this is probably not achievable.
In my testing, on both Linux and Windows,
SslStreamdisregards the close_notify alert and attempts to send data anyway. Therefore, the firstWritecall succeeds and sends an Application Data packet to the server; the server responds with a TCP RST, and subsequentWrite...errSSLClosedGracefulreturned fromSSLWriteon macOS #121272💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.