Use System.Security.Cryptography for DSA (#1458)

* Use System.Security.Cryptography for DSA This is the analogue of the RSA change #1373 for DSA. This has a couple of caveats: - The BCL supports only FIPS 186-compliant keys, that is, public (P, Q) lengths of (512 <= P <= 1024, 160) for FIPS 186-1/186-2; and (2048, 256), (3072, 256) for FIPS 186-3/186-4. The latter also specifies (2048, 224) but due to a quirk in the Windows API, the BCL does not support Q values of length 224[^1]. - OpenSSH, based on the SSH spec, only supports (supported) Q values of length 160, but appears to also work in non-FIPS-compliant cases such as in our integration tests with a (2048, 160) host key. That test now fails and I changed that host key to (1024, 160). This basically means that (1024, 160) is the largest DSA key size supported by both SSH.NET and OpenSSH. However, given that OpenSSH deprecated DSA in 2015[^2], and the alternative that I have been considering is just to delete support for DSA in the library, this change seems reasonable to me. I don't think we can justify keeping the current handwritten code around. I think we may still consider dropping DSA from the library, I just had this branch laying around and figured I'd finish it off. [^1]: https://github.com/dotnet/runtime/blob/fadd8313653f71abd0068c8bf914be88edb2c8d3/src/libraries/Common/src/System/Security/Cryptography/DSACng.ImportExport.cs#L259-L265 [^2]: https://www.openssh.com/txt/release-7.0 * Appease mono * test experiment * Revert "Appease mono" This reverts commit 881eefe.
sshnet · Aug 11, 2024 · fe65570 · fe65570
1 parent c7a0ca9
commit fe65570
Show file tree

Hide file tree

Showing 11 changed files with 372 additions and 196 deletions.
diff --git a/src/Renci.SshNet/Security/Cryptography/DsaDigitalSignature.cs b/src/Renci.SshNet/Security/Cryptography/DsaDigitalSignature.cs
@@ -1,4 +1,5 @@
-using System;
+#nullable enable
+using System;
 using System.Security.Cryptography;
 
 using Renci.SshNet.Common;
@@ -11,8 +12,6 @@ namespace Renci.SshNet.Security.Cryptography
     public class DsaDigitalSignature : DigitalSignature, IDisposable
     {
         private readonly DsaKey _key;
-        private HashAlgorithm _hash;
-        private bool _isDisposed;
 
         /// <summary>
         /// Initializes a new instance of the <see cref="DsaDigitalSignature" /> class.
@@ -27,71 +26,23 @@ public DsaDigitalSignature(DsaKey key)
             }
 
             _key = key;
-
-            _hash = SHA1.Create();
         }
 
