Browse Source

sshd: correct the protocol version exchange

RFC 4253 section 4.2 allows an ssh server to send additional lines
before its server identification string. Apache MINA sshd enforces
for these lines the constraints specified for the server identification
line, too: no NUL characters and not longer than 255 characters. That
is too strict. RFC 4253 doesn't mandate this, and it also doesn't
make sense given the rationale for these lines in RFC 4253: a TCP
wrapper may not be aware of SSH restrictions, and may not adhere to
these constraints.

Be more lenient when parsing the server's protocol version. Allow
NULs and longer lines in the preamble, and also handle line endings
more leniently. Only enforce the restrictions for the actual server
identification line.

Bug: 545939
Change-Id: I75955e9d8a8daef7c04fc0f39539c2ee93514e1c
Signed-off-by: Thomas Wolf <thomas.wolf@paranor.ch>
tags/v5.5.0.201908280940-m3
Thomas Wolf 4 years ago
parent
commit
b8a514fdcb

+ 18
- 0
org.eclipse.jgit.junit.ssh/src/org/eclipse/jgit/junit/ssh/SshTestGitServer.java View File

@@ -58,6 +58,7 @@ import java.util.Locale;

import org.apache.sshd.common.NamedFactory;
import org.apache.sshd.common.NamedResource;
import org.apache.sshd.common.PropertyResolverUtils;
import org.apache.sshd.common.SshConstants;
import org.apache.sshd.common.config.keys.AuthorizedKeyEntry;
import org.apache.sshd.common.config.keys.KeyUtils;
@@ -69,6 +70,7 @@ import org.apache.sshd.common.util.security.SecurityUtils;
import org.apache.sshd.common.util.threads.CloseableExecutorService;
import org.apache.sshd.common.util.threads.ThreadUtils;
import org.apache.sshd.server.ServerAuthenticationManager;
import org.apache.sshd.server.ServerFactoryManager;
import org.apache.sshd.server.SshServer;
import org.apache.sshd.server.auth.UserAuth;
import org.apache.sshd.server.auth.gss.GSSAuthenticator;
@@ -342,6 +344,22 @@ public class SshTestGitServer {
.resolvePublicKey(null, PublicKeyEntryResolver.IGNORING);
}

/**
* Sets the lines the server sends before its server identification in the
* initial protocol version exchange.
*
* @param lines
* to send
* @since 5.5
*/
public void setPreamble(String... lines) {
if (lines != null && lines.length > 0) {
PropertyResolverUtils.updateProperty(this.server,
ServerFactoryManager.SERVER_EXTRA_IDENTIFICATION_LINES,
String.join("|", lines));
}
}

private class GitUploadPackCommand extends AbstractCommandSupport {

protected GitUploadPackCommand(String command,

+ 2
- 1
org.eclipse.jgit.ssh.apache.test/META-INF/MANIFEST.MF View File

@@ -7,7 +7,8 @@ Bundle-Version: 5.5.0.qualifier
Bundle-Vendor: %Bundle-Vendor
Bundle-Localization: plugin
Bundle-RequiredExecutionEnvironment: JavaSE-1.8
Import-Package: org.eclipse.jgit.internal.transport.sshd.proxy;version="[5.5.0,5.6.0)",
Import-Package: org.eclipse.jgit.api.errors;version="[5.5.0,5.6.0)",
org.eclipse.jgit.internal.transport.sshd.proxy;version="[5.5.0,5.6.0)",
org.eclipse.jgit.junit;version="[5.5.0,5.6.0)",
org.eclipse.jgit.junit.ssh;version="[5.5.0,5.6.0)",
org.eclipse.jgit.lib;version="[5.5.0,5.6.0)",

+ 59
- 4
org.eclipse.jgit.ssh.apache.test/tst/org/eclipse/jgit/transport/sshd/ApacheSshTest.java View File

@@ -47,7 +47,7 @@ import java.io.IOException;
import java.io.UncheckedIOException;
import java.nio.file.Files;
import java.util.Arrays;
import org.eclipse.jgit.api.errors.TransportException;
import org.eclipse.jgit.lib.Constants;
import org.eclipse.jgit.transport.SshSessionFactory;
import org.eclipse.jgit.transport.ssh.SshTestBase;
@@ -82,11 +82,10 @@ public class ApacheSshTest extends SshTestBase {
}
}

// Using an ed25519 (unencrypted) user key is tested in the super class in
// testSshKeys(). sshd 2.0.0 cannot yet read encrypted ed25519 keys.

@Test
public void testEd25519HostKey() throws Exception {
// Using ed25519 user identities is tested in the super class in
// testSshKeys().
File newHostKey = new File(getTemporaryDirectory(), "newhostkey");
copyTestResource("id_ed25519", newHostKey);
server.addHostKey(newHostKey.toPath(), true);
@@ -102,4 +101,60 @@ public class ApacheSshTest extends SshTestBase {
"IdentityFile " + privateKey1.getAbsolutePath());
}

@Test
public void testPreamble() throws Exception {
// Test that the client can deal with strange lines being sent before
// the server identification string.
StringBuilder b = new StringBuilder();
for (int i = 0; i < 257; i++) {
b.append('a');
}
server.setPreamble("A line with a \000 NUL",
"A long line: " + b.toString());
cloneWith(
"ssh://" + TEST_USER + "@localhost:" + testPort
+ "/doesntmatter",
defaultCloneDir, null,
"IdentityFile " + privateKey1.getAbsolutePath());
}

@Test
public void testLongPreamble() throws Exception {
// Test that the client can deal with a long (about 60k) preamble.
StringBuilder b = new StringBuilder();
for (int i = 0; i < 1024; i++) {
b.append('a');
}
String line = b.toString();
String[] lines = new String[60];
for (int i = 0; i < lines.length; i++) {
lines[i] = line;
}
server.setPreamble(lines);
cloneWith(
"ssh://" + TEST_USER + "@localhost:" + testPort
+ "/doesntmatter",
defaultCloneDir, null,
"IdentityFile " + privateKey1.getAbsolutePath());
}

@Test (expected = TransportException.class)
public void testHugePreamble() throws Exception {
// Test that the connection fails when the preamble is longer than 64k.
StringBuilder b = new StringBuilder();
for (int i = 0; i < 1024; i++) {
b.append('a');
}
String line = b.toString();
String[] lines = new String[70];
for (int i = 0; i < lines.length; i++) {
lines[i] = line;
}
server.setPreamble(lines);
cloneWith(
"ssh://" + TEST_USER + "@localhost:" + testPort
+ "/doesntmatter",
defaultCloneDir, null,
"IdentityFile " + privateKey1.getAbsolutePath());
}
}

