From b8a514fdcbedc49f885c41216fb482ac49c19c8e Mon Sep 17 00:00:00 2001 From: Thomas Wolf Date: Sat, 13 Jul 2019 10:57:11 +0200 Subject: 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 --- .../META-INF/MANIFEST.MF | 3 +- .../eclipse/jgit/transport/sshd/ApacheSshTest.java | 63 ++++++++++++++++++++-- 2 files changed, 61 insertions(+), 5 deletions(-) (limited to 'org.eclipse.jgit.ssh.apache.test') 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()); + } } -- cgit v1.2.3