Skip to content
Closed
Show file tree
Hide file tree
Changes from 10 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: 2 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,8 @@ the missing test once you figure things out. 🤓
* aes128-cbc
* aes192-cbc
* aes256-cbc
* aes128-gcm<span></span>@openssh.com (.NET Standard 2.1, .NET 6 and higher)
* aes256-gcm<span></span>@openssh.com (.NET Standard 2.1, .NET 6 and higher)
* blowfish-cbc
* twofish-cbc
* twofish192-cbc
Expand Down
4 changes: 4 additions & 0 deletions src/Renci.SshNet/ConnectionInfo.cs
Original file line number Diff line number Diff line change
Expand Up @@ -388,6 +388,10 @@ public ConnectionInfo(string host, int port, string username, ProxyTypes proxyTy
{ "aes128-cbc", new CipherInfo(128, (key, iv) => new AesCipher(key, iv, AesCipherMode.CBC, pkcs7Padding: false)) },
{ "aes192-cbc", new CipherInfo(192, (key, iv) => new AesCipher(key, iv, AesCipherMode.CBC, pkcs7Padding: false)) },
{ "aes256-cbc", new CipherInfo(256, (key, iv) => new AesCipher(key, iv, AesCipherMode.CBC, pkcs7Padding: false)) },
#if NETSTANDARD2_1_OR_GREATER || NETCOREAPP3_0_OR_GREATER
{ "[email protected]", new CipherInfo(128, (key, iv) => new AesGcmCipher(key, iv)) },
{ "[email protected]", new CipherInfo(256, (key, iv) => new AesGcmCipher(key, iv)) },
#endif
{ "3des-cbc", new CipherInfo(192, (key, iv) => new TripleDesCipher(key, new CbcCipherMode(iv), padding: null)) },
{ "blowfish-cbc", new CipherInfo(128, (key, iv) => new BlowfishCipher(key, new CbcCipherMode(iv), padding: null)) },
{ "twofish-cbc", new CipherInfo(256, (key, iv) => new TwofishCipher(key, new CbcCipherMode(iv), padding: null)) },
Expand Down
10 changes: 5 additions & 5 deletions src/Renci.SshNet/Messages/Message.cs
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ protected override void WriteBytes(SshDataStream stream)
base.WriteBytes(stream);
}

