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>

diff --git a/org.eclipse.jgit.ssh.apache/META-INF/MANIFEST.MF b/org.eclipse.jgit.ssh.apache/META-INF/MANIFEST.MF
index 2ba94f2057c8b016dee9f1a9ffc47485fd6a379d..caa3e700d17fe317b13c18ec953e5b3746639617 100644 (file)
--- 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 b529e5b5aeb0aaf5f4b79189bd3d4663b1050147..4f735bab34bb44e70eae3fd1dc22d17eceac0318 100644 (file)
--- 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 b8c94eea59d6383582dd21914e7ad1535fab72bc..19ad85c83d9665c34b98ba60e9cf7c7a65c918a4 100644 (file)
--- 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 49b0d4ad77f7f999ae213f6ae8db31f9036e2f02..cbcb4d240ed9d9ca2d6f2cb06d78ca8297f63f6e 100644 (file)
--- 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));
        }