Add padding when encrypt and remove padding when decrypt (#1545)

* Tighten private key checking to reveal padding issue * `Encrypt` should take into account padding for length of `inputBuffer` passed to `EncryptBlock` if padding is specified, no matter input is divisible or not. * `Decrypt` should take into account unpadding for the final output if padding is specified. * `Decrypt` should take into account *manual* padding for length of `inputBuffer` passed to `DecryptBlock` and unpadding for the final output if padding is not specified and mode is CFB or OFB. * `Encrypt` should take into account *manual* padding for length of `inputBuffer` passed to `EncryptBlock` and unpadding for the final output if padding is not specified and mode is CFB or OFB. * Rectify DES cipher tests. There's no padding in the data. * Borrow `PadCount` method from BouncyCastle * Manually pad input in CTR mode as well. Update AesCipherTest. Co-Authored-By: Rob Hague <[email protected]> * Manually pad/unpad for Aes CFB/OFB mode * Update test/Renci.SshNet.Tests/Classes/Security/Cryptography/Ciphers/AesCipherTest.Gen.cs.txt Co-authored-by: Rob Hague <[email protected]> * Re-generate AES cipher tests --------- Co-authored-by: Rob Hague <[email protected]> Co-authored-by: Rob Hague <[email protected]>
sshnet · Dec 22, 2024 · 29997ae · 29997ae
1 parent 42d75bc
commit 29997ae
Show file tree

Hide file tree

Showing 13 changed files with 1,196 additions and 550 deletions.
diff --git a/src/Renci.SshNet/PrivateKeyFile.PKCS1.cs b/src/Renci.SshNet/PrivateKeyFile.PKCS1.cs
@@ -54,7 +54,7 @@ public Key Parse()
                             cipher = new CipherInfo(192, (key, iv) => new TripleDesCipher(key, new CbcCipherMode(iv), new PKCS7Padding()));
                             break;
                         case "DES-EDE3-CFB":
-                            cipher = new CipherInfo(192, (key, iv) => new TripleDesCipher(key, new CfbCipherMode(iv), new PKCS7Padding()));
+                            cipher = new CipherInfo(192, (key, iv) => new TripleDesCipher(key, new CfbCipherMode(iv), padding: null));
                             break;
                         case "DES-CBC":
                             cipher = new CipherInfo(64, (key, iv) => new DesCipher(key, new CbcCipherMode(iv), new PKCS7Padding()));

diff --git a/src/Renci.SshNet/PrivateKeyFile.SSHCOM.cs b/src/Renci.SshNet/PrivateKeyFile.SSHCOM.cs
@@ -8,7 +8,6 @@
 using Renci.SshNet.Security;
 using Renci.SshNet.Security.Cryptography.Ciphers;
 using Renci.SshNet.Security.Cryptography.Ciphers.Modes;
-using Renci.SshNet.Security.Cryptography.Ciphers.Paddings;
 
 namespace Renci.SshNet
 {
@@ -52,7 +51,7 @@ public Key Parse()
                     }
 
                     var key = GetCipherKey(_passPhrase, 192 / 8);
-                    var ssh2Сipher = new TripleDesCipher(key, new CbcCipherMode(new byte[8]), new PKCS7Padding());
+                    var ssh2Сipher = new TripleDesCipher(key, new CbcCipherMode(new byte[8]), padding: null);
                     keyData = ssh2Сipher.Decrypt(reader.ReadBytes(blobSize));
                 }
                 else

diff --git a/src/Renci.SshNet/Security/Cryptography/BlockCipher.cs b/src/Renci.SshNet/Security/Cryptography/BlockCipher.cs
@@ -1,6 +1,8 @@
 using System;
 
+using Renci.SshNet.Common;
 using Renci.SshNet.Security.Cryptography.Ciphers;
