You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mina.apache.org by "tomaswolf (via GitHub)" <gi...@apache.org> on 2024/01/03 07:13:15 UTC
[PR] "Strict KEX" implementation [mina-sshd]
tomaswolf opened a new pull request, #449:
URL: https://github.com/apache/mina-sshd/pull/449
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
Re: [PR] "Strict KEX" implementation [mina-sshd]
Posted by "tomaswolf (via GitHub)" <gi...@apache.org>.
tomaswolf commented on code in PR #449:
URL: https://github.com/apache/mina-sshd/pull/449#discussion_r1442312630
##########
CHANGES.md:
##########
@@ -43,6 +44,15 @@ acknowledgements of a `receive` related command. The user is free to inspect the
to handle it - including even throwing an exception if OK status (if this makes sense for whatever reason). The default implementation checks for ERROR code and throws
an exception if so.
+### OpenSSH protocol extension: strict key exchange
+
+[GH-445](https://github.com/apache/mina-sshd/issues/445) implements an extension to the SSH protocol introduced
+in OpenSSH 9.6. This ["strict key exchange" extension](https://github.com/openssh/openssh-portable/blob/master/PROTOCOL)
+hardens the SSH key exchange against the ["Terrapin attack"](https://www.terrapin-attack.com/). The extension
+is active if both parties announce their support for it at the start of the initial key exchange. If only one
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
Re: [PR] "Strict KEX" implementation [mina-sshd]
Posted by "tomaswolf (via GitHub)" <gi...@apache.org>.
tomaswolf commented on PR #449:
URL: https://github.com/apache/mina-sshd/pull/449#issuecomment-1875006335
> 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. :-(
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
Re: [PR] "Strict KEX" implementation [mina-sshd]
Posted by "lgoldstein (via GitHub)" <gi...@apache.org>.
lgoldstein commented on PR #449:
URL: https://github.com/apache/mina-sshd/pull/449#issuecomment-1878810520
> 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](https://github.com/apache/mina-sshd/blob/master/docs/howto.md)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
Re: [PR] "Strict KEX" implementation [mina-sshd]
Posted by "tomaswolf (via GitHub)" <gi...@apache.org>.
tomaswolf commented on PR #449:
URL: https://github.com/apache/mina-sshd/pull/449#issuecomment-1878947313
> AFAIK we never removed a switch once there
My point exactly.
Documentation added.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
Re: [PR] "Strict KEX" implementation [mina-sshd]
Posted by "tomaswolf (via GitHub)" <gi...@apache.org>.
tomaswolf merged PR #449:
URL: https://github.com/apache/mina-sshd/pull/449
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
Re: [PR] "Strict KEX" implementation [mina-sshd]
Posted by "tomaswolf (via GitHub)" <gi...@apache.org>.
tomaswolf commented on code in PR #449:
URL: https://github.com/apache/mina-sshd/pull/449#discussion_r1442186669
##########
sshd-core/src/main/java/org/apache/sshd/common/session/helpers/AbstractSession.java:
##########
@@ -2520,8 +2614,34 @@ protected String resolveSessionKexProposal(String hostKeyTypes) throws IOExcepti
}
}
+ protected Map<KexProposalOption, String> doStrictKexProposal(Map<KexProposalOption, String> proposal) {
+ String value = proposal.get(KexProposalOption.ALGORITHMS);
+ String askForStrictKex = isServerSession()
+ ? KexExtensions.STRICT_KEX_SERVER_EXTENSION
+ : KexExtensions.STRICT_KEX_CLIENT_EXTENSION;
+ if (!initialKexDone) {
+ // On the initial KEX, include the strict KEX flag
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
Re: [PR] "Strict KEX" implementation [mina-sshd]
Posted by "tomaswolf (via GitHub)" <gi...@apache.org>.
tomaswolf commented on code in PR #449:
URL: https://github.com/apache/mina-sshd/pull/449#discussion_r1442235454
##########
sshd-core/src/main/java/org/apache/sshd/common/session/helpers/AbstractSession.java:
##########
@@ -2056,11 +2128,33 @@ protected Map<KexProposalOption, String> negotiate() throws Exception {
Map<KexProposalOption, String> s2cOptions = getServerKexProposals();
signalNegotiationStart(c2sOptions, s2cOptions);
+ // Make modifiable. Strict KEX flags are to be heeded only in initial KEX, and to be ignored afterwards.
+ c2sOptions = new EnumMap<>(c2sOptions);
+ s2cOptions = new EnumMap<>(s2cOptions);
+ boolean strictKexClient = removeValue(c2sOptions, KexProposalOption.ALGORITHMS,
+ KexExtensions.STRICT_KEX_CLIENT_EXTENSION);
+ boolean strictKexServer = removeValue(s2cOptions, KexProposalOption.ALGORITHMS,
+ KexExtensions.STRICT_KEX_SERVER_EXTENSION);
+ // Make unmodifiable again
+ c2sOptions = Collections.unmodifiableMap(c2sOptions);
+ s2cOptions = Collections.unmodifiableMap(s2cOptions);
Map<KexProposalOption, String> guess = new EnumMap<>(KexProposalOption.class);
Map<KexProposalOption, String> negotiatedGuess = Collections.unmodifiableMap(guess);
try {
boolean debugEnabled = log.isDebugEnabled();
boolean traceEnabled = log.isTraceEnabled();
+ if (!initialKexDone) {
+ strictKex = strictKexClient && strictKexServer;
+ if (debugEnabled) {
+ log.debug("negotiate({}) strict KEX={} client={} server={}", this, strictKex, strictKexClient,
+ strictKexServer);
+ }
+ if (strictKex && initialKexInitSequenceNumber != 1) {
+ throw new SshException(SshConstants.SSH2_DISCONNECT_KEY_EXCHANGE_FAILED,
+ "Strict KEX negotiated but sequence number of first KEX_INIT received is not 1: "
+ + initialKexInitSequenceNumber);
+ }
+ }
Review Comment:
Yes, ext-info-c/s is specified a bit differently. `negotiate()` does check for ext-info-c/s having been negotiated as kex algorithm, and then disconnects. But otherwise we take advantage of the "MAY" and just ignore a wrong ext-info flag.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
Re: [PR] "Strict KEX" implementation [mina-sshd]
Posted by "tomaswolf (via GitHub)" <gi...@apache.org>.
tomaswolf commented on PR #449:
URL: https://github.com/apache/mina-sshd/pull/449#issuecomment-1877851047
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
Re: [PR] "Strict KEX" implementation [mina-sshd]
Posted by "ecki (via GitHub)" <gi...@apache.org>.
ecki commented on code in PR #449:
URL: https://github.com/apache/mina-sshd/pull/449#discussion_r1442111543
##########
sshd-common/src/main/java/org/apache/sshd/common/kex/extension/KexExtensions.java:
##########
@@ -59,6 +60,24 @@ public final class KexExtensions {
public static final String CLIENT_KEX_EXTENSION = "ext-info-c";
public static final String SERVER_KEX_EXTENSION = "ext-info-s";
+ /**
+ * Reminder:
+ *
+ * These pseudo-algorithms are only valid in the initial SSH2_MSG_KEXINIT and MUST be ignored if they are present in
+ * subsequent SSH2_MSG_KEXINIT packets.
+ *
+ * <B>Note:</B> these values are <U>appended</U> to the initial proposals and removed if received before proceeding
+ * with the standard KEX proposals negotiation.
+ *
+ * @see <A HREF="https://github.com/openssh/openssh-portable/blob/master/PROTOCOL">OpenSSH PROTOCOL - 1.9 transport:
+ * strict key exchange extension</A>
+ */
+ public static final String STRICT_KEX_CLIENT_EXTENSION = "kex-strict-c-v00@openssh.com";
+ public static final String STRICT_KEX_SERVER_EXTENSION = "kex-strict-s-v00@openssh.com";
+ public static final List<String> STRICT_KEX_EXTENSIONS = Collections.unmodifiableList(
+ Arrays.asList(
+ STRICT_KEX_CLIENT_EXTENSION, STRICT_KEX_SERVER_EXTENSION));
+
@SuppressWarnings("checkstyle:Indentation")
public static final Predicate<String> IS_KEX_EXTENSION_SIGNAL
Review Comment:
It is a bit unsymmetrical with the rfc8308 extension (strict-kex has no such predicate and instead defines the list), but I guess it’s ok since both have a bit different scope.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
Re: [PR] "Strict KEX" implementation [mina-sshd]
Posted by "tomaswolf (via GitHub)" <gi...@apache.org>.
tomaswolf commented on code in PR #449:
URL: https://github.com/apache/mina-sshd/pull/449#discussion_r1442314912
##########
CHANGES.md:
##########
@@ -43,6 +44,15 @@ acknowledgements of a `receive` related command. The user is free to inspect the
to handle it - including even throwing an exception if OK status (if this makes sense for whatever reason). The default implementation checks for ERROR code and throws
an exception if so.
+### OpenSSH protocol extension: strict key exchange
+
+[GH-445](https://github.com/apache/mina-sshd/issues/445) implements an extension to the SSH protocol introduced
+in OpenSSH 9.6. This ["strict key exchange" extension](https://github.com/openssh/openssh-portable/blob/master/PROTOCOL)
+hardens the SSH key exchange against the ["Terrapin attack"](https://www.terrapin-attack.com/). The extension
+is active if both parties announce their support for it at the start of the initial key exchange. If only one
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
Re: [PR] "Strict KEX" implementation [mina-sshd]
Posted by "lgoldstein (via GitHub)" <gi...@apache.org>.
lgoldstein commented on PR #449:
URL: https://github.com/apache/mina-sshd/pull/449#issuecomment-1878950168
I am fine with this - as far as I am concerned you can merge.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
Re: [PR] "Strict KEX" implementation [mina-sshd]
Posted by "tomaswolf (via GitHub)" <gi...@apache.org>.
tomaswolf commented on PR #449:
URL: https://github.com/apache/mina-sshd/pull/449#issuecomment-1877686195
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
Re: [PR] "Strict KEX" implementation [mina-sshd]
Posted by "tomaswolf (via GitHub)" <gi...@apache.org>.
tomaswolf commented on code in PR #449:
URL: https://github.com/apache/mina-sshd/pull/449#discussion_r1442183563
##########
sshd-core/src/main/java/org/apache/sshd/common/session/helpers/AbstractSession.java:
##########
@@ -2056,11 +2128,33 @@ protected Map<KexProposalOption, String> negotiate() throws Exception {
Map<KexProposalOption, String> s2cOptions = getServerKexProposals();
signalNegotiationStart(c2sOptions, s2cOptions);
+ // Make modifiable. Strict KEX flags are to be heeded only in initial KEX, and to be ignored afterwards.
+ c2sOptions = new EnumMap<>(c2sOptions);
+ s2cOptions = new EnumMap<>(s2cOptions);
+ boolean strictKexClient = removeValue(c2sOptions, KexProposalOption.ALGORITHMS,
+ KexExtensions.STRICT_KEX_CLIENT_EXTENSION);
+ boolean strictKexServer = removeValue(s2cOptions, KexProposalOption.ALGORITHMS,
+ KexExtensions.STRICT_KEX_SERVER_EXTENSION);
+ // Make unmodifiable again
+ c2sOptions = Collections.unmodifiableMap(c2sOptions);
+ s2cOptions = Collections.unmodifiableMap(s2cOptions);
Map<KexProposalOption, String> guess = new EnumMap<>(KexProposalOption.class);
Map<KexProposalOption, String> negotiatedGuess = Collections.unmodifiableMap(guess);
try {
boolean debugEnabled = log.isDebugEnabled();
boolean traceEnabled = log.isTraceEnabled();
+ if (!initialKexDone) {
+ strictKex = strictKexClient && strictKexServer;
+ if (debugEnabled) {
+ log.debug("negotiate({}) strict KEX={} client={} server={}", this, strictKex, strictKexClient,
+ strictKexServer);
+ }
+ if (strictKex && initialKexInitSequenceNumber != 1) {
+ throw new SshException(SshConstants.SSH2_DISCONNECT_KEY_EXCHANGE_FAILED,
+ "Strict KEX negotiated but sequence number of first KEX_INIT received is not 1: "
+ + initialKexInitSequenceNumber);
+ }
+ }
Review Comment:
You mean, a client sending the server flag or vice versa?
You're right, we must remove those, too, otherwise we might end up negotiating it as a kex algorithm :-)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
Re: [PR] "Strict KEX" implementation [mina-sshd]
Posted by "jonas19 (via GitHub)" <gi...@apache.org>.
jonas19 commented on PR #449:
URL: https://github.com/apache/mina-sshd/pull/449#issuecomment-1881599205
Hi team!
@tomaswolf may I ask what is the ETA for releasing this implementation?
Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
Re: [PR] "Strict KEX" implementation [mina-sshd]
Posted by "lgoldstein (via GitHub)" <gi...@apache.org>.
lgoldstein commented on PR #449:
URL: https://github.com/apache/mina-sshd/pull/449#issuecomment-1877567269
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...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
Re: [PR] "Strict KEX" implementation [mina-sshd]
Posted by "tomaswolf (via GitHub)" <gi...@apache.org>.
tomaswolf commented on code in PR #449:
URL: https://github.com/apache/mina-sshd/pull/449#discussion_r1442196805
##########
CHANGES.md:
##########
@@ -43,6 +44,15 @@ acknowledgements of a `receive` related command. The user is free to inspect the
to handle it - including even throwing an exception if OK status (if this makes sense for whatever reason). The default implementation checks for ERROR code and throws
an exception if so.
+### OpenSSH protocol extension: strict key exchange
+
+[GH-445](https://github.com/apache/mina-sshd/issues/445) implements an extension to the SSH protocol introduced
+in OpenSSH 9.6. This ["strict key exchange" extension](https://github.com/openssh/openssh-portable/blob/master/PROTOCOL)
+hardens the SSH key exchange against the ["Terrapin attack"](https://www.terrapin-attack.com/). The extension
+is active if both parties announce their support for it at the start of the initial key exchange. If only one
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
Re: [PR] "Strict KEX" implementation [mina-sshd]
Posted by "ecki (via GitHub)" <gi...@apache.org>.
ecki commented on code in PR #449:
URL: https://github.com/apache/mina-sshd/pull/449#discussion_r1442209864
##########
CHANGES.md:
##########
@@ -43,6 +44,15 @@ acknowledgements of a `receive` related command. The user is free to inspect the
to handle it - including even throwing an exception if OK status (if this makes sense for whatever reason). The default implementation checks for ERROR code and throws
an exception if so.
+### OpenSSH protocol extension: strict key exchange
+
+[GH-445](https://github.com/apache/mina-sshd/issues/445) implements an extension to the SSH protocol introduced
+in OpenSSH 9.6. This ["strict key exchange" extension](https://github.com/openssh/openssh-portable/blob/master/PROTOCOL)
+hardens the SSH key exchange against the ["Terrapin attack"](https://www.terrapin-attack.com/). The extension
+is active if both parties announce their support for it at the start of the initial key exchange. If only one
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
Re: [PR] "Strict KEX" implementation [mina-sshd]
Posted by "ecki (via GitHub)" <gi...@apache.org>.
ecki commented on code in PR #449:
URL: https://github.com/apache/mina-sshd/pull/449#discussion_r1442111543
##########
sshd-common/src/main/java/org/apache/sshd/common/kex/extension/KexExtensions.java:
##########
@@ -59,6 +60,24 @@ public final class KexExtensions {
public static final String CLIENT_KEX_EXTENSION = "ext-info-c";
public static final String SERVER_KEX_EXTENSION = "ext-info-s";
+ /**
+ * Reminder:
+ *
+ * These pseudo-algorithms are only valid in the initial SSH2_MSG_KEXINIT and MUST be ignored if they are present in
+ * subsequent SSH2_MSG_KEXINIT packets.
+ *
+ * <B>Note:</B> these values are <U>appended</U> to the initial proposals and removed if received before proceeding
+ * with the standard KEX proposals negotiation.
+ *
+ * @see <A HREF="https://github.com/openssh/openssh-portable/blob/master/PROTOCOL">OpenSSH PROTOCOL - 1.9 transport:
+ * strict key exchange extension</A>
+ */
+ public static final String STRICT_KEX_CLIENT_EXTENSION = "kex-strict-c-v00@openssh.com";
+ public static final String STRICT_KEX_SERVER_EXTENSION = "kex-strict-s-v00@openssh.com";
+ public static final List<String> STRICT_KEX_EXTENSIONS = Collections.unmodifiableList(
+ Arrays.asList(
+ STRICT_KEX_CLIENT_EXTENSION, STRICT_KEX_SERVER_EXTENSION));
+
@SuppressWarnings("checkstyle:Indentation")
public static final Predicate<String> IS_KEX_EXTENSION_SIGNAL
Review Comment:
It is a bit unsymmetrical with the rfc3808 extension (strict-kex has no such predicate and instead defines the list), but I guess it’s ok since both have a bit different scope.
##########
sshd-core/src/main/java/org/apache/sshd/common/session/helpers/AbstractSession.java:
##########
@@ -2056,11 +2128,33 @@ protected Map<KexProposalOption, String> negotiate() throws Exception {
Map<KexProposalOption, String> s2cOptions = getServerKexProposals();
signalNegotiationStart(c2sOptions, s2cOptions);
+ // Make modifiable. Strict KEX flags are to be heeded only in initial KEX, and to be ignored afterwards.
+ c2sOptions = new EnumMap<>(c2sOptions);
+ s2cOptions = new EnumMap<>(s2cOptions);
+ boolean strictKexClient = removeValue(c2sOptions, KexProposalOption.ALGORITHMS,
+ KexExtensions.STRICT_KEX_CLIENT_EXTENSION);
+ boolean strictKexServer = removeValue(s2cOptions, KexProposalOption.ALGORITHMS,
+ KexExtensions.STRICT_KEX_SERVER_EXTENSION);
+ // Make unmodifiable again
+ c2sOptions = Collections.unmodifiableMap(c2sOptions);
+ s2cOptions = Collections.unmodifiableMap(s2cOptions);
Map<KexProposalOption, String> guess = new EnumMap<>(KexProposalOption.class);
Map<KexProposalOption, String> negotiatedGuess = Collections.unmodifiableMap(guess);
try {
boolean debugEnabled = log.isDebugEnabled();
boolean traceEnabled = log.isTraceEnabled();
+ if (!initialKexDone) {
+ strictKex = strictKexClient && strictKexServer;
+ if (debugEnabled) {
+ log.debug("negotiate({}) strict KEX={} client={} server={}", this, strictKex, strictKexClient,
+ strictKexServer);
+ }
+ if (strictKex && initialKexInitSequenceNumber != 1) {
+ throw new SshException(SshConstants.SSH2_DISCONNECT_KEY_EXCHANGE_FAILED,
+ "Strict KEX negotiated but sequence number of first KEX_INIT received is not 1: "
+ + initialKexInitSequenceNumber);
+ }
+ }
Review Comment:
Not handling the case of a wrong client or server type extension? (It is correctly handles so it could be ignored or logged. Aborting would be extra paranoid.
##########
CHANGES.md:
##########
@@ -43,6 +44,15 @@ acknowledgements of a `receive` related command. The user is free to inspect the
to handle it - including even throwing an exception if OK status (if this makes sense for whatever reason). The default implementation checks for ERROR code and throws
an exception if so.
+### OpenSSH protocol extension: strict key exchange
+
+[GH-445](https://github.com/apache/mina-sshd/issues/445) implements an extension to the SSH protocol introduced
+in OpenSSH 9.6. This ["strict key exchange" extension](https://github.com/openssh/openssh-portable/blob/master/PROTOCOL)
+hardens the SSH key exchange against the ["Terrapin attack"](https://www.terrapin-attack.com/). The extension
+is active if both parties announce their support for it at the start of the initial key exchange. If only one
Review Comment:
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).
##########
sshd-core/src/main/java/org/apache/sshd/common/session/helpers/AbstractSession.java:
##########
@@ -2520,8 +2614,34 @@ protected String resolveSessionKexProposal(String hostKeyTypes) throws IOExcepti
}
}
+ protected Map<KexProposalOption, String> doStrictKexProposal(Map<KexProposalOption, String> proposal) {
+ String value = proposal.get(KexProposalOption.ALGORITHMS);
+ String askForStrictKex = isServerSession()
+ ? KexExtensions.STRICT_KEX_SERVER_EXTENSION
+ : KexExtensions.STRICT_KEX_CLIENT_EXTENSION;
+ if (!initialKexDone) {
+ // On the initial KEX, include the strict KEX flag
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
Re: [PR] "Strict KEX" implementation [mina-sshd]
Posted by "ecki (via GitHub)" <gi...@apache.org>.
ecki commented on code in PR #449:
URL: https://github.com/apache/mina-sshd/pull/449#discussion_r1442207857
##########
sshd-core/src/main/java/org/apache/sshd/common/session/helpers/AbstractSession.java:
##########
@@ -2056,11 +2128,33 @@ protected Map<KexProposalOption, String> negotiate() throws Exception {
Map<KexProposalOption, String> s2cOptions = getServerKexProposals();
signalNegotiationStart(c2sOptions, s2cOptions);
+ // Make modifiable. Strict KEX flags are to be heeded only in initial KEX, and to be ignored afterwards.
+ c2sOptions = new EnumMap<>(c2sOptions);
+ s2cOptions = new EnumMap<>(s2cOptions);
+ boolean strictKexClient = removeValue(c2sOptions, KexProposalOption.ALGORITHMS,
+ KexExtensions.STRICT_KEX_CLIENT_EXTENSION);
+ boolean strictKexServer = removeValue(s2cOptions, KexProposalOption.ALGORITHMS,
+ KexExtensions.STRICT_KEX_SERVER_EXTENSION);
+ // Make unmodifiable again
+ c2sOptions = Collections.unmodifiableMap(c2sOptions);
+ s2cOptions = Collections.unmodifiableMap(s2cOptions);
Map<KexProposalOption, String> guess = new EnumMap<>(KexProposalOption.class);
Map<KexProposalOption, String> negotiatedGuess = Collections.unmodifiableMap(guess);
try {
boolean debugEnabled = log.isDebugEnabled();
boolean traceEnabled = log.isTraceEnabled();
+ if (!initialKexDone) {
+ strictKex = strictKexClient && strictKexServer;
+ if (debugEnabled) {
+ log.debug("negotiate({}) strict KEX={} client={} server={}", this, strictKex, strictKexClient,
+ strictKexServer);
+ }
+ if (strictKex && initialKexInitSequenceNumber != 1) {
+ throw new SshException(SshConstants.SSH2_DISCONNECT_KEY_EXCHANGE_FAILED,
+ "Strict KEX negotiated but sequence number of first KEX_INIT received is not 1: "
+ + initialKexInitSequenceNumber);
+ }
+ }
Review Comment:
Yeah right,that what’s the additional list was used for. I think it’s not critical to not remove it (since you filter your own side) but it’s probably better to handle the case (with a comment). The rfc8308 verbage (for ext-info-*) would allow to disconnect or ignore for malicious/illegal offers:
> Implementations MUST NOT send an incorrect indicator name for their
role. Implementations MAY disconnect if the counterparty sends an
incorrect indicator. If "ext-info-c" or "ext-info-s" ends up being
negotiated as a key exchange method, the parties MUST disconnect.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
Re: [PR] "Strict KEX" implementation [mina-sshd]
Posted by "tomaswolf (via GitHub)" <gi...@apache.org>.
tomaswolf commented on code in PR #449:
URL: https://github.com/apache/mina-sshd/pull/449#discussion_r1442294794
##########
sshd-core/src/main/java/org/apache/sshd/common/session/helpers/AbstractSession.java:
##########
@@ -2056,11 +2128,33 @@ protected Map<KexProposalOption, String> negotiate() throws Exception {
Map<KexProposalOption, String> s2cOptions = getServerKexProposals();
signalNegotiationStart(c2sOptions, s2cOptions);
+ // Make modifiable. Strict KEX flags are to be heeded only in initial KEX, and to be ignored afterwards.
+ c2sOptions = new EnumMap<>(c2sOptions);
+ s2cOptions = new EnumMap<>(s2cOptions);
+ boolean strictKexClient = removeValue(c2sOptions, KexProposalOption.ALGORITHMS,
+ KexExtensions.STRICT_KEX_CLIENT_EXTENSION);
+ boolean strictKexServer = removeValue(s2cOptions, KexProposalOption.ALGORITHMS,
+ KexExtensions.STRICT_KEX_SERVER_EXTENSION);
+ // Make unmodifiable again
+ c2sOptions = Collections.unmodifiableMap(c2sOptions);
+ s2cOptions = Collections.unmodifiableMap(s2cOptions);
Map<KexProposalOption, String> guess = new EnumMap<>(KexProposalOption.class);
Map<KexProposalOption, String> negotiatedGuess = Collections.unmodifiableMap(guess);
try {
boolean debugEnabled = log.isDebugEnabled();
boolean traceEnabled = log.isTraceEnabled();
+ if (!initialKexDone) {
+ strictKex = strictKexClient && strictKexServer;
+ if (debugEnabled) {
+ log.debug("negotiate({}) strict KEX={} client={} server={}", this, strictKex, strictKexClient,
+ strictKexServer);
+ }
+ if (strictKex && initialKexInitSequenceNumber != 1) {
+ throw new SshException(SshConstants.SSH2_DISCONNECT_KEY_EXCHANGE_FAILED,
+ "Strict KEX negotiated but sequence number of first KEX_INIT received is not 1: "
+ + initialKexInitSequenceNumber);
+ }
+ }
Review Comment:
Done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
Re: [PR] "Strict KEX" implementation [mina-sshd]
Posted by "tomaswolf (via GitHub)" <gi...@apache.org>.
tomaswolf commented on code in PR #449:
URL: https://github.com/apache/mina-sshd/pull/449#discussion_r1442294901
##########
sshd-common/src/main/java/org/apache/sshd/common/kex/extension/KexExtensions.java:
##########
@@ -59,6 +60,24 @@ public final class KexExtensions {
public static final String CLIENT_KEX_EXTENSION = "ext-info-c";
public static final String SERVER_KEX_EXTENSION = "ext-info-s";
+ /**
+ * Reminder:
+ *
+ * These pseudo-algorithms are only valid in the initial SSH2_MSG_KEXINIT and MUST be ignored if they are present in
+ * subsequent SSH2_MSG_KEXINIT packets.
+ *
+ * <B>Note:</B> these values are <U>appended</U> to the initial proposals and removed if received before proceeding
+ * with the standard KEX proposals negotiation.
+ *
+ * @see <A HREF="https://github.com/openssh/openssh-portable/blob/master/PROTOCOL">OpenSSH PROTOCOL - 1.9 transport:
+ * strict key exchange extension</A>
+ */
+ public static final String STRICT_KEX_CLIENT_EXTENSION = "kex-strict-c-v00@openssh.com";
+ public static final String STRICT_KEX_SERVER_EXTENSION = "kex-strict-s-v00@openssh.com";
+ public static final List<String> STRICT_KEX_EXTENSIONS = Collections.unmodifiableList(
+ Arrays.asList(
+ STRICT_KEX_CLIENT_EXTENSION, STRICT_KEX_SERVER_EXTENSION));
+
@SuppressWarnings("checkstyle:Indentation")
public static final Predicate<String> IS_KEX_EXTENSION_SIGNAL
Review Comment:
Done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
Re: [PR] "Strict KEX" implementation [mina-sshd]
Posted by "tomaswolf (via GitHub)" <gi...@apache.org>.
tomaswolf commented on PR #449:
URL: https://github.com/apache/mina-sshd/pull/449#issuecomment-1877520579
> > 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
Re: [PR] "Strict KEX" implementation [mina-sshd]
Posted by "tomaswolf (via GitHub)" <gi...@apache.org>.
tomaswolf commented on code in PR #449:
URL: https://github.com/apache/mina-sshd/pull/449#discussion_r1442182483
##########
sshd-common/src/main/java/org/apache/sshd/common/kex/extension/KexExtensions.java:
##########
@@ -59,6 +60,24 @@ public final class KexExtensions {
public static final String CLIENT_KEX_EXTENSION = "ext-info-c";
public static final String SERVER_KEX_EXTENSION = "ext-info-s";
+ /**
+ * Reminder:
+ *
+ * These pseudo-algorithms are only valid in the initial SSH2_MSG_KEXINIT and MUST be ignored if they are present in
+ * subsequent SSH2_MSG_KEXINIT packets.
+ *
+ * <B>Note:</B> these values are <U>appended</U> to the initial proposals and removed if received before proceeding
+ * with the standard KEX proposals negotiation.
+ *
+ * @see <A HREF="https://github.com/openssh/openssh-portable/blob/master/PROTOCOL">OpenSSH PROTOCOL - 1.9 transport:
+ * strict key exchange extension</A>
+ */
+ public static final String STRICT_KEX_CLIENT_EXTENSION = "kex-strict-c-v00@openssh.com";
+ public static final String STRICT_KEX_SERVER_EXTENSION = "kex-strict-s-v00@openssh.com";
+ public static final List<String> STRICT_KEX_EXTENSIONS = Collections.unmodifiableList(
+ Arrays.asList(
+ STRICT_KEX_CLIENT_EXTENSION, STRICT_KEX_SERVER_EXTENSION));
+
@SuppressWarnings("checkstyle:Indentation")
public static final Predicate<String> IS_KEX_EXTENSION_SIGNAL
Review Comment:
It's not even used. Copy/paste from the original PR #446. Should be removed here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
Re: [PR] "Strict KEX" implementation [mina-sshd]
Posted by "tomaswolf (via GitHub)" <gi...@apache.org>.
tomaswolf commented on code in PR #449:
URL: https://github.com/apache/mina-sshd/pull/449#discussion_r1442183563
##########
sshd-core/src/main/java/org/apache/sshd/common/session/helpers/AbstractSession.java:
##########
@@ -2056,11 +2128,33 @@ protected Map<KexProposalOption, String> negotiate() throws Exception {
Map<KexProposalOption, String> s2cOptions = getServerKexProposals();
signalNegotiationStart(c2sOptions, s2cOptions);
+ // Make modifiable. Strict KEX flags are to be heeded only in initial KEX, and to be ignored afterwards.
+ c2sOptions = new EnumMap<>(c2sOptions);
+ s2cOptions = new EnumMap<>(s2cOptions);
+ boolean strictKexClient = removeValue(c2sOptions, KexProposalOption.ALGORITHMS,
+ KexExtensions.STRICT_KEX_CLIENT_EXTENSION);
+ boolean strictKexServer = removeValue(s2cOptions, KexProposalOption.ALGORITHMS,
+ KexExtensions.STRICT_KEX_SERVER_EXTENSION);
+ // Make unmodifiable again
+ c2sOptions = Collections.unmodifiableMap(c2sOptions);
+ s2cOptions = Collections.unmodifiableMap(s2cOptions);
Map<KexProposalOption, String> guess = new EnumMap<>(KexProposalOption.class);
Map<KexProposalOption, String> negotiatedGuess = Collections.unmodifiableMap(guess);
try {
boolean debugEnabled = log.isDebugEnabled();
boolean traceEnabled = log.isTraceEnabled();
+ if (!initialKexDone) {
+ strictKex = strictKexClient && strictKexServer;
+ if (debugEnabled) {
+ log.debug("negotiate({}) strict KEX={} client={} server={}", this, strictKex, strictKexClient,
+ strictKexServer);
+ }
+ if (strictKex && initialKexInitSequenceNumber != 1) {
+ throw new SshException(SshConstants.SSH2_DISCONNECT_KEY_EXCHANGE_FAILED,
+ "Strict KEX negotiated but sequence number of first KEX_INIT received is not 1: "
+ + initialKexInitSequenceNumber);
+ }
+ }
Review Comment:
You mean, a client sending the server flag or vice versa?
<s>You're right, we must remove those, too, otherwise we might end up negotiating it as a kex algorithm :-)</s>
Actually, no. In the worst case, the client proposal will be left with the server flag, and the server proposal with the client flag. They'll just be ignored, and will never be negotiated as kex algorithm. But it seems safer to remove and log them anyway.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
Re: [PR] "Strict KEX" implementation [mina-sshd]
Posted by "ecki (via GitHub)" <gi...@apache.org>.
ecki commented on code in PR #449:
URL: https://github.com/apache/mina-sshd/pull/449#discussion_r1442204693
##########
sshd-core/src/main/java/org/apache/sshd/common/session/helpers/AbstractSession.java:
##########
@@ -2520,8 +2614,34 @@ protected String resolveSessionKexProposal(String hostKeyTypes) throws IOExcepti
}
}
+ protected Map<KexProposalOption, String> doStrictKexProposal(Map<KexProposalOption, String> proposal) {
+ String value = proposal.get(KexProposalOption.ALGORITHMS);
+ String askForStrictKex = isServerSession()
+ ? KexExtensions.STRICT_KEX_SERVER_EXTENSION
+ : KexExtensions.STRICT_KEX_CLIENT_EXTENSION;
+ if (!initialKexDone) {
+ // On the initial KEX, include the strict KEX flag
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
Re: [PR] "Strict KEX" implementation [mina-sshd]
Posted by "tomaswolf (via GitHub)" <gi...@apache.org>.
tomaswolf commented on code in PR #449:
URL: https://github.com/apache/mina-sshd/pull/449#discussion_r1442233272
##########
sshd-core/src/main/java/org/apache/sshd/common/session/helpers/AbstractSession.java:
##########
@@ -2520,8 +2614,34 @@ protected String resolveSessionKexProposal(String hostKeyTypes) throws IOExcepti
}
}
+ protected Map<KexProposalOption, String> doStrictKexProposal(Map<KexProposalOption, String> proposal) {
+ String value = proposal.get(KexProposalOption.ALGORITHMS);
+ String askForStrictKex = isServerSession()
+ ? KexExtensions.STRICT_KEX_SERVER_EXTENSION
+ : KexExtensions.STRICT_KEX_CLIENT_EXTENSION;
+ if (!initialKexDone) {
+ // On the initial KEX, include the strict KEX flag
Review Comment:
It'll still be the first message the client sends. It'll just be sent only after having received the server's KEX_INIT.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org