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>

2 files changed, 61 insertions, 5 deletions

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());
+	}
 }