+using Renci.SshNet.Security.Cryptography.Ciphers.Modes;
 
 namespace Renci.SshNet.Security.Cryptography
 {
@@ -75,18 +77,29 @@ protected BlockCipher(byte[] key, byte blockSize, CipherMode mode, CipherPadding
         /// </returns>
         public override byte[] Encrypt(byte[] input, int offset, int length)
         {
-            if (length % _blockSize > 0)
+            var paddingLength = 0;
+            if (_padding is not null)
             {
-                if (_padding is null)
-                {
-                    throw new ArgumentException(string.Format("The data block size is incorrect for {0}.", GetType().Name), "data");
-                }
-
-                var paddingLength = _blockSize - (length % _blockSize);
+                paddingLength = _blockSize - (length % _blockSize);
                 input = _padding.Pad(input, offset, length, paddingLength);
                 length += paddingLength;
                 offset = 0;
             }
+            else if (length % _blockSize > 0)
+            {
+                if (_mode is CfbCipherMode or OfbCipherMode or CtrCipherMode)
+                {
+                    paddingLength = _blockSize - (length % _blockSize);
+                    input = input.Take(offset, length);
+                    length += paddingLength;
+                    Array.Resize(ref input, length);
+                    offset = 0;
+                }
+                else
+                {
+                    throw new ArgumentException(string.Format("The data block size is incorrect for {0}.", GetType().Name), "data");
+                }
+            }
 
             var output = new byte[length];
             var writtenBytes = 0;
@@ -108,6 +121,11 @@ public override byte[] Encrypt(byte[] input, int offset, int length)
                 throw new InvalidOperationException("Encryption error.");
             }
 
+            if (_padding is null && paddingLength > 0)
+            {
+                Array.Resize(ref output, output.Length - paddingLength);
+            }
+
             return output;
         }
 
@@ -122,16 +140,21 @@ public override byte[] Encrypt(byte[] input, int offset, int length)
         /// </returns>
         public override byte[] Decrypt(byte[] input, int offset, int length)
         {
+            var paddingLength = 0;
             if (length % _blockSize > 0)
             {
-                if (_padding is null)
+                if (_padding is null && _mode is CfbCipherMode or OfbCipherMode or CtrCipherMode)
+                {
+                    paddingLength = _blockSize - (length % _blockSize);
+                    input = input.Take(offset, length);
+                    length += paddingLength;
+                    Array.Resize(ref input, length);
+                    offset = 0;
+                }
+                else
                 {
                     throw new ArgumentException(string.Format("The data block size is incorrect for {0}.", GetType().Name), "data");
                 }
-
-                input = _padding.Pad(_blockSize, input, offset, length);
-                offset = 0;
-                length = input.Length;
             }
 
             var output = new byte[length];
@@ -154,6 +177,16 @@ public override byte[] Decrypt(byte[] input, int offset, int length)
                 throw new InvalidOperationException("Encryption error.");
             }
 
+            if (_padding is not null)
+            {
+                paddingLength = _padding.PadCount(output);
+            }
+
+            if (paddingLength > 0)
+            {
+                Array.Resize(ref output, output.Length - paddingLength);
+            }
+
             return output;
         }
 

diff --git a/src/Renci.SshNet/Security/Cryptography/Ciphers/AesCipher.BclImpl.cs b/src/Renci.SshNet/Security/Cryptography/Ciphers/AesCipher.BclImpl.cs
@@ -47,12 +47,31 @@ public override byte[] Encrypt(byte[] input, int offset, int length)
                     return _encryptor.TransformFinalBlock(input, offset, length);
                 }
 
+                var paddingLength = 0;
+                if (length % BlockSize > 0)
+                {
+                    if (_aes.Mode is System.Security.Cryptography.CipherMode.CFB or System.Security.Cryptography.CipherMode.OFB)
+                    {
+                        paddingLength = BlockSize - (length % BlockSize);
+                        input = input.Take(offset, length);
+                        length += paddingLength;
+                        Array.Resize(ref input, length);
+                        offset = 0;
+                    }
+                }
+
                 // Otherwise, (the most important case) assume this instance is
                 // used for one direction of an SSH connection, whereby the
                 // encrypted data in all packets are considered a single data
                 // stream i.e. we do not want to reset the state between calls to Encrypt.
                 var output = new byte[length];
                 _ = _encryptor.TransformBlock(input, offset, length, output, 0);