internal byte[] GetPacket(byte paddingMultiplier, Compressor compressor, bool isEncryptThenMAC = false)
internal byte[] GetPacket(byte paddingMultiplier, Compressor compressor, bool excludePacketLengthFieldWhenPadding = false)
{
const int outboundPacketSequenceSize = 4;

Expand Down Expand Up @@ -78,9 +78,9 @@ internal byte[] GetPacket(byte paddingMultiplier, Compressor compressor, bool is
var packetLength = messageLength + 4 + 1;

// determine the padding length
// in Encrypt-then-MAC mode, the length field is not encrypted, so we should keep it out of the
// in Encrypt-then-MAC mode or AEAD, the length field is not encrypted, so we should keep it out of the
// padding length calculation
var paddingLength = GetPaddingLength(paddingMultiplier, isEncryptThenMAC ? packetLength - 4 : packetLength);
var paddingLength = GetPaddingLength(paddingMultiplier, excludePacketLengthFieldWhenPadding ? packetLength - 4 : packetLength);

// add padding bytes
var paddingBytes = new byte[paddingLength];
Expand All @@ -106,9 +106,9 @@ internal byte[] GetPacket(byte paddingMultiplier, Compressor compressor, bool is
var packetLength = messageLength + 4 + 1;

// determine the padding length
// in Encrypt-then-MAC mode, the length field is not encrypted, so we should keep it out of the
// in Encrypt-then-MAC mode or AEAD, the length field is not encrypted, so we should keep it out of the
// padding length calculation
var paddingLength = GetPaddingLength(paddingMultiplier, isEncryptThenMAC ? packetLength - 4 : packetLength);
var paddingLength = GetPaddingLength(paddingMultiplier, excludePacketLengthFieldWhenPadding ? packetLength - 4 : packetLength);

var packetDataLength = GetPacketDataLength(messageLength, paddingLength);

Expand Down
39 changes: 39 additions & 0 deletions src/Renci.SshNet/Security/Cryptography/AeadCipher.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
namespace Renci.SshNet.Security.Cryptography
{
/// <summary>
/// Represents algorithm for Authenticated Encryption with Associated data.
/// </summary>
public abstract class AeadCipher : SymmetricCipher
{
/// <summary>
/// Gets the size of the block in bytes.
/// </summary>
/// <value>
/// The size of the block in bytes.
/// </value>
private readonly byte _blockSize;

/// <summary>
/// Gets the tag size in bytes.
/// </summary>
public int TagSize { get; }

/// <inheritdoc/>
public override byte MinimumSize
{
get { return _blockSize; }
}

/// <summary>
/// Initializes a new instance of the <see cref="AeadCipher"/> class.
/// </summary>
/// <param name="key">The key.</param>
/// <param name="tagSize">The tag size in bytes.</param>
protected AeadCipher(byte[] key, int tagSize)
: base(key)
{
_blockSize = 16;
TagSize = tagSize;
}
}
}
38 changes: 26 additions & 12 deletions src/Renci.SshNet/Security/Cryptography/BlockCipher.cs
Original file line number Diff line number Diff line change
Expand Up @@ -110,18 +110,6 @@ public override byte[] Encrypt(byte[] input, int offset, int length)
return output;
}

/// <summary>
/// Decrypts the specified data.
/// </summary>
/// <param name="input">The data.</param>
/// <returns>
/// The decrypted data.
/// </returns>
public override byte[] Decrypt(byte[] input)
{
return Decrypt(input, 0, input.Length);
}

/// <summary>
/// Decrypts the specified input.
/// </summary>
Expand Down Expand Up @@ -167,5 +155,31 @@ public override byte[] Decrypt(byte[] input, int offset, int length)

return output;
}

/// <summary>
/// Encrypts the specified region of the input byte array and copies the encrypted data to the specified region of the output byte array.
/// </summary>
/// <param name="inputBuffer">The input data to encrypt.</param>
/// <param name="inputOffset">The offset into the input byte array from which to begin using data.</param>
/// <param name="inputCount">The number of bytes in the input byte array to use as data.</param>
/// <param name="outputBuffer">The output to which to write encrypted data.</param>
/// <param name="outputOffset">The offset into the output byte array from which to begin writing data.</param>
/// <returns>
/// The number of bytes encrypted.
/// </returns>
public abstract int EncryptBlock(byte[] inputBuffer, int inputOffset, int inputCount, byte[] outputBuffer, int outputOffset);

/// <summary>
/// Decrypts the specified region of the input byte array and copies the decrypted data to the specified region of the output byte array.
/// </summary>
/// <param name="inputBuffer">The input data to decrypt.</param>
/// <param name="inputOffset">The offset into the input byte array from which to begin using data.</param>
/// <param name="inputCount">The number of bytes in the input byte array to use as data.</param>
/// <param name="outputBuffer">The output to which to write decrypted data.</param>
/// <param name="outputOffset">The offset into the output byte array from which to begin writing data.</param>
/// <returns>
/// The number of bytes decrypted.
/// </returns>
public abstract int DecryptBlock(byte[] inputBuffer, int inputOffset, int inputCount, byte[] outputBuffer, int outputOffset);
}
}
5 changes: 4 additions & 1 deletion src/Renci.SshNet/Security/Cryptography/Cipher.cs
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,10 @@ public byte[] Encrypt(byte[] input)
/// <returns>
/// The decrypted data.
/// </returns>
public abstract byte[] Decrypt(byte[] input);
public byte[] Decrypt(byte[] input)
{
return Decrypt(input, 0, input.Length);
}

/// <summary>
/// Decrypts the specified input.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,22 +5,22 @@
/// </summary>
public enum AesCipherMode
{
/// <summary>CBC Mode.</summary>
/// <summary>Cipher Block Chain Mode.</summary>
CBC = 1,

/// <summary>ECB Mode.</summary>
/// <summary>Electronic Codebook Mode.</summary>
ECB = 2,

/// <summary>OFB Mode.</summary>
/// <summary>Output Feedback Mode.</summary>
OFB = 3,

/// <summary>CFB Mode.</summary>
/// <summary>Cipher Feedback Mode.</summary>
CFB = 4,

/// <summary>CTS Mode.</summary>
/// <summary>Cipher Text Stealing Mode.</summary>
CTS = 5,

/// <summary>CTR Mode.</summary>
/// <summary>Counter Mode.</summary>
CTR = 6
}
}
101 changes: 101 additions & 0 deletions src/Renci.SshNet/Security/Cryptography/Ciphers/AesGcmCipher.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@
#if NETSTANDARD2_1_OR_GREATER || NETCOREAPP3_0_OR_GREATER
using System;
using System.Buffers.Binary;
using System.Security.Cryptography;

