Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add padding when encrypt and remove padding when decrypt #1545

Merged
merged 16 commits into from
Dec 22, 2024
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/Renci.SshNet/PrivateKeyFile.PKCS1.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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()));
Expand Down
3 changes: 1 addition & 2 deletions src/Renci.SshNet/PrivateKeyFile.SSHCOM.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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
{
Expand Down Expand Up @@ -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
Expand Down
57 changes: 45 additions & 12 deletions src/Renci.SshNet/Security/Cryptography/BlockCipher.cs
Original file line number Diff line number Diff line change
@@ -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
{
Expand Down Expand Up @@ -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;
}
Rob-Hague marked this conversation as resolved.
Show resolved Hide resolved
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;
Expand All @@ -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;
}

Expand All @@ -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];
Expand All @@ -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);
Rob-Hague marked this conversation as resolved.
Show resolved Hide resolved
}

if (paddingLength > 0)
{
Array.Resize(ref output, output.Length - paddingLength);
}

return output;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand All @@ -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;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can consider replacing CipherPadding and PKCS7Padding with BouncyCastle's Pkcs7Padding

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, I think we should take this opportunity to delete the useless CipherPadding and replace it with BC IBlockCipherPadding, and delete PKCS7Padding and replace it with BC Pkcs7Padding, if possible

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me have a try

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just had a quick look. It would be easier to remove it in my next PR #1546.

}
}
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
using System;

using Renci.SshNet.Common;

namespace Renci.SshNet.Security.Cryptography.Ciphers.Paddings
{
/// <summary>
Expand Down Expand Up @@ -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;
}
}
}
19 changes: 11 additions & 8 deletions src/Renci.SshNet/Security/Cryptography/DsaKey.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
Expand Down
15 changes: 9 additions & 6 deletions src/Renci.SshNet/Security/Cryptography/EcdsaKey.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
29 changes: 16 additions & 13 deletions src/Renci.SshNet/Security/Cryptography/RsaKey.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand Down
Loading
Loading