Enable RSA-OAEP(SHA-2) and RSA-PSS on Unix systems #27394

bartonjs · 2018-02-23T02:39:20Z

This change provides an implementation of the OAEP padding algorithm and
the PSS encoding and verification routines in managed code. On the platforms
where we currently lack support for SHA-2-based OAEP and/or PSS the
managed implementation will be used in conjunction with the native layer in
a pre-padded operational context.

The suite of tests which were added uncovered other bugs which are being
addressed in this change, as well. Mainly that RSACng and
RSASecurityTransforms both failed at encrypting zero-length data. To solve
that problem the RSA padding class can build PKCS#1 encryption padding,
but since the native layers are capable of correctly decrypting the payloads
no unpadding code is needed at this time.

Fixes #2522
Fixes #2523
Fixes #27120

This change provides an implementation of the OAEP padding algorithm and the PSS encoding and verification routines in managed code. On the platforms where we currently lack support for SHA-2-based OAEP and/or PSS the managed implementation will be used in conjunction with the native layer in a pre-padded operational context. The suite of tests which were added uncovered other bugs which are being addressed in this change, as well. Mainly that RSACng and RSASecurityTransforms both failed at encrypting zero-length data. To solve that problem the RSA padding class can build PKCS#1 encryption padding, but since the native layers are capable of correctly decrypting the payloads no unpadding code is needed at this time.

The first call went as expected, the second call failed, and the throw was missing.

stephentoub · 2018-02-23T15:49:31Z

src/Common/src/Interop/OSX/Interop.CoreFoundation.CFData.cs

@@ -61,10 +68,13 @@ internal static unsafe bool TryCFWriteData(SafeCFDataHandle cfData, Span<byte> d
                    return false;
                }

-                byte* dataBytes = CFDataGetBytePtr(cfData);
-                fixed (byte* destinationPtr = &MemoryMarshal.GetReference(destination))
+                if (length > 0)


Minor, but how about moving the above if block into this one? If length <= 0, then destination.Length will never be < length.

src/Common/src/Interop/OSX/System.Security.Cryptography.Native.Apple/Interop.RSA.cs

stephentoub · 2018-02-23T16:01:32Z

src/Common/src/System/Security/Cryptography/RSACng.SignVerify.cs

