Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

make sure failed SSL does not impact other sessions #64256

Merged
merged 5 commits into from
Feb 4, 2022
Merged

make sure failed SSL does not impact other sessions #64256

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
33cf59b
make sure failed SSL does not imapct other sessions
wfurt Jan 25, 2022
10fe5bc
move innerError
wfurt Jan 25, 2022
78a17cd
feedback from review
wfurt Feb 3, 2022
4ea0f2b
remove try
wfurt Feb 3, 2022
b8fb508
feedback from review
wfurt Feb 3, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 17 additions & 33 deletions src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.OpenSsl.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -359,13 +359,12 @@ internal static SafeSslHandle AllocateSslHandle(SafeFreeSslCredentials credentia

internal static SecurityStatusPal SslRenegotiate(SafeSslHandle sslContext, out byte[]? outputBuffer)
{
int ret = Interop.Ssl.SslRenegotiate(sslContext);
int ret = Interop.Ssl.SslRenegotiate(sslContext, out Ssl.SslErrorCode errorCode);

outputBuffer = Array.Empty<byte>();
if (ret != 1)
{
GetSslError(sslContext, ret, out Exception? exception);
return new SecurityStatusPal(SecurityStatusPalErrorCode.InternalError, exception);
return new SecurityStatusPal(SecurityStatusPalErrorCode.InternalError, GetSslError(ret, errorCode));
}
return new SecurityStatusPal(SecurityStatusPalErrorCode.OK);
}
Expand All @@ -385,23 +384,21 @@ internal static SecurityStatusPalErrorCode DoSslHandshake(SafeSslHandle context,
}
}

int retVal = Ssl.SslDoHandshake(context);
int retVal = Ssl.SslDoHandshake(context, out Ssl.SslErrorCode errorCode);
if (retVal != 1)
{
Exception? innerError;
Ssl.SslErrorCode error = GetSslError(context, retVal, out innerError);

if (error == Ssl.SslErrorCode.SSL_ERROR_WANT_X509_LOOKUP)
if (errorCode == Ssl.SslErrorCode.SSL_ERROR_WANT_X509_LOOKUP)
{
return SecurityStatusPalErrorCode.CredentialsNeeded;
}

if ((retVal != -1) || (error != Ssl.SslErrorCode.SSL_ERROR_WANT_READ))
if ((retVal != -1) || (errorCode != Ssl.SslErrorCode.SSL_ERROR_WANT_READ))
{
Exception? innerError = GetSslError(retVal, errorCode);

// Handshake failed, but even if the handshake does not need to read, there may be an Alert going out.
// To handle that we will fall-through the block below to pull it out, and we will fail after.
handshakeException = new SslException(SR.Format(SR.net_ssl_handshake_failed_error, error), innerError);
Crypto.ErrClearError();
handshakeException = new SslException(SR.Format(SR.net_ssl_handshake_failed_error, errorCode), innerError);
}
}