+
+                if (paddingLength > 0)
+                {
+                    Array.Resize(ref output, output.Length - paddingLength);
+                }
+
                 return output;
             }
 
@@ -65,12 +84,32 @@ public override byte[] Decrypt(byte[] input, int offset, int length)
                     return _decryptor.TransformFinalBlock(input, offset, length);
                 }
 
+                var paddingLength = 0;
+                if (length % BlockSize > 0)
+                {
+                    if (_aes.Mode is System.Security.Cryptography.CipherMode.CFB or System.Security.Cryptography.CipherMode.OFB)
+                    {
+                        paddingLength = BlockSize - (length % BlockSize);
+                        var newInput = new byte[input.Length + paddingLength];
+                        Buffer.BlockCopy(input, offset, newInput, 0, length);
+                        input = newInput;
+                        length = input.Length;
+                        offset = 0;
+                    }
+                }
+
                 // Otherwise, (the most important case) assume this instance is
                 // used for one direction of an SSH connection, whereby the
                 // encrypted data in all packets are considered a single data
                 // stream i.e. we do not want to reset the state between calls to Decrypt.
                 var output = new byte[length];
                 _ = _decryptor.TransformBlock(input, offset, length, output, 0);
+
+                if (paddingLength > 0)
+                {
+                    Array.Resize(ref output, output.Length - paddingLength);
+                }
+
                 return output;
             }
 

diff --git a/src/Renci.SshNet/Security/Cryptography/Ciphers/CipherPadding.cs b/src/Renci.SshNet/Security/Cryptography/Ciphers/CipherPadding.cs
@@ -54,5 +54,12 @@ public byte[] Pad(byte[] input, int paddinglength)
         /// The padded data array.
         /// </returns>
         public abstract byte[] Pad(byte[] input, int offset, int length, int paddinglength);
+
+        /// <summary>
+        /// Gets the padd count from the specified input.
+        /// </summary>
+        /// <param name="input">The input.</param>
+        /// <returns>The padd count.</returns>
+        public abstract int PadCount(byte[] input);
     }
 }
diff --git a/src/Renci.SshNet/Security/Cryptography/Ciphers/Paddings/PKCS7Padding.cs b/src/Renci.SshNet/Security/Cryptography/Ciphers/Paddings/PKCS7Padding.cs
@@ -1,5 +1,7 @@
 using System;
 
+using Renci.SshNet.Common;
+
 namespace Renci.SshNet.Security.Cryptography.Ciphers.Paddings
 {
     /// <summary>
@@ -45,5 +47,26 @@ public override byte[] Pad(byte[] input, int offset, int length, int paddingleng
 
             return output;
         }
+
+        /// <inheritdoc/>
+        public override int PadCount(byte[] input)
+        {
+            var padValue = input[input.Length - 1];
+            int count = padValue;
+            var position = input.Length - count;
+
+            var failed = (position | (count - 1)) >> 31;
+            for (var i = 0; i < input.Length; ++i)
+            {
+                failed |= (input[i] ^ padValue) & ~((i - position) >> 31);
+            }
+
+            if (failed != 0)
+            {
+                throw new SshException("pad block corrupted");
+            }
+
+            return count;
+        }
     }
 }
diff --git a/src/Renci.SshNet/Security/Cryptography/DsaKey.cs b/src/Renci.SshNet/Security/Cryptography/DsaKey.cs
@@ -113,16 +113,19 @@ public DsaKey(byte[] privateKeyData)
         {
             ThrowHelper.ThrowIfNull(privateKeyData);
 
-            var der = new AsnReader(privateKeyData, AsnEncodingRules.DER).ReadSequence();
-            _ = der.ReadInteger(); // skip version
+            var keyReader = new AsnReader(privateKeyData, AsnEncodingRules.DER);
+            var sequenceReader = keyReader.ReadSequence();
+            keyReader.ThrowIfNotEmpty();
 
-            P = der.ReadInteger();
-            Q = der.ReadInteger();
-            G = der.ReadInteger();
-            Y = der.ReadInteger();
-            X = der.ReadInteger();
+            _ = sequenceReader.ReadInteger(); // skip version
 
-            der.ThrowIfNotEmpty();
+            P = sequenceReader.ReadInteger();
+            Q = sequenceReader.ReadInteger();
+            G = sequenceReader.ReadInteger();
+            Y = sequenceReader.ReadInteger();
+            X = sequenceReader.ReadInteger();
+
+            sequenceReader.ThrowIfNotEmpty();
 
             DSA = LoadDSA();
         }

