From f41929708e79d7b36e0a653ae3d7464d4f20b606 Mon Sep 17 00:00:00 2001 From: Thomas Wolf Date: Wed, 29 Dec 2021 20:33:33 +0100 Subject: [PATCH] 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 --- .../META-INF/MANIFEST.MF | 1 + .../transport/sshd/SshdText.properties | 2 + .../internal/transport/sshd/SshdText.java | 2 + .../transport/sshd/agent/SshAgentClient.java | 58 +++++++++++++++++-- 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> 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 not propagated. + *

+ * This is needed because an SSH agent might contain and deliver keys that + * we cannot handle (for instance ed448 keys). + *

+ * + * @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)); } -- 2.39.5