Padding is broken

.editorconfig

@@ -263,6 +263,9 @@ dotnet_diagnostic.SA1413.severity = none
 # This is a duplicate of IDE0011.
 dotnet_diagnostic.SA1503.severity = none
 
+# SA1512: Single line comments should not be followed by a blank line
+dotnet_diagnostic.SA1512.severity = none
+
 # SA1516: Elements must be separated by blank line
 #
 # When enabled, a diagnostic is produced for properties with both a get and set accessor.

src/Renci.SshNet/Security/Cryptography/BlockCipher.cs

@@ -1,4 +1,5 @@
 using System;
+
 using Renci.SshNet.Security.Cryptography.Ciphers;
 
 namespace Renci.SshNet.Security.Cryptography
@@ -74,18 +75,18 @@ protected BlockCipher(byte[] key, byte blockSize, CipherMode mode, CipherPadding
         /// </returns>
         public override byte[] Encrypt(byte[] input, int offset, int length)
         {
-            if (length % _blockSize > 0)
-            {
-                if (_padding is null)
-                {
-                    throw new ArgumentException("data");
-                }
+            var paddingLength = _blockSize - (length % _blockSize);
 
-                var paddingLength = _blockSize - (length % _blockSize);
+            if (_padding is not null)
+            {
                 input = _padding.Pad(input, offset, length, paddingLength);
-                length += paddingLength;
+                length = input.Length;
                 offset = 0;
             }
+            else if (paddingLength != _blockSize)
+            {
+                throw new ArgumentException("The specified plaintext size is not valid for the padding and block size.");
+            }
 
             var output = new byte[length];
             var writtenBytes = 0;
@@ -133,14 +134,17 @@ public override byte[] Decrypt(byte[] input)
         /// </returns>
         public override byte[] Decrypt(byte[] input, int offset, int length)
         {
-            if (length % _blockSize > 0)
+            var originalLength = length;
+
+            if (length % _blockSize != 0)
             {
-                if (_padding is null)
-                {
-                    throw new ArgumentException("data");
-                }
+                // Resize the input to allow decrypting non-block-sized inputs
+                // in a block-by-block manner.
+                var tmpInput = new byte[length + _blockSize - (length % _blockSize)];
+
+                Buffer.BlockCopy(input, offset, tmpInput, 0, length);
 
-                input = _padding.Pad(_blockSize, input, offset, length);
+                input = tmpInput;
                 offset = 0;
                 length = input.Length;
             }
@@ -165,6 +169,36 @@ public override byte[] Decrypt(byte[] input, int offset, int length)
                 throw new InvalidOperationException("Encryption error.");
             }
 
+            // Do a dance around padding to satisfy the cases where _padding
+            // has been specified but the plaintext is not actually padded,
+            // and where a non-block-sized input has been given so we have
+            // increased the length above.
+
+            if (length > originalLength)
+            {
+                // The input was a non-block-sized length, so it was not padded.
+                // Just resize back to the original length and return.
+
+                Array.Resize(ref output, originalLength);
+            }
+            else if (_padding is not null)
+            {
+                var unpaddedLength = _padding.GetUnpaddedLength(output);
+
+                if (unpaddedLength < length)
+                {
+                    // _padding has been specified and the plaintext does have
+                    // valid padding. Remove it.
+
+                    Array.Resize(ref output, unpaddedLength);
+                }
+                else
+                {
+                    // _padding has been specified but the plaintext is not padded.
+                    // Do nothing.
+                }
+            }
+
             return output;
         }
     }

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>
+        /// Calculates the length of the input with padding removed, if any.
+        /// </summary>
+        /// <param name="input">The byte input possibly containing padding.</param>
+        /// <returns>The length of the data without padding.</returns>
+        public abstract int GetUnpaddedLength(byte[] input);
     }
 }

src/Renci.SshNet/Security/Cryptography/Ciphers/Paddings/PKCS7Padding.cs

@@ -45,5 +45,23 @@ public override byte[] Pad(byte[] input, int offset, int length, int paddingleng
 
             return output;
         }
+
+        /// <inheritdoc/>
+        public override int GetUnpaddedLength(byte[] input)
+        {
+            var paddingLength = input[input.Length - 1];
+
+            for (var i = input.Length - 1; i >= input.Length - paddingLength; i--)
+            {
+                if (i < 0 || input[i] != paddingLength)
+                {
+                    // This is not valid PKCS7 padding. Just return the full length
+                    // assuming the ciphertext is not actually padded.
+                    return input.Length;
+                }
+            }
+
+            return input.Length - paddingLength;
+        }
     }
 }

test/Renci.SshNet.Tests/Classes/Security/Cryptography/BlockCipherTest.cs

@@ -1,5 +1,4 @@
 using System;
-using System.Linq;
 using Microsoft.VisualStudio.TestTools.UnitTesting;
 using Renci.SshNet.Security.Cryptography;
 using Renci.SshNet.Security.Cryptography.Ciphers;
@@ -17,7 +16,7 @@ public void EncryptShouldTakeIntoAccountPaddingForLengthOfOutputBufferPassedToEn
             var input = new byte[] { 0x2c, 0x1a, 0x05, 0x00, 0x68 };
             var output = new byte[] { 0x0a, 0x00, 0x03, 0x02, 0x06, 0x08, 0x07, 0x05 };
             var key = new byte[] { 0x17, 0x78, 0x56, 0xe1, 0x3e, 0xbd, 0x3e, 0x50, 0x1d, 0x79, 0x3f, 0x0f, 0x55, 0x37, 0x45, 0x54 };
-            var blockCipher = new BlockCipherStub(key, 8, null, new PKCS5Padding())
+            var blockCipher = new BlockCipherStub(key, 8, null, new PKCS7Padding())
                 {
                     EncryptBlockDelegate = (inputBuffer, inputOffset, inputCount, outputBuffer, outputOffset) =>
                         {
@@ -29,16 +28,16 @@ public void EncryptShouldTakeIntoAccountPaddingForLengthOfOutputBufferPassedToEn
 
             var actual = blockCipher.Encrypt(input);
 
-            Assert.IsTrue(output.SequenceEqual(actual));
+            CollectionAssert.AreEqual(output, actual);
         }
 
         [TestMethod]
         public void DecryptShouldTakeIntoAccountPaddingForLengthOfOutputBufferPassedToDecryptBlock()
         {
             var input = new byte[] { 0x2c, 0x1a, 0x05, 0x00, 0x68 };
-            var output = new byte[] { 0x0a, 0x00, 0x03, 0x02, 0x06, 0x08, 0x07, 0x05 };
+            var output = new byte[] { 0x0a, 0x00, 0x03, 0x02, 0x06 };
             var key = new byte[] { 0x17, 0x78, 0x56, 0xe1, 0x3e, 0xbd, 0x3e, 0x50, 0x1d, 0x79, 0x3f, 0x0f, 0x55, 0x37, 0x45, 0x54 };
-            var blockCipher = new BlockCipherStub(key, 8, null, new PKCS5Padding())
+            var blockCipher = new BlockCipherStub(key, 8, null, new PKCS7Padding())
                 {
                     DecryptBlockDelegate = (inputBuffer, inputOffset, inputCount, outputBuffer, outputOffset) =>
                         {
@@ -50,7 +49,7 @@ public void DecryptShouldTakeIntoAccountPaddingForLengthOfOutputBufferPassedToDe
 
             var actual = blockCipher.Decrypt(input);
 
-            Assert.IsTrue(output.SequenceEqual(actual));
+            CollectionAssert.AreEqual(output, actual);
         }