[email protected] KEX causing "incorrect signature" with OpenSSH client

[lukegb]

Version

2.13.1

Bug description

Using Mina SSHd inside Gerrit, if [email protected] 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 [email protected] 
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 [email protected] and not using [email protected] 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?

mina-sshd/sshd-core/src/main/java/org/apache/sshd/common/kex/SNTRUP761.java

Line 44 in 04ae57b

static boolean isSupported() {

[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

mina-sshd/sshd-core/src/main/java/org/apache/sshd/common/kex/BuiltinDHFactories.java

Line 340 in 4f2ccf8

public boolean isSupported() {

doesn't call that method?

[tomaswolf]

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

doesn't call that method?

mina-sshd/sshd-core/src/main/java/org/apache/sshd/common/kex/BuiltinDHFactories.java

Lines 340 to 343 in 4f2ccf8

	public boolean isSupported() {
	return MontgomeryCurve.x25519.isSupported() && BuiltinDigests.sha512.isSupported()
	&& BuiltinKEM.sntrup761.isSupported();
	}

calls BuiltinKEM.sntrup761.isSupported():

mina-sshd/sshd-core/src/main/java/org/apache/sshd/common/kex/BuiltinKEM.java

Lines 41 to 44 in 4f2ccf8

	@Override
	public boolean isSupported() {
	return SNTRUP761.isSupported();
	}

which calls SNTRUP761.isSupported():

mina-sshd/sshd-core/src/main/java/org/apache/sshd/common/kex/SNTRUP761.java

Lines 44 to 53 in 4f2ccf8

	static boolean isSupported() {
	if (!SecurityUtils.isBouncyCastleRegistered()) {
	return false;
	}
	try {
	return SNTRUPrimeParameters.sntrup761.getSessionKeySize() == 256; // BC < 1.78 had only 128
	} catch (Throwable e) {
	return false;
	}
	}

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

mina-sshd/sshd-core/src/main/java/org/apache/sshd/server/ServerBuilder.java

Lines 177 to 179 in 4f2ccf8

	public static List<KeyExchangeFactory> setUpDefaultKeyExchanges(boolean ignoreUnsupported) {
	return NamedFactory.setUpTransformedFactories(ignoreUnsupported, DEFAULT_KEX_PREFERENCE, DH2KEX);
	}

which gets called at

mina-sshd/sshd-core/src/main/java/org/apache/sshd/server/ServerBuilder.java

Line 119 in 4f2ccf8

keyExchangeFactories = setUpDefaultKeyExchanges(false);

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

mina-sshd/sshd-common/src/main/java/org/apache/sshd/common/NamedFactory.java

Lines 51 to 57 in 4f2ccf8

	static <S extends OptionalFeature, E extends NamedResource> List<E> setUpTransformedFactories(
	boolean ignoreUnsupported, Collection<? extends S> preferred, Function<? super S, ? extends E> xform) {
	return preferred.stream()
	.filter(f -> ignoreUnsupported || f.isSupported())
	.map(xform)
	.collect(Collectors.toList());
	}

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:

mina-sshd/sshd-common/src/main/java/org/apache/sshd/common/NamedFactory.java

Line 54 in 4f2ccf8

.filter(f -> ignoreUnsupported || f.isSupported())

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.