Expand Down Expand Up @@ -450,17 +447,7 @@ internal static int Encrypt(SafeSslHandle context, ReadOnlySpan<byte> input, ref
ulong assertNoError = Crypto.ErrPeekError();
Debug.Assert(assertNoError == 0, $"OpenSsl error queue is not empty, run: 'openssl errstr {assertNoError:X}' for original error.");
#endif
errorCode = Ssl.SslErrorCode.SSL_ERROR_NONE;

int retVal;
Exception? innerError = null;

retVal = Ssl.SslWrite(context, ref MemoryMarshal.GetReference(input), input.Length);

if (retVal != input.Length)
{
errorCode = GetSslError(context, retVal, out innerError);
}
int retVal = Ssl.SslWrite(context, ref MemoryMarshal.GetReference(input), input.Length, out errorCode);

if (retVal != input.Length)
{
Expand All @@ -474,7 +461,7 @@ internal static int Encrypt(SafeSslHandle context, ReadOnlySpan<byte> input, ref
break;

default:
throw new SslException(SR.Format(SR.net_ssl_encrypt_failed, errorCode), innerError);
throw new SslException(SR.Format(SR.net_ssl_encrypt_failed, errorCode), GetSslError(retVal, errorCode));
}
}
else
Expand Down Expand Up @@ -504,17 +491,14 @@ internal static int Decrypt(SafeSslHandle context, Span<byte> buffer, out Ssl.Ss
ulong assertNoError = Crypto.ErrPeekError();
Debug.Assert(assertNoError == 0, $"OpenSsl error queue is not empty, run: 'openssl errstr {assertNoError:X}' for original error.");
#endif
errorCode = Ssl.SslErrorCode.SSL_ERROR_NONE;

BioWrite(context.InputBio!, buffer);

int retVal = Ssl.SslRead(context, ref MemoryMarshal.GetReference(buffer), buffer.Length);
int retVal = Ssl.SslRead(context, ref MemoryMarshal.GetReference(buffer), buffer.Length, out errorCode);
if (retVal > 0)
{
return retVal;
}

errorCode = GetSslError(context, retVal, out Exception? innerError);
switch (errorCode)
{
// indicate end-of-file
Expand All @@ -529,7 +513,7 @@ internal static int Decrypt(SafeSslHandle context, Span<byte> buffer, out Ssl.Ss
break;

default:
throw new SslException(SR.Format(SR.net_ssl_decrypt_failed, errorCode), innerError);
throw new SslException(SR.Format(SR.net_ssl_decrypt_failed, errorCode), GetSslError(retVal, errorCode));
}

return 0;
Expand Down Expand Up @@ -650,14 +634,13 @@ private static void BioWrite(SafeBioHandle bio, ReadOnlySpan<byte> buffer)
}
}

private static Ssl.SslErrorCode GetSslError(SafeSslHandle context, int result, out Exception? innerError)
private static Exception? GetSslError(int result, Ssl.SslErrorCode retVal)
{
ErrorInfo lastErrno = Sys.GetLastErrorInfo(); // cache it before we make more P/Invoke calls, just in case we need it

Ssl.SslErrorCode retVal = Ssl.SslGetError(context, result);
Exception? innerError;
switch (retVal)
{
case Ssl.SslErrorCode.SSL_ERROR_SYSCALL:
ErrorInfo lastErrno = Sys.GetLastErrorInfo();
// Some I/O error occurred
innerError =
Crypto.ErrPeekError() != 0 ? Crypto.CreateOpenSslCryptographicException() : // crypto error queue not empty
Expand All @@ -676,7 +659,8 @@ private static Ssl.SslErrorCode GetSslError(SafeSslHandle context, int result, o
innerError = null;
break;
}
return retVal;

return innerError;
}

private static void SetSslCertificate(SafeSslContextHandle contextPtr, SafeX509Handle certPtr, SafeEvpPKeyHandle keyPtr)
Expand Down
8 changes: 4 additions & 4 deletions src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.Ssl.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,13 +75,13 @@ internal static partial class Ssl
}

[GeneratedDllImport(Libraries.CryptoNative, EntryPoint = "CryptoNative_SslWrite", SetLastError = true)]
internal static partial int SslWrite(SafeSslHandle ssl, ref byte buf, int num);
internal static partial int SslWrite(SafeSslHandle ssl, ref byte buf, int num, out SslErrorCode error);

[GeneratedDllImport(Libraries.CryptoNative, EntryPoint = "CryptoNative_SslRead", SetLastError = true)]
internal static partial int SslRead(SafeSslHandle ssl, ref byte buf, int num);
internal static partial int SslRead(SafeSslHandle ssl, ref byte buf, int num, out SslErrorCode error);

[GeneratedDllImport(Libraries.CryptoNative, EntryPoint = "CryptoNative_SslRenegotiate")]
internal static partial int SslRenegotiate(SafeSslHandle ssl);
internal static partial int SslRenegotiate(SafeSslHandle ssl, out SslErrorCode error);

[GeneratedDllImport(Libraries.CryptoNative, EntryPoint = "CryptoNative_IsSslRenegotiatePending")]
[return: MarshalAs(UnmanagedType.Bool)]
Expand All @@ -97,7 +97,7 @@ internal static partial class Ssl
internal static partial void SslSetBio(SafeSslHandle ssl, SafeBioHandle rbio, SafeBioHandle wbio);

[GeneratedDllImport(Libraries.CryptoNative, EntryPoint = "CryptoNative_SslDoHandshake", SetLastError = true)]
internal static partial int SslDoHandshake(SafeSslHandle ssl);
internal static partial int SslDoHandshake(SafeSslHandle ssl, out SslErrorCode error);

[GeneratedDllImport(Libraries.CryptoNative, EntryPoint = "CryptoNative_IsSslStateOK")]
[return: MarshalAs(UnmanagedType.Bool)]
Expand Down
50 changes: 42 additions & 8 deletions src/native/libs/System.Security.Cryptography.Native/pal_ssl.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -358,14 +358,34 @@ int32_t CryptoNative_SslSessionReused(SSL* ssl)
return SSL_session_reused(ssl) == 1;
}