using Renci.SshNet.Common;

namespace Renci.SshNet.Security.Cryptography.Ciphers
{
/// <summary>
/// AES GCM cipher implementation.
/// <see href="https://datatracker.ietf.org/doc/html/rfc5647"/>.
/// </summary>
public sealed class AesGcmCipher : AeadCipher, IDisposable
{
private readonly byte[] _nonce;
private readonly AesGcm _aesGcm;

/// <summary>
/// Initializes a new instance of the <see cref="AesGcmCipher"/> class.
/// </summary>
/// <param name="key">The key.</param>
/// <param name="iv">The IV.</param>
public AesGcmCipher(byte[] key, byte[] iv)
: base(key, tagSize: 16)
{
_nonce = iv.Take(12);
#if NET8_0_OR_GREATER
_aesGcm = new AesGcm(key, TagSize);
#else
_aesGcm = new AesGcm(key);
#endif
}

/// <inheritdoc/>
public override byte[] Encrypt(byte[] input, int offset, int length)
{
// [outbound sequence][packet length field][padding length field sz][payload][random paddings]
// [--4 bytes(offset)][------4 bytes------][-------------------Plain Text--------------------]
var packetLengthField = new ReadOnlySpan<byte>(input, offset, 4);
var plainText = new ReadOnlySpan<byte>(input, offset + 4, length - 4);

var result = new byte[length + TagSize];
packetLengthField.CopyTo(result);
var cipherText = new Span<byte>(result, 4, length - 4);
var tag = new Span<byte>(result, length, TagSize);

_aesGcm.Encrypt(_nonce, plainText, cipherText, tag, packetLengthField);

IncrementCounter();

return result;
}

/// <inheritdoc/>
public override byte[] Decrypt(byte[] input, int offset, int length)
{
// [inbound sequence][packet length field][padding length field sz][payload][random paddings][Authenticated TAG]
// [-----4 bytes----][----4 bytes(offset)][------------------Cipher Text--------------------][-------TAG-------]
var packetLengthField = new ReadOnlySpan<byte>(input, offset - 4, 4);
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's quite unpleasant/unintuitive to assume there is data to process in input before offset. Is there a nicer way to do it?

Could packet_length (i.e. the associated data) start at offset?

At the very least, there should be some explicit /// xmldoc here explaining the usage.

Copy link
Collaborator Author

@scott-xu scott-xu Apr 1, 2024

Choose a reason for hiding this comment

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

I understand your concern. Typically, the cipher only decrypts "cipher text" which starts from offset and ends before MAC (tag). However, AesGcm requires the packet_length (aad) and MAC (tag) to do the decryption and authentication which is before and after the cipher text.
The Decrypt(...) method of AesGcm is called (and only called) from https://github.com/sshnet/SSH.NET/blob/develop/src/Renci.SshNet/Session.cs#L1302 which guarantees the data layout.

Copy link
Contributor

Choose a reason for hiding this comment

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

The Decrypt(...) method of AesGcm is called (and only called) from https://github.com/sshnet/SSH.NET/blob/develop/src/Renci.SshNet/Session.cs#L1302 which guarantees the data layout.

Still, adding a bounds check could be a good thing for future proofing.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There are some options here:

  1. keep it as is but add bounds check
  2. in Session class, treat aead cipher as special and set offset before packet_length and include MAC(tag) in length
  3. in Session class, treat aead cipher as special and set offset to 0 and include all in length (consider that ChaCha20Poly1305 requires sequence block if I read the spec correctly)

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 checked the source code of the constructor about ReadOnlySpan<T>. It does check the bounds:

  • It casts the start to ulong/uint to make sure it is positive.
  • It checks the length to no larger than array length - offset.
        /// <summary>
        /// Creates a new read-only span over the portion of the target array beginning
        /// at 'start' index and ending at 'end' index (exclusive).
        /// </summary>
        /// <param name="array">The target array.</param>
        /// <param name="start">The index at which to begin the read-only span.</param>
        /// <param name="length">The number of items in the read-only span.</param>
        /// <remarks>Returns default when <paramref name="array"/> is null.</remarks>
        /// <exception cref="System.ArgumentOutOfRangeException">
        /// Thrown when the specified <paramref name="start"/> or end index is not in the range (&lt;0 or &gt;Length).
        /// </exception>
        [MethodImpl(MethodImplOptions.AggressiveInlining)]
        public ReadOnlySpan(T[]? array, int start, int length)
        {
            if (array == null)
            {
                if (start != 0 || length != 0)
                    ThrowHelper.ThrowArgumentOutOfRangeException();
                this = default;
                return; // returns default
            }
#if TARGET_64BIT
            // See comment in Span<T>.Slice for how this works.
            if ((ulong)(uint)start + (ulong)(uint)length > (ulong)(uint)array.Length)
                ThrowHelper.ThrowArgumentOutOfRangeException();
#else
            if ((uint)start > (uint)array.Length || (uint)length > (uint)(array.Length - start))
                ThrowHelper.ThrowArgumentOutOfRangeException();
#endif

            _pointer = new ByReference<T>(ref Unsafe.Add(ref MemoryMarshal.GetArrayDataReference(array), (nint)(uint)start /* force zero-extension */));
            _length = length;
        }

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There are some options here:

  1. keep it as is but add bounds check
  2. in Session class, treat aead cipher as special and set offset before packet_length and include MAC(tag) in length
  3. in Session class, treat aead cipher as special and set offset to 0 and include all in length (consider that ChaCha20Poly1305 requires sequence block if I read the spec correctly)

Personally I prefer option1, because it is consistent for all kinds of ciphers. No special handing in Session class. The current implementation in #1369 is option2. It defers the issue when add ChaCha20Poly1305 in the future.

Life will be much easier if there's no such many options 🙃

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changed back to option1 with some tiny enhancements in the latest commit.

var cipherText = new ReadOnlySpan<byte>(input, offset, length);
var tag = new ReadOnlySpan<byte>(input, offset + length, TagSize);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here, in assuming there is space beyond offset + length


var plainText = new byte[length];

_aesGcm.Decrypt(_nonce, cipherText, tag, plainText, packetLengthField);

IncrementCounter();

return plainText;
}

private void IncrementCounter()
{
var invocationCounter = new Span<byte>(_nonce, 4, 8);
var count = BinaryPrimitives.ReadUInt64BigEndian(invocationCounter);
BinaryPrimitives.WriteUInt64BigEndian(invocationCounter, count + 1);
}

/// <summary>
/// Dispose the instance.
/// </summary>
/// <param name="disposing">Set to True to dispose of resouces.</param>
public void Dispose(bool disposing)
{
if (disposing)
{
_aesGcm.Dispose();
}
}

/// <inheritdoc/>
public void Dispose()
{
// Do not change this code. Put cleanup code in 'Dispose(bool disposing)' method
Dispose(disposing: true);
GC.SuppressFinalize(this);
}
}
}
#endif
44 changes: 0 additions & 44 deletions src/Renci.SshNet/Security/Cryptography/Ciphers/Arc4Cipher.cs
Original file line number Diff line number Diff line change
Expand Up @@ -50,38 +50,6 @@ public Arc4Cipher(byte[] key, bool dischargeFirstBytes)
}
}