-        /// <summary>
-        /// Verifies the signature.
-        /// </summary>
-        /// <param name="input">The input.</param>
-        /// <param name="signature">The signature.</param>
-        /// <returns>
-        /// <see langword="true"/> if signature was successfully verified; otherwise <see langword="false"/>.
-        /// </returns>
-        /// <exception cref="InvalidOperationException">Invalid signature.</exception>
+        /// <inheritdoc/>
         public override bool Verify(byte[] input, byte[] signature)
         {
-            var hashInput = _hash.ComputeHash(input);
-
-            var hm = new BigInteger(hashInput.Reverse().Concat(new byte[] { 0 }));
-
-            if (signature.Length != 40)
-            {
-                throw new InvalidOperationException("Invalid signature.");
-            }
-
-            // Extract r and s numbers from the signature
-            var rBytes = new byte[21];
-            var sBytes = new byte[21];
-
-            for (int i = 0, j = 20; i < 20; i++, j--)
-            {
-                rBytes[i] = signature[j - 1];
-                sBytes[i] = signature[j + 20 - 1];
-            }
-
-            var r = new BigInteger(rBytes);
-            var s = new BigInteger(sBytes);
-
-            // Reject the signature if 0 < r < q or 0 < s < q is not satisfied.
-            if (r <= 0 || r >= _key.Q)
+#if NETSTANDARD2_1_OR_GREATER || NET
+            return _key.DSA.VerifyData(input, signature, HashAlgorithmName.SHA1);
+#else
+            // VerifyData does not exist on netstandard2.0.
+            // It does exist on net462, but in order to keep the path tested,
+            // use it on netfx as well.
+            using (var sha1 = SHA1.Create())
             {
-                return false;
+                var hash = sha1.ComputeHash(input);
+                return _key.DSA.VerifySignature(hash, signature);
             }
-
-            if (s <= 0 || s >= _key.Q)
-            {
-                return false;
-            }
-
-            // Calculate w = s−1 mod q
-            var w = BigInteger.ModInverse(s, _key.Q);
-
-            // Calculate u1 = H(m)·w mod q
-            var u1 = (hm * w) % _key.Q;
-
-            // Calculate u2 = r * w mod q
-            var u2 = (r * w) % _key.Q;
-
-            u1 = BigInteger.ModPow(_key.G, u1, _key.P);
-            u2 = BigInteger.ModPow(_key.Y, u2, _key.P);
-
-            // Calculate v = ((g pow u1 * y pow u2) mod p) mod q
-            var v = ((u1 * u2) % _key.P) % _key.Q;
-
-            // The signature is valid if v = r
-            return v == r;
+#endif
         }
 
         /// <summary>
@@ -104,60 +55,18 @@ public override bool Verify(byte[] input, byte[] signature)
         /// <exception cref="SshException">Invalid DSA key.</exception>
         public override byte[] Sign(byte[] input)
         {
-            var hashInput = _hash.ComputeHash(input);
-
-            var m = new BigInteger(hashInput.Reverse().Concat(new byte[] { 0 }));
-
-            BigInteger s;
-            BigInteger r;
-
-            do
+#if NETSTANDARD2_1_OR_GREATER || NET
+            return _key.DSA.SignData(input, HashAlgorithmName.SHA1);
+#else
+            // SignData does not exist on netstandard2.0.
+            // It does exist on net462, but in order to keep the path tested,
+            // use it on netfx as well.
+            using (var sha1 = SHA1.Create())
             {
-                var k = BigInteger.Zero;
-
-                do
-                {
-                    // Generate a random per-message value k where 0 < k < q
-                    var bitLength = _key.Q.BitLength;
-
-                    if (_key.Q < BigInteger.Zero)
-                    {
-                        throw new SshException("Invalid DSA key.");
-                    }
-
-                    while (k <= 0 || k >= _key.Q)
-                    {
-                        k = BigInteger.Random(bitLength);
-                    }
-
-                    // Calculate r = ((g pow k) mod p) mod q
-                    r = BigInteger.ModPow(_key.G, k, _key.P) % _key.Q;
-
-                    // In the unlikely case that r = 0, start again with a different random k
-                }
-                while (r.IsZero);
-
-                // Calculate s = ((k pow −1)(H(m) + x*r)) mod q
-                k = BigInteger.ModInverse(k, _key.Q) * (m + (_key.X * r));
-
-                s = k % _key.Q;
-
-                // In the unlikely case that s = 0, start again with a different random k
+                var hash = sha1.ComputeHash(input);
+                return _key.DSA.CreateSignature(hash);
             }
-            while (s.IsZero);
-
-            // The signature is (r, s)
-            var signature = new byte[40];
-
-            // issue #1918: pad part with zero's on the left if length is less than 20
-            var rBytes = r.ToByteArray().Reverse().TrimLeadingZeros();
-            Array.Copy(rBytes, 0, signature, 20 - rBytes.Length, rBytes.Length);
-
-            // issue #1918: pad part with zero's on the left if length is less than 20
-            var sBytes = s.ToByteArray().Reverse().TrimLeadingZeros();
-            Array.Copy(sBytes, 0, signature, 40 - sBytes.Length, sBytes.Length);
-
-            return signature;
+#endif
         }
 
         /// <summary>
