From 3b00adf52ff7c213d3d2f0d2f4776b034a753619 Mon Sep 17 00:00:00 2001 From: Kevin Jones Date: Tue, 26 Jan 2021 13:44:43 -0500 Subject: [PATCH 1/5] Remove password member for Rfc2898DeriveBytes. The _password field is not needed since CryptDeriveKey was not ported from the Desktop framework. Removing the field also allows removing a defensive copy and clearing it during disposal. --- .../Cryptography/Rfc2898DeriveBytes.cs | 22 ++++++++----------- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/src/libraries/System.Security.Cryptography.Algorithms/src/System/Security/Cryptography/Rfc2898DeriveBytes.cs b/src/libraries/System.Security.Cryptography.Algorithms/src/System/Security/Cryptography/Rfc2898DeriveBytes.cs index b6e615e2b880c..b2e29c995cbb2 100644 --- a/src/libraries/System.Security.Cryptography.Algorithms/src/System/Security/Cryptography/Rfc2898DeriveBytes.cs +++ b/src/libraries/System.Security.Cryptography.Algorithms/src/System/Security/Cryptography/Rfc2898DeriveBytes.cs @@ -16,7 +16,6 @@ public class Rfc2898DeriveBytes : DeriveBytes { private const int MinimumSaltSize = 8; - private readonly byte[] _password; private byte[] _salt; private uint _iterations; private HMAC _hmac; @@ -51,9 +50,8 @@ public Rfc2898DeriveBytes(byte[] password, byte[] salt, int iterations, HashAlgo _salt = new byte[salt.Length + sizeof(uint)]; salt.AsSpan().CopyTo(_salt); _iterations = (uint)iterations; - _password = password.CloneByteArray(); HashAlgorithm = hashAlgorithm; - _hmac = OpenHmac(); + _hmac = OpenHmac(password); // _blockSize is in bytes, HashSize is in bits. _blockSize = _hmac.HashSize >> 3; @@ -98,9 +96,9 @@ public Rfc2898DeriveBytes(string password, int saltSize, int iterations, HashAlg RandomNumberGenerator.Fill(_salt.AsSpan(0, saltSize)); _iterations = (uint)iterations; - _password = Encoding.UTF8.GetBytes(password); + byte[] passwordBytes = Encoding.UTF8.GetBytes(password); HashAlgorithm = hashAlgorithm; - _hmac = OpenHmac(); + _hmac = OpenHmac(passwordBytes); // _blockSize is in bytes, HashSize is in bits. _blockSize = _hmac.HashSize >> 3; @@ -155,8 +153,6 @@ protected override void Dispose(bool disposing) if (_buffer != null) Array.Clear(_buffer, 0, _buffer.Length); - if (_password != null) - Array.Clear(_password, 0, _password.Length); if (_salt != null) Array.Clear(_salt, 0, _salt.Length); } @@ -228,9 +224,9 @@ public override void Reset() } [System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Security", "CA5350", Justification = "HMACSHA1 is needed for compat. (https://github.com/dotnet/runtime/issues/17618)")] - private HMAC OpenHmac() + private HMAC OpenHmac(byte[] password) { - Debug.Assert(_password != null); + Debug.Assert(password != null); HashAlgorithmName hashAlgorithm = HashAlgorithm; @@ -238,13 +234,13 @@ private HMAC OpenHmac() throw new CryptographicException(SR.Cryptography_HashAlgorithmNameNullOrEmpty); if (hashAlgorithm == HashAlgorithmName.SHA1) - return new HMACSHA1(_password); + return new HMACSHA1(password); if (hashAlgorithm == HashAlgorithmName.SHA256) - return new HMACSHA256(_password); + return new HMACSHA256(password); if (hashAlgorithm == HashAlgorithmName.SHA384) - return new HMACSHA384(_password); + return new HMACSHA384(password); if (hashAlgorithm == HashAlgorithmName.SHA512) - return new HMACSHA512(_password); + return new HMACSHA512(password); throw new CryptographicException(SR.Format(SR.Cryptography_UnknownHashAlgorithm, hashAlgorithm.Name)); } From 5649f61de6e19153805c8d5771b5384bda71feeb Mon Sep 17 00:00:00 2001 From: Kevin Jones Date: Tue, 26 Jan 2021 14:12:35 -0500 Subject: [PATCH 2/5] Add a test --- .../tests/Rfc2898Tests.cs | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/src/libraries/System.Security.Cryptography.Algorithms/tests/Rfc2898Tests.cs b/src/libraries/System.Security.Cryptography.Algorithms/tests/Rfc2898Tests.cs index 8090be717e487..e17a4f1aa8cde 100644 --- a/src/libraries/System.Security.Cryptography.Algorithms/tests/Rfc2898Tests.cs +++ b/src/libraries/System.Security.Cryptography.Algorithms/tests/Rfc2898Tests.cs @@ -409,6 +409,26 @@ public static void GetBytes_ExceedCounterLimit() } } + [Fact] + public static void Ctor_PasswordMutatedAfterCreate() + { + byte[] passwordBytes = Encoding.UTF8.GetBytes(TestPassword); + byte[] derived; + + using (Rfc2898DeriveBytes deriveBytes = new Rfc2898DeriveBytes(passwordBytes, s_testSaltB, DefaultIterationCount)) + { + derived = deriveBytes.GetBytes(64); + } + + using (Rfc2898DeriveBytes deriveBytes = new Rfc2898DeriveBytes(passwordBytes, s_testSaltB, DefaultIterationCount)) + { + passwordBytes[0] ^= 0xFF; // Flipping a byte after the object is constructed should not be observed. + + byte[] actual = deriveBytes.GetBytes(64); + Assert.Equal(derived, actual); + } + } + private static void TestKnownValue(string password, byte[] salt, int iterationCount, byte[] expected) { byte[] output; From b24fabb9560ee06e90151a7454ce56229382f170 Mon Sep 17 00:00:00 2001 From: Kevin Jones Date: Tue, 26 Jan 2021 15:22:45 -0500 Subject: [PATCH 3/5] Zero password bytes after use --- .../src/System/Security/Cryptography/Rfc2898DeriveBytes.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/libraries/System.Security.Cryptography.Algorithms/src/System/Security/Cryptography/Rfc2898DeriveBytes.cs b/src/libraries/System.Security.Cryptography.Algorithms/src/System/Security/Cryptography/Rfc2898DeriveBytes.cs index b2e29c995cbb2..75a96b43f7e34 100644 --- a/src/libraries/System.Security.Cryptography.Algorithms/src/System/Security/Cryptography/Rfc2898DeriveBytes.cs +++ b/src/libraries/System.Security.Cryptography.Algorithms/src/System/Security/Cryptography/Rfc2898DeriveBytes.cs @@ -99,6 +99,7 @@ public Rfc2898DeriveBytes(string password, int saltSize, int iterations, HashAlg byte[] passwordBytes = Encoding.UTF8.GetBytes(password); HashAlgorithm = hashAlgorithm; _hmac = OpenHmac(passwordBytes); + CryptographicOperations.ZeroMemory(passwordBytes); // _blockSize is in bytes, HashSize is in bits. _blockSize = _hmac.HashSize >> 3; From 00e8674638293407482cf346db9f78cfe14ddbae Mon Sep 17 00:00:00 2001 From: Kevin Jones Date: Tue, 26 Jan 2021 21:40:06 -0500 Subject: [PATCH 4/5] Clear text password bytes for provided salt and random salt. --- .../Cryptography/Rfc2898DeriveBytes.cs | 49 ++++++++++++------- .../tests/Rfc2898Tests.cs | 12 +++++ 2 files changed, 42 insertions(+), 19 deletions(-) diff --git a/src/libraries/System.Security.Cryptography.Algorithms/src/System/Security/Cryptography/Rfc2898DeriveBytes.cs b/src/libraries/System.Security.Cryptography.Algorithms/src/System/Security/Cryptography/Rfc2898DeriveBytes.cs index 75a96b43f7e34..04abaea65a228 100644 --- a/src/libraries/System.Security.Cryptography.Algorithms/src/System/Security/Cryptography/Rfc2898DeriveBytes.cs +++ b/src/libraries/System.Security.Cryptography.Algorithms/src/System/Security/Cryptography/Rfc2898DeriveBytes.cs @@ -37,25 +37,8 @@ public Rfc2898DeriveBytes(byte[] password, byte[] salt, int iterations) } public Rfc2898DeriveBytes(byte[] password, byte[] salt, int iterations, HashAlgorithmName hashAlgorithm) + :this(password, salt, iterations, hashAlgorithm, clearPassword: false) { - if (salt == null) - throw new ArgumentNullException(nameof(salt)); - if (salt.Length < MinimumSaltSize) - throw new ArgumentException(SR.Cryptography_PasswordDerivedBytes_FewBytesSalt, nameof(salt)); - if (iterations <= 0) - throw new ArgumentOutOfRangeException(nameof(iterations), SR.ArgumentOutOfRange_NeedPosNum); - if (password == null) - throw new NullReferenceException(); // This "should" be ArgumentNullException but for compat, we throw NullReferenceException. - - _salt = new byte[salt.Length + sizeof(uint)]; - salt.AsSpan().CopyTo(_salt); - _iterations = (uint)iterations; - HashAlgorithm = hashAlgorithm; - _hmac = OpenHmac(password); - // _blockSize is in bytes, HashSize is in bits. - _blockSize = _hmac.HashSize >> 3; - - Initialize(); } public Rfc2898DeriveBytes(string password, byte[] salt) @@ -69,7 +52,7 @@ public Rfc2898DeriveBytes(string password, byte[] salt, int iterations) } public Rfc2898DeriveBytes(string password, byte[] salt, int iterations, HashAlgorithmName hashAlgorithm) - : this(Encoding.UTF8.GetBytes(password), salt, iterations, hashAlgorithm) + : this(Encoding.UTF8.GetBytes(password), salt, iterations, hashAlgorithm, clearPassword: true) { } @@ -106,6 +89,34 @@ public Rfc2898DeriveBytes(string password, int saltSize, int iterations, HashAlg Initialize(); } + private Rfc2898DeriveBytes(byte[] password, byte[] salt, int iterations, HashAlgorithmName hashAlgorithm, bool clearPassword) + { + if (salt is null) + throw new ArgumentNullException(nameof(salt)); + if (salt.Length < MinimumSaltSize) + throw new ArgumentException(SR.Cryptography_PasswordDerivedBytes_FewBytesSalt, nameof(salt)); + if (iterations <= 0) + throw new ArgumentOutOfRangeException(nameof(iterations), SR.ArgumentOutOfRange_NeedPosNum); + if (password is null) + throw new NullReferenceException(); // This "should" be ArgumentNullException but for compat, we throw NullReferenceException. + + _salt = new byte[salt.Length + sizeof(uint)]; + salt.AsSpan().CopyTo(_salt); + _iterations = (uint)iterations; + HashAlgorithm = hashAlgorithm; + _hmac = OpenHmac(password); + + if (clearPassword) + { + CryptographicOperations.ZeroMemory(password); + } + + // _blockSize is in bytes, HashSize is in bits. + _blockSize = _hmac.HashSize >> 3; + Initialize(); + } + + public int IterationCount { get diff --git a/src/libraries/System.Security.Cryptography.Algorithms/tests/Rfc2898Tests.cs b/src/libraries/System.Security.Cryptography.Algorithms/tests/Rfc2898Tests.cs index e17a4f1aa8cde..c8fabd02d466a 100644 --- a/src/libraries/System.Security.Cryptography.Algorithms/tests/Rfc2898Tests.cs +++ b/src/libraries/System.Security.Cryptography.Algorithms/tests/Rfc2898Tests.cs @@ -429,6 +429,18 @@ public static void Ctor_PasswordMutatedAfterCreate() } } + [Fact] + public static void Ctor_PasswordBytes_NotCleared() + { + byte[] passwordBytes = Encoding.UTF8.GetBytes(TestPassword); + byte[] passwordBytesOriginal = passwordBytes.AsSpan().ToArray(); + + using (Rfc2898DeriveBytes deriveBytes = new Rfc2898DeriveBytes(passwordBytes, s_testSaltB, DefaultIterationCount)) + { + Assert.Equal(passwordBytesOriginal, passwordBytes); + } + } + private static void TestKnownValue(string password, byte[] salt, int iterationCount, byte[] expected) { byte[] output; From 406e47896cbc22643bf7b76dd991d831f91329da Mon Sep 17 00:00:00 2001 From: Kevin Jones Date: Tue, 26 Jan 2021 21:41:55 -0500 Subject: [PATCH 5/5] Fix extra blank line --- .../src/System/Security/Cryptography/Rfc2898DeriveBytes.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/libraries/System.Security.Cryptography.Algorithms/src/System/Security/Cryptography/Rfc2898DeriveBytes.cs b/src/libraries/System.Security.Cryptography.Algorithms/src/System/Security/Cryptography/Rfc2898DeriveBytes.cs index 04abaea65a228..bf833303ecc90 100644 --- a/src/libraries/System.Security.Cryptography.Algorithms/src/System/Security/Cryptography/Rfc2898DeriveBytes.cs +++ b/src/libraries/System.Security.Cryptography.Algorithms/src/System/Security/Cryptography/Rfc2898DeriveBytes.cs @@ -116,7 +116,6 @@ private Rfc2898DeriveBytes(byte[] password, byte[] salt, int iterations, HashAlg Initialize(); } - public int IterationCount { get