Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
/ corefx Public archive

More SSL error queue cleaning #29171

Merged
merged 3 commits into from
Apr 18, 2018
Merged

More SSL error queue cleaning #29171

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
176416a
Cleaning up more instances leaving errors on SSL queue
Apr 18, 2018
7befdab
Remove active issue #25676
Apr 18, 2018
c8f08f7
PR Feedback 00
Apr 18, 2018
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
5 changes: 5 additions & 0 deletions src/Common/src/Interop/Unix/System.Net.Security.Native/Interop.Initialization.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,11 @@ static SslInitializer()

//Call ssl specific initializer
Ssl.EnsureLibSslInitialized();
if (Interop.Crypto.ErrPeekLastError() != 0)
{
// It is going to be wrapped in a type load exception but will have the error information
throw Interop.Crypto.CreateOpenSslCryptographicException();
}
}
#endif

Expand Down
1 change: 0 additions & 1 deletion src/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.AcceptAllCerts.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ public void SingletonReturnsTrue()
Assert.True(HttpClientHandler.DangerousAcceptAnyServerCertificateValidator(null, null, null, SslPolicyErrors.None));
}

[ActiveIssue(25676, TestPlatforms.Linux)]
[Theory]
[InlineData(SslProtocols.Tls, false)] // try various protocols to ensure we correctly set versions even when accepting all certs
[InlineData(SslProtocols.Tls, true)]
Expand Down
18 changes: 11 additions & 7 deletions ...curity.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.Unix/CertificatePal.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,12 +34,13 @@ public static ICertificatePal FromBlob(byte[] rawData, SafePasswordHandle passwo
Debug.Assert(password != null);

ICertificatePal cert;
Exception openSslException;

if (TryReadX509Der(rawData, out cert) ||
TryReadX509Pem(rawData, out cert) ||
PkcsFormatReader.TryReadPkcs7Der(rawData, out cert) ||
PkcsFormatReader.TryReadPkcs7Pem(rawData, out cert) ||
PkcsFormatReader.TryReadPkcs12(rawData, password, out cert))
PkcsFormatReader.TryReadPkcs12(rawData, password, out cert, out openSslException))
{
if (cert == null)
{
Expand All @@ -51,7 +52,8 @@ public static ICertificatePal FromBlob(byte[] rawData, SafePasswordHandle passwo
}

// Unsupported
throw Interop.Crypto.CreateOpenSslCryptographicException();
Debug.Assert(openSslException != null);
throw openSslException;
}

public static ICertificatePal FromFile(string fileName, SafePasswordHandle password, X509KeyStorageFlags keyStorageFlags)
Expand Down Expand Up @@ -104,22 +106,21 @@ private static ICertificatePal FromBio(SafeBioHandle bio, SafePasswordHandle pas
// Rewind, try again.
RewindBio(bio, bioPosition);

if (PkcsFormatReader.TryReadPkcs12(bio, password, out certPal))
// Capture the exception so in case of failure, the call to BioSeek does not override it.
Exception openSslException;
if (PkcsFormatReader.TryReadPkcs12(bio, password, out certPal, out openSslException))
{
return certPal;
}

// Since we aren't going to finish reading, leaving the buffer where it was when we got
// it seems better than leaving it in some arbitrary other position.
//
// But, before seeking back to start, save the Exception representing the last reported
// OpenSSL error in case the last BioSeek would change it.
Exception openSslException = Interop.Crypto.CreateOpenSslCryptographicException();

// Use BioSeek directly for the last seek attempt, because any failure here should instead
// report the already created (but not yet thrown) exception.
Interop.Crypto.BioSeek(bio, bioPosition);

Debug.Assert(openSslException != null);
throw openSslException;
}

Expand All @@ -141,6 +142,7 @@ internal static bool TryReadX509Der(byte[] rawData, out ICertificatePal certPal)
{
certHandle.Dispose();
certPal = null;
Interop.Crypto.ErrClearError();
return false;
}

Expand All @@ -156,6 +158,7 @@ internal static bool TryReadX509Pem(SafeBioHandle bio, out ICertificatePal certP
{
cert.Dispose();
certPal = null;
Interop.Crypto.ErrClearError();
return false;
}

Expand All @@ -182,6 +185,7 @@ internal static bool TryReadX509Der(SafeBioHandle bio, out ICertificatePal fromB
{
cert.Dispose();
fromBio = null;
Interop.Crypto.ErrClearError();
return false;
}

Expand Down
83 changes: 63 additions & 20 deletions ...y.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.Unix/OpenSslPkcs12Reader.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,32 +24,24 @@ private OpenSslPkcs12Reader(SafePkcs12Handle pkcs12Handle)

public static bool TryRead(byte[] data, out OpenSslPkcs12Reader pkcs12Reader)
{
SafePkcs12Handle handle = Interop.Crypto.DecodePkcs12(data, data.Length);

if (!handle.IsInvalid)
{
pkcs12Reader = new OpenSslPkcs12Reader(handle);
return true;
}
Exception ignored;
return TryRead(data, out pkcs12Reader, out ignored, captureException: false);
Copy link
Member

@stephentoub stephentoub Apr 18, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: this can be:

public static bool TryRead(byte[] data, out OpenSslPkcs12Reader pkcs12Reader) =>
    TryRead(data, out pkcs12Reader, out _, captureException: false);

Same in various places below.

}

handle.Dispose();
pkcs12Reader = null;
return false;
public static bool TryRead(byte[] data, out OpenSslPkcs12Reader pkcs12Reader, out Exception openSslException)
{
return TryRead(data, out pkcs12Reader, out openSslException, captureException: true);
}

public static bool TryRead(SafeBioHandle fileBio, out OpenSslPkcs12Reader pkcs12Reader)
{
SafePkcs12Handle p12 = Interop.Crypto.DecodePkcs12FromBio(fileBio);

if (!p12.IsInvalid)
{
pkcs12Reader = new OpenSslPkcs12Reader(p12);
return true;
}
Exception ignored;
return TryRead(fileBio, out pkcs12Reader, out ignored, captureException: false);
}

p12.Dispose();
pkcs12Reader = null;
return false;
public static bool TryRead(SafeBioHandle fileBio, out OpenSslPkcs12Reader pkcs12Reader, out Exception openSslException)
{
return TryRead(fileBio, out pkcs12Reader, out openSslException, captureException: true);
}

public void Dispose()
Expand Down Expand Up @@ -132,5 +124,56 @@ public List<OpenSslX509CertificateReader> ReadCertificates()

return certs;
}

private static bool TryRead(byte[] data, out OpenSslPkcs12Reader pkcs12Reader, out Exception openSslException, bool captureException)
{
SafePkcs12Handle handle = Interop.Crypto.DecodePkcs12(data, data.Length);
openSslException = null;

if (!handle.IsInvalid)
{
pkcs12Reader = new OpenSslPkcs12Reader(handle);
return true;
}

handle.Dispose();
pkcs12Reader = null;
if (captureException)
{
openSslException = Interop.Crypto.CreateOpenSslCryptographicException();
}
else
{
Interop.Crypto.ErrClearError();
}

return false;
}

private static bool TryRead(SafeBioHandle fileBio, out OpenSslPkcs12Reader pkcs12Reader, out Exception openSslException, bool captureException)
{
SafePkcs12Handle p12 = Interop.Crypto.DecodePkcs12FromBio(fileBio);
openSslException = null;

if (!p12.IsInvalid)
{
pkcs12Reader = new OpenSslPkcs12Reader(p12);
openSslException = null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This setting to null isn't needed, or alternatively moved the earlier setting to null down to the else block below (in which case it'd be good to keep the previous method that follows a similar form in sync with this pattern).

return true;
}

p12.Dispose();
pkcs12Reader = null;
if (captureException)
{
openSslException = Interop.Crypto.CreateOpenSslCryptographicException();
}
else
{
Interop.Crypto.ErrClearError();
}

return false;
}
}
}
58 changes: 42 additions & 16 deletions ...rity.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.Unix/PkcsFormatReader.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,15 @@ internal static bool IsPkcs7(byte[] rawData)
{
using (SafePkcs7Handle pkcs7 = Interop.Crypto.DecodePkcs7(rawData, rawData.Length))
{
if (!pkcs7.IsInvalid)
if (pkcs7.IsInvalid)
{
Interop.Crypto.ErrClearError();
}
else
{
return true;
}

}

using (SafeBioHandle bio = Interop.Crypto.CreateMemoryBio())
Expand All @@ -29,7 +34,13 @@ internal static bool IsPkcs7(byte[] rawData)

using (SafePkcs7Handle pkcs7 = Interop.Crypto.PemReadBioPkcs7(bio))
{
return !pkcs7.IsInvalid;
if (pkcs7.IsInvalid)
{
Interop.Crypto.ErrClearError();
return false;
}

return true;
}
}
}
Expand All @@ -38,6 +49,11 @@ internal static bool IsPkcs7Der(SafeBioHandle fileBio)
{
using (SafePkcs7Handle pkcs7 = Interop.Crypto.D2IPkcs7Bio(fileBio))
{
if (pkcs7.IsInvalid)
{
Interop.Crypto.ErrClearError();
}

return !pkcs7.IsInvalid;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: This is inconsistent from other similar cases. To be consistent it would be:

if (pkcs7.IsInvalid)
{
    Interop.Crypto.ErrClearError();
    return false;
}

return true;

}
}
Expand All @@ -46,7 +62,13 @@ internal static bool IsPkcs7Pem(SafeBioHandle fileBio)
{
using (SafePkcs7Handle pkcs7 = Interop.Crypto.PemReadBioPkcs7(fileBio))
{
return !pkcs7.IsInvalid;
if (pkcs7.IsInvalid)
{
Interop.Crypto.ErrClearError();
return false;
}

return true;
}
}

