[SSHD-506] Add support for RFC 5647 #132

jvz · 2020-05-18T16:24:05Z

This is a work in progress that does not work yet, and the API changes to Cipher/CipherInformation are still in flux.

I've been testing this by changing CoreTestSupportUtils to set up clients and servers with only the AES/GCM cipher enabled. So far, I've had varying problems with getting this to work, and I've been using OpenSSH as a comparison to no further help. For reference, here are the relevant source files:

packet.c
cipher.c (and maybe a reference to the OpenSSL API to understand all the EVP_ functions)
kex.c (slightly less relevant)

This is a work in progress that does not work yet, and the API changes to Cipher/CipherInformation are still in flux. Signed-off-by: Matt Sicker <boards@gmail.com>

lgoldstein

Looks very promising - see a few minor comments. Please make sure you run the full mvn clean install (I see there are some Checkstyle issues...)

sshd-core/src/main/java/org/apache/sshd/common/session/helpers/AbstractSession.java

sshd-common/src/main/java/org/apache/sshd/common/cipher/BaseGCMCipher.java

sshd-common/src/main/java/org/apache/sshd/common/cipher/BuiltinCiphers.java

Signed-off-by: Matt Sicker <boards@gmail.com>

lgoldstein

Shouldn't we also add these ciphers to the default ones used by the client/server (see their respective builder(s)) ? If so, at which priority ?

sshd-common/src/main/java/org/apache/sshd/common/cipher/BaseAEADCipher.java

jvz · 2020-05-21T18:33:09Z

Yes, these should be added as defaults once they're working. Based on how OpenSSH is distributed nowadays, these should the highest priority ciphers. I've seen ChaCha20 taking precedence as well, though that's not supported here yet (plus requires either Java 11 or Bouncycastle or some other library).

Signed-off-by: Matt Sicker <boards@gmail.com>

jvz · 2020-05-22T00:12:17Z

So it seems I've created an incompatible implementation with OpenSSH at the moment, but it talks to itself. Still debugging this before this can move anywhere.

This seems to match what OpenSSH is doing. I think.

lgoldstein · 2020-05-22T15:51:36Z

sshd-common/src/main/java/org/apache/sshd/common/cipher/BaseGCMCipher.java

+    protected AlgorithmParameterSpec getNextAlgorithmParameters() {
+        Cipher cipher = getCipherInstance();
+        Buffer iv = new ByteArrayBuffer(cipher.getIV());
+        iv.rpos(Integer.BYTES);
+        long ic = iv.getLong();
+        ic = (ic + 1) & 0x0ffffffffL;
+        iv.wpos(Integer.BYTES);
+        iv.putLong(ic);
+        return new GCMParameterSpec(getAuthenticationTagSize() * Byte.SIZE, iv.array());
+    }


Pretty expensive in memory - every time it is called (which is for every packet) a new byte[] (cipher.getIv() returns a clone), new Buffer and new GCMParameterSpec instance is created. Can we do something about it ?

Moreover, this code is exactly the same as initializeAlgorithmParameters except it does +1 instead of -1 - perhaps we could use an internal (protected) updateAlgorithmParameters(byte[] iv, long delta) and call it once with +1 and once with -1

I believe we can avoid creating the Buffer since all the code does is replace an 8-byte section of the IV at offset 4:

byte[] iv = cipher.getIV(); // Need to write decodeLong but it should be similar to decodeInt in the same class // can use the code of Buffer#getLong as base long ic = KeyEntryResolver.decodeLong(iv, Integer.BYTES, iv.length - Integer.BYTES); ic = (ic + delta) & 0x0ffffffffL; // Need to write it - can use the code of Buffer#putLong as base KeyEntryResolver.encodeLong(iv, ic, Integer.BYTES, iv.length - Integer.BYTES);

Ooh, nice find. Yes, we just need to update the 8-byte section of the buffer. The update algorithm parameters API idea sounds good to me; it's more in line with how OpenSSH is structured, too.

Regarding the GCMParameterSpec based on this link we might be able to execute an ugly hack via reflection API and manipulate the internal IV data directly instead of creating a new instance every time... will need to think about it some more - but only after this code is working.

I came up with a better idea: extend the class and don't clone the IV. Less safe, but doesn't continually create garbage.

I've mostly optimized away all the garbage here. I do an initial IV clone when calling the public init method, but that's the only time. I don't see the IV being cloned further by the Cipher classes that use GCMParameterSpec.

Wow, I optimized it so much, both AES/GCM engines can't tell that the IV changes between invocations and throw an exception. Nice.

I can't figure out how to get the engine to consider the IV as different, so I still have some clone calls happening, but much less waste than before.

I looked into the internal implementation of the cipher engine, and it makes its own clone regardless of what we do, so this seems to be about as optimal as I can get it without digging into Bouncycastle internals or something similar.

