Port DES and RC2 in System.Security.Cryptography.Algorithms and .Csp #12744

steveharter · 2016-10-18T02:19:07Z

Add support for RC2 and DES
Issues addressed:
#11117 Port T:System.Security.Cryptography.RC2xx
#11116 Port T:System.Security.Cryptography.DESxx
#12313 Port System.Security.Cryptography.CipherMode.CFB and .OFB

RC2\DES support is enabled for S.S.C.Csp via CAPI, and S.S.C.Algorithms for Windows (via Cng), Linux (via OpenSsl) and Apple.

In .Csp, support for ICryptoTransform was achieved by extending the UniversalCryptoTransform pattern which is much cleaner than porting the netfx CryptoAPITransform.

Note that there is no RC2\DES support in S.S.C.Cng as there is no netfx support for Cng RC2\DES.

For RC2, there was a lot of code that needed to be touched to set the effective key length property.

@bartonjs please review

bartonjs · 2016-10-18T16:19:42Z

src/Common/src/Interop/Windows/BCrypt/Cng.cs

@@ -229,6 +242,13 @@ private static class Interop
            [DllImport(Libraries.BCrypt, CharSet = CharSet.Unicode)]
            public static extern unsafe NTSTATUS BCryptSetProperty(SafeAlgorithmHandle hObject, String pszProperty, String pbInput, int cbInput, int dwFlags);

+            [DllImport(Libraries.BCrypt, CharSet = CharSet.Unicode, EntryPoint = "BCryptSetProperty")]
+            private static extern unsafe NTSTATUS BCryptSetIntPropertyPrivate(SafeBCryptHandle hObject, string pszProperty, ref int pdwInput, int cbInput, int dwFlags);
+            public static unsafe NTSTATUS BCryptSetIntProperty(SafeBCryptHandle hObject, string pszProperty, ref int pdwInput, int dwFlags)


Style: Don't glue these together

bartonjs · 2016-10-18T16:31:11Z

src/System.Security.Cryptography.Algorithms/src/Internal/Cryptography/DesImplementation.cs

+
+        public override ICryptoTransform CreateEncryptor()
+        {
+            return CreateTransform(this.Key, this.IV, encrypting: true);


Style: There are a lot of this.s in this file that don't seem required.

Sure. Removed them from 4 providers (including TripleDes)

bartonjs · 2016-10-18T16:34:04Z

src/System.Security.Cryptography.Algorithms/src/System.Security.Cryptography.Algorithms.csproj

@@ -5,8 +5,7 @@
  </PropertyGroup>
  <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.props))\dir.props" />
  <PropertyGroup>
-    <Configuration Condition=" '$(Configuration)' == '' ">Windows_Debug</Configuration>
-    <Platform Condition=" '$(Platform)' == '' ">AnyCPU</Platform>
+    <Platform Condition="'$(Platform)' == '' ">AnyCPU</Platform>
    <ProjectGuid>{81A05E2E-E3AE-4246-B4E6-DD5F31FB71F9}</ProjectGuid>


Why did the default configuration go away? And why did the padding space around the platform condition go away?

Reverted those. There is a duplicate config entry above before the dir.prop import; not sure if both are necessary

bartonjs · 2016-10-18T16:42:02Z

src/System.Security.Cryptography.Algorithms/src/System/Security/Cryptography/RC2.cs

+                        throw new CryptographicException(SR.Cryptography_InvalidKeySize);
+                }
+            }
+        }


There are a lot of branches in this setter, and I don't know that they're all tested since the implementations require that KeySizeValue be EffectiveKeySizeValue (so the backwards-looking first if never gets hit... I think).

I'm also not sure that it can actually be reset to 0.

This is essentially the existing implementation in netfx; didn't change the semantics assuming perhaps someone may have derived from this class and is expecting this behavior.

Yeah, but we should still have tests for it.

bartonjs · 2016-10-18T16:45:57Z

src/System.Security.Cryptography.Csp/src/Internal/Cryptography/BasicSymmetricCipherCsp.cs