/// <summary>
/// Encrypts the specified region of the input byte array and copies the encrypted data to the specified region of the output byte array.
/// </summary>
/// <param name="inputBuffer">The input data to encrypt.</param>
/// <param name="inputOffset">The offset into the input byte array from which to begin using data.</param>
/// <param name="inputCount">The number of bytes in the input byte array to use as data.</param>
/// <param name="outputBuffer">The output to which to write encrypted data.</param>
/// <param name="outputOffset">The offset into the output byte array from which to begin writing data.</param>
/// <returns>
/// The number of bytes encrypted.
/// </returns>
public override int EncryptBlock(byte[] inputBuffer, int inputOffset, int inputCount, byte[] outputBuffer, int outputOffset)
{
return ProcessBytes(inputBuffer, inputOffset, inputCount, outputBuffer, outputOffset);
}

/// <summary>
/// Decrypts the specified region of the input byte array and copies the decrypted data to the specified region of the output byte array.
/// </summary>
/// <param name="inputBuffer">The input data to decrypt.</param>
/// <param name="inputOffset">The offset into the input byte array from which to begin using data.</param>
/// <param name="inputCount">The number of bytes in the input byte array to use as data.</param>
/// <param name="outputBuffer">The output to which to write decrypted data.</param>
/// <param name="outputOffset">The offset into the output byte array from which to begin writing data.</param>
/// <returns>
/// The number of bytes decrypted.
/// </returns>
public override int DecryptBlock(byte[] inputBuffer, int inputOffset, int inputCount, byte[] outputBuffer, int outputOffset)
{
return ProcessBytes(inputBuffer, inputOffset, inputCount, outputBuffer, outputOffset);
}

