From b8a514fdcbedc49f885c41216fb482ac49c19c8e Mon Sep 17 00:00:00 2001 From: Thomas Wolf Date: Sat, 13 Jul 2019 10:57:11 +0200 Subject: [PATCH] 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 --- .../jgit/junit/ssh/SshTestGitServer.java | 18 +++ .../META-INF/MANIFEST.MF | 3 +- .../jgit/transport/sshd/ApacheSshTest.java | 63 ++++++++- .../transport/sshd/SshdText.properties | 3 + .../transport/sshd/JGitClientSession.java | 133 +++++++++++++++++- .../internal/transport/sshd/SshdText.java | 3 + 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 25d952f18..7f155d486 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 e8164a9de..504a20eae 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 ee58083a5..df0b832a0 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 bdb4a7d0d..4f85ebe10 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 56f8ade66..2f089474e 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 + * Copyright (C) 2018, 2019 Thomas Wolf * 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 RFC 4253, + * section 4.2 + */ + @Override + protected List 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 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 bf432be5a..f67170e40 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;