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>

6 files changed, 217 insertions, 6 deletions

diff --git a/org.eclipse.jgit.junit.ssh/src/org/eclipse/jgit/junit/ssh/SshTestGitServer.java b/org.eclipse.jgit.junit.ssh/src/org/eclipse/jgit/junit/ssh/SshTestGitServer.java
index 25d952f189..7f155d4867 100644
--- a/org.eclipse.jgit.junit.ssh/src/org/eclipse/jgit/junit/ssh/SshTestGitServer.java
+++ b/org.eclipse.jgit.junit.ssh/src/org/eclipse/jgit/junit/ssh/SshTestGitServer.java
@@ -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,
diff --git a/org.eclipse.jgit.ssh.apache.test/META-INF/MANIFEST.MF b/org.eclipse.jgit.ssh.apache.test/META-INF/MANIFEST.MF
index e8164a9de7..504a20eae4 100644
--- a/org.eclipse.jgit.ssh.apache.test/META-INF/MANIFEST.MF
+++ b/org.eclipse.jgit.ssh.apache.test/META-INF/MANIFEST.MF
@@ -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)",
diff --git a/org.eclipse.jgit.ssh.apache.test/tst/org/eclipse/jgit/transport/sshd/ApacheSshTest.java b/org.eclipse.jgit.ssh.apache.test/tst/org/eclipse/jgit/transport/sshd/ApacheSshTest.java
index ee58083a5a..df0b832a0f 100644
--- a/org.eclipse.jgit.ssh.apache.test/tst/org/eclipse/jgit/transport/sshd/ApacheSshTest.java
+++ b/org.eclipse.jgit.ssh.apache.test/tst/org/eclipse/jgit/transport/sshd/ApacheSshTest.java
@@ -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());
+	}
 }
diff --git a/org.eclipse.jgit.ssh.apache/resources/org/eclipse/jgit/internal/transport/sshd/SshdText.properties b/org.eclipse.jgit.ssh.apache/resources/org/eclipse/jgit/internal/transport/sshd/SshdText.properties
index bdb4a7d0d1..4f85ebe100 100644
--- a/org.eclipse.jgit.ssh.apache/resources/org/eclipse/jgit/internal/transport/sshd/SshdText.properties
+++ b/org.eclipse.jgit.ssh.apache/resources/org/eclipse/jgit/internal/transport/sshd/SshdText.properties
@@ -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
diff --git a/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/JGitClientSession.java b/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/JGitClientSession.java
index 56f8ade667..2f089474e7 100644
--- a/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/JGitClientSession.java
+++ b/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/JGitClientSession.java
@@ -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();
+	}
+
 }
diff --git a/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/SshdText.java b/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/SshdText.java
index bf432be5a6..f67170e407 100644
--- a/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/SshdText.java
+++ b/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/SshdText.java
@@ -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;