-
-
Notifications
You must be signed in to change notification settings - Fork 931
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
Support AesGcm cipher #1364
Support AesGcm cipher #1364
Conversation
I will review in next few days. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The OpenSSH version of AES-GCM (https://github.com/openssh/openssh-portable/blob/43e7c1c07cf6aae7f4394ca8ae91a3efc46514e2/PROTOCOL#L82) states:
Additionally, if AES-GCM is selected as the cipher [...] there doesn't have to be a matching MAC.
could you please implement that?
{ | ||
// [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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
Decrypt(...)
method ofAesGcm
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.
There was a problem hiding this comment.
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:
- keep it as is but add bounds check
- in
Session
class, treat aead cipher as special and set offset before packet_length and include MAC(tag) in length - 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)
There was a problem hiding this comment.
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 (<0 or >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;
}
There was a problem hiding this comment.
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:
- keep it as is but add bounds check
- in
Session
class, treat aead cipher as special and set offset before packet_length and include MAC(tag) in length- 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 🙃
There was a problem hiding this comment.
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.
// [-----4 bytes----][----4 bytes(offset)][------------------Cipher Text--------------------][-------TAG-------] | ||
var packetLengthField = new ReadOnlySpan<byte>(input, offset - 4, 4); | ||
var cipherText = new ReadOnlySpan<byte>(input, offset, length); | ||
var tag = new ReadOnlySpan<byte>(input, offset + length, TagSize); |
There was a problem hiding this comment.
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
I think the "matching MAC" means the matching MAC algorithm. See code here: https://github.com/sshnet/SSH.NET/pull/1364/files#diff-8cb58eb5577b3c26c820c320ea463952036f2aeadf28470a6265575168756164R1520-R1528 |
That's good but I think if we offer:
and they offer:
then we are going to error out here despite not using the HMAC SSH.NET/src/Renci.SshNet/Security/KeyExchange.cs Lines 99 to 107 in db3d7e8
|
@Rob-Hague Can I ask you for a review? My time is limited now. |
I renamed the source branch and this PR is closed automatically. I just created another PR #1369 |
You mean #1369. |
@zybexXL thanks! I corrected the comments just now. Unfortunately, this PR cannot be reopened. The button is grayed out. |
Admins/Moderators should be able to reopen it. |
Presumably, you will at least need to restore the branch ref with the same name as before |
The PR is closed but this thread is not deleted. The history is not lost. |
This PR adds support for aes128-gcm@openssh.com, aes256-gcm@openssh.com ciphers.
This PR does not add chacha20-poly1305@openssh.com cipher.
Fix #792 Fix #1356 Fix #774 Fix #773 Fix #555 Fix #477 Fix #994