Reducing chances of polluting SSL error queue #29351
Conversation
| @@ -382,6 +382,7 @@ private static bool VerifyCertChain(SafeX509StoreCtxHandle storeCtx, EasyRequest | |||
|
|
|||
| // X509_verify_cert can return < 0 in the case of programmer error | |||
| Debug.Assert(sslResult == 0, "Unexpected error from X509_verify_cert: " + sslResult); | |||
| Interop.Crypto.ErrClearError(); | |||
There was a problem hiding this comment.
This should only be necessary if the result wasn't 0. (No need to grab the lock when it isn't needed)
|
What happened to the src/Native/Unix/System.Security.Cryptography.Native/pal_ecc_import_export.cpp and the src/System.Security.Cryptography.Algorithms/tests/DefaultECDiffieHellmanProvider.Unix.cs (and ECDSA tests) changes? |
|
@dotnet-bot test Outerloop Linux x64 Debug |
|
The Interop.Crypto in the test projects isn't the same Interop.Crypto, so you still need the fixes there. And it looks like Interop.Crypto.EcKeyCreateByKeyParameters doesn't clear the error queue before throwing a PNSE. (And... apparently... neglects to throw at all if rc was 0; though the caller would catch it as an invalid key handle and throw) |
Though, applying logic: if all we care about is SslStream-correctness, the crypto algorithms tests aren't really a risk for that. If the tests mess themselves up, I guess that's their own problem. |
|
Thanks for pointing the location that I missed. |
| @@ -242,6 +246,10 @@ internal static bool DoSslHandshake(SafeSslHandle context, byte[] recvBuf, int r | |||
|
|
|||
| internal static int Encrypt(SafeSslHandle context, ReadOnlyMemory<byte> input, ref byte[] output, out Ssl.SslErrorCode errorCode) | |||
| { | |||
| #if DEBUG | |||
There was a problem hiding this comment.
I missed this when I separated a larger PR into smaller clean ups for SSL. I will add something to capture the whole error queue but perhaps not on this PR.
|
@dotnet-bot test this |
| // If it ever happens, ensure the error queue gets cleared. | ||
| // And since it didn't write the data, reporting an exception is good too. | ||
| throw CreateOpenSslCryptographicException(); | ||
| } |
There was a problem hiding this comment.
Is this necessary? Presumably we're asserting because it's never possible.
There was a problem hiding this comment.
It is more on the paranoid side but at least for one of the encode delegates called via this method the OpenSsl docs states the following: "This may be fixed in future so code should not assume that i2d_X509() will always succeed." - granted that it is in the context of not fully initialized struct.
@bartonjs what is your take here?
There was a problem hiding this comment.
It depends on the goals. We've never seen the assert fire, so this is probabilisticly dead code.
Under weird not-supported multi-threading modes, though, maybe one of the values being sent to Encode is capable of failing; and that would then cause an error to show up in SslStream.
If we're going for "perfect" then this is warranted. If we're going for "probabilistic" then it isn't.
There was a problem hiding this comment.
Let's go for "asymptotically perfect" ...
| @@ -171,7 +187,8 @@ internal static RSAParameters ExportRsaParameters(SafeRsaHandle key, bool includ | |||
| out IntPtr iqmp); | |||
|
|
|||
| [DllImport(Libraries.CryptoNative, EntryPoint = "CryptoNative_SetRsaParameters")] | |||
| internal static extern void SetRsaParameters( | |||
| [return: MarshalAs(UnmanagedType.Bool)] | |||
There was a problem hiding this comment.
Is this MarshalAs necessary? Isn't this the default marshaling for a bool?
There was a problem hiding this comment.
To be fair I actually don't know I just followed the pattern from other declarations... I will check this during the day.
There was a problem hiding this comment.
It is not required, but with the exception of 2 "CryptoNative_*" all extern bool have the MarshalAs - unless there is a cost associated to it I would say to keep it for consistency.
| @@ -49,7 +49,8 @@ internal static partial class Ssl | |||
| private static extern IntPtr SslGetVersion(SafeSslHandle ssl); | |||
|
|
|||
| [DllImport(Libraries.CryptoNative, EntryPoint = "CryptoNative_SslSetTlsExtHostName")] | |||
| internal static extern int SslSetTlsExtHostName(SafeSslHandle ssl, string host); | |||
| [return: MarshalAs(UnmanagedType.Bool)] | |||
There was a problem hiding this comment.
Same question for all of these.
|
@dotnet-bot test Outerloop Linux x64 Debug Build |
|
Failure in Outerloop Linux x64 Debug (SLES.12.Amd64.Open-x64) was unrelated: https://mc.dot.net/#/user/pjanotti/pr~2Fjenkins~2Fdotnet~2Fcorefx~2Fmaster~2F/test~2Ffunctional~2Fcli~2F/3911d5ff2777d9acc68901aa5372b3b1707aa466/workItem/System.Net.NameResolution.Functional.Tests Doing another round, just to get more runs with the changes: |
|
One more round just for extra precaution: |
* 1st pass to reduce chances of polluting SSL error queue * Remove extra line added by mistake * PR feedback: remove 1 unneeded ErrClearError * PR feedback (1 missed clean up) + Assert that was missed from earlier PR
* Reducing chances of polluting SSL error queue (#29351)
This is a regression in 2.1.0 (dotnet#29351). Fixes https://github.com/dotnet/corefx/issues/29942.
* Skip certificates we can't read when populating machine store. This is a regression in 2.1.0 (#29351). Fixes https://github.com/dotnet/corefx/issues/29942. * Add test * Assert chmod returns 0 * Skip test on OSX * Add valid content to the unreadable cert file
…t#29973) * Skip certificates we can't read when populating machine store. This is a regression in 2.1.0 (dotnet#29351). Fixes https://github.com/dotnet/corefx/issues/29942. * Add test * Assert chmod returns 0 * Skip test on OSX * Add valid content to the unreadable cert file
…t#29973) * Skip certificates we can't read when populating machine store. This is a regression in 2.1.0 (dotnet#29351). Fixes https://github.com/dotnet/corefx/issues/29942. * Add test * Assert chmod returns 0 * Skip test on OSX * Add valid content to the unreadable cert file
…t#29973) * Skip certificates we can't read when populating machine store. This is a regression in 2.1.0 (dotnet#29351). Fixes https://github.com/dotnet/corefx/issues/29942. * Add test * Assert chmod returns 0 * Skip test on OSX * Add valid content to the unreadable cert file
… (#30155) * Skip certificates we can't read when populating machine store. This is a regression in 2.1.0 (#29351). Fixes https://github.com/dotnet/corefx/issues/29942. * Add test * Assert chmod returns 0 * Skip test on OSX * Add valid content to the unreadable cert file
* 1st pass to reduce chances of polluting SSL error queue * Remove extra line added by mistake * PR feedback: remove 1 unneeded ErrClearError * PR feedback (1 missed clean up) + Assert that was missed from earlier PR Commit migrated from dotnet/corefx@f95b511
…t/corefx#29973) * Skip certificates we can't read when populating machine store. This is a regression in 2.1.0 (dotnet/corefx#29351). Fixes https://github.com/dotnet/corefx/issues/29942. * Add test * Assert chmod returns 0 * Skip test on OSX * Add valid content to the unreadable cert file Commit migrated from dotnet/corefx@ca9e224
This merges my sample change with some changes coded by @bartonjs.
If errors were being ignored the typical approach is for adding a managed wrapper that P/Invokes the native PAL and clear the error there, alternatively it could have been done on the native but per conversation, @bartonjs asked for consistency and to make it in managed.
In a few cases it is more economical (in terms of # of lines changed) to do everything on the code calling the PAL methods, e.g.: single call can clean up multiple methods. In other situations this is required to keep the current exceptions unchanged and still having something to report. Some errors related to OOM on the native PAL were being ignored and likely causing failures later, these now should throw with proper exception. A few changes to the native PAL were required either to return errors to managed or in a special case clean-up the errors that were ignored.
A wider refactoring regarding error handling for usage of OpenSsl is desirable in the future (perhaps when upgrading to newer version of OpenSsl). This change is tactical with the goal of being relatively low risk while reducing the risk of, self-inflicted, spurious SslStream errors during Write/Read.