sntrup761x25519-sha512@openssh.com KEX causing "incorrect signature" with OpenSSH client

[lukegb]

Version

2.13.1

Bug description

Using Mina SSHd inside Gerrit, if sntrup761x25519-sha512@openssh.com is enabled, then OpenSSH cannot connect with an "incorrect signature" error.

Actual behavior

$ ssh -p 29418 admin@localhost -o ControlMaster=no
ssh_dispatch_run_fatal: Connection to ::1 port 29418: incorrect signature
$ ssh -p 29418 admin@localhost -o ControlMaster=no -o KexAlgorithms=sntrup761x25519-sha512@openssh.com 
ssh_dispatch_run_fatal: Connection to ::1 port 29418: incorrect signature
$ ssh -p 29418 admin@localhost -o ControlMaster=no -o KexAlgorithms=curve25519-sha256

  ****    Welcome to Gerrit Code Review    ****

  Hi Administrator, you have successfully connected over SSH.

  Unfortunately, interactive shells are disabled.
  To clone a hosted Git repository, use:

  git clone ssh://admin@localhost:29418/REPOSITORY_NAME.git

Connection to localhost closed.

Expected behavior

Both connections using sntrup761x25519-sha512@openssh.com and not using sntrup761x25519-sha512@openssh.com should work.

Relevant log output

No response

Other information

$ ssh -V
OpenSSH_9.8p1, OpenSSL 3.0.13 30 Jan 2024

I'm not 100% sure if BouncyCastle is available on the classpath; I'll add some more debugging information once it's available.

This KEX method is new and was added in #498 by @tomaswolf

[lukegb]

OK, it looks like if I bump BouncyCastle from 1.72 to 1.78.1 then it starts working; this might not "really" be a Mina-SSHd bug at all, but it might be a good idea to disable this KEX method if the available BouncyCastle is too old?

[tomaswolf]

disable this KEX method if the available BouncyCastle is too old?

I thought that was done? https://github.com/apache/mina-sshd/blob/04ae57b7c55961c271d9fa38990ae1d5982661cc/sshd-core/src/main/java/org/apache/sshd/common/kex/SNTRUP761.java#L44

[JinHeap]

i test client need package eddsa-0.3.0.jar at

        <dependency>
            <groupId>net.i2p.crypto</groupId>
            <artifactId>eddsa</artifactId>
            <optional>true</optional>
        </dependency>

you can try like this

[tomaswolf]

As far as I see all checks for Bouncy Castle and that eddsa library being available are done.

There is nothing to do in that respect.

However:

There is indeed a bug in our implementation of sntrup761x25519-sha512 that causes the key exchange to fail with probability 1/256 with an SshException "KeyExchange signature verification failed for key type=...". So most of the time it works, but sometimes it'll fail.

Work-around: don't use sntrup761x25519-sha512. Explicitly set the DH factories you want on the SshClient, and do not include sntrup761x25519-sha512 in that list.

This bug will be fixed in the next release.

[quic-nasserg]

@tomaswolf there's something going on with the BC version. I can repro this on Gerrit master with sshd 2.13.2, but it goes away when I bump BC to 1.78.1 (to get that key size fix). That said, the code you linked to in isSupported() looks correct to me, so I don't know what's going on. Is it perhaps because https://github.com/apache/mina-sshd/blob/4f2ccf885292adde1d3a0d5f9abd9fb513b07688/sshd-core/src/main/java/org/apache/sshd/common/kex/BuiltinDHFactories.java#L340 doesn't call that method?

[tomaswolf]

@quic-nasserg Hi Nasser! What do you mean by

doesn't call that method?

https://github.com/apache/mina-sshd/blob/4f2ccf885292adde1d3a0d5f9abd9fb513b07688/sshd-core/src/main/java/org/apache/sshd/common/kex/BuiltinDHFactories.java#L340-L343

calls BuiltinKEM.sntrup761.isSupported():

https://github.com/apache/mina-sshd/blob/4f2ccf885292adde1d3a0d5f9abd9fb513b07688/sshd-core/src/main/java/org/apache/sshd/common/kex/BuiltinKEM.java#L41-L44

which calls SNTRUP761.isSupported():

https://github.com/apache/mina-sshd/blob/4f2ccf885292adde1d3a0d5f9abd9fb513b07688/sshd-core/src/main/java/org/apache/sshd/common/kex/SNTRUP761.java#L44-L53

which does the key size check and which also returns false if BC doesn't have SNTRUPrimeParameters.sntrup761 at all.

The DH factories are set up by default in

https://github.com/apache/mina-sshd/blob/4f2ccf885292adde1d3a0d5f9abd9fb513b07688/sshd-core/src/main/java/org/apache/sshd/server/ServerBuilder.java#L177-L179

which gets called at

https://github.com/apache/mina-sshd/blob/4f2ccf885292adde1d3a0d5f9abd9fb513b07688/sshd-core/src/main/java/org/apache/sshd/server/ServerBuilder.java#L119

with ignoreUnsupported == false (and likewise for ClientBuilder), so

https://github.com/apache/mina-sshd/blob/4f2ccf885292adde1d3a0d5f9abd9fb513b07688/sshd-common/src/main/java/org/apache/sshd/common/NamedFactory.java#L51-L57

should call isSupported().

I have no idea why the check might not be effective for some setups. I have been wondering, though, why everybody seems to use "soft requirements" with maven. In OSGi (e.g., the JGit MANIFEST.MFs) we always use hard requirements with strict version ranges.

It seems that even though we specify 1.78.1 as minimum it can at runtime still resolve to an earlier BC version?

There might also be something not quite right with our bnd setup to generate manifests. I see that org.apache.sshd:sshd-osgi:2.13.2 has for BC the version range "[1.78,2)" in its MANIFEST.MF. I would have expected that to be "[1.78.1,2)".

[tomaswolf]

Oh dear. This is wrong:

https://github.com/apache/mina-sshd/blob/4f2ccf885292adde1d3a0d5f9abd9fb513b07688/sshd-common/src/main/java/org/apache/sshd/common/NamedFactory.java#L54

That's not "ignore unsupported", it's "include unsupported"!

Gerrit calls it (quite naturally) with the value true here, but then it will never call isSupported()!

The filter should be ignoreUnsupported ? f.isSupported() : true , and it should be called in ClientBuilder and ServerBuilder with the value true!

Interestingly this bug has been in the code base since the method was introcued in 2015. :-(

[quic-nasserg]

Thanks for finding that! I wonder how many people make this kind of mistake with stream.filter(). I use it infrequently enough that I always double-check the javadoc to see if true means "keep" or "remove" :(

My eyes completely missed the BuiltinKEM.sntrup761.isSupported() call in the method I pointed out before. Sorry about that.

[tomaswolf]

I've created issue #582 for this.

NamedFactory.setupBuiltinFactories() has the same problem, and looking at the Gerrit code, especially lines 573ff, I can't help but feel that some Gerrit developer stumbled onto this before and included a work-around in Gerrit to fix the wrong behavior of this Apache MINA sshd function.