Skip to content

Commit 8354338

Browse files
author
Lyor Goldstein
committed
[apacheGH-445] Added StrictKexTest
1 parent 1c11e3a commit 8354338

File tree

16 files changed

+677
-28
lines changed

16 files changed

+677
-28
lines changed

CHANGES.md

+12-1
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@
3838

3939
### [GH-445 - Terrapin attack mitigation](https://github.com/apache/mina-sshd/issues/429)
4040

41-
There is a **new** `CoreModuleProperties` property that controls the mitigation for the [Terrapin attach](https://terrapin-attack.com/) via what is known as
41+
There is a **new** `CoreModuleProperties` property that controls the mitigation for the [Terrapin attack](https://terrapin-attack.com/) via what is known as
4242
"strict-KEX" (see [OpenSSH PROTOCOL - 1.9 transport: strict key exchange extension](https://github.com/openssh/openssh-portable/blob/master/PROTOCOL)).
4343
It is **disabled** by default due to its experimental nature and possible interoperability issues, so users who wish to use this feature must turn it on *explicitly*.
4444

@@ -56,7 +56,18 @@ Provide (read-only) public access to internal session state values related to KE
5656
* *getSessionKexDetails*
5757
* *getSessionCountersDetails*
5858

59+
### Added `SessionListener#sessionPeerIdentificationSent` callback
60+
61+
Invoked after successful or failed attempt to send client/server identification to peer. The callback provides the failure error if such occurred.
62+
5963
## Potential compatibility issues
6064

65+
### Added finite wait time for default implementation of `ClientSession#executeRemoteCommand`
66+
67+
* `CoreModuleProperties#EXEC_CHANNEL_OPEN_TIMEOUT` - default = 30 seconds.
68+
* `CoreModuleProperties#EXEC_CHANNEL_CMD_TIMEOUT` - default = 30 seconds.
69+
70+
This may cause failures for code that was running long execution commands using the default method implementations.
71+
6172
## Major Code Re-factoring
6273

docs/standards.md

+17-1
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
# Supported standards
22

33
## Reference implementation documentation
4+
45
* [RFC 4251 - The Secure Shell (SSH) Protocol Architecture](https://tools.ietf.org/html/rfc4251)
56
* [RFC 4252 - The Secure Shell (SSH) Authentication Protocol](https://tools.ietf.org/html/rfc4252)
67
* [RFC 4253 - The Secure Shell (SSH) Transport Layer Protocol](https://tools.ietf.org/html/rfc4253)
@@ -31,10 +32,26 @@
3132
* [Key Exchange (KEX) Method Updates and Recommendations for Secure Shell](https://tools.ietf.org/html/draft-ietf-curdle-ssh-kex-sha2-03)
3233

3334
## *OpenSSH*
35+
3436
* [OpenSSH support for U2F/FIDO security keys](https://github.com/openssh/openssh-portable/blob/master/PROTOCOL.u2f)
3537
* **Note:** the server side supports these keys by default. The client side requires specific initialization
3638
* [OpenSSH public-key certificate authentication system for use by SSH](https://github.com/openssh/openssh-portable/blob/master/PROTOCOL.certkeys)
3739
* [OpenSSH 1.9 transport: strict key exchange extension](https://github.com/openssh/openssh-portable/blob/master/PROTOCOL)
40+
* [(Some) OpenSSH SFTP extensions](https://github.com/openssh/openssh-portable/blob/master/PROTOCOL)
41+
42+
**Note:** some implementations may be limited to client-side - i.e., we provide a capability for the client to detect if the server
43+
supports the extension and then use it, but our server does not publish it as being supported.
44+
45+
| Section | Extension | Client | Server |
46+
| ------- | -------------------------- | ------ | ------ |
47+
| 4.3 | `[email protected]` | Yes | Yes |
48+
| 4.4 | `[email protected]` | Yes | Yes |
49+
| 4.4 | `[email protected]` | Yes | Yes |
50+
| 4.5 | `[email protected]` | Yes | Yes |
51+
| 4.6 | `[email protected]` | Yes | Yes |
52+
| 4.7 | `[email protected]` | Yes | Yes |
53+
| 4.8 | `[email protected]` | Yes | Yes |
54+
| 4.10 | `copy-data` | Yes | Yes |
3855

3956
## SFTP version 3-6 + extensions
4057

@@ -49,7 +66,6 @@
4966
* `copy-file`, `copy-data` - [DRAFT 00 - sections 6, 7](https://tools.ietf.org/id/draft-ietf-secsh-filexfer-extensions-00.txt)
5067
* `space-available` - [DRAFT 09 - section 9.2](https://datatracker.ietf.org/doc/html/draft-ietf-secsh-filexfer-09#section-9.2)
5168
* `filename-charset`, `filename-translation-control` - [DRAFT 13 - section 6](https://tools.ietf.org/html/draft-ietf-secsh-filexfer-13#section-6) - only client side
52-
* Several [OpenSSH SFTP extensions](https://github.com/openssh/openssh-portable/blob/master/PROTOCOL)
5369

5470
## Miscellaneous
5571

sshd-checkstyle-suppressions.xml

+1-1
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,6 @@
66
<suppressions>
77
<suppress files="[\\/]*\.txt" checks="[a-zA-Z0-9]*"/>
88
<suppress files="[\\/]*\.properties" checks="[a-zA-Z0-9]*"/>
9-
<!-- Imported file that we want to keep as-is -->
9+
<!-- Imported file that we want to keep as-is -->
1010
<suppress files="[\\/]*BCrypt.java" checks="[a-zA-Z0-9]*"/>
1111
</suppressions>

sshd-common/src/test/java/org/apache/sshd/util/test/JUnitTestSupport.java

+19-1
Original file line numberDiff line numberDiff line change
@@ -703,7 +703,7 @@ public static void outputDebugMessage(String format, Object o) {
703703

704704
public static void outputDebugMessage(String format, Object... args) {
705705
if (OUTPUT_DEBUG_MESSAGES) {
706-
outputDebugMessage(String.format(format, args));
706+
outputDebugMessage(GenericUtils.isEmpty(args) ? format : String.format(format, args));
707707
}
708708
}
709709

@@ -713,6 +713,24 @@ public static void outputDebugMessage(Object message) {
713713
}
714714
}
715715

716+
public static void failWithWrittenErrorMessage(String format, Object... args) {
717+
failWithWrittenErrorMessage(GenericUtils.isEmpty(args) ? format : String.format(format, args));
718+
}
719+
720+
public static void failWithWrittenErrorMessage(Object message) {
721+
writeErrorMessage(message);
722+
fail(Objects.toString(message));
723+
}
724+
725+
public static void writeErrorMessage(String format, Object... args) {
726+
writeErrorMessage(GenericUtils.isEmpty(args) ? format : String.format(format, args));
727+
}
728+
729+
public static void writeErrorMessage(Object message) {
730+
System.err.append("===[ERROR]=== ").println(message);
731+
System.err.flush();
732+
}
733+
716734
/* ---------------------------------------------------------------------------- */
717735

718736
public static void replaceJULLoggers() {

sshd-core/src/main/java/org/apache/sshd/client/session/ClientSession.java

+6-3
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,7 @@
5959
import org.apache.sshd.common.util.io.output.NoCloseOutputStream;
6060
import org.apache.sshd.common.util.io.output.NullOutputStream;
6161
import org.apache.sshd.common.util.net.SshdSocketAddress;
62+
import org.apache.sshd.core.CoreModuleProperties;
6263

6364
/**
6465
* <P>
@@ -304,10 +305,12 @@ default void executeRemoteCommand(
304305
ClientChannel channel = createExecChannel(command)) {
305306
channel.setOut(channelOut);
306307
channel.setErr(channelErr);
307-
channel.open().await(); // TODO use verify and a configurable timeout
308308

309-
// TODO use a configurable timeout
310-
Collection<ClientChannelEvent> waitMask = channel.waitFor(REMOTE_COMMAND_WAIT_EVENTS, 0L);
309+
Duration openTimeout = CoreModuleProperties.EXEC_CHANNEL_OPEN_TIMEOUT.getRequired(channel);
310+
channel.open().verify(openTimeout);
311+
312+
Duration execTimeout = CoreModuleProperties.EXEC_CHANNEL_CMD_TIMEOUT.getRequired(channel);
313+
Collection<ClientChannelEvent> waitMask = channel.waitFor(REMOTE_COMMAND_WAIT_EVENTS, execTimeout);
311314
if (waitMask.contains(ClientChannelEvent.TIMEOUT)) {
312315
throw new SocketTimeoutException("Failed to retrieve command result in time: " + command);
313316
}

sshd-core/src/main/java/org/apache/sshd/client/session/ClientSessionImpl.java

+1
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,7 @@ public class ClientSessionImpl extends AbstractClientSession {
7474

7575
public ClientSessionImpl(ClientFactoryManager client, IoSession ioSession) throws Exception {
7676
super(client, ioSession);
77+
7778
if (log.isDebugEnabled()) {
7879
log.debug("Client session created: {}", ioSession);
7980
}

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

+16
Original file line numberDiff line numberDiff line change
@@ -103,6 +103,22 @@ default void sessionPeerIdentificationLine(
103103
// ignored
104104
}
105105

106+
/**
107+
* Identification sent to peer
108+
*
109+
* @param session The {@link Session} instance
110+
* @param version The resolved identification version
111+
* @param extraLines Extra data preceding the identification to be sent. <B>Note:</B> the list is modifiable only if
112+
* this is a server session. The user may modify it based on the peer.
113+
* @param error {@code null} if sending was successful
114+
* @see <A HREF="https://tools.ietf.org/html/rfc4253#section-4.2">RFC 4253 - section 4.2 - Protocol
115+
* Version Exchange</A>
116+
*/
117+
default void sessionPeerIdentificationSent(
118+
Session session, String version, List<String> extraLines, Throwable error) {
119+
// ignored
120+
}
121+
106122
/**
107123
* The peer's identification version was received
108124
*

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

+3-2
Original file line numberDiff line numberDiff line change
@@ -113,7 +113,7 @@
113113
*
114114
* @author <a href="mailto:[email protected]">Apache MINA SSHD Project</a>
115115
*/
116-
@SuppressWarnings("checkstyle:MethodCount")
116+
@SuppressWarnings("checkstyle:MethodCount") // Number of methods exceeds max. allowed
117117
public abstract class AbstractSession extends SessionHelper {
118118
/**
119119
* Name of the property where this session is stored in the attributes of the underlying MINA session. See
@@ -601,7 +601,8 @@ protected void doHandleMessage(Buffer buffer) throws Exception {
601601
&& CoreModuleProperties.USE_STRICT_KEX.getRequired(this)
602602
&& (cmd != SshConstants.SSH_MSG_KEXINIT)) {
603603
log.error("doHandleMessage({}) invalid 1st message: {}", this, SshConstants.getCommandMessageName(cmd));
604-
throw new SshException(SshConstants.SSH2_DISCONNECT_PROTOCOL_ERROR, "Strict KEX Error");
604+
disconnect(SshConstants.SSH2_DISCONNECT_PROTOCOL_ERROR, "Strict KEX Error");
605+
return;
605606
}
606607

607608
if (log.isDebugEnabled()) {

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

+58-14
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,7 @@
4949
import org.apache.sshd.common.channel.throttle.ChannelStreamWriterResolverManager;
5050
import org.apache.sshd.common.digest.Digest;
5151
import org.apache.sshd.common.forward.Forwarder;
52+
import org.apache.sshd.common.future.SshFutureListener;
5253
import org.apache.sshd.common.io.IoSession;
5354
import org.apache.sshd.common.io.IoWriteFuture;
5455
import org.apache.sshd.common.kex.AbstractKexFactoryManager;
@@ -77,6 +78,7 @@
7778
/**
7879
* Contains split code in order to make {@link AbstractSession} class smaller
7980
*/
81+
@SuppressWarnings("checkstyle:MethodCount") // Number of methods exceeds max. allowed
8082
public abstract class SessionHelper extends AbstractKexFactoryManager implements Session {
8183

8284
// Session timeout measurements
@@ -628,6 +630,31 @@ protected void signalSendIdentification(SessionListener listener, String version
628630
listener.sessionPeerIdentificationSend(this, version, extraLines);
629631
}
630632

633+
protected void signalIdentificationSent(String version, List<String> extraLines, Throwable error) throws Exception {
634+
try {
635+
invokeSessionSignaller(l -> {
636+
signalIdentificationSent(l, version, extraLines, error);
637+
return null;
638+
});
639+
} catch (Throwable err) {
640+
Throwable e = ExceptionUtils.peelException(err);
641+
if (e instanceof Exception) {
642+
throw (Exception) e;
643+
} else {
644+
throw new RuntimeSshException(e);
645+
}
646+
}
647+
}
648+
649+
protected void signalIdentificationSent(
650+
SessionListener listener, String version, List<String> extraLines, Throwable err) throws Exception {
651+
if (listener == null) {
652+
return;
653+
}
654+
655+
listener.sessionPeerIdentificationSent(this, version, extraLines, err);
656+
}
657+
631658
protected void signalReadPeerIdentificationLine(String line, List<String> extraLines) throws Exception {
632659
try {
633660
invokeSessionSignaller(l -> {
@@ -833,28 +860,45 @@ protected IoWriteFuture sendIdentification(String version, List<String> extraLin
833860
ReservedSessionMessagesHandler handler = getReservedSessionMessagesHandler();
834861
IoWriteFuture future = (handler == null) ? null : handler.sendIdentification(this, version, extraLines);
835862
boolean debugEnabled = log.isDebugEnabled();
836-
if (future != null) {
863+
if (future == null) {
864+
String ident = version;
865+
if (GenericUtils.size(extraLines) > 0) {
866+
ident = GenericUtils.join(extraLines, "\r\n") + "\r\n" + version;
867+
}
868+
869+
if (debugEnabled) {
870+
log.debug("sendIdentification({}): {}",
871+
this, ident.replace('\r', '|').replace('\n', '|'));
872+
}
873+
874+
IoSession networkSession = getIoSession();
875+
byte[] data = (ident + "\r\n").getBytes(StandardCharsets.UTF_8);
876+
future = networkSession.writeBuffer(new ByteArrayBuffer(data));
877+
} else {
837878
if (debugEnabled) {
838879
log.debug("sendIdentification({})[{}] sent {} lines via reserved handler",
839880
this, version, GenericUtils.size(extraLines));
840881
}
841-
842-
return future;
843882
}
844883

845-
String ident = version;
846-
if (GenericUtils.size(extraLines) > 0) {
847-
ident = GenericUtils.join(extraLines, "\r\n") + "\r\n" + version;
848-
}
884+
future.addListener(new SshFutureListener<IoWriteFuture>() {
885+
@Override
886+
public void operationComplete(IoWriteFuture future) {
887+
try {
888+
signalIdentificationSent(version, extraLines, future.getException());
889+
} catch (Throwable err) {
890+
Throwable e = ExceptionUtils.peelException(err);
891+
if (e instanceof RuntimeException) {
892+
throw (RuntimeException) e;
893+
} else {
894+
throw new RuntimeSshException(e);
895+
}
849896

850-
if (debugEnabled) {
851-
log.debug("sendIdentification({}): {}",
852-
this, ident.replace('\r', '|').replace('\n', '|'));
853-
}
897+
}
898+
}
899+
});
854900

855-
IoSession networkSession = getIoSession();
856-
byte[] data = (ident + "\r\n").getBytes(StandardCharsets.UTF_8);
857-
return networkSession.writeBuffer(new ByteArrayBuffer(data));
901+
return future;
858902
}
859903

860904
/**

sshd-core/src/main/java/org/apache/sshd/core/CoreModuleProperties.java

+15
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,21 @@ public final class CoreModuleProperties {
6161
public static final Property<Duration> CHANNEL_OPEN_TIMEOUT
6262
= Property.duration("ssh-agent-server-channel-open-timeout", Duration.ofSeconds(30));
6363

64+
/**
65+
* Value that can be set on the {@link org.apache.sshd.common.FactoryManager} the session or the channel to
66+
* configure the channel open timeout value (millis) for executing a remote command using default implementation.
67+
*/
68+
public static final Property<Duration> EXEC_CHANNEL_OPEN_TIMEOUT
69+
= Property.duration("ssh-exec-channel-open-timeout", Duration.ofSeconds(30));
70+
71+
/**
72+
* Value that can be set on the {@link org.apache.sshd.common.FactoryManager} the session or the channel to
73+
* configure the channel command execution timeout value (millis) for executing a remote command using default
74+
* implementation. By default it waits <U>forever</U> for the command execution to complete.
75+
*/
76+
public static final Property<Duration> EXEC_CHANNEL_CMD_TIMEOUT
77+
= Property.duration("ssh-exec-channel-cmd-timeout", Duration.ofSeconds(0));
78+
6479
/**
6580
* Value used to configure the type of proxy forwarding channel to be used. See also
6681
* https://tools.ietf.org/html/draft-ietf-secsh-agent-02

sshd-core/src/test/java/org/apache/sshd/client/ClientTest.java

-1
Original file line numberDiff line numberDiff line change
@@ -1510,7 +1510,6 @@ public void testKeyboardInteractiveInSessionUserInteractiveFailure() throws Exce
15101510
CoreModuleProperties.PASSWORD_PROMPTS.set(client, maxPrompts);
15111511
AtomicInteger numberOfRequests = new AtomicInteger();
15121512
UserAuthKeyboardInteractiveFactory auth = new UserAuthKeyboardInteractiveFactory() {
1513-
15141513
@Override
15151514
public UserAuthKeyboardInteractive createUserAuth(ClientSession session) throws IOException {
15161515
return new UserAuthKeyboardInteractive() {

0 commit comments

Comments
 (0)