+        {
+            SafeKeyHandle safeKeyHandle;
+
+            SafeProvHandle safeProvHandleTemp = AcquireSafeProviderHandle();


This looks like it gets handed off to the finalizer. Can we add a using here?

Good catch. I created a field for the provhandle (as not sure of lifetime semantics between provider and keys) and clean up in Dispose()

bartonjs · 2016-10-18T16:50:31Z

src/System.Security.Cryptography.Csp/src/System/Security/Cryptography/CapiHelper.cs

+            Debug.Assert((outputCount % 8) == 0);
+
+            // Figure out how big the encrypted data will be
+            // REVIEW NOTE: we could just bump up the buffer by 8 bytes always instead of doing the CryptEncrypt check for buffer size


Well, not 8 bytes... blocksize. But we don't have blocksize available.

(And even then only when isFinal is true. And even then only in CBC mode. But it's definitely safest to just ask the crypto library how big it is)

FWIW the netfx native code that does this bumps up the buffer by 16 bytes (doesn't ask for the size)

16 being the biggest block size that it knows about, yeah.

bartonjs · 2016-10-18T16:58:25Z

src/System.Security.Cryptography.Csp/src/System/Security/Cryptography/CapiHelper.cs

+            Debug.Assert(encryptedDataLength == cbEncryptedData);
+
+            // If isFinal, padding was added so ignore it by using outputCount as size
+            Buffer.BlockCopy(encryptedData, 0, output, outputOffset, outputCount);


I don't get this. Why are we eliminating data? How'd the caller know the desired outputCount?

This might just be because we never set isFinal to true. And so maybe some of this code can be ripped out?

isFinal does get set to true in this implementation; the netfx c++ also sets true and similarly removes the added padding

Looking at that again, it seems as though it pads the data twice. Once manually (which for this implementation we're doing in UniversalTransform, IIRC) then again because they opened the key in the desired mode.

For CTS (which isn't supported) that would produce the wrong answer. For ECB, CBC, CFB, and OFB it's fine.

But a much simpler result would be to just always use the native object in "no padding" mode, and then the isFinal flag should just be resetting the key object to the start state.

Since this is an entirely new implementation we should be verifying that using the same ICryptoTransform object twice works the same way as it does in Desktop (which can validate that simplified code proposal).

Or, we can just drop all the CAPI code and just make this type wrap an instance of RC2.Create(). (@morganB do you have a feeling between "simple" (wrap CNG) and "highly compatible" (port all the symmetric CAPI code) for this portion of the change?)

Since this is an entirely new implementation we should be verifying that using the same ICryptoTransform object twice works the same way as it does in Desktop (which can validate that simplified code proposal).

Sure will add test to use same encryptors\decryptor instances over the same data

Do you mean once transformfinal is called we can do another transform (the tests already chain multiple transforms)

Yeah. Transform, TransformFinal, Transform, TransformFinal. If that's already part of the test suite I must have missed it. (I know we have it somewhere in the stacks, but didn't see it explicitly)

bartonjs · 2016-10-18T17:01:18Z

src/System.Security.Cryptography.Csp/tests/RC2CryptoServiceProviderTests.cs

+    {
+        [Fact]
+        public static void RC2KeySize()
+        {


This test doesn't seem specific to the CSP instance... why isn't it in the shared tests?

It is specific. I just used the factory pattern and not the ctor. The key sizes are more constrained for csp

Please switch it to the ctor if it's supposed to be specific to that one type.

bartonjs · 2016-10-18T17:01:52Z

src/System.Security.Cryptography.Primitives/ref/System.Security.Cryptography.Primitives.cs

        CTS = 5,
        ECB = 2,
+        [System.ComponentModel.EditorBrowsableAttribute((System.ComponentModel.EditorBrowsableState)(1))]
+        OFB = 3,


While this is part of netstandard2.0 it's not really related to the subject of this change...

At one point the code checked against the new values; now it doesn't. Included the issue in the PR description

bartonjs · 2016-10-18T17:02:52Z

...stem.Security.Cryptography.Primitives/src/System/Security/Cryptography/SymmetricAlgorithm.cs

@@ -125,6 +125,7 @@ public virtual CipherMode Mode

            set
            {
+                // Review note: should we throw PNSE instead for OFB and CFB?


We could, I suppose.

Though note that CTS seems to have already resulted in this exception in Desktop.

bartonjs · 2016-10-18T18:08:32Z

...ystem.Security.Cryptography.Csp/src/System/Security/Cryptography/DESCryptoServiceProvider.cs

+            // done to maintain backward desktop compatibility with the behavior shipped in V1.x.
+            // The call to set the IV in CryptoAPI will ignore any bytes after the first 8
+            // bytes. We'll still reject IV's that are shorter than the block size though.
+            if (rgbIV.Length < 8)


Can't the IV be null in ECB mode?

There will always have an IV at this point (the SymmetricAlgorithm base class enforces that), and is stripped off after this code in the BasicSymmetricCipherCsp ctor if ECB

DES des = new DESCryptoServiceProvider(); des.Mode = CipherMode.ECB; ICryptoTransform encryptor = des.CreateEncryptor(key, null);

Since the CloneByteArray extension method returns null for a null input, that'll nullref here.

Thanks. Fixed + added tests that use explicit CreateEncryptor(key, null)

bartonjs · 2016-10-18T18:09:18Z

src/System.Security.Cryptography.Csp/src/System/Security/Cryptography/CapiHelper.cs

+            Debug.Assert((outputCount % 8) == 0);
+
+            // Figure out how big the encrypted data will be
+            // REVIEW NOTE: we could just bump up the buffer by 8 bytes always instead of doing the CryptEncrypt check for buffer size


16 being the biggest block size that it knows about, yeah.

bartonjs · 2016-10-18T18:22:58Z

src/System.Security.Cryptography.Csp/src/System/Security/Cryptography/CapiHelper.cs

+            Debug.Assert(encryptedDataLength == cbEncryptedData);
+
+            // If isFinal, padding was added so ignore it by using outputCount as size
+            Buffer.BlockCopy(encryptedData, 0, output, outputOffset, outputCount);


Looking at that again, it seems as though it pads the data twice. Once manually (which for this implementation we're doing in UniversalTransform, IIRC) then again because they opened the key in the desired mode.

For CTS (which isn't supported) that would produce the wrong answer. For ECB, CBC, CFB, and OFB it's fine.

But a much simpler result would be to just always use the native object in "no padding" mode, and then the isFinal flag should just be resetting the key object to the start state.

Since this is an entirely new implementation we should be verifying that using the same ICryptoTransform object twice works the same way as it does in Desktop (which can validate that simplified code proposal).

Or, we can just drop all the CAPI code and just make this type wrap an instance of RC2.Create(). (@morganB do you have a feeling between "simple" (wrap CNG) and "highly compatible" (port all the symmetric CAPI code) for this portion of the change?)

bartonjs · 2016-10-18T18:23:51Z

src/System.Security.Cryptography.Algorithms/src/System/Security/Cryptography/RC2.cs

+                        throw new CryptographicException(SR.Cryptography_InvalidKeySize);
+                }
+            }
+        }


Yeah, but we should still have tests for it.

bartonjs · 2016-10-18T18:27:57Z

::mutter:: I meant @morganbr (tagged a wrong Morgan). MorganBr: do you have a feeling between "simple" (wrap CNG) and "highly compatible" (port all the symmetric CAPI code) for the RC2CryptoServiceProvider portion of the change?

bartonjs · 2016-10-18T20:52:10Z

src/Common/tests/System/Security/Cryptography/AlgorithmImplementations/DES/DESCipherTests.cs

+                        byte[] expectedDecrypted1 = s_multiBlockString.HexToByteArray();
+                        Assert.Equal<byte>(expectedDecrypted1, decrypted1);
+
+                        byte[] plainText2 = s_multiBlockString_8.HexToByteArray();


I think it would help clarity if it encrypted the same plaintext a second time, to produce the same stable output.

The pattern was copied from 3des. It does iterate. Note that there was an issue with second decrypt (fixed now) so added a blank call to encrypt to ensure final=true clears the handle and resets the IV

morganbr · 2016-10-18T23:05:42Z

@bartonjs The only factor about using CNG vs CAPI for RC2 and DES that comes to mind for me is UWP apps where CAPI might not be available. I'm not too worried about compat as long as we have a reasonable set of tests.

morganbr · 2016-10-18T23:07:04Z

src/Common/tests/System/Security/Cryptography/AlgorithmImplementations/DES/DESCipherTests.cs

+{
+    public static class DesCipherTests
+    {
+        // These are the expected output of many decryptions. Changing these values requires re-generating test input.


Are there public test vectors we can use to confirm compatibility?

Yes the current tests use RFC2268 for RC2+ECB and FIPS81 for DES (ECB and CBC) in addition to comparing against values from .net desktop. I could not find public test vectors for RC2+CBC.

Are ALL the DES test cases from FIPS81?

No, just the two flagged (one for ECB and one for CBC). The rest are based on desktop output.

morganbr · 2016-10-18T23:09:27Z

src/Common/tests/System/Security/Cryptography/AlgorithmImplementations/RC2/RC2CipherTests.cs

+
+    public static class RC2CipherTests
+    {
+        // These are the expected output of many decryptions. Changing these values requires re-generating test input.


Same question on public test vectors

morganbr · 2016-10-19T01:36:53Z

src/System.Security.Cryptography.Primitives/src/System/Security/Cryptography/CipherMode.cs

        ECB = 2,
+        [EditorBrowsable(EditorBrowsableState.Never)]OFB = 3,
+        [EditorBrowsable(EditorBrowsableState.Never)]CFB = 4,


It looks like TripleDes is supposed to have some logic around CFB and OFB (see multiple references in https://referencesource.microsoft.com/#mscorlib/system/security/cryptography/tripledescryptoserviceprovider.cs)

It looks like AESManaged rejects these as well. Not sure if we'd want to try to port that.

Note that SymmeticAlgorithm currently rejects everything other than ECB and CBC https://github.com/dotnet/corefx/blob/master/src/System.Security.Cryptography.Primitives/src/System/Security/Cryptography/SymmetricAlgorithm.cs#L128-L129 so if that changes we would need to fix up other classes as appropriate

stephentoub · 2016-10-19T15:16:01Z

src/System.Security.Cryptography.Algorithms/src/Internal/Cryptography/RC2Implementation.cs

+    internal sealed partial class RC2Implementation : RC2
+    {
+        private const int BitsPerByte = 8;
+        private static readonly RandomNumberGenerator s_rng = RandomNumberGenerator.Create();


I noticed a few of these types have such an s_rng field initialized to its own RandomNumberGenerator.Create() instance. Could we instead put a single instance on some shared class such that all of them can use the same instance?

I think just putting an internal static method\field on RandomNumberGenerator would work although we may need to be concerned about thread safety if used incorrectly. I think that could\should be addressed by a new issue however and not here. Thanks

although we may need to be concerned about thread safety if used incorrectly

What kind of concerns?

If a method somewhere is thread-safe and calls the static method which uses the static field (and assuming RandomNumberGenerator is not thead-safe). Does not apply to the crypto classes that use s_rng, but could be misused elsewhere

RandomNumberGenerator is thread-safe. (If it weren't, this existing code using s_rng would be broken.)

You're right - it has to be. When I replied originally I was thinking it was not static here (but the s_ gives it away)

bartonjs · 2016-10-19T17:50:25Z

src/System.Security.Cryptography.Algorithms/src/System/Security/Cryptography/RC2.cs

+            LegalBlockSizesValue = s_legalBlockSizes;
+            LegalKeySizesValue = s_legalKeySizes;
+            KeySize = 128;
+            BlockSize = 64;


I'm currently fixing the existing SymmetricAlgorithm classes to assign the fields instead of the properties. Please switch these new ones to set KeySizeValue and BlockSizeValue.

Can point me to you PR when ready to make sure we're the same. The cloning semantics are inconsistent also. Thanks

bartonjs · 2016-10-19T21:57:51Z

src/System.Security.Cryptography.Algorithms/src/System/Security/Cryptography/RC2.cs

+            new KeySizes(minSize: 64, maxSize: 64, skipSize: 0)
+        };
+
+        private static KeySizes[] s_legalKeySizes =


These can be readonly

bartonjs · 2016-10-19T21:58:50Z

src/System.Security.Cryptography.Algorithms/src/System/Security/Cryptography/RC2.cs

+        protected RC2()
+        {
+            LegalBlockSizesValue = s_legalBlockSizes;
+            LegalKeySizesValue = s_legalKeySizes;


Since this assignment came up in the AES/3DES review you'll probably want to match the fixed behavior, and merge in the tamper tests.

bartonjs · 2016-10-20T13:45:27Z

src/System.Security.Cryptography.Csp/src/System/Security/Cryptography/CapiHelper.cs

+
+                    break;
+                default:
+                    Debug.Assert(false);


I believe we use Debug.Fail() instead of Debug.Assert(false)

bartonjs · 2016-10-20T13:47:21Z

src/System.Security.Cryptography.Csp/src/System/Security/Cryptography/CapiHelper.cs

+
+            byte[] dataTobeDecrypted = new byte[inputCount];
+            Buffer.BlockCopy(input, inputOffset, dataTobeDecrypted, 0, inputCount);
+            //Array.Reverse(dataTobeDecrypted);


Please delete commented out code.

bartonjs

A couple tiny nits from this pass, but otherwise LGTM

Enabled for .Csp via CAPI, and .Algorithms for Windows (via Cng), Linux (via OpenSsl) and Apple.

dnfclas added the cla-already-signed label Oct 18, 2016

steveharter force-pushed the PortDesRc2 branch 3 times, most recently from 80c788f to ea9004d Compare October 18, 2016 15:54

steveharter changed the title ~~Port Crypto DES and RC2 for S.S.C.Csp and .Algorithms~~ Port DES and RC2 in System.Security.Cryptography.Algorithms and .Csp Oct 18, 2016

karelz added area-System.Security netstandard2.0 labels Oct 18, 2016

karelz assigned steveharter Oct 18, 2016

steveharter force-pushed the PortDesRc2 branch from cee8077 to b0bd20d Compare October 18, 2016 16:48

bartonjs suggested changes Oct 18, 2016

View reviewed changes

bartonjs reviewed Oct 18, 2016

View reviewed changes

steveharter force-pushed the PortDesRc2 branch 3 times, most recently from 75adc83 to b8248c4 Compare October 19, 2016 00:56

morganbr reviewed Oct 19, 2016

View reviewed changes

stephentoub reviewed Oct 19, 2016

View reviewed changes

bartonjs reviewed Oct 19, 2016

View reviewed changes

steveharter force-pushed the PortDesRc2 branch from 068a3d5 to 7913633 Compare October 19, 2016 22:52

bartonjs reviewed Oct 20, 2016

View reviewed changes

bartonjs approved these changes Oct 20, 2016

View reviewed changes

Add DES and RC2 to System.Security.Cryptography.Algorithms and .Csp

ec92082

Enabled for .Csp via CAPI, and .Algorithms for Windows (via Cng), Linux (via OpenSsl) and Apple.

steveharter force-pushed the PortDesRc2 branch from a03275d to ec92082 Compare October 20, 2016 15:17

steveharter merged commit d2ff60b into dotnet:master Oct 20, 2016

karelz modified the milestone: 1.2.0 Dec 3, 2016

steveharter deleted the PortDesRc2 branch April 15, 2019 18:20

Port DES and RC2 in System.Security.Cryptography.Algorithms and .Csp #12744

Port DES and RC2 in System.Security.Cryptography.Algorithms and .Csp #12744

Conversation

steveharter commented Oct 18, 2016 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steveharter Oct 18, 2016 • edited

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

steveharter Oct 18, 2016 • edited

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 Oct 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

morganbr commented Oct 18, 2016

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bartonjs left a comment

Choose a reason for hiding this comment

steveharter commented Oct 18, 2016 •

edited

steveharter Oct 18, 2016 •

edited

steveharter Oct 18, 2016 •

edited