@@ -175,30 +84,6 @@ public void Dispose()
         /// <param name="disposing"><see langword="true"/> to release both managed and unmanaged resources; <see langword="false"/> to release only unmanaged resources.</param>
         protected virtual void Dispose(bool disposing)
         {
-            if (_isDisposed)
-            {
-                return;
-            }
-
-            if (disposing)
-            {
-                var hash = _hash;
-                if (hash != null)
-                {
-                    hash.Dispose();
-                    _hash = null;
-                }
-
-                _isDisposed = true;
-            }
-        }
-
-        /// <summary>
-        /// Finalizes an instance of the <see cref="DsaDigitalSignature"/> class.
-        /// </summary>
-        ~DsaDigitalSignature()
-        {
-            Dispose(disposing: false);
         }
     }
 }
diff --git a/src/Renci.SshNet/Security/Cryptography/DsaKey.cs b/src/Renci.SshNet/Security/Cryptography/DsaKey.cs
@@ -1,4 +1,6 @@
-using System;
+#nullable enable
+using System;
+using System.Security.Cryptography;
 
 using Renci.SshNet.Common;
 using Renci.SshNet.Security.Cryptography;
@@ -10,8 +12,9 @@ namespace Renci.SshNet.Security
     /// </summary>
     public class DsaKey : Key, IDisposable
     {
-        private DsaDigitalSignature _digitalSignature;
-        private bool _isDisposed;
+        private DsaDigitalSignature? _digitalSignature;
+
+        internal DSA DSA { get; }
 
         /// <summary>
         /// Gets the P.
@@ -39,10 +42,10 @@ public class DsaKey : Key, IDisposable
         public BigInteger X { get; }
 
         /// <summary>
-        /// Gets the length of the key.
+        /// Gets the length of the key in bits.
         /// </summary>
         /// <value>
-        /// The length of the key.
+        /// The bit-length of the key.
         /// </value>
         public override int KeyLength
         {
@@ -104,6 +107,8 @@ public DsaKey(SshKeyData publicKeyData)
             Q = publicKeyData.Keys[1];
             G = publicKeyData.Keys[2];
             Y = publicKeyData.Keys[3];
+
+            DSA = LoadDSA();
         }
 
         /// <summary>
@@ -130,6 +135,8 @@ public DsaKey(byte[] privateKeyData)
             {
                 throw new InvalidOperationException("Invalid private key (expected EOF).");
             }
+
+            DSA = LoadDSA();
         }
 
         /// <summary>
@@ -147,6 +154,54 @@ public DsaKey(BigInteger p, BigInteger q, BigInteger g, BigInteger y, BigInteger
             G = g;
             Y = y;
             X = x;
+
+            DSA = LoadDSA();
+        }
+
+#pragma warning disable CA1859 // Use concrete types when possible for improved performance
+#pragma warning disable CA5384 // Do Not Use Digital Signature Algorithm (DSA)
+        private DSA LoadDSA()
+        {
+#if NETFRAMEWORK
+            // On .NET Framework we use the concrete CNG type which is FIPS-186-3
+            // compatible. The CryptoServiceProvider type returned by DSA.Create()
+            // is limited to FIPS-186-1 (max 1024 bit key).
+            var dsa = new DSACng();
+#else
+            var dsa = DSA.Create();
+#endif
+            dsa.ImportParameters(GetDSAParameters());
+
+            return dsa;
+        }
+#pragma warning restore CA5384 // Do Not Use Digital Signature Algorithm (DSA)
+#pragma warning restore CA1859 // Use concrete types when possible for improved performance
+
+        internal DSAParameters GetDSAParameters()
+        {
+            // P, G, Y, Q are required.
+            // P, G, Y must have the same length.
+            // If X is present, it must have the same length as Q.
+
+            // See https://github.com/dotnet/runtime/blob/fadd8313653f71abd0068c8bf914be88edb2c8d3/src/libraries/Common/src/System/Security/Cryptography/DSACng.ImportExport.cs#L23
+            // and https://github.com/dotnet/runtime/blob/fadd8313653f71abd0068c8bf914be88edb2c8d3/src/libraries/Common/src/System/Security/Cryptography/DSAKeyFormatHelper.cs#L18
+            // (and similar code in RsaKey.cs)
+
+            var ret = new DSAParameters
+            {
+                P = P.ToByteArray(isUnsigned: true, isBigEndian: true),
+                Q = Q.ToByteArray(isUnsigned: true, isBigEndian: true),
+            };
+
+            ret.G = G.ExportKeyParameter(ret.P.Length);
+            ret.Y = Y.ExportKeyParameter(ret.P.Length);
+
+            if (!X.IsZero)
+            {
+                ret.X = X.ExportKeyParameter(ret.Q.Length);
+            }
+
+            return ret;
         }
 
         /// <summary>
