Support certificate validation TLS alerts for Linux #115996
+367
−108
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -11,6 +11,7 @@ | |||||||
| using System.IO; | ||||||||
| using System.Net; | ||||||||
| using System.Net.Security; | ||||||||
| using System.Runtime.ExceptionServices; | ||||||||
| using System.Runtime.InteropServices; | ||||||||
| using System.Runtime.InteropServices.Marshalling; | ||||||||
| using System.Security.Authentication; | ||||||||
|
|
@@ -217,6 +218,8 @@ internal static unsafe SafeSslContextHandle AllocateSslContext(SslAuthentication | |||||||
| throw CreateSslException(SR.net_allocate_ssl_context_failed); | ||||||||
| } | ||||||||
|
|
||||||||
| Ssl.SslCtxSetCertVerifyCallback(sslCtx, &CertVerifyCallback); | ||||||||
|
|
||||||||
| Ssl.SslCtxSetProtocolOptions(sslCtx, protocols); | ||||||||
|
|
||||||||
| if (sslAuthenticationOptions.EncryptionPolicy != EncryptionPolicy.RequireEncryption) | ||||||||
|
|
@@ -388,7 +391,7 @@ internal static SafeSslHandle AllocateSslHandle(SslAuthenticationOptions sslAuth | |||||||
| // Dispose() here will not close the handle. | ||||||||
| using SafeSslContextHandle sslCtxHandle = GetOrCreateSslContextHandle(sslAuthenticationOptions, cacheSslContext); | ||||||||
|
|
||||||||
| GCHandle authOptionsHandle = default; | ||||||||
| try | ||||||||
| { | ||||||||
| sslHandle = SafeSslHandle.Create(sslCtxHandle, sslAuthenticationOptions.IsServer); | ||||||||
|
|
@@ -418,21 +421,19 @@ internal static SafeSslHandle AllocateSslHandle(SslAuthenticationOptions sslAuth | |||||||
| sslHandle.SslContextHandle = sslCtxHandle; | ||||||||
| } | ||||||||
|
|
||||||||
| authOptionsHandle = GCHandle.Alloc(sslAuthenticationOptions); | ||||||||
| Interop.Ssl.SslSetData(sslHandle, GCHandle.ToIntPtr(authOptionsHandle)); | ||||||||
| sslHandle.AuthOptionsHandle = authOptionsHandle; | ||||||||
| authOptionsHandle = default; // the ownership is transferred to sslHandle, so we should not free it in the caller anymore. | ||||||||
|
|
||||||||
| if (!sslAuthenticationOptions.AllowRsaPssPadding || !sslAuthenticationOptions.AllowRsaPkcs1Padding) | ||||||||
| { | ||||||||
| ConfigureSignatureAlgorithms(sslHandle, sslAuthenticationOptions.AllowRsaPssPadding, sslAuthenticationOptions.AllowRsaPkcs1Padding); | ||||||||
| } | ||||||||
|
|
||||||||
| if (sslAuthenticationOptions.ApplicationProtocols != null && sslAuthenticationOptions.ApplicationProtocols.Count != 0) | ||||||||
| { | ||||||||
| if (!sslAuthenticationOptions.IsServer) | ||||||||
| { | ||||||||
| if (Interop.Ssl.SslSetAlpnProtos(sslHandle, sslAuthenticationOptions.ApplicationProtocols) != 0) | ||||||||
| { | ||||||||
|
|
@@ -443,6 +444,9 @@ internal static SafeSslHandle AllocateSslHandle(SslAuthenticationOptions sslAuth | |||||||
|
|
||||||||
| if (sslAuthenticationOptions.IsClient) | ||||||||
| { | ||||||||
| // Client side always verifies the server's certificate. | ||||||||
| Ssl.SslSetVerifyPeer(sslHandle, failIfNoPeerCert: false); | ||||||||
|
|
||||||||
| if (!string.IsNullOrEmpty(sslAuthenticationOptions.TargetHost) && !IPAddress.IsValid(sslAuthenticationOptions.TargetHost)) | ||||||||
| { | ||||||||
| // Similar to windows behavior, set SNI on openssl by default for client context, ignore errors. | ||||||||
|
|
@@ -474,7 +478,14 @@ internal static SafeSslHandle AllocateSslHandle(SslAuthenticationOptions sslAuth | |||||||
| { | ||||||||
| if (sslAuthenticationOptions.RemoteCertRequired) | ||||||||
| { | ||||||||
| // When no user callback is registered, also set | ||||||||
| // SSL_VERIFY_FAIL_IF_NO_PEER_CERT so that OpenSSL sends the | ||||||||
| // appropriate TLS alert when the client doesn't provide a | ||||||||
| // certificate. When a callback IS registered, the application | ||||||||
| // may choose to accept connections without a client certificate, | ||||||||
| // so we only set SSL_VERIFY_PEER and let managed code handle it. | ||||||||
| bool failIfNoPeerCert = sslAuthenticationOptions.CertValidationDelegate is null; | ||||||||
| Ssl.SslSetVerifyPeer(sslHandle, failIfNoPeerCert); | ||||||||
| } | ||||||||
|
|
||||||||
| if (sslAuthenticationOptions.CertificateContext != null) | ||||||||
|
|
@@ -514,9 +525,9 @@ internal static SafeSslHandle AllocateSslHandle(SslAuthenticationOptions sslAuth | |||||||
| } | ||||||||
| catch | ||||||||
| { | ||||||||
| if (authOptionsHandle.IsAllocated) | ||||||||
| { | ||||||||
| authOptionsHandle.Free(); | ||||||||
| } | ||||||||
|
|
||||||||
| throw; | ||||||||
|
|
@@ -694,7 +705,12 @@ internal static SecurityStatusPalErrorCode DoSslHandshake(SafeSslHandle context, | |||||||
| return SecurityStatusPalErrorCode.CredentialsNeeded; | ||||||||
| } | ||||||||
|
|
||||||||
| if (errorCode == Ssl.SslErrorCode.SSL_ERROR_SSL && context.CertificateValidationException is Exception ex) | ||||||||
| { | ||||||||
| handshakeException = ex; | ||||||||
| context.CertificateValidationException = null; | ||||||||
| } | ||||||||
| else if ((retVal != -1) || (errorCode != Ssl.SslErrorCode.SSL_ERROR_WANT_READ)) | ||||||||
| { | ||||||||
| Exception? innerError = GetSslError(retVal, errorCode); | ||||||||
|
|
||||||||
|
|
@@ -731,7 +747,7 @@ internal static SecurityStatusPalErrorCode DoSslHandshake(SafeSslHandle context, | |||||||
|
|
||||||||
| if (handshakeException != null) | ||||||||
| { | ||||||||
| ExceptionDispatchInfo.Throw(handshakeException); | ||||||||
| } | ||||||||
|
|
||||||||
| // in case of TLS 1.3 post-handshake authentication, SslDoHandhaske | ||||||||
|
|
@@ -858,18 +874,110 @@ private static void QueryUniqueChannelBinding(SafeSslHandle context, SafeChannel | |||||||
| bindingHandle.SetCertHashLength(certHashLength); | ||||||||
| } | ||||||||
|
|
||||||||
| [UnmanagedCallersOnly] | ||||||||
| internal static Interop.Crypto.X509VerifyStatusCodeUniversal CertVerifyCallback(IntPtr ssl, IntPtr store) | ||||||||
| { | ||||||||
| IntPtr data = Ssl.SslGetData(ssl); | ||||||||
| Debug.Assert(data != IntPtr.Zero, "Expected non-null data pointer from SslGetData"); | ||||||||
| GCHandle gch = GCHandle.FromIntPtr(data); | ||||||||
| SslAuthenticationOptions options = (SslAuthenticationOptions)gch.Target!; | ||||||||
|
|
||||||||
| using SafeX509StoreCtxHandle storeHandle = new(store, ownsHandle: false); | ||||||||
|
|
||||||||
| // the chain will get properly disposed inside the VerifyRemoteCertificate call | ||||||||
| X509Chain chain = new X509Chain(); | ||||||||
| if (options.CertificateChainPolicy is not null) | ||||||||
| { | ||||||||
| chain.ChainPolicy = options.CertificateChainPolicy; | ||||||||
| } | ||||||||
| X509Certificate2? certificate = null; | ||||||||
| SafeSslHandle sslHandle = (SafeSslHandle)options.SslStream!._securityContext!; | ||||||||
|
|
||||||||
| using (SafeSharedX509StackHandle chainStack = Interop.Crypto.X509StoreCtxGetSharedUntrusted(storeHandle)) | ||||||||
| { | ||||||||
| if (!chainStack.IsInvalid) | ||||||||
| { | ||||||||
| int count = Interop.Crypto.GetX509StackFieldCount(chainStack); | ||||||||
|
|
||||||||
| for (int i = 0; i < count; i++) | ||||||||
| { | ||||||||
| IntPtr certPtr = Interop.Crypto.GetX509StackField(chainStack, i); | ||||||||
|
|
||||||||
| if (certPtr != IntPtr.Zero) | ||||||||
| { | ||||||||
| // X509Certificate2(IntPtr) calls X509_dup, so the reference is appropriately tracked. | ||||||||
| X509Certificate2 chainCert = new X509Certificate2(certPtr); | ||||||||
|
|
||||||||
| if (certificate is null) | ||||||||
| { | ||||||||
| // First cert in the stack is the leaf cert. | ||||||||
| certificate = chainCert; | ||||||||
| } | ||||||||
| else | ||||||||
| { | ||||||||
| chain.ChainPolicy.ExtraStore.Add(chainCert); | ||||||||
| } | ||||||||
| } | ||||||||
| } | ||||||||
| } | ||||||||
| } | ||||||||
|
|
||||||||
| Debug.Assert(certificate != null, "OpenSSL only calls the callback if the certificate is present."); | ||||||||
|
|
||||||||
| SslCertificateTrust? trust = options.CertificateContext?.Trust; | ||||||||
|
|
||||||||
| ProtocolToken alertToken = default; | ||||||||
|
|
||||||||
| try | ||||||||
| { | ||||||||
| if (options.SslStream!.VerifyRemoteCertificate(certificate, chain, trust, ref alertToken, out SslPolicyErrors sslPolicyErrors, out X509ChainStatusFlags chainStatus)) | ||||||||
| { | ||||||||
| // success | ||||||||
| return Interop.Crypto.X509VerifyStatusCodeUniversal.X509_V_OK; | ||||||||
| } | ||||||||
|
|
||||||||
| sslHandle.CertificateValidationException = SslStream.CreateCertificateValidationException(options, sslPolicyErrors, chainStatus); | ||||||||
|
|
||||||||
| if (options.CertValidationDelegate != null) | ||||||||
| { | ||||||||
| // rejected by user validation callback | ||||||||
| return Interop.Crypto.X509VerifyStatusCodeUniversal.X509_V_ERR_APPLICATION_VERIFICATION; | ||||||||
| } | ||||||||
|
|
||||||||
| TlsAlertMessage alert; | ||||||||
| if ((sslPolicyErrors & SslPolicyErrors.RemoteCertificateChainErrors) != SslPolicyErrors.None) | ||||||||
| { | ||||||||
| alert = SslStream.GetAlertMessageFromChain(chain); | ||||||||
| } | ||||||||
| else if ((sslPolicyErrors & SslPolicyErrors.RemoteCertificateNameMismatch) != SslPolicyErrors.None) | ||||||||
| { | ||||||||
| alert = TlsAlertMessage.BadCertificate; | ||||||||
| } | ||||||||
| else | ||||||||
| { | ||||||||
| alert = TlsAlertMessage.CertificateUnknown; | ||||||||
| } | ||||||||
|
|
||||||||
| // since we can't set the alert directly, we pick one of the error verify statuses | ||||||||
| // which will result in the same alert being sent | ||||||||
|
|
||||||||
| return alert switch | ||||||||
| { | ||||||||
| TlsAlertMessage.BadCertificate => Interop.Crypto.X509VerifyStatusCodeUniversal.X509_V_ERR_CERT_REJECTED, | ||||||||
| TlsAlertMessage.UnknownCA => Interop.Crypto.X509VerifyStatusCodeUniversal.X509_V_ERR_DEPTH_ZERO_SELF_SIGNED_CERT, | ||||||||
|
Member
There was a problem hiding this comment. I think the end result of my two comments (that I'm leaving in the same comment-posting) are "leave the code as-is", but I'll go ahead and say both of them: This un-mapping looks confusing"Depth-0 self-signed cert is just one of the statuses that could have caused UnknownCA...", after which I re-read the comment above and it clicked that this is "OpenSSL has a switch of status code to TLS alert, we just need to land in the right block" Why call GetAlertMessageFromChain at all?X509Chain turned an X509VerifyStatusCode into an X509ChainStatus, which GetAlertMessageFromChain turns into a TlsAlertMessage which this is turning back into an X509VerifyStatusCode... so why not just avoid all of the intermediate status? Well, X509Chain won't give you the original VerifyStatusCode, so you're already taking an ambiguous code, like NotTimeValid, and guessing that the more common reason is "expired" vs "not yet valid" (math could, of course, be done). And while it's true that GetAlertMessageFromChain is doing bit-testing when chain.ChainStatus does only one set bit per element ("So why is the enum [Flags]?!?!?!"), it does feel brittle to do something akin to |
||||||||
| TlsAlertMessage.CertificateRevoked => Interop.Crypto.X509VerifyStatusCodeUniversal.X509_V_ERR_CERT_REVOKED, | ||||||||
| TlsAlertMessage.CertificateExpired => Interop.Crypto.X509VerifyStatusCodeUniversal.X509_V_ERR_CERT_HAS_EXPIRED, | ||||||||
rzikm marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
| TlsAlertMessage.UnsupportedCert => Interop.Crypto.X509VerifyStatusCodeUniversal.X509_V_ERR_INVALID_PURPOSE, | ||||||||
|
There was a problem hiding this comment. The new mapping from managed cert validation failures (
Suggested change
|
||||||||
| _ => Interop.Crypto.X509VerifyStatusCodeUniversal.X509_V_ERR_CERT_REJECTED, | ||||||||
| }; | ||||||||
| } | ||||||||
| catch (Exception ex) | ||||||||
| { | ||||||||
| sslHandle.CertificateValidationException = ex; | ||||||||
|
|
||||||||
| return Interop.Crypto.X509VerifyStatusCodeUniversal.X509_V_ERR_UNSPECIFIED; | ||||||||
| } | ||||||||
| } | ||||||||
|
|
||||||||
| [UnmanagedCallersOnly] | ||||||||
| private static unsafe int AlpnServerSelectCallback(IntPtr ssl, byte** outp, byte* outlen, byte* inp, uint inlen, IntPtr arg) | ||||||||
|
|
@@ -883,8 +991,8 @@ private static unsafe int AlpnServerSelectCallback(IntPtr ssl, byte** outp, byte | |||||||
| return Ssl.SSL_TLSEXT_ERR_ALERT_FATAL; | ||||||||
| } | ||||||||
|
|
||||||||
| GCHandle authOptionsHandle = GCHandle.FromIntPtr(sslData); | ||||||||
| if (!((authOptionsHandle.Target as SslAuthenticationOptions)?.ApplicationProtocols is List<SslApplicationProtocol> protocolList)) | ||||||||
| { | ||||||||
| return Ssl.SSL_TLSEXT_ERR_ALERT_FATAL; | ||||||||
| } | ||||||||
|
|
@@ -911,17 +1019,9 @@ private static unsafe int AlpnServerSelectCallback(IntPtr ssl, byte** outp, byte | |||||||
| } | ||||||||
| catch | ||||||||
| { | ||||||||
| } | ||||||||
|
|
||||||||
| // No common application protocol was negotiated | ||||||||
| return Ssl.SSL_TLSEXT_ERR_ALERT_FATAL; | ||||||||
| } | ||||||||
|
|
||||||||
|
|
@@ -955,20 +1055,28 @@ private static unsafe int NewSessionCallback(IntPtr ssl, IntPtr session) | |||||||
|
|
||||||||
| Interop.Ssl.SslSessionSetData(session, cert); | ||||||||
|
|
||||||||
| IntPtr ctx = Ssl.SslGetSslCtx(ssl); | ||||||||
| IntPtr ptr = Ssl.SslCtxGetData(ctx); | ||||||||
| // while SSL_CTX is kept alive by reference from SSL, the same is not true | ||||||||
| // for the stored GCHandle pointing to SafeSslContextHandle. The managed | ||||||||
| // SafeSslContextHandle may have been disposed and its GCHandle freed, in | ||||||||
| // which case SslCtxGetData returns IntPtr.Zero and no GCHandle should be | ||||||||
| // reconstructed. | ||||||||
| if (ptr != IntPtr.Zero) | ||||||||
| { | ||||||||
| GCHandle gch = GCHandle.FromIntPtr(ptr); | ||||||||
| byte* name = Ssl.SslGetServerName(ssl); | ||||||||
| Debug.Assert(name != null); | ||||||||
|
|
||||||||
| SafeSslContextHandle? ctxHandle = gch.Target as SafeSslContextHandle; | ||||||||
|
|
||||||||
| if (ctxHandle != null) | ||||||||
| { | ||||||||
| if (ctxHandle.TryAddSession(name, session)) | ||||||||
| { | ||||||||
| // offered session was stored in our cache. | ||||||||
| return 1; | ||||||||
| } | ||||||||
| } | ||||||||
| } | ||||||||
|
|
||||||||
|
|
||||||||
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
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
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
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
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
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CertVerifyCallbackpopulateschain.ChainPolicy.ExtraStore, butSslStream.VerifyRemoteCertificatewill overwritechain.ChainPolicywhenoptions.CertificateChainPolicyis non-null, which discards the just-added certs and can break validation for callers supplying a custom chain policy. Build the chain using the sameX509ChainPolicyinstance thatVerifyRemoteCertificatewill apply (or copy the extracted certs into that policy) so the extracted chain is preserved.