Skip to content

Commit 02ac5c3

Browse files
wfurtwfurt
authored
make sure failed SSL does not impact other sessions (#64256)
* make sure failed SSL does not imapct other sessions * move innerError * feedback from review * remove try * feedback from review
1 parent e5faab0 commit 02ac5c3

File tree

4 files changed

+67
-49
lines changed

4 files changed

+67
-49
lines changed

‎src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.OpenSsl.cs‎

Lines changed: 17 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -356,13 +356,12 @@ internal static SafeSslHandle AllocateSslHandle(SafeFreeSslCredentials credentia
356356

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

361361
outputBuffer = Array.Empty<byte>();
362362
if (ret != 1)
363363
{
364-
GetSslError(sslContext, ret, out Exception? exception);
365-
return new SecurityStatusPal(SecurityStatusPalErrorCode.InternalError, exception);
364+
return new SecurityStatusPal(SecurityStatusPalErrorCode.InternalError, GetSslError(ret, errorCode));
366365
}
367366
return new SecurityStatusPal(SecurityStatusPalErrorCode.OK);
368367
}
@@ -382,23 +381,21 @@ internal static SecurityStatusPalErrorCode DoSslHandshake(SafeSslHandle context,
382381
}
383382
}
384383

385-
int retVal = Ssl.SslDoHandshake(context);
384+
int retVal = Ssl.SslDoHandshake(context, out Ssl.SslErrorCode errorCode);
386385
if (retVal != 1)
387386
{
388-
Exception? innerError;
389-
Ssl.SslErrorCode error = GetSslError(context, retVal, out innerError);
390-
391-
if (error == Ssl.SslErrorCode.SSL_ERROR_WANT_X509_LOOKUP)
387+
if (errorCode == Ssl.SslErrorCode.SSL_ERROR_WANT_X509_LOOKUP)
392388
{
393389
return SecurityStatusPalErrorCode.CredentialsNeeded;
394390
}
395391

396-
if ((retVal != -1) || (error != Ssl.SslErrorCode.SSL_ERROR_WANT_READ))
392+
if ((retVal != -1) || (errorCode != Ssl.SslErrorCode.SSL_ERROR_WANT_READ))
397393
{
394+
Exception? innerError = GetSslError(retVal, errorCode);
395+
398396
// Handshake failed, but even if the handshake does not need to read, there may be an Alert going out.
399397
// To handle that we will fall-through the block below to pull it out, and we will fail after.
400-
handshakeException = new SslException(SR.Format(SR.net_ssl_handshake_failed_error, error), innerError);
401-
Crypto.ErrClearError();
398+
handshakeException = new SslException(SR.Format(SR.net_ssl_handshake_failed_error, errorCode), innerError);
402399
}
403400
}
404401