+ 3
- 0
org.eclipse.jgit.ssh.apache/resources/org/eclipse/jgit/internal/transport/sshd/SshdText.properties View File

@@ -71,6 +71,9 @@ proxySocksPasswordTooLong=Password for proxy {0} must be at most 255 bytes long,
proxySocksUnexpectedMessage=Unexpected message received from SOCKS5 proxy {0}; client state {1}: {2}
proxySocksUnexpectedVersion=Expected SOCKS version 5, got {0}
proxySocksUsernameTooLong=User name for proxy {0} must be at most 255 bytes long, is {1} bytes: {2}
serverIdNotReceived=No server identification received within {0} bytes
serverIdTooLong=Server identification is longer than 255 characters (including line ending): {0}
serverIdWithNul=Server identification contains a NUL character: {0}
sessionCloseFailed=Closing the session failed
sshClosingDown=Apache MINA sshd session factory is closing down; cannot create new ssh sessions on this factory
sshCommandTimeout={0} timed out after {1} seconds while opening the channel

+ 132
- 1
org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/JGitClientSession.java View File

@@ -1,5 +1,5 @@
/*
* Copyright (C) 2018, Thomas Wolf <thomas.wolf@paranor.ch>
* Copyright (C) 2018, 2019 Thomas Wolf <thomas.wolf@paranor.ch>
* and other copyright owners as documented in the project's IP log.
*
* This program and the accompanying materials are made available
@@ -46,6 +46,7 @@ import static java.text.MessageFormat.format;

import java.io.IOException;
import java.net.SocketAddress;
import java.nio.charset.StandardCharsets;
import java.security.GeneralSecurityException;
import java.security.PublicKey;
import java.util.ArrayList;
@@ -60,11 +61,13 @@ import org.apache.sshd.client.keyverifier.KnownHostsServerKeyVerifier.HostEntryP
import org.apache.sshd.client.keyverifier.ServerKeyVerifier;
import org.apache.sshd.client.session.ClientSessionImpl;
import org.apache.sshd.common.FactoryManager;
import org.apache.sshd.common.PropertyResolverUtils;
import org.apache.sshd.common.SshException;
import org.apache.sshd.common.config.keys.KeyUtils;
import org.apache.sshd.common.io.IoSession;
import org.apache.sshd.common.io.IoWriteFuture;
import org.apache.sshd.common.util.Readable;
import org.apache.sshd.common.util.buffer.Buffer;
import org.eclipse.jgit.errors.InvalidPatternException;
import org.eclipse.jgit.fnmatch.FileNameMatcher;
import org.eclipse.jgit.internal.transport.sshd.proxy.StatefulProxyConnector;
@@ -82,6 +85,15 @@ import org.eclipse.jgit.transport.SshConstants;
*/
public class JGitClientSession extends ClientSessionImpl {

/**
* Default setting for the maximum number of bytes to read in the initial
* protocol version exchange. 64kb is what OpenSSH < 8.0 read; OpenSSH 8.0
* changed it to 8Mb, but that seems excessive for the purpose stated in RFC
* 4253. The Apache MINA sshd default in
* {@link FactoryManager#DEFAULT_MAX_IDENTIFICATION_SIZE} is 16kb.
*/
private static final int DEFAULT_MAX_IDENTIFICATION_SIZE = 64 * 1024;

private HostConfigEntry hostConfig;

private CredentialsProvider credentialsProvider;
@@ -332,4 +344,123 @@ public class JGitClientSession extends ClientSessionImpl {
return newNames;
}

@Override
protected boolean readIdentification(Buffer buffer) throws IOException {
// Propagate a failure from doReadIdentification.
// TODO: remove for sshd > 2.3.0; its doReadIdentification throws
// StreamCorruptedException instead of IllegalStateException.
try {
return super.readIdentification(buffer);
} catch (IllegalStateException e) {
throw new IOException(e.getLocalizedMessage(), e);
}
}

/**
* Reads the RFC 4253, section 4.2 protocol version identification. The
* Apache MINA sshd default implementation checks for NUL bytes also in any
* preceding lines, whereas RFC 4253 requires such a check only for the
* actual identification string starting with "SSH-". Likewise, the 255
* character limit exists only for the identification string, not for the
* preceding lines. CR-LF handling is also relaxed.
*
* @param buffer
* to read from
* @param server
* whether we're an SSH server (should always be {@code false})
* @return the lines read, with the server identification line last, or
* {@code null} if no identification line was found and more bytes
* are needed
*
* @see <a href="https://tools.ietf.org/html/rfc4253#section-4.2">RFC 4253,
* section 4.2</a>
*/
@Override
protected List<String> doReadIdentification(Buffer buffer, boolean server) {
if (server) {
// Should never happen. No translation; internal bug.
throw new IllegalStateException(
"doReadIdentification of client called with server=true"); //$NON-NLS-1$
}
int maxIdentSize = PropertyResolverUtils.getIntProperty(this,
FactoryManager.MAX_IDENTIFICATION_SIZE,
DEFAULT_MAX_IDENTIFICATION_SIZE);
int current = buffer.rpos();
int end = current + buffer.available();
if (current >= end) {
return null;
}
byte[] raw = buffer.array();
List<String> ident = new ArrayList<>();
int start = current;
boolean hasNul = false;
for (int i = current; i < end; i++) {
switch (raw[i]) {
case 0:
hasNul = true;
break;
case '\n':
int eol = 1;
if (i > start && raw[i - 1] == '\r') {
eol++;
}
String line = new String(raw, start, i + 1 - eol - start,
StandardCharsets.UTF_8);
start = i + 1;
if (log.isDebugEnabled()) {
log.debug(format("doReadIdentification({0}) line: ", this) + //$NON-NLS-1$
escapeControls(line));
}
ident.add(line);
if (line.startsWith("SSH-")) { //$NON-NLS-1$
if (hasNul) {
throw new IllegalStateException(
format(SshdText.get().serverIdWithNul,
escapeControls(line)));
}
if (line.length() + eol > 255) {
throw new IllegalStateException(
format(SshdText.get().serverIdTooLong,
escapeControls(line)));
}
buffer.rpos(start);
return ident;
}
// If this were a server, we could throw an exception here: a
// client is not supposed to send any extra lines before its
// identification string.
hasNul = false;
break;
default:
break;
}
if (i - current + 1 >= maxIdentSize) {
String msg = format(SshdText.get().serverIdNotReceived,
Integer.toString(maxIdentSize));
if (log.isDebugEnabled()) {
log.debug(msg);
log.debug(buffer.toHex());
}
throw new IllegalStateException(msg);
}
}
// Need more data
return null;
}

private static String escapeControls(String s) {
StringBuilder b = new StringBuilder();
int l = s.length();
for (int i = 0; i < l; i++) {
char ch = s.charAt(i);
if (Character.isISOControl(ch)) {
b.append(ch <= 0xF ? "\\u000" : "\\u00") //$NON-NLS-1$ //$NON-NLS-2$
.append(Integer.toHexString(ch));
} else {
b.append(ch);
}
}
return b.toString();
}

}

+ 3
- 0
org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/SshdText.java View File

@@ -83,6 +83,9 @@ public final class SshdText extends TranslationBundle {
/***/ public String proxySocksUnexpectedMessage;
/***/ public String proxySocksUnexpectedVersion;
/***/ public String proxySocksUsernameTooLong;
/***/ public String serverIdNotReceived;
/***/ public String serverIdTooLong;
/***/ public String serverIdWithNul;
/***/ public String sessionCloseFailed;
/***/ public String sshClosingDown;
/***/ public String sshCommandTimeout;

Loading…
Cancel
Save