/// <summary>
/// Encrypts the specified input.
/// </summary>
Expand All @@ -98,18 +66,6 @@ public override byte[] Encrypt(byte[] input, int offset, int length)
return output;
}

/// <summary>
/// Decrypts the specified input.
/// </summary>
/// <param name="input">The input.</param>
/// <returns>
/// The decrypted data.
/// </returns>
public override byte[] Decrypt(byte[] input)
{
return Decrypt(input, 0, input.Length);
}

/// <summary>
/// Decrypts the specified input.
/// </summary>
Expand Down
14 changes: 0 additions & 14 deletions src/Renci.SshNet/Security/Cryptography/Ciphers/RsaCipher.cs
Original file line number Diff line number Diff line change
Expand Up @@ -49,20 +49,6 @@ public override byte[] Encrypt(byte[] input, int offset, int length)
return Transform(paddedBlock);
}

/// <summary>
/// Decrypts the specified data.
/// </summary>
/// <param name="input">The data.</param>
/// <returns>
/// The decrypted data.
/// </returns>
/// <exception cref="NotSupportedException">Only block type 01 or 02 are supported.</exception>
/// <exception cref="NotSupportedException">Thrown when decrypted block type is not supported.</exception>
public override byte[] Decrypt(byte[] input)
{
return Decrypt(input, 0, input.Length);
}

/// <summary>
/// Decrypts the specified input.
/// </summary>
Expand Down
26 changes: 0 additions & 26 deletions src/Renci.SshNet/Security/Cryptography/SymmetricCipher.cs
Original file line number Diff line number Diff line change
Expand Up @@ -26,31 +26,5 @@ protected SymmetricCipher(byte[] key)

Key = key;
}

/// <summary>
/// Encrypts the specified region of the input byte array and copies the encrypted data to the specified region of the output byte array.
/// </summary>
/// <param name="inputBuffer">The input data to encrypt.</param>
/// <param name="inputOffset">The offset into the input byte array from which to begin using data.</param>
/// <param name="inputCount">The number of bytes in the input byte array to use as data.</param>
/// <param name="outputBuffer">The output to which to write encrypted data.</param>
/// <param name="outputOffset">The offset into the output byte array from which to begin writing data.</param>
/// <returns>
/// The number of bytes encrypted.
/// </returns>
public abstract int EncryptBlock(byte[] inputBuffer, int inputOffset, int inputCount, byte[] outputBuffer, int outputOffset);

/// <summary>
/// Decrypts the specified region of the input byte array and copies the decrypted data to the specified region of the output byte array.
/// </summary>
/// <param name="inputBuffer">The input data to decrypt.</param>
/// <param name="inputOffset">The offset into the input byte array from which to begin using data.</param>
/// <param name="inputCount">The number of bytes in the input byte array to use as data.</param>
/// <param name="outputBuffer">The output to which to write decrypted data.</param>
/// <param name="outputOffset">The offset into the output byte array from which to begin writing data.</param>
/// <returns>
/// The number of bytes decrypted.
/// </returns>
public abstract int DecryptBlock(byte[] inputBuffer, int inputOffset, int inputCount, byte[] outputBuffer, int outputOffset);
}
}
Loading