diff --git a/src/Renci.SshNet/Security/Cryptography/EcdsaKey.cs b/src/Renci.SshNet/Security/Cryptography/EcdsaKey.cs
@@ -218,18 +218,21 @@ public EcdsaKey(string curve, byte[] publickey, byte[] privatekey)
         /// <param name="data">DER encoded private key data.</param>
         public EcdsaKey(byte[] data)
         {
-            var der = new AsnReader(data, AsnEncodingRules.DER).ReadSequence();
-            _ = der.ReadInteger(); // skip version
+            var keyReader = new AsnReader(data, AsnEncodingRules.DER);
+            var sequenceReader = keyReader.ReadSequence();
+            keyReader.ThrowIfNotEmpty();
 
-            var privatekey = der.ReadOctetString().TrimLeadingZeros();
+            _ = sequenceReader.ReadInteger(); // skip version
 
-            var s0 = der.ReadSequence(new Asn1Tag(TagClass.ContextSpecific, 0, isConstructed: true));
+            var privatekey = sequenceReader.ReadOctetString().TrimLeadingZeros();
+
+            var s0 = sequenceReader.ReadSequence(new Asn1Tag(TagClass.ContextSpecific, 0, isConstructed: true));
             var curve = s0.ReadObjectIdentifier();
 
-            var s1 = der.ReadSequence(new Asn1Tag(TagClass.ContextSpecific, 1, isConstructed: true));
+            var s1 = sequenceReader.ReadSequence(new Asn1Tag(TagClass.ContextSpecific, 1, isConstructed: true));
             var pubkey = s1.ReadBitString(out _);
 
-            der.ThrowIfNotEmpty();
+            sequenceReader.ThrowIfNotEmpty();
 
             _impl = Import(curve, pubkey, privatekey);
         }

diff --git a/src/Renci.SshNet/Security/Cryptography/RsaKey.cs b/src/Renci.SshNet/Security/Cryptography/RsaKey.cs
@@ -161,19 +161,22 @@ public RsaKey(byte[] privateKeyData)
         {
             ThrowHelper.ThrowIfNull(privateKeyData);
 
-            var der = new AsnReader(privateKeyData, AsnEncodingRules.DER).ReadSequence();
-            _ = der.ReadInteger(); // skip version
-
-            Modulus = der.ReadInteger();
-            Exponent = der.ReadInteger();
-            D = der.ReadInteger();
-            P = der.ReadInteger();
-            Q = der.ReadInteger();
-            DP = der.ReadInteger();
-            DQ = der.ReadInteger();
-            InverseQ = der.ReadInteger();
-
-            der.ThrowIfNotEmpty();
+            var keyReader = new AsnReader(privateKeyData, AsnEncodingRules.DER);
+            var sequenceReader = keyReader.ReadSequence();
+            keyReader.ThrowIfNotEmpty();
+
+            _ = sequenceReader.ReadInteger(); // skip version
+
+            Modulus = sequenceReader.ReadInteger();
+            Exponent = sequenceReader.ReadInteger();
+            D = sequenceReader.ReadInteger();
+            P = sequenceReader.ReadInteger();
+            Q = sequenceReader.ReadInteger();
+            DP = sequenceReader.ReadInteger();
+            DQ = sequenceReader.ReadInteger();
+            InverseQ = sequenceReader.ReadInteger();
+
+            sequenceReader.ThrowIfNotEmpty();
 
             RSA = RSA.Create();
             RSA.ImportParameters(GetRSAParameters());