@@ -164,30 +219,11 @@ public void Dispose()
         /// <param name="disposing"><see langword="true"/> to release both managed and unmanaged resources; <see langword="false"/> to release only unmanaged resources.</param>
         protected virtual void Dispose(bool disposing)
         {
-            if (_isDisposed)
-            {
-                return;
-            }
-
             if (disposing)
             {
-                var digitalSignature = _digitalSignature;
-                if (digitalSignature != null)
-                {
-                    digitalSignature.Dispose();
-                    _digitalSignature = null;
-                }
-
-                _isDisposed = true;
+                _digitalSignature?.Dispose();
+                DSA.Dispose();
             }
         }
-
-        /// <summary>
-        /// Finalizes an instance of the <see cref="DsaKey"/> class.
-        /// </summary>
-        ~DsaKey()
-        {
-            Dispose(disposing: false);
-        }
     }
 }
diff --git a/src/Renci.SshNet/Security/Cryptography/RsaKey.cs b/src/Renci.SshNet/Security/Cryptography/RsaKey.cs
@@ -92,10 +92,10 @@ public override string ToString()
         public BigInteger InverseQ { get; }
 
         /// <summary>
-        /// Gets the length of the key.
+        /// Gets the length of the key in bits.
         /// </summary>
         /// <value>