Signed-off-by: Matt Sicker <boards@gmail.com>

Forgot about this Signed-off-by: Matt Sicker <boards@gmail.com>

Works on my machine® Signed-off-by: Matt Sicker <boards@gmail.com>

jvz · 2020-05-23T03:08:10Z

The tests all pass on my machine. I'm not sure what's going on with the failing one on here.

lgoldstein · 2020-05-24T16:11:19Z

The tests all pass on my machine. I'm not sure what's going on with the failing one on here.

That's OK - if you feel the code is ready as far as you are concerned, let me know. Once I have examined it and successfully ran a full mvn clean install on my machine I am willing to merge it. Our Travis builds are not yet 100% stable so if they fail but the tests succeed on my machine I usually consider the code safe to merge.

That being said, please run some tests with OpenSSH clients/servers that support these ciphers as well as some popular applications such as Putty, WinSCP, FileZilla

sshd-common/src/main/java/org/apache/sshd/common/cipher/BaseGCMCipher.java

jvz · 2020-05-24T16:14:57Z

I still need to do some manual integration testing. Thanks for the suggestions since OpenSSH was the only thing I remembered exists. This may be on hold for a week or so as I’m on vacation this week.

On Sun, May 24, 2020 at 11:11 Lyor Goldstein ***@***.***> wrote: The tests all pass on my machine. I'm not sure what's going on with the failing one on here. That's OK - if you feel the code is ready as far as you are concerned, let me know. Once I have examined it and successfully ran a full mvn clean install on my machine I am willing to merge it. Our Travis builds are not yet 100% stable so if they fail but the tests succeed on my machine I usually consider the code safe to merge. That being said, please run some tests with OpenSSH clients/servers that support these ciphers as well as some popular applications such as *Putty, WinSCP, FileZilla* — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#132 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAGBF275RDGO6M5CYXBHUJLRTFBLFANCNFSM4NEHJDRQ> .

-- Matt Sicker <boards@gmail.com>

lgoldstein · 2020-05-27T18:01:13Z

This may be on hold for a week or so as I’m on vacation this week.

Enjoy your vacation - no hurry - when you feel ready I will be happy to review...

jvz · 2020-06-02T01:40:54Z

I merged the latest changes from master and did some basic smoketesting using the following:

Server: OpenSSH_8.1p1, LibreSSL 2.7.3
Command: ./ssh.sh -c 'aes128-gcm@openssh.com' localhost

I'll do some more testing in the opposite direction as well as some cleanups.

jvz · 2020-06-02T01:59:32Z

Also tested with FileZilla so far. It even defaulted to selecting aes256-gcm! Seems to work.

Signed-off-by: Matt Sicker <boards@gmail.com>

jvz · 2020-06-02T03:33:11Z

I can't seem to find any official documentation on what the RFC 5647 cipher names are supposed to be unless they meant to literally use AEAD_AES_128_GCM and AEAD_AES_256_GCM as the cipher names. That seems odd since every other cipher name is in lowercase and uses dashes instead of underscores.

Edit: neither FileZilla nor OpenSSH even reference RFC 5647. Is this the first implementation of the spec in 11 years?

jvz · 2020-06-02T03:56:19Z

Based on what I'm finding online, it does seem like the RFC specifies the all caps version. I'm going to try adding that as well.

This simplifies the new AEAD API a bit based on usage. Signed-off-by: Matt Sicker <boards@gmail.com>

Signed-off-by: Matt Sicker <boards@gmail.com>

jvz · 2020-06-14T01:59:51Z

I also tested this with dropbear so far, all good. Also tested with PuTTY (on macOS).

Feel free to test in any other platforms you're concerned with, but I feel fairly confident with the patch.

lgoldstein · 2020-06-14T16:05:46Z

Feel free to test in any other platforms you're concerned with, but I feel fairly confident with the patch.

I will do so some time soon - does this mean that I can merge this patch if I am satisfied with it ?

sshd-common/src/main/java/org/apache/sshd/common/cipher/BaseGCMCipher.java

Signed-off-by: Matt Sicker <boards@gmail.com>

jvz · 2020-06-14T17:15:30Z

I will do so some time soon - does this mean that I can merge this patch if I am satisfied with it ?

Yes, I think so. I'm a little iffy on the BaseGCMCipher name for the class as I'm imagining a different use of AES/GCM along the lines of how OpenSSH implements ChaCha20-Poly1305 which would behave differently (the additional authenticated data would be encrypted but not authenticated using another AES key using AES/CTR like how in the packet length is encrypted). Otherwise ready to merge.

lgoldstein · 2020-06-15T16:55:37Z

Yes, I think so. I'm a little iffy on the BaseGCMCipher name for the class as I'm imagining a different use of AES/GCM along the lines of how OpenSSH implements ChaCha20-Poly1305 which would behave differently (the additional authenticated data would be encrypted but not authenticated using another AES key using AES/CTR like how in the packet length is encrypted).