int32_t CryptoNative_SslWrite(SSL* ssl, const void* buf, int32_t num)
int32_t CryptoNative_SslWrite(SSL* ssl, const void* buf, int32_t num, int32_t* error)
{
return SSL_write(ssl, buf, num);
int32_t result = SSL_write(ssl, buf, num);
if (result > 0)
{
*error = SSL_ERROR_NONE;
}
else
{
*error = CryptoNative_SslGetError(ssl, result);
}

return result;
}

int32_t CryptoNative_SslRead(SSL* ssl, void* buf, int32_t num)
int32_t CryptoNative_SslRead(SSL* ssl, void* buf, int32_t num, int32_t* error)
{
return SSL_read(ssl, buf, num);
int32_t result = SSL_read(ssl, buf, num);
if (result > 0)
{
*error = SSL_ERROR_NONE;
}
else
{
*error = CryptoNative_SslGetError(ssl, result);
}

return result;
}

static int verify_callback(int preverify_ok, X509_STORE_CTX* store)
Expand All @@ -376,7 +396,7 @@ static int verify_callback(int preverify_ok, X509_STORE_CTX* store)
return 1;
}

int32_t CryptoNative_SslRenegotiate(SSL* ssl)
int32_t CryptoNative_SslRenegotiate(SSL* ssl, int32_t* error)
{
// The openssl context is destroyed so we can't use ticket or session resumption.
SSL_set_options(ssl, SSL_OP_NO_TICKET | SSL_OP_NO_SESSION_RESUMPTION_ON_RENEGOTIATION);
Expand All @@ -387,11 +407,15 @@ int32_t CryptoNative_SslRenegotiate(SSL* ssl)
SSL_set_verify(ssl, SSL_VERIFY_PEER, verify_callback);
int ret = SSL_renegotiate(ssl);
if(ret != 1)
{
*error = CryptoNative_SslGetError(ssl, ret);
return ret;
}

return SSL_do_handshake(ssl);
return CryptoNative_SslDoHandshake(ssl, error);
}

*error = SSL_ERROR_NONE;
return 0;
}

Expand All @@ -412,10 +436,20 @@ void CryptoNative_SslSetBio(SSL* ssl, BIO* rbio, BIO* wbio)
SSL_set_bio(ssl, rbio, wbio);
}

int32_t CryptoNative_SslDoHandshake(SSL* ssl)
int32_t CryptoNative_SslDoHandshake(SSL* ssl, int32_t* error)
{
ERR_clear_error();
return SSL_do_handshake(ssl);
int32_t result = SSL_do_handshake(ssl);
if (result == 1)
{
*error = SSL_ERROR_NONE;
}
else
{
*error = CryptoNative_SslGetError(ssl, result);
}

return result;
}

int32_t CryptoNative_IsSslStateOK(SSL* ssl)
Expand Down
8 changes: 4 additions & 4 deletions src/native/libs/System.Security.Cryptography.Native/pal_ssl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,22 +211,22 @@ Shims the SSL_write method.
Returns the positive number of bytes written when successful, 0 or a negative number
when an error is encountered.
*/
PALEXPORT int32_t CryptoNative_SslWrite(SSL* ssl, const void* buf, int32_t num);
PALEXPORT int32_t CryptoNative_SslWrite(SSL* ssl, const void* buf, int32_t num, int32_t* error);

/*
Shims the SSL_read method.

Returns the positive number of bytes read when successful, 0 or a negative number
when an error is encountered.
*/
PALEXPORT int32_t CryptoNative_SslRead(SSL* ssl, void* buf, int32_t num);
PALEXPORT int32_t CryptoNative_SslRead(SSL* ssl, void* buf, int32_t num, int32_t* error);

/*
Shims the SSL_renegotiate method.

Returns 1 when renegotiation started; 0 on error.
*/
PALEXPORT int32_t CryptoNative_SslRenegotiate(SSL* ssl);
PALEXPORT int32_t CryptoNative_SslRenegotiate(SSL* ssl, int32_t* error);

/*
Shims the SSL_renegotiate_pending method.
Expand Down Expand Up @@ -259,7 +259,7 @@ Shims the SSL_do_handshake method.
and by the specifications of the TLS/SSL protocol;
<0 if the handshake was not successful because of a fatal error.
*/
PALEXPORT int32_t CryptoNative_SslDoHandshake(SSL* ssl);
PALEXPORT int32_t CryptoNative_SslDoHandshake(SSL* ssl, int32_t* error);

/*
Gets a value indicating whether the SSL_state is SSL_ST_OK.
Expand Down