diff options
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; |