@@ -447,17 +444,7 @@ internal static int Encrypt(SafeSslHandle context, ReadOnlySpan<byte> input, ref
447444
ulong assertNoError = Crypto.ErrPeekError();
448445
Debug.Assert(assertNoError == 0, $"OpenSsl error queue is not empty, run: 'openssl errstr {assertNoError:X}' for original error.");
449446
#endif
450-
errorCode = Ssl.SslErrorCode.SSL_ERROR_NONE;
451-
452-
int retVal;
453-
Exception? innerError = null;
454-
455-
retVal = Ssl.SslWrite(context, ref MemoryMarshal.GetReference(input), input.Length);
456-
457-
if (retVal != input.Length)
458-
{
459-
errorCode = GetSslError(context, retVal, out innerError);
460-
}
447+
int retVal = Ssl.SslWrite(context, ref MemoryMarshal.GetReference(input), input.Length, out errorCode);
461448

462449
if (retVal != input.Length)
463450
{
@@ -471,7 +458,7 @@ internal static int Encrypt(SafeSslHandle context, ReadOnlySpan<byte> input, ref
471458
break;
472459

473460
default:
474-
throw new SslException(SR.Format(SR.net_ssl_encrypt_failed, errorCode), innerError);
461+
throw new SslException(SR.Format(SR.net_ssl_encrypt_failed, errorCode), GetSslError(retVal, errorCode));
475462
}
476463
}
477464
else
@@ -501,17 +488,14 @@ internal static int Decrypt(SafeSslHandle context, Span<byte> buffer, out Ssl.Ss
501488
ulong assertNoError = Crypto.ErrPeekError();
502489
Debug.Assert(assertNoError == 0, $"OpenSsl error queue is not empty, run: 'openssl errstr {assertNoError:X}' for original error.");
503490
#endif
504-
errorCode = Ssl.SslErrorCode.SSL_ERROR_NONE;
505-
506491
BioWrite(context.InputBio!, buffer);
507492

508-
int retVal = Ssl.SslRead(context, ref MemoryMarshal.GetReference(buffer), buffer.Length);
493+
int retVal = Ssl.SslRead(context, ref MemoryMarshal.GetReference(buffer), buffer.Length, out errorCode);
509494
if (retVal > 0)
510495
{
511496
return retVal;
512497
}
513498

514-
errorCode = GetSslError(context, retVal, out Exception? innerError);
515499
switch (errorCode)
516500
{
517501
// indicate end-of-file
@@ -526,7 +510,7 @@ internal static int Decrypt(SafeSslHandle context, Span<byte> buffer, out Ssl.Ss
526510
break;
527511

528512
default:
529-
throw new SslException(SR.Format(SR.net_ssl_decrypt_failed, errorCode), innerError);
513+
throw new SslException(SR.Format(SR.net_ssl_decrypt_failed, errorCode), GetSslError(retVal, errorCode));
530514
}
531515

532516
return 0;
@@ -647,14 +631,13 @@ private static void BioWrite(SafeBioHandle bio, ReadOnlySpan<byte> buffer)
647631
}
648632
}
649633

650-
private static Ssl.SslErrorCode GetSslError(SafeSslHandle context, int result, out Exception? innerError)
634+
private static Exception? GetSslError(int result, Ssl.SslErrorCode retVal)
651635
{
652-
ErrorInfo lastErrno = Sys.GetLastErrorInfo(); // cache it before we make more P/Invoke calls, just in case we need it
653-
654-
Ssl.SslErrorCode retVal = Ssl.SslGetError(context, result);
636+
Exception? innerError;
655637
switch (retVal)
656638
{
657639
case Ssl.SslErrorCode.SSL_ERROR_SYSCALL:
640+
ErrorInfo lastErrno = Sys.GetLastErrorInfo();
658641
// Some I/O error occurred
659642
innerError =
660643
Crypto.ErrPeekError() != 0 ? Crypto.CreateOpenSslCryptographicException() : // crypto error queue not empty
@@ -673,7 +656,8 @@ private static Ssl.SslErrorCode GetSslError(SafeSslHandle context, int result, o
673656
innerError = null;
674657
break;
675658
}
676-
return retVal;
659+
660+
return innerError;
677661
}
678662

679663
private static void SetSslCertificate(SafeSslContextHandle contextPtr, SafeX509Handle certPtr, SafeEvpPKeyHandle keyPtr)

‎src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.Ssl.cs‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -75,13 +75,13 @@ internal static partial class Ssl
7575
}
7676

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

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

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

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

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

102102
[GeneratedDllImport(Libraries.CryptoNative, EntryPoint = "CryptoNative_IsSslStateOK")]
103103
[return: MarshalAs(UnmanagedType.Bool)]

‎src/native/libs/System.Security.Cryptography.Native/pal_ssl.c‎

Lines changed: 42 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -371,14 +371,34 @@ int32_t CryptoNative_SslSessionReused(SSL* ssl)
371371
return SSL_session_reused(ssl) == 1;
372372
}
373373

374-
int32_t CryptoNative_SslWrite(SSL* ssl, const void* buf, int32_t num)
374+
int32_t CryptoNative_SslWrite(SSL* ssl, const void* buf, int32_t num, int32_t* error)
375375
{
376-
return SSL_write(ssl, buf, num);
376+
int32_t result = SSL_write(ssl, buf, num);
377+
if (result > 0)
378+
{
379+
*error = SSL_ERROR_NONE;
380+
}
381+
else
382+
{
383+
*error = CryptoNative_SslGetError(ssl, result);
384+
}
385+
386+
return result;
377387
}
378388

379-
int32_t CryptoNative_SslRead(SSL* ssl, void* buf, int32_t num)
389+
int32_t CryptoNative_SslRead(SSL* ssl, void* buf, int32_t num, int32_t* error)
380390
{
381-
return SSL_read(ssl, buf, num);
391+
int32_t result = SSL_read(ssl, buf, num);
392+
if (result > 0)
393+
{
394+
*error = SSL_ERROR_NONE;
395+
}
396+
else
397+
{
398+
*error = CryptoNative_SslGetError(ssl, result);
399+
}
400+
401+
return result;
382402
}
383403

