diff options
author | Paulo Janotti <pauloja@microsoft.com> | 2018-04-09 22:28:59 +0300 |
---|---|---|
committer | GitHub <noreply@github.com> | 2018-04-09 22:28:59 +0300 |
commit | 275752f322defe4e4d0d6e9ccb4330df146d57f1 (patch) | |
tree | a3b5f80cb89a952662e91a8cac7cee415428a8a5 | |
parent | a036d70307f7d9dabb7fcb9d9d2b3d3a23011469 (diff) |
Fix regression on OpenSsl exception message (#28931)
* Fix regression on OpenSsl exception message
Managed code relies on SSL error queue to give proper error information. PAL layer was clearing up the error queue and the exception messages were not precise.
This changes returns to the state before in which the managed side was in charge of cleaning up the queue (basically we care only about the last error). Perhaps in the future we should look in changing the PAL signature and doing all at once in the same place, but, as it is the fix gets us to where we were before.
Removing the error queue clean up is not a concern in light of the investigation of #25676 and as it is the managed will clean up the queue whenever necessary.
Fixes #28365
* PR feedback
4 files changed, 64 insertions, 5 deletions
diff --git a/src/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.OpenSsl.cs b/src/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.OpenSsl.cs index 861ee6ca9b..1962389401 100644 --- a/src/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.OpenSsl.cs +++ b/src/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.OpenSsl.cs @@ -463,7 +463,7 @@ internal static partial class Interop break; case Ssl.SslErrorCode.SSL_ERROR_SSL: - // OpenSSL failure occurred. The error queue contains more details. + // OpenSSL failure occurred. The error queue contains more details, when building the exception the queue will be cleared. innerError = Interop.Crypto.CreateOpenSslCryptographicException(); break; diff --git a/src/Native/Unix/System.Security.Cryptography.Native/pal_ssl.cpp b/src/Native/Unix/System.Security.Cryptography.Native/pal_ssl.cpp index 5b6ce02160..f0fb128166 100644 --- a/src/Native/Unix/System.Security.Cryptography.Native/pal_ssl.cpp +++ b/src/Native/Unix/System.Security.Cryptography.Native/pal_ssl.cpp @@ -113,15 +113,16 @@ extern "C" SSL* CryptoNative_SslCreate(SSL_CTX* ctx) extern "C" int32_t CryptoNative_SslGetError(SSL* ssl, int32_t ret) { // This pops off "old" errors left by other operations - // until the first and last error are the same + // until the first error is equal to the last one, // this should be looked at again when OpenSsl 1.1 is migrated to while (ERR_peek_error() != ERR_peek_last_error()) { ERR_get_error(); } - int32_t errorCode = SSL_get_error(ssl, ret); - ERR_clear_error(); - return errorCode; + + // The error queue should be cleaned outside, if done here there will be no info + // for managed exception. + return SSL_get_error(ssl, ret); } extern "C" void CryptoNative_SslDestroy(SSL* ssl) diff --git a/src/System.Net.Security/src/System/Net/Security/SslStreamPal.Unix.cs b/src/System.Net.Security/src/System/Net/Security/SslStreamPal.Unix.cs index e0d7ebc9d2..37e197434b 100644 --- a/src/System.Net.Security/src/System/Net/Security/SslStreamPal.Unix.cs +++ b/src/System.Net.Security/src/System/Net/Security/SslStreamPal.Unix.cs @@ -255,6 +255,11 @@ namespace System.Net.Security { return new SecurityStatusPal(SecurityStatusPalErrorCode.OK); } + else if (code == Interop.Ssl.SslErrorCode.SSL_ERROR_SSL) + { + // OpenSSL failure occurred. The error queue contains more details, when building the exception the queue will be cleared. + return new SecurityStatusPal(SecurityStatusPalErrorCode.InternalError, Interop.Crypto.CreateOpenSslCryptographicException()); + } else { return new SecurityStatusPal(SecurityStatusPalErrorCode.InternalError, new Interop.OpenSsl.SslException((int)code)); diff --git a/src/System.Net.Security/tests/FunctionalTests/SslStreamNetworkStreamTest.cs b/src/System.Net.Security/tests/FunctionalTests/SslStreamNetworkStreamTest.cs index 2d02da12ee..160765c4b9 100644 --- a/src/System.Net.Security/tests/FunctionalTests/SslStreamNetworkStreamTest.cs +++ b/src/System.Net.Security/tests/FunctionalTests/SslStreamNetworkStreamTest.cs @@ -78,6 +78,59 @@ namespace System.Net.Security.Tests } [Fact] + [PlatformSpecific(TestPlatforms.Linux)] // This only applies where OpenSsl is used. + public async Task SslStream_SendReceiveOverNetworkStream_AuthenticationException() + { + TcpListener listener = new TcpListener(IPAddress.Loopback, 0); + + using (X509Certificate2 serverCertificate = Configuration.Certificates.GetServerCertificate()) + using (TcpClient client = new TcpClient()) + { + listener.Start(); + + Task clientConnectTask = client.ConnectAsync(IPAddress.Loopback, ((IPEndPoint)listener.LocalEndpoint).Port); + Task<TcpClient> listenerAcceptTask = listener.AcceptTcpClientAsync(); + + await TestConfiguration.WhenAllOrAnyFailedWithTimeout(clientConnectTask, listenerAcceptTask); + + TcpClient server = listenerAcceptTask.Result; + using (SslStream clientStream = new SslStream( + client.GetStream(), + false, + new RemoteCertificateValidationCallback(ValidateServerCertificate), + null, + EncryptionPolicy.RequireEncryption)) + using (SslStream serverStream = new SslStream( + server.GetStream(), + false, + null, + null, + EncryptionPolicy.RequireEncryption)) + { + + Task clientAuthenticationTask = clientStream.AuthenticateAsClientAsync( + serverCertificate.GetNameInfo(X509NameType.SimpleName, false), + null, + SslProtocols.Tls11, + false); + + AuthenticationException e = await Assert.ThrowsAsync<AuthenticationException>(() => serverStream.AuthenticateAsServerAsync( + serverCertificate, + false, + SslProtocols.Tls12, + false)); + + Assert.NotNull(e.InnerException); + Assert.True(e.InnerException.Message.Contains("SSL_ERROR_SSL")); + Assert.NotNull(e.InnerException.InnerException); + Assert.True(e.InnerException.InnerException.Message.Contains("protocol")); + } + } + + listener.Stop(); + } + + [Fact] [OuterLoop] // Test hits external azure server. public async Task SslStream_NetworkStream_Renegotiation_Succeeds() { |