Expand Down Expand Up @@ -90,6 +112,7 @@ private static bool TryReadPkcs7Der(
{
certPal = null;
certPals = null;
Interop.Crypto.ErrClearError();
return false;
}

Expand All @@ -109,6 +132,7 @@ private static bool TryReadPkcs7Der(
{
certPal = null;
certPals = null;
Interop.Crypto.ErrClearError();
return false;
}

Expand Down Expand Up @@ -172,6 +196,7 @@ private static bool TryReadPkcs7Pem(
{
certPal = null;
certPals = null;
Interop.Crypto.ErrClearError();
return false;
}

Expand Down Expand Up @@ -218,46 +243,46 @@ private static bool TryReadPkcs7(
return true;
}

internal static bool TryReadPkcs12(byte[] rawData, SafePasswordHandle password, out ICertificatePal certPal)
internal static bool TryReadPkcs12(byte[] rawData, SafePasswordHandle password, out ICertificatePal certPal, out Exception openSslException)
{
List<ICertificatePal> ignored;

return TryReadPkcs12(rawData, password, true, out certPal, out ignored);

return TryReadPkcs12(rawData, password, true, out certPal, out ignored, out openSslException);
}

internal static bool TryReadPkcs12(SafeBioHandle bio, SafePasswordHandle password, out ICertificatePal certPal)
internal static bool TryReadPkcs12(SafeBioHandle bio, SafePasswordHandle password, out ICertificatePal certPal, out Exception openSslException)
{
List<ICertificatePal> ignored;

return TryReadPkcs12(bio, password, true, out certPal, out ignored);
return TryReadPkcs12(bio, password, true, out certPal, out ignored, out openSslException);
}

internal static bool TryReadPkcs12(byte[] rawData, SafePasswordHandle password, out List<ICertificatePal> certPals)
internal static bool TryReadPkcs12(byte[] rawData, SafePasswordHandle password, out List<ICertificatePal> certPals, out Exception openSslException)
{
ICertificatePal ignored;

return TryReadPkcs12(rawData, password, false, out ignored, out certPals);
return TryReadPkcs12(rawData, password, false, out ignored, out certPals, out openSslException);
}

internal static bool TryReadPkcs12(SafeBioHandle bio, SafePasswordHandle password, out List<ICertificatePal> certPals)
internal static bool TryReadPkcs12(SafeBioHandle bio, SafePasswordHandle password, out List<ICertificatePal> certPals, out Exception openSslException)
{
ICertificatePal ignored;

return TryReadPkcs12(bio, password, false, out ignored, out certPals);
return TryReadPkcs12(bio, password, false, out ignored, out certPals, out openSslException);
}

private static bool TryReadPkcs12(
byte[] rawData,
SafePasswordHandle password,
bool single,
out ICertificatePal readPal,
out List<ICertificatePal> readCerts)
out List<ICertificatePal> readCerts,
out Exception openSslException)
{
// DER-PKCS12
OpenSslPkcs12Reader pfx;

if (!OpenSslPkcs12Reader.TryRead(rawData, out pfx))
if (!OpenSslPkcs12Reader.TryRead(rawData, out pfx, out openSslException))
{
readPal = null;
readCerts = null;
Expand All @@ -275,12 +300,13 @@ private static bool TryReadPkcs12(
SafePasswordHandle password,
bool single,
out ICertificatePal readPal,
out List<ICertificatePal> readCerts)
out List<ICertificatePal> readCerts,
out Exception openSslException)
{
// DER-PKCS12
OpenSslPkcs12Reader pfx;

if (!OpenSslPkcs12Reader.TryRead(bio, out pfx))
if (!OpenSslPkcs12Reader.TryRead(bio, out pfx, out openSslException))
{
readPal = null;
readCerts = null;
Expand Down
Loading