384404
static int verify_callback(int preverify_ok, X509_STORE_CTX* store)
@@ -389,7 +409,7 @@ static int verify_callback(int preverify_ok, X509_STORE_CTX* store)
389409
return 1;
390410
}
391411

392-
int32_t CryptoNative_SslRenegotiate(SSL* ssl)
412+
int32_t CryptoNative_SslRenegotiate(SSL* ssl, int32_t* error)
393413
{
394414
// The openssl context is destroyed so we can't use ticket or session resumption.
395415
SSL_set_options(ssl, SSL_OP_NO_TICKET | SSL_OP_NO_SESSION_RESUMPTION_ON_RENEGOTIATION);
@@ -400,11 +420,15 @@ int32_t CryptoNative_SslRenegotiate(SSL* ssl)
400420
SSL_set_verify(ssl, SSL_VERIFY_PEER, verify_callback);
401421
int ret = SSL_renegotiate(ssl);
402422
if(ret != 1)
423+
{
424+
*error = CryptoNative_SslGetError(ssl, ret);
403425
return ret;
426+
}
404427

405-
return SSL_do_handshake(ssl);
428+
return CryptoNative_SslDoHandshake(ssl, error);
406429
}
407430

431+
*error = SSL_ERROR_NONE;
408432
return 0;
409433
}
410434

@@ -425,10 +449,20 @@ void CryptoNative_SslSetBio(SSL* ssl, BIO* rbio, BIO* wbio)
425449
SSL_set_bio(ssl, rbio, wbio);
426450
}
427451

428-
int32_t CryptoNative_SslDoHandshake(SSL* ssl)
452+
int32_t CryptoNative_SslDoHandshake(SSL* ssl, int32_t* error)
429453
{
430454
ERR_clear_error();
431-
return SSL_do_handshake(ssl);
455+
int32_t result = SSL_do_handshake(ssl);
456+
if (result == 1)
457+
{
458+
*error = SSL_ERROR_NONE;
459+
}
460+
else
461+
{
462+
*error = CryptoNative_SslGetError(ssl, result);
463+
}
464+
465+
return result;
432466
}
433467

434468
int32_t CryptoNative_IsSslStateOK(SSL* ssl)

‎src/native/libs/System.Security.Cryptography.Native/pal_ssl.h‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -211,22 +211,22 @@ Shims the SSL_write method.
211211
Returns the positive number of bytes written when successful, 0 or a negative number
212212
when an error is encountered.
213213
*/
214-
PALEXPORT int32_t CryptoNative_SslWrite(SSL* ssl, const void* buf, int32_t num);
214+
PALEXPORT int32_t CryptoNative_SslWrite(SSL* ssl, const void* buf, int32_t num, int32_t* error);
215215

216216
/*
217217
Shims the SSL_read method.
218218
219219
Returns the positive number of bytes read when successful, 0 or a negative number
220220
when an error is encountered.
221221
*/
222-
PALEXPORT int32_t CryptoNative_SslRead(SSL* ssl, void* buf, int32_t num);
222+
PALEXPORT int32_t CryptoNative_SslRead(SSL* ssl, void* buf, int32_t num, int32_t* error);
223223

224224
/*
225225
Shims the SSL_renegotiate method.
226226
227227
Returns 1 when renegotiation started; 0 on error.
228228
*/
229-
PALEXPORT int32_t CryptoNative_SslRenegotiate(SSL* ssl);
229+
PALEXPORT int32_t CryptoNative_SslRenegotiate(SSL* ssl, int32_t* error);
230230

231231
/*
232232
Shims the SSL_renegotiate_pending method.
@@ -259,7 +259,7 @@ Shims the SSL_do_handshake method.
259259
and by the specifications of the TLS/SSL protocol;
260260
<0 if the handshake was not successful because of a fatal error.
261261
*/
262-
PALEXPORT int32_t CryptoNative_SslDoHandshake(SSL* ssl);
262+
PALEXPORT int32_t CryptoNative_SslDoHandshake(SSL* ssl, int32_t* error);
263263

264264
/*
265265
Gets a value indicating whether the SSL_state is SSL_ST_OK.

0 commit comments

Comments
 (0)