+
+        private static int GetHashSizeInBytes(HashAlgorithmName hashAlgorithm)
+        {
+            lock (s_hashSizes)


Can we use ConcurrentDictionary instead?

stephentoub · 2018-02-23T16:02:41Z

src/Common/src/System/Security/Cryptography/RSAOpenSsl.cs

-                return buf;
+                if (buf != null)
+                {
+                    Array.Clear(buf, 0, rsaSize);


How are you deciding when to use Array.Clear vs CryptographicOperation.ZeroMemory?

Honest: The parts I wrote before rebasing are Array.Clear, the parts after are ZeroMemory... unless they're in a file that gets built for something other than netcoreapp.

Less honest: Just the !netcoreapp part, but with a bad self-analysis.

Normalized to ZeroMemory

stephentoub · 2018-02-23T16:05:35Z

src/Common/src/System/Security/Cryptography/RSAOpenSsl.cs

-            CheckReturn(returnValue);
+            if (!encrypted || bytesWritten != buf.Length)
+            {
+                Debug.Fail("TryEncrypt behaved unexpectedly");


Since there are multiple conditions that could have led to this failure, consider putting details into the assert, e.g.

Debug.Fail($"TryEncrypt behaved unexpectedly: {nameof(encrypt)}=={encrypted}, {nameof(bytesWritten)}=={bytesWritten}, {nameof(buf.Length)}=={buf.Length}");

stephentoub · 2018-02-23T16:08:12Z

src/Common/src/System/Security/Cryptography/RSASecurityTransforms.cs

-                Interop.AppleCrypto.PAL_HashAlgorithm palAlgId =
-                    PalAlgorithmFromAlgorithmName(hashAlgorithm, out expectedSize);
+                // A signature will always be the keysize (in ceiling-bytes) in length.
+                byte[] output = new byte[(KeySize + 7) / 8];


This math appears a lot... I know you'd proposed a public API for it, but short of that, how about an internal/private one?

Added a utility method on RsaPaddingProcessor for now.

stephentoub · 2018-02-23T16:09:14Z

src/Common/src/System/Security/Cryptography/RsaPaddingProcessor.cs

+
+        internal static RsaPaddingProcessor OpenProcessor(HashAlgorithmName hashAlgorithmName)
+        {
+            lock (s_lookup)


Same ConcurrentDictionary question. It'd be nice to avoid a global lock like this for already initialized values.

Changed both to ConcurrentDictionary.

stephentoub · 2018-02-23T16:11:29Z

src/Common/src/System/Security/Cryptography/RsaPaddingProcessor.cs

+        }
+
+        // This is a copy of RandomNumberGeneratorImplementation.GetNonZeroBytes, but adapted
+        // to the object-less RandomNumberGenerator.Fill.


Should we have a public RandomNumberGenerator.FillNonZeroBytes?

The only algorithm that I know of which needs it is this one. Absent valid customer scenarios for it I don't think we should bother.

stephentoub · 2018-02-23T16:13:26Z

src/Native/Unix/System.Security.Cryptography.Native.Apple/pal_rsa.cpp

+        return kErrorBadInput;
+    }
+
+    assert(func != nullptr);


Just curious, why is this an assert rather than verified in the above return kErrorBadInput check?

The above checks are for values that came across the P/Invoke boundary, so it's an API constraint enforcement. func has to be passed in via a non-C-static function in this library, so it's an internal consistency check instead of an API constraint

…ix RSACSP Because SignData defers to _impl.SignData it doesn't come back to the wrapper SignHash to throw there.

bartonjs · 2018-02-24T01:16:20Z

Yay, the last commit fixed the 10.12 problem. (Apparently in 10.12 you couldn't call SecKeyCreateDecryptedData with a public key, which mostly makes sense. And in 10.13 you can, which is where I landed).

And since I have the tests that are for bugfixes in CoreFx RSACng suppressed on NetFx now I'll push that and the code review feedback and hopefully be green.

bartonjs · 2018-02-24T06:19:48Z

@dotnet-bot Test OSX x64 Debug Build please (build never finished?)

bartonjs · 2018-02-24T21:22:26Z

@dotnet-bot Test Linux x64 Release Build please (System.Xml.XmlSchemaSet.Tests segfault on Ubuntu 16.04)

GrabYourPitchforks

Only minor feedback.

GrabYourPitchforks · 2018-02-26T21:25:22Z

src/Common/src/System/Security/Cryptography/RSACng.EncryptDecrypt.cs

+                    }
+                    finally
+                    {
+                        CryptographicOperations.ZeroMemory(paddedMessage);


I realize this particular routine is for zero-length data so this isn't critical, but we should be in the habit of pinning buffers if we're going to zero them. This has caused a bit of ugliness in the code in previous reviews. I wonder if it makes sense to have an internal crypto-specific MemoryPool<byte> where the returned Memory<byte> instances are guaranteed to be pinned and where the Return function is guaranteed to call ZeroMemory?

(Not necessary for this review - just thoughts for the future.)

We'd need to be careful with it, since we need to make sure it's disposable (for using-goodness) and maybe Finalizable (to avoid memory leaks).

ArrayPooled, eventually, tends to be Gen2, so it doesn't move. But if we're doing guaranteed pin we need to be careful.

GrabYourPitchforks · 2018-02-26T21:29:37Z

src/Common/src/System/Security/Cryptography/RSACng.SignVerify.cs

@@ -40,6 +64,11 @@ public override byte[] SignHash(byte[] hash, HashAlgorithmName hashAlgorithm, RS

            using (SafeNCryptKeyHandle keyHandle = GetDuplicatedKeyHandle())
            {
+                if (hash.Length != GetHashSizeInBytes(hashAlgorithm))


Nit: move this up to the top of the method with the other parameter checking (before the using statement) if possible.

I intentionally had it down this far so that we more closely emulated the behavior of the OS being the thing to reject the value. That said, I can't think of a reason that this needs to stay this way. So I'll move it.

GrabYourPitchforks · 2018-02-26T21:34:20Z

src/Common/src/System/Security/Cryptography/RSAOpenSsl.cs

+                }
+                finally
+                {
+                    tmp.Clear();


GrabYourPitchforks · 2018-02-26T21:45:43Z

src/Common/src/System/Security/Cryptography/RsaPaddingProcessor.cs

+
+        internal static int BytesRequiredForBitCount(int keySizeInBits)
+        {
+            return (keySizeInBits + 7) / 8;


return (int)(((uint)keySizeInBits + 7) / 8);, just in case somebody passes in an absurdly large value.

GrabYourPitchforks · 2018-02-27T20:08:56Z

src/Common/src/System/Security/Cryptography/RsaPaddingProcessor.cs

+            ReadOnlySpan<byte> source,
+            Span<byte> destination)
+        {
+            // https://tools.ietf.org/html/rfc3447#section-7.1.2


GrabYourPitchforks · 2018-02-27T20:18:45Z

src/Common/src/System/Security/Cryptography/RsaPaddingProcessor.cs

+                    Mgf1(hasher, db, seedMask);
+
+                    // 2(h)
+                    for (int i = 0; i < seedMask.Length; i++)


Nit: Split this out into a helper method. I want to add a void Span.Xor(Span<byte> a, ReadOnlySpan<byte> b) static method to BCL in the future, and it'll help find callers if this is already split out.

GrabYourPitchforks · 2018-02-27T20:37:57Z

src/Common/src/System/Security/Cryptography/RsaPaddingProcessor.cs

+                    bool yIsZero = y == 0;
+                    bool separatorMadeSense = separatorPos < dbMask.Length;
+
+                    bool shouldContinue = lHashMatches & yIsZero & separatorMadeSense;


Add comment that intentionally using non-short-circuiting operators.

GrabYourPitchforks · 2018-02-27T20:49:12Z

src/Common/src/System/Security/Cryptography/RsaPaddingProcessor.cs

+
+                    Span<byte> message = dbMask.Slice(separatorPos + 1);
+
+                    if (message.Length <= destination.Length)


I wonder if this pattern would encourage timing attacks which disclose the decrypted message length. Consider this: the RSA decryption consumer knows that a well-formed decrypted message is n bytes in length. When the consumer calls RSA-decrypt they pass a buffer of length n as the destination. The adversary doesn't necessarily know n, perhaps because it's a computed value instead of a hardcoded value. If the adversary can then generate encrypted messages which are well-formed but overlong, they can time how long it takes the server to reject those messages, and eventually they'll see a spike in the amount of time taken, which discloses n.

I don't know if this is a legitimate concern in practice, since there may exist other signals which disclose n for any arbitrary message. Just food for thought.

Perhaps. I took my cues from CNG on this one: they accept accurate destination sizes, not just destinations at least as big as n.

GrabYourPitchforks · 2018-02-27T21:00:20Z

src/Common/src/System/Security/Cryptography/RsaPaddingProcessor.cs

+            ArrayPool<byte>.Shared.Return(dbMaskRented);
+        }
+
+        internal bool VerifyPss(ReadOnlySpan<byte> mHash, ReadOnlySpan<byte> em, int keySize)


// https://tools.ietf.org/html/rfc3447#section-9.1.2

ianhays · 2018-02-27T20:19:05Z

....Security.Cryptography.Csp/src/System/Security/Cryptography/RSACryptoServiceProvider.Unix.cs

@@ -170,12 +170,18 @@ public override int KeySize
        public override string SignatureAlgorithm => "http://www.w3.org/2000/09/xmldsig#rsa-sha1";

        public override byte[] SignData(Stream data, HashAlgorithmName hashAlgorithm, RSASignaturePadding padding) =>
+            padding == null ? throw new ArgumentNullException(nameof(padding)) :
+            padding != RSASignaturePadding.Pkcs1 ? throw PaddingModeNotSupported() :
            _impl.SignData(data, hashAlgorithm, padding);


nit: not crazy about the => into the nested ternary conditionals. Would be more slightly readable as a bracketed declaration:

public override byte[] SignData(Stream data, HashAlgorithmName hashAlgorithm, RSASignaturePadding padding) { if (padding == null) throw new ArgumentNullException(nameof(padding)); if (padding != RSASignaturePadding.Pkcs1) throw PaddingModeNotSupported(); return _impl.SignData(data, hashAlgorithm, padding); }

Yeah, me neither, but it matched existing style.

ianhays · 2018-02-27T20:20:56Z

...curity.Cryptography.Csp/src/System/Security/Cryptography/RSACryptoServiceProvider.Windows.cs

+                int rsaSize = (KeySize + 7) / 8;
+                const int OaepSha1Overhead = 20 + 20 + 2;
+
+                // Normalize the Windows 7 and Windows 8.1+ exception


nit: move this comment to above the if (foaep).

ianhays · 2018-02-27T20:41:31Z

src/Common/src/System/Security/Cryptography/RsaPaddingProcessor.cs

+            }
+        }
+
+        // https://tools.ietf.org/html/rfc3447#appendix-B.2.1


nit: I'm having trouble matching up what this function does and the steps for MGF1 in RFC3447. They're structured very differently, which makes the correctness of this hard for me to verify.

Yeah, this one is sort of weird to write in the order they wrote it in the RFC.

If maskLen > 2^32 hLen, output "mask too long" and stop.

We know that's not the case, because maskLen was specified as mask.Length

For counter from 0 to \ceil (maskLen / hLen) - 1, do the
following:

It wants us to hash the minimum number of times we need to to produce maskLen bytes.

a. Convert counter to an octet string C of length 4 octets (see
Section 4.1):

C = I2OSP (counter, 4) .

That means "make a 4 byte big endian value for counter". Aka BinaryPrimitives.WriteInt32BigEndian(bigEndianCount, count);

b. Concatenate the hash of the seed mgfSeed and C to the octet
string T:

T = T || Hash(mgfSeed || C) .

Let's scope it in to Hash(mgfSeed || C):

hasher.AppendData(mgfSeed); BinaryPrimitives.WriteInt32BigEndian(bigEndianCount, count); hasher.AppendData(bigEndianCount);

Let's skip ahead:

Output the leading maskLen octets of T as the octet string mask.

Okay, so rather than build up a big T we can just write this out to the destination because we don't use the collective T value.

The only problem we have is when the last hash is bigger than it needs to be, so we need to write it to a temporary location and then only copy over the remaining data.

Now that we've seen how we got to the end, we look back at

Let T be the empty octet string.

And conclude that it was required for the formula, but not the implementation.

ianhays · 2018-02-27T22:38:39Z

src/Common/src/System/Security/Cryptography/RsaPaddingProcessor.cs

+                // 1(a) does not apply, we do not allow custom label values.
+
+                // 1(b)
+                if (source.Length > maxInput)


I'm not certain I understand what's happening here for 1B. What we appear to be doing here is based on the assumption that mLen is correct, so the caller knows the destination length? 1C I get.

// Source = C // Destination = M int mLen = destination.Length; int min_k = m_Len + 2*_hLen + 2; // from mLen <= k - 2hLen - 2 // 1(c) // if (min_k < 2*_hLen + 2) throw cryptoException // => if (destination.Length +2*_hLen + 2 < 2*_hLen + 2) // => if (destination.Length < 0) // => always false because span.Length is always nonnegative. // 1 (b) // if (source.Length != k) throw cryptoException // => if (source.Length < min_k) // => if (source.Length < destination.Length + 2*_hLen + 2) // ... something here? // => if (source.Length > destination.Length - 2*_hLen - 2)

I'm missing how to get to the assertion that your maxInput statement is correct.

When I flip source/destination the math makes sense, but then nothing I don't understand the parameters. That would mean that source holds the message and destination holds the ciphertext, so we would be doing the inverse of how the RFC defined the function.

// source = M // destination = C int mLen = source.Length int min_k = mLen + 2hlen +2 // from mLen <= k - 2hLen - 2 // 1(c) // if (min_k < 2*_hLen + 2) throw cryptoException // => if (source.Length +2*_hLen + 2 < 2*_hLen + 2) // => if (source.Length < 0) // => always false because span.Length is always nonnegative. // 1 (b) // if (destination.Length != k) throw cryptoexception // => if (destination.Length < min_k) // => if (destination.Length < source.Length + 2hlen + 2) // => if (source.Length > destination.Length - 2hlen - 2

Is that the case, or am I misunderstanding something fundamental here?

lol, I was comparing this to the argument validation to the 7.1.2 section that you mentioned at the top. No wonder it didn't link up. Serves me right for trying to understand it line-by-line and not looking ahead :)

bartonjs added the area-System.Security label Feb 23, 2018

bartonjs added this to the 2.1.0 milestone Feb 23, 2018

bartonjs self-assigned this Feb 23, 2018

bartonjs requested review from GrabYourPitchforks and ianhays February 23, 2018 02:39

bartonjs added 6 commits February 22, 2018 21:02

Make RSACryptoServiceProvider correctly throw when encryption fails

53e4033

The first call went as expected, the second call failed, and the throw was missing.

Fix a bad assert

4557563

The OpenSsl tests should understand they support SHA-2 OAEP now

8d5a4ac

Fix macOS build break

03c53de

Don't call RNG.Fill since these tests are shared with netfx

aecb53b

Unix RSACryptoServiceProvider should fail when PSS signing

caee8b5

stephentoub reviewed Feb 23, 2018

View reviewed changes

src/Common/src/Interop/OSX/System.Security.Cryptography.Native.Apple/Interop.RSA.cs Show resolved Hide resolved

stephentoub reviewed Feb 23, 2018

View reviewed changes

bartonjs added 5 commits February 23, 2018 08:47

Normalize the RSACryptoServiceProvider exception on Win7 and Win8.1+

41b4ca8

Make the rest of the Sign and Verify overloads work as expected on Un…

b963286

…ix RSACSP Because SignData defers to _impl.SignData it doesn't come back to the wrapper SignHash to throw there.

Indicate that macOS 10.12 can't do PSS

a3bd971

Maybe fix the macOS 10.12 problem

023360a

Disable some tests on NetFx

d33cad5

bartonjs added 2 commits February 23, 2018 17:17

Clean up SupportsPss in DefaultRSAProvider

6d8087d

Review feedback

9f8fb43

GrabYourPitchforks approved these changes Feb 27, 2018

View reviewed changes

ianhays reviewed Feb 27, 2018

View reviewed changes

ianhays approved these changes Feb 27, 2018

View reviewed changes

Review feedback

6e64b94

bartonjs merged commit 59aeab3 into dotnet:master Feb 28, 2018

daviddesmet mentioned this pull request Mar 21, 2018

Paseto Version 1 and RSA PSS Padding on Linux systems daviddesmet/paseto-dotnet#1

Closed

bartonjs removed their assignment Mar 22, 2018

filipnavara mentioned this pull request Apr 28, 2021

[macOS] Use data keys on macOS for RSA, EC cryptography dotnet/runtime#51976

Merged


		Span<byte> message = dbMask.Slice(separatorPos + 1);

		if (message.Length <= destination.Length)

Enable RSA-OAEP(SHA-2) and RSA-PSS on Unix systems #27394

Enable RSA-OAEP(SHA-2) and RSA-PSS on Unix systems #27394

Conversation

bartonjs commented Feb 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bartonjs commented Feb 24, 2018

bartonjs commented Feb 24, 2018

bartonjs commented Feb 24, 2018

GrabYourPitchforks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bartonjs Feb 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bartonjs Feb 27, 2018 •

edited

Loading