We'll cross that bridge when we come to it - refactoring is not a dirty word in this project

jvz · 2020-06-20T20:32:54Z

Tested on an RPi4 (arm64), works fine.

Signed-off-by: Matt Sicker <boards@gmail.com>

jvz · 2020-06-27T02:52:51Z

While working on SSHD-1017, I discovered a couple refactoring improvements to add here.

lgoldstein · 2020-06-27T06:12:35Z

Seems like it is still work in progress - I will hold off testing it and merging into master branch for now

jvz · 2020-06-27T16:38:07Z

Once I’ve figured out the ChaCha part, I think this PR will be ready to go. I’ll add that cipher in a separate PR, but I’m trying to make the changes required for both ciphers to be as similar and minimal as possible. I’ll let you know once this is final again!

On Sat, Jun 27, 2020 at 01:12 Lyor Goldstein ***@***.***> wrote: Seems like it is still work in progress - I will hold off testing it and merging into master branch for now — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#132 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAGBF23JQGPESOSGRSUTI6DRYWEV5ANCNFSM4NEHJDRQ> .

-- Matt Sicker <boards@gmail.com>

lgoldstein · 2020-06-28T17:12:35Z

I’m trying to make the changes required for both ciphers to be as similar and minimal as possible. I’ll let you know once this is final again!

Great - this contribution is very important so thank you for the effort...

lgoldstein · 2020-07-28T15:31:52Z

