"Strict KEX" implementation

[tomaswolf]

Alternate proposal to #446 for implementing "strict KEX". This is a minimal single-purpose PR for this feature.

• One commit implementing the functionality in AbstractSession.
• One commit adding unit tests to verify sessions are disconnected on spurious messages.
• One commit adding container tests for interoperability testing.

The implementation in AbstractSession is a bit leaner than the original proposal because it makes use of the already existing initialKexDone flag, doesn't use atomic variables where not necessary, and has no optional settings to bother with. But most importantly the issue is not clouded by unrelated changes or spurious reformatting.

There are no configuration settings to disable the "strict KEX" implementation. "Strict KEX" is a hardening of a core SSH protocol; I will not provide knobs to dumb down and make that protocol implementation less secure. If someone absolutely wants to disable this, he or she can subclass the session (for instance ClientSessionImpl) and override doStrictKexProposal() to not do anything.

The unit tests in StrictKexTest work with all transport back-ends (NIO2, Netty, and Mina).

The container tests test an Apache MINA sshd client against an OpenSSH server, using Alpine 20231219/OpenSSH 9.6 for a strick-kex-enabled server, and CentOS 7/OpenSSH 7.4 for one that doesn't have strict KEX. These tests explicitly ensure that communication and re-KEX work with or without "strict KEX".

Note that other container tests in the overall test suite already would fail if strict KEX was implemented wrongly, but I prefer having explicit tests for this.

There are no container tests using an OpenSSH client and an Apache MINS sshd server. Besides being somewhat harder to implement, they wouldn't add much value. The KEX sub-protocol is symmetric and is wholly implemented in AbstractSession, which is the same for client and server.

Fixes #445.

[tomaswolf]

The unit tests in StrictKexTest work with all transport back-ends (NIO2, Netty, and Mina).

