sshd: Skip unknown keys from the SSH agent

An SSH agent might contain keys that Apache MINA sshd cannot handle.
Pageant for instance can contain ed448 keys, which are not implemented
in OpenSSH or in Apache MINA sshd.

When an agent delivers such keys, simply skip (and log) them. That way,
we can work with the remaining keys. Otherwise a single unknown key in
the agent would break pubkey authentication.

Change-Id: I3945d932c7e64b628465004cfbaf10f4dc05f3e4
Signed-off-by: Thomas Wolf <thomas.wolf@paranor.ch>

4 files changed, 57 insertions, 6 deletions

diff --git a/org.eclipse.jgit.ssh.apache/META-INF/MANIFEST.MF b/org.eclipse.jgit.ssh.apache/META-INF/MANIFEST.MF
index 2ba94f2057..caa3e700d1 100644
--- a/org.eclipse.jgit.ssh.apache/META-INF/MANIFEST.MF
+++ b/org.eclipse.jgit.ssh.apache/META-INF/MANIFEST.MF
@@ -72,6 +72,7 @@ Import-Package: net.i2p.crypto.eddsa;version="[0.3.0,0.4.0)",
  org.apache.sshd.common.signature;version="[2.8.0,2.9.0)",
  org.apache.sshd.common.util;version="[2.8.0,2.9.0)",
  org.apache.sshd.common.util.buffer;version="[2.8.0,2.9.0)",
+ org.apache.sshd.common.util.buffer.keys;version="[2.8.0,2.9.0)",
  org.apache.sshd.common.util.closeable;version="[2.8.0,2.9.0)",
  org.apache.sshd.common.util.io;version="[2.8.0,2.9.0)",
  org.apache.sshd.common.util.io.der;version="[2.8.0,2.9.0)",
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 b529e5b5ae..4f735bab34 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
@@ -93,6 +93,8 @@ sshAgentPayloadLengthError=Expected {0,choice,0#no bytes|1#one byte|1<{0} bytes}
 sshAgentReplyLengthError=Invalid SSH agent reply message length {0} after command {1}
 sshAgentReplyUnexpected=Unexpected reply from ssh-agent: {0}
 sshAgentShortReadBuffer=Short read from SSH agent
+sshAgentUnknownKey=SSH agent delivered a key that cannot be handled
+sshAgentWrongKeyLength=SSH agent delivered illogical key length {0} at offset {1} in buffer of length {2}
 sshAgentWrongNumberOfKeys=Invalid number of SSH agent keys: {0}
 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/SshdText.java b/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/SshdText.java
index b8c94eea59..19ad85c83d 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
@@ -114,6 +114,8 @@ public final class SshdText extends TranslationBundle {
 	/***/ public String sshAgentReplyLengthError;
 	/***/ public String sshAgentReplyUnexpected;
 	/***/ public String sshAgentShortReadBuffer;
+	/***/ public String sshAgentUnknownKey;
+	/***/ public String sshAgentWrongKeyLength;
 	/***/ public String sshAgentWrongNumberOfKeys;
 	/***/ public String sshClosingDown;
 	/***/ public String sshCommandTimeout;
diff --git a/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/agent/SshAgentClient.java b/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/agent/SshAgentClient.java
index 49b0d4ad77..cbcb4d240e 100644
--- a/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/agent/SshAgentClient.java
+++ b/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/agent/SshAgentClient.java
@@ -33,6 +33,7 @@ import org.apache.sshd.common.util.buffer.Buffer;
 import org.apache.sshd.common.util.buffer.BufferException;
 import org.apache.sshd.common.util.buffer.BufferUtils;
 import org.apache.sshd.common.util.buffer.ByteArrayBuffer;
+import org.apache.sshd.common.util.buffer.keys.BufferPublicKeyParser;
 import org.apache.sshd.common.util.io.der.DERParser;
 import org.eclipse.jgit.internal.transport.sshd.SshdText;
 import org.eclipse.jgit.transport.sshd.agent.Connector;
@@ -141,14 +142,17 @@ public class SshAgentClient implements SshAgent {
 			List<Map.Entry<PublicKey, String>> keys = new ArrayList<>(
 					numberOfKeys);
 			for (int i = 0; i < numberOfKeys; i++) {
-				PublicKey key = reply.getPublicKey();
+				PublicKey key = readKey(reply);
 				String comment = reply.getString();
-				if (tracing) {
-					LOG.trace("Got SSH agent {} key: {} {}", //$NON-NLS-1$
-							KeyUtils.getKeyType(key),
-							KeyUtils.getFingerPrint(key), comment);
+				if (key != null) {
+					if (tracing) {
+						LOG.trace("Got SSH agent {} key: {} {}", //$NON-NLS-1$
+								KeyUtils.getKeyType(key),
+								KeyUtils.getFingerPrint(key), comment);
+					}
+					keys.add(new AbstractMap.SimpleImmutableEntry<>(key,
+							comment));
 				}
-				keys.add(new AbstractMap.SimpleImmutableEntry<>(key, comment));
 			}
 			return keys;
 		} catch (BufferException e) {
@@ -404,6 +408,48 @@ public class SshAgentClient implements SshAgent {
 		}
 	}
 
+	/**
+	 * A safe version of {@link Buffer#getPublicKey()}. Upon return the
+	 * buffers's read position is always after the key blob; any exceptions
+	 * thrown by trying to read the key are logged and <em>not</em> propagated.
+	 * <p>
+	 * This is needed because an SSH agent might contain and deliver keys that
+	 * we cannot handle (for instance ed448 keys).
+	 * </p>
+	 *
+	 * @param buffer
+	 *            to read the key from
+	 * @return the {@link PublicKey}, or {@code null} if the key could not be
+	 *         read
+	 * @throws BufferException
+	 *             if the length of the key blob cannot be read or is corrupted
+	 */
+	private static PublicKey readKey(Buffer buffer) throws BufferException {
+		int endOfBuffer = buffer.wpos();
+		int keyLength = buffer.getInt();
+		int afterKey = buffer.rpos() + keyLength;
+		if (keyLength <= 0 || afterKey > endOfBuffer) {
+			throw new BufferException(
+					MessageFormat.format(SshdText.get().sshAgentWrongKeyLength,
+							Integer.toString(keyLength),
+							Integer.toString(buffer.rpos()),
+							Integer.toString(endOfBuffer)));
+		}
+		// Limit subsequent reads to the public key blob
+		buffer.wpos(afterKey);
+		try {
+			return buffer.getRawPublicKey(BufferPublicKeyParser.DEFAULT);
+		} catch (Exception e) {
+			LOG.warn(SshdText.get().sshAgentUnknownKey, e);
+			return null;
+		} finally {
+			// Restore real buffer end
+			buffer.wpos(endOfBuffer);
+			// Set the read position to after this key, even if failed
+			buffer.rpos(afterKey);
+		}
+	}
+
 	private Buffer rpc(byte command, byte[] message) throws IOException {
 		return new ByteArrayBuffer(connector.rpc(command, message));
 	}