Sorry to bother you, but have you made any progress ? The reason I ask is that I would like to start deprecating unsafe settings (see SSHD-1004) - and that includes some ciphers. Since this may require some non-trivial changes, I would prefer to do it after having added these ciphers. If the work on ChaCha seems to take a short while (let's say another 1-2 weeks) then I can wait. Otherwise, I would rather incorporate the GCM code into the master branch.

jvz · 2020-07-28T16:22:04Z

Oh hey, sorry about that. This branch is basically ready at this point as I got stuck trying to get ChaCha20-Poly1305 to work properly. From what I recall, I already updated the code in this branch to match the same style I was hoping to use for that cipher (particularly by making the ChaCha version of updateAAD handle the encryption/decryption aspect, too). As that aspect shouldn't change too much, and based on the refactoring culture here, I don't think there's much need to wait any longer on the second feature.

lgoldstein · 2020-07-28T16:32:37Z

Thanks - then I'll start merging it and testing...

lgoldstein · 2020-07-31T06:23:58Z

Committed dc73e260f with many thanks - you can close the PR

jvz · 2020-07-31T14:19:10Z

Thanks!

* Implement AES-GCM cipher support Fixes #217. A port of AES-GCM cipher support from Apache MINA-SSHD, based on apache/mina-sshd#132. Included tests for decoding SSH packets sent from Apache MINA-SSHD and OpenSSH (Version 7.9p1 as used by Debian 10). Manual tests also done on OpenSSH server 7.9p1 running Debian 10 with its available ciphers, including 3des-cbc, aes128-cbc, aes192-cbc, aes256-cbc, aes128-ctr, aes192-ctr, aes256-ctr, aes128-gcm@openssh.com and aes256-gcm@openssh.com. * Changes per PR feedback - Fixed variable/statement whitespaces and add back missing braces per coding standard requirement - Moved Buffer.putLong() and Buffer.getLong() into GcmCipher.CounterGCMParameterSpec since it's the only user - Moved BaseCipher.authSize into GcmCipher since it is the only cipher that would return a non-zero. BaseCipher will keep return 0 instead - Made BaseCipher.cipher protected instead of making it publicly accessible - Combined the three decoding modes in Decoder.decode() into one single method, to reduce code duplication - Added integration test for the ciphers, along with the newly implemented AES-GCM ciphers

[SSHD-506] Add support for RFC 5647

dc39193

This is a work in progress that does not work yet, and the API changes to Cipher/CipherInformation are still in flux. Signed-off-by: Matt Sicker <boards@gmail.com>

lgoldstein reviewed May 18, 2020

View reviewed changes

sshd-core/src/main/java/org/apache/sshd/common/session/helpers/AbstractSession.java Outdated Show resolved Hide resolved

sshd-core/src/main/java/org/apache/sshd/common/session/helpers/AbstractSession.java Outdated Show resolved Hide resolved

jvz commented May 20, 2020

View reviewed changes

sshd-core/src/main/java/org/apache/sshd/common/session/helpers/AbstractSession.java Outdated Show resolved Hide resolved

lgoldstein reviewed May 20, 2020

View reviewed changes

jvz added 2 commits May 20, 2020 17:55

Update to almost work

8b2ef07

Signed-off-by: Matt Sicker <boards@gmail.com>

Remove generated final keywords

85f89cf

lgoldstein reviewed May 21, 2020

View reviewed changes

sshd-common/src/main/java/org/apache/sshd/common/cipher/BaseAEADCipher.java Outdated Show resolved Hide resolved

Clean up API updates and add docs

bd03dd2

Signed-off-by: Matt Sicker <boards@gmail.com>

Decrement IV before initial increment

3500697

This seems to match what OpenSSH is doing. I think.

lgoldstein reviewed May 22, 2020

View reviewed changes

jvz added 6 commits May 22, 2020 18:51

Merge branch 'master' into gcm-cipher-SSHD-506

77672f4

Optimize GCM IV usage

b7a3ec3

Signed-off-by: Matt Sicker <boards@gmail.com>

Fix iv reuse exception

0b12782

Signed-off-by: Matt Sicker <boards@gmail.com>

Make code less C-like to make PMD happy

2de46fc

Signed-off-by: Matt Sicker <boards@gmail.com>

Re-add counter decrement

f874c3c

Forgot about this Signed-off-by: Matt Sicker <boards@gmail.com>

Fix tests

1de84ef

Works on my machine® Signed-off-by: Matt Sicker <boards@gmail.com>

lgoldstein reviewed May 24, 2020

View reviewed changes

sshd-common/src/main/java/org/apache/sshd/common/cipher/BaseGCMCipher.java Outdated Show resolved Hide resolved

Merge branch 'master' into gcm-cipher-SSHD-506

2bd1dc9

Use safer arithmetic

42d4bed

Signed-off-by: Matt Sicker <boards@gmail.com>

jvz marked this pull request as ready for review June 2, 2020 03:34

jvz added 2 commits June 13, 2020 16:28

Clean up AEAD API and add tests

0f37aec

This simplifies the new AEAD API a bit based on usage. Signed-off-by: Matt Sicker <boards@gmail.com>

Simplify code

953b999

Signed-off-by: Matt Sicker <boards@gmail.com>

lgoldstein suggested changes Jun 14, 2020

View reviewed changes

sshd-common/src/main/java/org/apache/sshd/common/cipher/BaseGCMCipher.java Outdated Show resolved Hide resolved

sshd-common/src/main/java/org/apache/sshd/common/cipher/BaseGCMCipher.java Outdated Show resolved Hide resolved

Make internals protected

f797955

Signed-off-by: Matt Sicker <boards@gmail.com>

jvz added 3 commits June 21, 2020 14:18

Simplify IV management

b0c814f

Signed-off-by: Matt Sicker <boards@gmail.com>

Split updateAAD into own method

cc5a53c

Signed-off-by: Matt Sicker <boards@gmail.com>

Merge branch 'master' into gcm-cipher-SSHD-506

f4ca814

jvz closed this Jul 31, 2020

jvz deleted the gcm-cipher-SSHD-506 branch July 31, 2020 14:19

jvz restored the gcm-cipher-SSHD-506 branch July 31, 2020 14:19

jvz deleted the gcm-cipher-SSHD-506 branch July 31, 2020 14:19

jvz restored the gcm-cipher-SSHD-506 branch July 31, 2020 14:19

TranceLove mentioned this pull request Aug 23, 2020

SFTP - security options improvment TeamAmaze/AmazeFileManager#1961

Closed

This was referenced Sep 5, 2020

Support for aesXXX-gcm@openssh.com ciphers hierynomus/sshj#217

Closed

Implement AES-GCM cipher support hierynomus/sshj#630

Merged

[SSHD-506] Add support for RFC 5647 #132

[SSHD-506] Add support for RFC 5647 #132

Conversation

jvz commented May 18, 2020

lgoldstein left a comment

Choose a reason for hiding this comment

lgoldstein left a comment

Choose a reason for hiding this comment

jvz commented May 21, 2020

jvz commented May 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jvz commented May 23, 2020

lgoldstein commented May 24, 2020

jvz commented May 24, 2020 via email

lgoldstein commented May 27, 2020

jvz commented Jun 2, 2020

jvz commented Jun 2, 2020

jvz commented Jun 2, 2020 • edited Loading

jvz commented Jun 2, 2020

jvz commented Jun 14, 2020

lgoldstein commented Jun 14, 2020

jvz commented Jun 14, 2020

lgoldstein commented Jun 15, 2020

jvz commented Jun 20, 2020

jvz commented Jun 27, 2020

lgoldstein commented Jun 27, 2020

jvz commented Jun 27, 2020 via email

lgoldstein commented Jun 28, 2020

lgoldstein commented Jul 28, 2020

jvz commented Jul 28, 2020

lgoldstein commented Jul 28, 2020

lgoldstein commented Jul 31, 2020

jvz commented Jul 31, 2020

jvz commented Jun 2, 2020 •

edited

Loading