On Linux/MacOS. Somehow not in CI on Windows. StrictKexTest consistently fails on Windows/Java 8, and is unstable with Java 11 or 17 on Windows. :-(

[tomaswolf]

The unit tests in StrictKexTest work with all transport back-ends (NIO2, Netty, and Mina).

On Linux/MacOS. Somehow not in CI on Windows. StrictKexTest consistently fails on Windows/Java 8, and is unstable with Java 11 or 17 on Windows. :-(

The test failure on Windows was due to an unrelated bug that exists at least in the NIO2 transport back-end: when a session is terminated due to an exception, the SSH session layer sends a DISCONNECT message and is careful to close the ServerSession only once that message has been sent. But the underlying Nio2Session layer then closes itself immediately all the same. This gives a race -- sometimes, the write completes earlier, but sometimes it doesn't, and then the DISCONNECT message never goes out. Then the client doesn't get the expected disconnection reason code.

That minor bug is to fixed in some follow-up change; I don't want to do it here in this PR. (Fixing it properly might not be trivial.) The connection is closed in either case, so I prefer to have less strict assertions in the test instead.

This PR is ready for review.

[lgoldstein]

Looks OK to me (have not gone over the tests in details - I trust you checked them). I do feel I need to insist on a CoreModuleProperties property that controls this feature. As I have said previously, I am fine with its default being "enabled", but I strongly feel we should have it...

[ecki]

The implementation looks good. I miss a „require“ option, but that’s not a showstopper for me. Exposing the strictkex state to a listener and some counter statistics might be a Seperate topic.

[ecki]

Servers could include it only if client requested it, this might be good for interop (but it also might be less secure, not sure?). Maybe add a comment documenting the decision.

[tomaswolf]

There is nothing in the SSH protocol that says the client had to send its proposal first. Apache MINA sshd even has a config flag to delay sending the client's KEX_INIT only once the server's has been received. Apparently that is needed to get it to work with some servers out there (don't know what SSH implementation).

A peer that doesn't know about this extension will just treat it as a kex algorithm, but since its own proposal will not have it, it'll never end up being negotiated as a kex algorithm and thus be ignored by that peer.

[ecki]

Hm not sure how you can delay kex_init under strictKex, after all the peer will reject sequence != 1, but yes it’s just a minor point, maybe even unnecessary complexity.

[tomaswolf]

It'll still be the first message the client sends. It'll just be sent only after having received the server's KEX_INIT.

[ecki]

As mentioned, I think it will help admins if they can set a flag to disconnect connections if they do not negotiate strict-kex. This makes is much easier to re-enable the vulnerable (but modern) ciphers. Maybe it’s an option for later, but why not offer it from the get-go. If not offered maybe a sample listener who does it would be good (validating the info is exposed to make that call by the app).

nb: I am not saying make sending of the offer configurable, it’s perfectly fine with me to do that unconditional (even though some other impls offer this option).

[tomaswolf]

A "RequireStrictKex" config for ~/.ssh/config (default "no") may make sense. The proposal has come up before, for instance on the SSH mailing list.

It could be added in a follow-up change. My main motivation for this PR was to have a minimal change with sufficient tests so that we can be confident it works, and that it works also when talking to peers that don't have strict kex at all.

A problem with custom configs is that they cause OpenSSH to complain and fail unless the user also adds an IgnoreUnknown RequireStrictKex to the config file. But for this option you probably don't want to ignore it. I didn't see such a config option in OpenSSH. So I think we should add such an option only if OpenSSH has it.

[ecki]

Ok, I am not sure who would actually use Mina-sshd for such things, but yes a common config option would be great. I didn’t receive any reaction to my OpenSSH-dev question, though.

Jsch has a enable (aka offer/support) and require option.

[tomaswolf]

I'm aware of the option in JSch 0.2.15. A CoreModuleProporties property for requiring strict kex and failing the connection if strict kex is not negotiated in negotiate() could be considered, and could easily be added in a follow-up commit. Or we could add it to the negotiation result in a follow-up commit; then a SessionListener.sessionNegotiationEnd() could be used to close the connection by throwing an exception.

To be really useful one would need the ~/.ssh/config option, though. And as I wrote, I believe that should be done only if OpenSSH gets it, too.

In any case, IMO this is something for follow-up changes.

[tomaswolf]

About the "offer" option: see my comment about a "kill switch" below. Users can already switch strict kex off if they absolutely want to do so. But it should require a bit more conscious thought and effort than just flipping a boolean flag.

[tomaswolf]

The implementation looks good. I miss a „require“ option, but that’s not a showstopper for me. Exposing the strictkex state to a listener and some counter statistics might be a Seperate topic.

Thanks for the review, I appreciate it. Indeed, exposing more internal state can be done in follow-up changes. This is just about the basic protocol extension.

[tomaswolf]

Looks OK to me (have not gone over the tests in details - I trust you checked them). I do feel I need to insist on a CoreModuleProperties property that controls this feature. As I have said previously, I am fine with its default being "enabled", but I strongly feel we should have it...

I think we should minimize "kill switches". They obfuscate the issue permanently (removing the property in the future would be an API-breaking change). In this particular case:

• This is a fairly important modification of a core sub-protocol in the SSH protocols.
• It is backwards compatible with older implementations that don't know about it.
• It is tested.
• If we did it wrong and people can't use it because it breaks something, they'll just stay on 2.11.0 and we'll have to have a 2.12.1 with a fix.

Finally, if someone really wants to shut off strict KEX usage, here's one way of doing so:

SshClient client = ...;
SessionFactory factory = new NoStrictKexSessionFactory(client);
client.setSessionFactory(factory);
client.start();

and

class NoStrictKexSessionFactory extends SessionFactory {
    NoStrictKexSessionFactory(ClientFactoryManager client) {
        super(client);
    }

    @Override
    protected ClientSessionImpl doCreateSession(IoSession ioSession) throws Exception {
        return new NoStrictKexSession(getClient(), ioSession);
    }
}

class NoStrictKexSession extends ClientSessionImpl {

    NoStrictKexSession(ClientFactoryManager client, IoSession ioSession) throws Exception {
        super(client, ioSession);
    }

    @Override
    protected Map<KexProposalOption, String> doStrictKexProposal(Map<KexProposalOption, String> proposal) {
        return proposal;
    }
}

(Or likewise for a ServerSession.)

Yes, it's a little bit of code to write, but it's IMO about the right level of difficulty for switching off a security feature. I don't want to make it too easy for users to shoot themselves in the foot by inadvertently switching strict kex off. And yes, a boolean property in CoreModuleProperties fits my definition of "too easy" in this case.

[lgoldstein]

I think we should minimize "kill switches". They obfuscate the issue permanently (removing the property in the future would be an API-breaking change).

AFAIK we never removed a switch once there, but we can defer this to a future change (see also below)

Finally, if someone really wants to shut off strict KEX usage, here's one way of doing so:

Let's document this - I recommend adding a section in the HOTOW(s) document

[tomaswolf]

AFAIK we never removed a switch once there

My point exactly.

Documentation added.

[lgoldstein]

I am fine with this - as far as I am concerned you can merge.

[jonas19]

Hi team!
@tomaswolf may I ask what is the ETA for releasing this implementation?
Thanks!