-        /// The length of the key.
+        /// The bit-length of the key.
         /// </value>
         public override int KeyLength
         {

diff --git a/test/Data/Key.SSH2.DSA.pub b/test/Data/Key.SSH2.DSA.pub
@@ -1 +1 @@
-ssh-dss AAAAB3NzaC1kc3MAAACBAI8gyHFchkVhkPiwkhkjFDqN6w2nFWTqVy9sLjFs38oEWLMpAw9+c132erUptAhNQ6JZUAVZGllv/3V5hksSDyChe9WY5IfsOlh6X0dcZCwBKysEzQlPyMFqAtbc9uv7oUWNzBfvEbtV6WN/VmcmXf7dyo3EBVXbBFdPl1NKC7W9AAAAFQDY1+bTt7s2iNmYoBE4C9hdWRCyeQAAAIAEtj09ugx/Tdl6bo7X6mX17hcgVgIxcYj5VNONg2k6IHmRFriLviYaS68mIB4SG3jmvvxbXAGqR1bWBUrv90n0wpxxcuuNoCFylJQyuqUkzSsUHb0WMcncZ/tBQt+NJnRB1Zp9sw8n20ocpg3WVPdaXTtc4pk83NYB6ywG6UFPvgAAAIAX+De5dwo33LMl9W8IvA4dY8Q1wshdycAGJzhy+qYF9dCcwD1Pg+4EbPjYPmzJopsVrK97v9QhxyYcXMr/iHhngGwd9nYNzzSKx665vkSjzyeJWpeQ+fvNV3CLItP01ypbUreM+s+Vz1wor5joLKcDS4X0oQ0RIVZNEHnekuLuFg== 
+ssh-dss AAAAB3NzaC1kc3MAAACBAOCSGuOxxY/DQBowG7fkS30AJmwN4fDPXToyTaAaxwpOWnGJXFhgP4Il+GSKlpaxnUWkajKpMc1b1hhawPWN4sxoT8QRb6SAW30ErnT/pUtsxqoFlkFla4xvWSgNAuHAVkUBrgPsJ4sHehSbNFn3I6q8Rgle2jyHDHTOqPj2KEXhAAAAFQC740YkVJdVpTJRTxd9Myi0Nx3t4wAAAIArvP7AGh5jY+zxeQRb52zxcUamRBkVYL/ferdJNi9hoM8ZaO4++Xgs8wMbpmoEch9DsXdtufjqXWpk7ywlPjcdhhsb3MxJAeEeFtTRsu2/IUTKqKPHIOgoiPzs8q69AxWhV10aDDUdYWLkqPV/tMGl6S/jC7vTJLmhZum4BUv8MQAAAIEAt19oHPIPyv/8mbMaYpu8I6kvj1/97Ejw0neN7Cd9cGZLqIPBwTyUHEQvKSAm4BvNP0Le3JDn3RFayhRmf+8RrAmS4d1MjrCOs6fmeyLnk2kTpRPFZ2x0H1udIRAhzRehyfj6OsAHn7Jclr+mqDhoq7nIfC3tSgWxFH5g948+7ks= imported-openssh-key
diff --git a/test/Data/Key.SSH2.DSA.txt b/test/Data/Key.SSH2.DSA.txt
@@ -1,12 +1,13 @@
 ---- BEGIN SSH2 ENCRYPTED PRIVATE KEY ----
+Comment: "imported-openssh-key"
 P2/56wAAAgIAAAAmZGwtbW9kcHtzaWdue2RzYS1uaXN0LXNoYTF9LGRoe3BsYWlufX0AAA
-AEbm9uZQAAAcQAAAHAAAAAAAAABACPIMhxXIZFYZD4sJIZIxQ6jesNpxVk6lcvbC4xbN/K
-BFizKQMPfnNd9nq1KbQITUOiWVAFWRpZb/91eYZLEg8goXvVmOSH7DpYel9HXGQsASsrBM
-0JT8jBagLW3Pbr+6FFjcwX7xG7Veljf1ZnJl3+3cqNxAVV2wRXT5dTSgu1vQAAA/sEtj09
-ugx/Tdl6bo7X6mX17hcgVgIxcYj5VNONg2k6IHmRFriLviYaS68mIB4SG3jmvvxbXAGqR1
-bWBUrv90n0wpxxcuuNoCFylJQyuqUkzSsUHb0WMcncZ/tBQt+NJnRB1Zp9sw8n20ocpg3W
-VPdaXTtc4pk83NYB6ywG6UFPvgAAAKDY1+bTt7s2iNmYoBE4C9hdWRCyeQAAA/0X+De5dw
-o33LMl9W8IvA4dY8Q1wshdycAGJzhy+qYF9dCcwD1Pg+4EbPjYPmzJopsVrK97v9QhxyYc
-XMr/iHhngGwd9nYNzzSKx665vkSjzyeJWpeQ+fvNV3CLItP01ypbUreM+s+Vz1wor5joLK
-cDS4X0oQ0RIVZNEHnekuLuFgAAAJ4j+lpXSvEZexhbiACKenUniZ/Qkg==
+AEbm9uZQAAAcQAAAHAAAAAAAAABADgkhrjscWPw0AaMBu35Et9ACZsDeHwz106Mk2gGscK
+TlpxiVxYYD+CJfhkipaWsZ1FpGoyqTHNW9YYWsD1jeLMaE/EEW+kgFt9BK50/6VLbMaqBZ
+ZBZWuMb1koDQLhwFZFAa4D7CeLB3oUmzRZ9yOqvEYJXto8hwx0zqj49ihF4QAAA/4rvP7A
+Gh5jY+zxeQRb52zxcUamRBkVYL/ferdJNi9hoM8ZaO4++Xgs8wMbpmoEch9DsXdtufjqXW
+pk7ywlPjcdhhsb3MxJAeEeFtTRsu2/IUTKqKPHIOgoiPzs8q69AxWhV10aDDUdYWLkqPV/
+tMGl6S/jC7vTJLmhZum4BUv8MQAAAKC740YkVJdVpTJRTxd9Myi0Nx3t4wAABAC3X2gc8g
+/K//yZsxpim7wjqS+PX/3sSPDSd43sJ31wZkuog8HBPJQcRC8pICbgG80/Qt7ckOfdEVrK
+FGZ/7xGsCZLh3UyOsI6zp+Z7IueTaROlE8VnbHQfW50hECHNF6HJ+Po6wAefslyWv6aoOG
+iruch8Le1KBbEUfmD3jz7uSwAAAJ9mUEtdk3zSMZJ1umUnNSo5zC+UxA==
 ---- END SSH2 ENCRYPTED PRIVATE KEY ----
diff --git a/test/Renci.SshNet.IntegrationTests/HostKeyAlgorithmTests.cs b/test/Renci.SshNet.IntegrationTests/HostKeyAlgorithmTests.cs
@@ -26,7 +26,7 @@ public void TearDown()
         [TestMethod]
         public void SshDss()
         {
-            DoTest(HostKeyAlgorithm.SshDss, HostKeyFile.Dsa, 2048);
+            DoTest(HostKeyAlgorithm.SshDss, HostKeyFile.Dsa, 1024);
         }
 
         [TestMethod]

diff --git a/test/Renci.SshNet.IntegrationTests/HostKeyFile.cs b/test/Renci.SshNet.IntegrationTests/HostKeyFile.cs
@@ -3,7 +3,7 @@
     public sealed class HostKeyFile
     {
         public static readonly HostKeyFile Rsa = new HostKeyFile("ssh-rsa", "/etc/ssh/ssh_host_rsa_key", new byte[] { 0x3d, 0x90, 0xd8, 0x0d, 0xd5, 0xe0, 0xb6, 0x13, 0x42, 0x7c, 0x78, 0x1e, 0x19, 0xa3, 0x99, 0x2b });
-        public static readonly HostKeyFile Dsa = new HostKeyFile("ssh-dsa", "/etc/ssh/ssh_host_dsa_key", new byte[] { 0x50, 0xe0, 0xd5, 0x11, 0xf7, 0xed, 0x54, 0x75, 0x0d, 0x03, 0xc6, 0x52, 0x9b, 0x3b, 0x3c, 0x9f });
+        public static readonly HostKeyFile Dsa = new HostKeyFile("ssh-dsa", "/etc/ssh/ssh_host_dsa_key", new byte[] { 0xcc, 0xb4, 0x4c, 0xe1, 0xba, 0x6d, 0x15, 0x79, 0xec, 0xe1, 0x31, 0x9f, 0xc0, 0x4a, 0x07, 0x9d });
         public static readonly HostKeyFile Ed25519 = new HostKeyFile("ssh-ed25519", "/etc/ssh/ssh_host_ed25519_key", new byte[] { 0xb3, 0xb9, 0xd0, 0x1b, 0x73, 0xc4, 0x60, 0xb4, 0xce, 0xed, 0x06, 0xf8, 0x58, 0x49, 0xa3, 0xda });
         public const string Ecdsa = "/etc/ssh/ssh_host_ecdsa_key";