From d02eae83f501984d554b3a8fca2f0eb0a39304f4 Mon Sep 17 00:00:00 2001 From: Kevin Jones Date: Tue, 13 Jul 2021 21:16:06 -0400 Subject: [PATCH 1/3] Require CFB8 for persisted keys in CFB mode. This changes AesCng and TripleDESCng to require the feedback size to be set to '8' when in CFB mode and using a persisted CNG key. Prior to this change, BasicSymmetricCipherNCrypt ignored the feedback size which resulted in CFB8 always being used, even if the FeedbackSize was set to another value. However, when padding was applied, the padding size would be padded to the feedback size. In the case of AesCng, this would mean it would be encrypted with CFB8 and padded as if it were CFB128. This changes the implementation so that the feedback size is required to be set to 8 for persisted keys. No change is made for ephemeral keys. --- .../BasicSymmetricCipherNCrypt.cs | 2 +- .../Cryptography/CngSymmetricAlgorithmCore.cs | 26 +++++++++++++++-- .../Cryptography/ICngSymmetricAlgorithm.cs | 1 + .../System/Security/Cryptography/AesCng.cs | 29 ++++--------------- .../Security/Cryptography/TripleDESCng.cs | 29 ++++--------------- .../tests/AesCngTests.cs | 2 +- .../tests/SymmetricCngTestHelpers.cs | 22 ++++++++++++-- .../tests/TripleDESCngTests.cs | 2 +- 8 files changed, 58 insertions(+), 55 deletions(-) diff --git a/src/libraries/System.Security.Cryptography.Cng/src/Internal/Cryptography/BasicSymmetricCipherNCrypt.cs b/src/libraries/System.Security.Cryptography.Cng/src/Internal/Cryptography/BasicSymmetricCipherNCrypt.cs index 4efd7174c96c06..1e38c44fef8fbb 100644 --- a/src/libraries/System.Security.Cryptography.Cng/src/Internal/Cryptography/BasicSymmetricCipherNCrypt.cs +++ b/src/libraries/System.Security.Cryptography.Cng/src/Internal/Cryptography/BasicSymmetricCipherNCrypt.cs @@ -21,7 +21,7 @@ internal sealed class BasicSymmetricCipherNCrypt : BasicSymmetricCipher // // The delegate must instantiate a new CngKey, based on a new underlying NCryptKeyHandle, each time is called. // - public BasicSymmetricCipherNCrypt(Func cngKeyFactory, CipherMode cipherMode, int blockSizeInBytes, byte[]? iv, bool encrypting, int feedbackSizeInBytes, int paddingSize) + public BasicSymmetricCipherNCrypt(Func cngKeyFactory, CipherMode cipherMode, int blockSizeInBytes, byte[]? iv, bool encrypting, int paddingSize) : base(iv, blockSizeInBytes, paddingSize) { _encrypting = encrypting; diff --git a/src/libraries/System.Security.Cryptography.Cng/src/Internal/Cryptography/CngSymmetricAlgorithmCore.cs b/src/libraries/System.Security.Cryptography.Cng/src/Internal/Cryptography/CngSymmetricAlgorithmCore.cs index 6da56739bb854b..96b162777724d9 100644 --- a/src/libraries/System.Security.Cryptography.Cng/src/Internal/Cryptography/CngSymmetricAlgorithmCore.cs +++ b/src/libraries/System.Security.Cryptography.Cng/src/Internal/Cryptography/CngSymmetricAlgorithmCore.cs @@ -144,6 +144,8 @@ private UniversalCryptoTransform CreateCryptoTransform(byte[] rgbKey, byte[]? rg if (rgbKey == null) throw new ArgumentNullException(nameof(rgbKey)); + ValidateFeedbackSize(mode, feedbackSizeInBits); + byte[] key = rgbKey.CloneByteArray(); long keySize = key.Length * (long)BitsPerByte; @@ -187,6 +189,9 @@ private UniversalCryptoTransform CreatePersistedCryptoTransformCore(Func { // note: iv is guaranteed to be cloned before this method, so no need to clone it again + ValidateFeedbackSize(mode, feedbackSizeInBits); + Debug.Assert(mode == CipherMode.CFB ? feedbackSizeInBits == 8 : true); + int blockSizeInBytes = _outer.BlockSize.BitSizeToByteSize(); BasicSymmetricCipher cipher = new BasicSymmetricCipherNCrypt( cngKeyFactory, @@ -194,7 +199,6 @@ private UniversalCryptoTransform CreatePersistedCryptoTransformCore(Func blockSizeInBytes, iv, encrypting, - feedbackSizeInBits, _outer.GetPaddingSize(mode, feedbackSizeInBits)); return UniversalCryptoTransform.Create(padding, cipher, encrypting); } @@ -206,11 +210,29 @@ private CngKey ProduceCngKey() return CngKey.Open(_keyName!, _provider!, _optionOptions); } - public bool KeyInPlainText + private bool KeyInPlainText { get { return _keyName == null; } } + private void ValidateFeedbackSize(CipherMode mode, int feedbackSizeInBits) + { + if (mode != CipherMode.CFB) + return; + + if (KeyInPlainText) + { + if (!_outer.IsValidEphemeralFeedbackSize(feedbackSizeInBits)) + { + throw new CryptographicException(string.Format(SR.Cryptography_CipherModeFeedbackNotSupported, feedbackSizeInBits, CipherMode.CFB)); + } + } + else if (feedbackSizeInBits != 8) + { + throw new CryptographicException(string.Format(SR.Cryptography_CipherModeFeedbackNotSupported, feedbackSizeInBits, CipherMode.CFB)); + } + } + private readonly ICngSymmetricAlgorithm _outer; // If using a stored CNG key, these fields provide the CngKey.Open() parameters. If using a plaintext key, _keyName is set to null. diff --git a/src/libraries/System.Security.Cryptography.Cng/src/Internal/Cryptography/ICngSymmetricAlgorithm.cs b/src/libraries/System.Security.Cryptography.Cng/src/Internal/Cryptography/ICngSymmetricAlgorithm.cs index eae2d44dec5c1b..fa46b5cd2ad0b1 100644 --- a/src/libraries/System.Security.Cryptography.Cng/src/Internal/Cryptography/ICngSymmetricAlgorithm.cs +++ b/src/libraries/System.Security.Cryptography.Cng/src/Internal/Cryptography/ICngSymmetricAlgorithm.cs @@ -32,5 +32,6 @@ internal interface ICngSymmetricAlgorithm string GetNCryptAlgorithmIdentifier(); byte[] PreprocessKey(byte[] key); int GetPaddingSize(CipherMode mode, int feedbackSizeBits); + bool IsValidEphemeralFeedbackSize(int feedbackSizeInBits); } } diff --git a/src/libraries/System.Security.Cryptography.Cng/src/System/Security/Cryptography/AesCng.cs b/src/libraries/System.Security.Cryptography.Cng/src/System/Security/Cryptography/AesCng.cs index dbcbd0bd8cea68..297cd511688889 100644 --- a/src/libraries/System.Security.Cryptography.Cng/src/System/Security/Cryptography/AesCng.cs +++ b/src/libraries/System.Security.Cryptography.Cng/src/System/Security/Cryptography/AesCng.cs @@ -178,8 +178,6 @@ protected override bool TryDecryptCfbCore( int feedbackSizeInBits, out int bytesWritten) { - ValidateCFBFeedbackSize(feedbackSizeInBits); - UniversalCryptoTransform transform = _core.CreateCryptoTransform( iv: iv.ToArray(), encrypting: false, @@ -201,8 +199,6 @@ protected override bool TryEncryptCfbCore( int feedbackSizeInBits, out int bytesWritten) { - ValidateCFBFeedbackSize(feedbackSizeInBits); - UniversalCryptoTransform transform = _core.CreateCryptoTransform( iv: iv.ToArray(), encrypting: true, @@ -216,26 +212,6 @@ protected override bool TryEncryptCfbCore( } } - private void ValidateCFBFeedbackSize(int feedback) - { - if (_core.KeyInPlainText) - { - // CFB8 and CFB128 are valid for bcrypt keys. - if (feedback != 8 && feedback != 128) - { - throw new CryptographicException(string.Format(SR.Cryptography_CipherModeFeedbackNotSupported, feedback, CipherMode.CFB)); - } - } - else - { - // only CFB8 is supported for ncrypt keys. - if (feedback != 8) - { - throw new CryptographicException(string.Format(SR.Cryptography_CipherModeFeedbackNotSupported, feedback, CipherMode.CFB)); - } - } - } - protected override void Dispose(bool disposing) { base.Dispose(disposing); @@ -276,6 +252,11 @@ byte[] ICngSymmetricAlgorithm.PreprocessKey(byte[] key) return key; } + bool ICngSymmetricAlgorithm.IsValidEphemeralFeedbackSize(int feedbackSizeInBits) + { + return feedbackSizeInBits == 8 || feedbackSizeInBits == 128; + } + private CngSymmetricAlgorithmCore _core; } } diff --git a/src/libraries/System.Security.Cryptography.Cng/src/System/Security/Cryptography/TripleDESCng.cs b/src/libraries/System.Security.Cryptography.Cng/src/System/Security/Cryptography/TripleDESCng.cs index 09d2b02243530d..710053fa4e5595 100644 --- a/src/libraries/System.Security.Cryptography.Cng/src/System/Security/Cryptography/TripleDESCng.cs +++ b/src/libraries/System.Security.Cryptography.Cng/src/System/Security/Cryptography/TripleDESCng.cs @@ -179,8 +179,6 @@ protected override bool TryDecryptCfbCore( int feedbackSizeInBits, out int bytesWritten) { - ValidateCFBFeedbackSize(feedbackSizeInBits); - UniversalCryptoTransform transform = _core.CreateCryptoTransform( iv: iv.ToArray(), encrypting: false, @@ -202,8 +200,6 @@ protected override bool TryEncryptCfbCore( int feedbackSizeInBits, out int bytesWritten) { - ValidateCFBFeedbackSize(feedbackSizeInBits); - UniversalCryptoTransform transform = _core.CreateCryptoTransform( iv: iv.ToArray(), encrypting: true, @@ -222,26 +218,6 @@ protected override void Dispose(bool disposing) base.Dispose(disposing); } - private void ValidateCFBFeedbackSize(int feedback) - { - if (_core.KeyInPlainText) - { - // CFB8 and CFB164 are valid for bcrypt keys. - if (feedback != 8 && feedback != 64) - { - throw new CryptographicException(string.Format(SR.Cryptography_CipherModeFeedbackNotSupported, feedback, CipherMode.CFB)); - } - } - else - { - // only CFB8 is supported for ncrypt keys. - if (feedback != 8) - { - throw new CryptographicException(string.Format(SR.Cryptography_CipherModeFeedbackNotSupported, feedback, CipherMode.CFB)); - } - } - } - byte[] ICngSymmetricAlgorithm.BaseKey { get { return base.Key; } set { base.Key = value; } } int ICngSymmetricAlgorithm.BaseKeySize { get { return base.KeySize; } set { base.KeySize = value; } } @@ -280,6 +256,11 @@ byte[] ICngSymmetricAlgorithm.PreprocessKey(byte[] key) return key; } + bool ICngSymmetricAlgorithm.IsValidEphemeralFeedbackSize(int feedbackSizeInBits) + { + return feedbackSizeInBits == 8 || feedbackSizeInBits == 64; + } + private CngSymmetricAlgorithmCore _core; } } diff --git a/src/libraries/System.Security.Cryptography.Cng/tests/AesCngTests.cs b/src/libraries/System.Security.Cryptography.Cng/tests/AesCngTests.cs index 47d4f4b5dd6c47..94d5defd5cb11b 100644 --- a/src/libraries/System.Security.Cryptography.Cng/tests/AesCngTests.cs +++ b/src/libraries/System.Security.Cryptography.Cng/tests/AesCngTests.cs @@ -102,7 +102,7 @@ public static void VerifyMachineKey() [ConditionalFact(nameof(SupportsPersistedSymmetricKeys))] public static void VerifyUnsupportedFeedbackSizeForPersistedCfb() { - SymmetricCngTestHelpers.VerifyOneShotCfbPersistedUnsupportedFeedbackSize( + SymmetricCngTestHelpers.VerifyCfbPersistedUnsupportedFeedbackSize( s_cngAlgorithm, keyName => new AesCng(keyName), notSupportedFeedbackSizeInBits: 128); diff --git a/src/libraries/System.Security.Cryptography.Cng/tests/SymmetricCngTestHelpers.cs b/src/libraries/System.Security.Cryptography.Cng/tests/SymmetricCngTestHelpers.cs index 9a3c728efb16d0..1923c6b24273fc 100644 --- a/src/libraries/System.Security.Cryptography.Cng/tests/SymmetricCngTestHelpers.cs +++ b/src/libraries/System.Security.Cryptography.Cng/tests/SymmetricCngTestHelpers.cs @@ -304,12 +304,13 @@ public static void VerifyMachineKey( } } - public static void VerifyOneShotCfbPersistedUnsupportedFeedbackSize( + public static void VerifyCfbPersistedUnsupportedFeedbackSize( CngAlgorithm algorithm, Func persistedFunc, int notSupportedFeedbackSizeInBits) { string keyName = Guid.NewGuid().ToString(); + string feedbackSizeString = notSupportedFeedbackSizeInBits.ToString(); // We try to delete the key later which will also dispose of it, so no need // to put this in a using. @@ -319,9 +320,26 @@ public static void VerifyOneShotCfbPersistedUnsupportedFeedbackSize( { using (SymmetricAlgorithm alg = persistedFunc(keyName)) { + alg.Mode = CipherMode.CFB; + alg.FeedbackSize = notSupportedFeedbackSizeInBits; + alg.Padding = PaddingMode.None; + byte[] destination = new byte[alg.BlockSize / 8]; - Assert.ThrowsAny(() => + CryptographicException ce = Assert.ThrowsAny(() => alg.EncryptCfb(Array.Empty(), destination, PaddingMode.None, notSupportedFeedbackSizeInBits)); + + Assert.Contains(feedbackSizeString, ce.Message); + + ce = Assert.ThrowsAny(() => + alg.DecryptCfb(Array.Empty(), destination, PaddingMode.None, notSupportedFeedbackSizeInBits)); + + Assert.Contains(feedbackSizeString, ce.Message); + + ce = Assert.ThrowsAny(() => alg.CreateDecryptor()); + Assert.Contains(feedbackSizeString, ce.Message); + + ce = Assert.ThrowsAny(() => alg.CreateEncryptor()); + Assert.Contains(feedbackSizeString, ce.Message); } } finally diff --git a/src/libraries/System.Security.Cryptography.Cng/tests/TripleDESCngTests.cs b/src/libraries/System.Security.Cryptography.Cng/tests/TripleDESCngTests.cs index 4a913be0a2bbb1..0e4bc9441a45b0 100644 --- a/src/libraries/System.Security.Cryptography.Cng/tests/TripleDESCngTests.cs +++ b/src/libraries/System.Security.Cryptography.Cng/tests/TripleDESCngTests.cs @@ -108,7 +108,7 @@ public static void VerifyMachineKey() [ConditionalFact(nameof(SupportsPersistedSymmetricKeys))] public static void VerifyUnsupportedFeedbackSizeForPersistedCfb() { - SymmetricCngTestHelpers.VerifyOneShotCfbPersistedUnsupportedFeedbackSize( + SymmetricCngTestHelpers.VerifyCfbPersistedUnsupportedFeedbackSize( s_cngAlgorithm, keyName => new TripleDESCng(keyName), notSupportedFeedbackSizeInBits: 64); From f2603272a70e3eec5f7dcc37f0a2f7a877ea03ad Mon Sep 17 00:00:00 2001 From: Kevin Jones Date: Tue, 13 Jul 2021 21:36:05 -0400 Subject: [PATCH 2/3] Add a comment back --- .../src/Internal/Cryptography/CngSymmetricAlgorithmCore.cs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/libraries/System.Security.Cryptography.Cng/src/Internal/Cryptography/CngSymmetricAlgorithmCore.cs b/src/libraries/System.Security.Cryptography.Cng/src/Internal/Cryptography/CngSymmetricAlgorithmCore.cs index 96b162777724d9..401a8c1a7d645b 100644 --- a/src/libraries/System.Security.Cryptography.Cng/src/Internal/Cryptography/CngSymmetricAlgorithmCore.cs +++ b/src/libraries/System.Security.Cryptography.Cng/src/Internal/Cryptography/CngSymmetricAlgorithmCore.cs @@ -229,6 +229,8 @@ private void ValidateFeedbackSize(CipherMode mode, int feedbackSizeInBits) } else if (feedbackSizeInBits != 8) { + // Persisted CNG keys in CFB mode always use CFB8 when in CFB mode, + // so require the feedback size to be set to 8. throw new CryptographicException(string.Format(SR.Cryptography_CipherModeFeedbackNotSupported, feedbackSizeInBits, CipherMode.CFB)); } } From 88ac83363e783345b506e5f8f85f98ff59d24ccf Mon Sep 17 00:00:00 2001 From: Kevin Jones Date: Tue, 13 Jul 2021 22:11:21 -0400 Subject: [PATCH 3/3] Add tests to validate CFB8 can decrypt block-padded data --- .../AES/AesContractTests.cs | 20 +++++++++++++++++++ .../TripleDES/TripleDESContractTests.cs | 19 ++++++++++++++++++ ...tem.Security.Cryptography.Cng.Tests.csproj | 2 ++ 3 files changed, 41 insertions(+) diff --git a/src/libraries/Common/tests/System/Security/Cryptography/AlgorithmImplementations/AES/AesContractTests.cs b/src/libraries/Common/tests/System/Security/Cryptography/AlgorithmImplementations/AES/AesContractTests.cs index 89e3ebbe61f3a8..878d2c00a0766e 100644 --- a/src/libraries/Common/tests/System/Security/Cryptography/AlgorithmImplementations/AES/AesContractTests.cs +++ b/src/libraries/Common/tests/System/Security/Cryptography/AlgorithmImplementations/AES/AesContractTests.cs @@ -1,6 +1,7 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. +using Test.Cryptography; using Xunit; namespace System.Security.Cryptography.Encryption.Aes.Tests @@ -380,6 +381,25 @@ public static void ValidateOffsetAndCount() } } + [Fact] + public static void Cfb8ModeCanDepadCfb128Padding() + { + using (Aes aes = AesFactory.Create()) + { + // 1, 2, 3, 4, 5 encrypted with CFB8 but padded with block-size padding. + byte[] ciphertext = "68C272ACF16BE005A361DB1C147CA3AD".HexToByteArray(); + aes.Key = "3279CE2E9669A54E038AA62818672150D0B5A13F6757C27F378115501F83B119".HexToByteArray(); + aes.IV = new byte[16]; + aes.Padding = PaddingMode.PKCS7; + aes.Mode = CipherMode.CFB; + aes.FeedbackSize = 8; + + using ICryptoTransform transform = aes.CreateDecryptor(); + byte[] decrypted = transform.TransformFinalBlock(ciphertext, 0, ciphertext.Length); + Assert.Equal(new byte[] { 1, 2, 3, 4, 5 }, decrypted); + } + } + private static void ValidateTransformProperties(Aes aes, ICryptoTransform transform) { Assert.NotNull(transform); diff --git a/src/libraries/Common/tests/System/Security/Cryptography/AlgorithmImplementations/TripleDES/TripleDESContractTests.cs b/src/libraries/Common/tests/System/Security/Cryptography/AlgorithmImplementations/TripleDES/TripleDESContractTests.cs index e12aa7196c8473..725fa30eca791e 100644 --- a/src/libraries/Common/tests/System/Security/Cryptography/AlgorithmImplementations/TripleDES/TripleDESContractTests.cs +++ b/src/libraries/Common/tests/System/Security/Cryptography/AlgorithmImplementations/TripleDES/TripleDESContractTests.cs @@ -89,5 +89,24 @@ public static void ValidCFBFeedbackSizes(int feedbackSize) Assert.NotNull(encryptor); } } + + [Fact] + public static void Cfb8ModeCanDepadCfb64Padding() + { + using (TripleDES tdes = TripleDESFactory.Create()) + { + // 1, 2, 3, 4, 5 encrypted with CFB8 but padded with block-size padding. + byte[] ciphertext = "97F1CE6A6D869A85".HexToByteArray(); + tdes.Key = "3D1ECCEE6C99B029950ED23688AA229AF85177421609F7BF".HexToByteArray(); + tdes.IV = new byte[8]; + tdes.Padding = PaddingMode.PKCS7; + tdes.Mode = CipherMode.CFB; + tdes.FeedbackSize = 8; + + using ICryptoTransform transform = tdes.CreateDecryptor(); + byte[] decrypted = transform.TransformFinalBlock(ciphertext, 0, ciphertext.Length); + Assert.Equal(new byte[] { 1, 2, 3, 4, 5 }, decrypted); + } + } } } diff --git a/src/libraries/System.Security.Cryptography.Cng/tests/System.Security.Cryptography.Cng.Tests.csproj b/src/libraries/System.Security.Cryptography.Cng/tests/System.Security.Cryptography.Cng.Tests.csproj index 8b6195ed23fe37..3ab50a56b049ec 100644 --- a/src/libraries/System.Security.Cryptography.Cng/tests/System.Security.Cryptography.Cng.Tests.csproj +++ b/src/libraries/System.Security.Cryptography.Cng/tests/System.Security.Cryptography.Cng.Tests.csproj @@ -142,6 +142,8 @@ Link="CommonTest\System\Security\Cryptography\AlgorithmImplementations\RSA\SignVerify.netcoreapp.cs" /> +