From 6becee176acd6ba70d2a8bb79ceaa074f298c9f1 Mon Sep 17 00:00:00 2001 From: Fabian Henneke Date: Tue, 28 Jul 2020 12:30:29 +0200 Subject: [PATCH] Fix matching of pubkeys to key algorithms (#607) * Fix matching of pubkeys to key algorithms Allow all configured key algorithms for pubkey authentication, even if these algorithms are not supported as host key algorithms by the server. Preference is given to the modern rsa-sha2-* signature algorithms if the server indicates support for them as host keys signature algorithms. * Replace Boolean with primitive boolean * Add integration tests for ecdsa-sha2-nistp384/521 * Remove redundant import * Clean up Transport interface Co-authored-by: Jeroen van Erp --- src/itest/docker-image/authorized_keys | 2 ++ .../hierynomus/sshj/IntegrationSpec.groovy | 2 ++ .../keyfiles/id_ecdsa_nistp384_opensshv1 | 10 ++++++ .../keyfiles/id_ecdsa_nistp521_opensshv1 | 12 +++++++ .../hierynomus/sshj/key/KeyAlgorithms.java | 5 +++ .../schmizz/sshj/transport/KeyExchanger.java | 9 ++--- .../sshj/transport/NegotiatedAlgorithms.java | 26 ++++++++------ .../net/schmizz/sshj/transport/Proposal.java | 8 +++-- .../net/schmizz/sshj/transport/Transport.java | 3 +- .../schmizz/sshj/transport/TransportImpl.java | 35 +++++++++++++------ .../sshj/transport/kex/AbstractDHG.java | 2 +- .../sshj/transport/kex/AbstractDHGex.java | 2 +- .../sshj/userauth/method/KeyedAuthMethod.java | 4 +-- 13 files changed, 86 insertions(+), 34 deletions(-) create mode 100644 src/itest/resources/keyfiles/id_ecdsa_nistp384_opensshv1 create mode 100644 src/itest/resources/keyfiles/id_ecdsa_nistp521_opensshv1 diff --git a/src/itest/docker-image/authorized_keys b/src/itest/docker-image/authorized_keys index f6f7fdf1..cf4d9dd4 100644 --- a/src/itest/docker-image/authorized_keys +++ b/src/itest/docker-image/authorized_keys @@ -5,3 +5,5 @@ ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIJ8ww4hJG/gHJYdkjTTBDF1GNz+228nuWprPV+NbQauA ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIOaWrwt3drIOjeBq2LSHRavxAT7ja2f+5soOUJl/zKSI ajvanerp@Heimdall.xebialabs.com ssh-rsa AAAAB3NzaC1yc2EAAAABIwAAAQEAoZ9l6Tkm2aL1tSBy2yw4xU5s8BE9MfqS/4J7DzvsYJxF6oQmTIjmStuhH/CT7UjuDtKXdXZUsIhKtafiizxGO8kHSzKDeitpth2RSr8ddMzZKyD6RNs7MfsgjA3UTtrrSrCXEY6O43S2cnuJrWzkPxtwxaQ3zOvDbS2tiulzyq0VzYmuhA/a4CyuQtJBuu+P2oqmu6pU/VB6IzONpvBvYbNPsH1WDmP7zko5wHPihXPCliztspKxS4DRtOZ7BGXyvg44UmIy0Kf4jOkaBV/eCCA4qH7ZHz71/5ceMOpszPcNOEmLGGYhwI+P3OuGMpkrSAv1f8IY6R8spZNncP6UaQ== no-passphrase ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAACAQDKRyZAtOJJfAhPU6xE6ZXY564vwErAI3n3Yn4lTHL9bxev9Ily6eCqPLcV0WbSV04pztngFn9MjT7yb8mcXheHpIaWEH569sMpmpOtyfn4p68SceuXBGyyPGMIcfOTknkASd1JYSD4EPkd9rZmCzcx3vEnLu8ChnA/G221xSVQ5VC/jD/c/CgNUayhQ+xbn57qHKKtZwfTa21QmwIabGYJNwlVjlKTCdddeVnZfKqKrG7cxHQApsxd21rhM9IT/C/f4Y/Tx3WUUVeam0iZ265oiPHoPALqJIWSQIUheRYAxYAQqJwSQ0Or9MM8XXun2Iy3RUSGk6eIvrCsFbNURsHNs7Pu0UnpYv6FZ3vCkFep/1pAT6fQvY7pDOOWDHKXArD4watc9gIWaQBH73wDW/KgBcnMRSoGWgQjsYqIamP4oV1+HqUI3lRAsXZaX+eiBGt3+3A5KebP27UJ1YUwhwlzs7wzTKaCu0OaL+hOsP1F2AxAa995bgFksMd23645ux3YCJKXG4sGpJ1Z/Hs49K72gv+QjLZVxXqY623c8+3OUhlixqoEFd4iG7UMc5a552ch/VA+jaspmLZoFhPz99aBRVb1oCSPxSwLw+Q/wxv6pZmT+14rqTzY2farjU53hM+CsUPh7dnWXhGG7RuA5wCdeOXOYjuksfzAoHIZhPqTgQ== ajvanerp@Heimdall.local +ecdsa-sha2-nistp384 AAAAE2VjZHNhLXNoYTItbmlzdHAzODQAAAAIbmlzdHAzODQAAABhBMvfRYSe44VQGwxexOMibcM3+fWeUP1jrBofOxFDRRrzRF8dK/vll2svqTPXMRnITnT1UoemEcB5OHtvH4hzfh/HFeDxJ5S7UncYxoClTSa8MeMFG2Zj9CoUZs1SHbwSGg== root@sshj +ecdsa-sha2-nistp521 AAAAE2VjZHNhLXNoYTItbmlzdHA1MjEAAAAIbmlzdHA1MjEAAACFBAHquUYgkU9wJrcxDWVtdqtfqf6SBDdPDRxwDl7OCohV2UNu2KdjJwSj8j0fsPeMdHjSiv9OCnHYrVilQ+W5WW5q5wGXwk10oIcV0JJscohLA0nS7mKinBrxUwVHnNZbPExFciicnEArcYRb1BuT7HF8hfjuSSpWS0rob6kloSSi/jV7ZA== root@sshj diff --git a/src/itest/groovy/com/hierynomus/sshj/IntegrationSpec.groovy b/src/itest/groovy/com/hierynomus/sshj/IntegrationSpec.groovy index bc7233be..060384f0 100644 --- a/src/itest/groovy/com/hierynomus/sshj/IntegrationSpec.groovy +++ b/src/itest/groovy/com/hierynomus/sshj/IntegrationSpec.groovy @@ -77,6 +77,8 @@ class IntegrationSpec extends IntegrationBaseSpec { "id_ed25519_opensshv1_protected" | "sshjtest" "id_rsa" | null "id_rsa_opensshv1" | null + "id_ecdsa_nistp384_opensshv1" | null + "id_ecdsa_nistp521_opensshv1" | null } def "should not authenticate with wrong key"() { diff --git a/src/itest/resources/keyfiles/id_ecdsa_nistp384_opensshv1 b/src/itest/resources/keyfiles/id_ecdsa_nistp384_opensshv1 new file mode 100644 index 00000000..68de9fa9 --- /dev/null +++ b/src/itest/resources/keyfiles/id_ecdsa_nistp384_opensshv1 @@ -0,0 +1,10 @@ +-----BEGIN OPENSSH PRIVATE KEY----- +b3BlbnNzaC1rZXktdjEAAAAABG5vbmUAAAAEbm9uZQAAAAAAAAABAAAAiAAAABNlY2RzYS +1zaGEyLW5pc3RwMzg0AAAACG5pc3RwMzg0AAAAYQTL30WEnuOFUBsMXsTjIm3DN/n1nlD9 +Y6waHzsRQ0Ua80RfHSv75ZdrL6kz1zEZyE509VKHphHAeTh7bx+Ic34fxxXg8SeUu1J3GM +aApU0mvDHjBRtmY/QqFGbNUh28EhoAAADYHWlHLx1pRy8AAAATZWNkc2Etc2hhMi1uaXN0 +cDM4NAAAAAhuaXN0cDM4NAAAAGEEy99FhJ7jhVAbDF7E4yJtwzf59Z5Q/WOsGh87EUNFGv +NEXx0r++WXay+pM9cxGchOdPVSh6YRwHk4e28fiHN+H8cV4PEnlLtSdxjGgKVNJrwx4wUb +ZmP0KhRmzVIdvBIaAAAAMQD3sx28SrtkuhN+Yu06BAoFLMMgneIqguM3jowaz0LWfP1Nhx +Rnh9tNKM6YYvygCggAAAAPZmhlbm5la2VATGFwdG9w +-----END OPENSSH PRIVATE KEY----- diff --git a/src/itest/resources/keyfiles/id_ecdsa_nistp521_opensshv1 b/src/itest/resources/keyfiles/id_ecdsa_nistp521_opensshv1 new file mode 100644 index 00000000..57d42087 --- /dev/null +++ b/src/itest/resources/keyfiles/id_ecdsa_nistp521_opensshv1 @@ -0,0 +1,12 @@ +-----BEGIN OPENSSH PRIVATE KEY----- +b3BlbnNzaC1rZXktdjEAAAAABG5vbmUAAAAEbm9uZQAAAAAAAAABAAAArAAAABNlY2RzYS +1zaGEyLW5pc3RwNTIxAAAACG5pc3RwNTIxAAAAhQQB6rlGIJFPcCa3MQ1lbXarX6n+kgQ3 +Tw0ccA5ezgqIVdlDbtinYycEo/I9H7D3jHR40or/Tgpx2K1YpUPluVluaucBl8JNdKCHFd +CSbHKISwNJ0u5iopwa8VMFR5zWWzxMRXIonJxAK3GEW9Qbk+xxfIX47kkqVktK6G+pJaEk +ov41e2QAAAEQwljQZcJY0GUAAAATZWNkc2Etc2hhMi1uaXN0cDUyMQAAAAhuaXN0cDUyMQ +AAAIUEAeq5RiCRT3AmtzENZW12q1+p/pIEN08NHHAOXs4KiFXZQ27Yp2MnBKPyPR+w94x0 +eNKK/04KcditWKVD5blZbmrnAZfCTXSghxXQkmxyiEsDSdLuYqKcGvFTBUec1ls8TEVyKJ +ycQCtxhFvUG5PscXyF+O5JKlZLSuhvqSWhJKL+NXtkAAAAQVXt20tSeLzMU1U2nMv8CEEY +Oyl1WIkGAcRatDBAfsE0+NcJ/eSbPXywWAqCzOElQ5ftNFz9t1kNXwW5qiLaaIBpAAAAD2 +ZoZW5uZWtlQExhcHRvcAECAwQ= +-----END OPENSSH PRIVATE KEY----- diff --git a/src/main/java/com/hierynomus/sshj/key/KeyAlgorithms.java b/src/main/java/com/hierynomus/sshj/key/KeyAlgorithms.java index 4f6f6d8c..449308ec 100644 --- a/src/main/java/com/hierynomus/sshj/key/KeyAlgorithms.java +++ b/src/main/java/com/hierynomus/sshj/key/KeyAlgorithms.java @@ -22,8 +22,13 @@ import net.schmizz.sshj.signature.SignatureDSA; import net.schmizz.sshj.signature.SignatureECDSA; import net.schmizz.sshj.signature.SignatureRSA; +import java.util.Arrays; +import java.util.List; + public class KeyAlgorithms { + public static List SSH_RSA_SHA2_ALGORITHMS = Arrays.asList("rsa-sha2-512", "rsa-sha2-256"); + public static Factory SSHRSA() { return new Factory("ssh-rsa", new SignatureRSA.FactorySSHRSA(), KeyType.RSA); } public static Factory SSHRSACertV01() { return new Factory("ssh-rsa-cert-v01@openssh.com", new SignatureRSA.FactoryCERT(), KeyType.RSA_CERT); } public static Factory RSASHA256() { return new Factory("rsa-sha2-256", new SignatureRSA.FactoryRSASHA256(), KeyType.RSA); } diff --git a/src/main/java/net/schmizz/sshj/transport/KeyExchanger.java b/src/main/java/net/schmizz/sshj/transport/KeyExchanger.java index 93686255..eedaccae 100644 --- a/src/main/java/net/schmizz/sshj/transport/KeyExchanger.java +++ b/src/main/java/net/schmizz/sshj/transport/KeyExchanger.java @@ -231,12 +231,9 @@ final class KeyExchanger } kex = Factory.Named.Util.create(transport.getConfig().getKeyExchangeFactories(), negotiatedAlgs.getKeyExchangeAlgorithm()); - - List keyAlgorithms = new ArrayList(); - for (String signatureAlgorithm : negotiatedAlgs.getSignatureAlgorithms()) { - keyAlgorithms.add(Factory.Named.Util.create(transport.getConfig().getKeyAlgorithms(), signatureAlgorithm)); - } - transport.setKeyAlgorithms(keyAlgorithms); + transport.setHostKeyAlgorithm(Factory.Named.Util.create(transport.getConfig().getKeyAlgorithms(), + negotiatedAlgs.getSignatureAlgorithm())); + transport.setRSASHA2Support(negotiatedAlgs.getRSASHA2Support()); try { kex.init(transport, diff --git a/src/main/java/net/schmizz/sshj/transport/NegotiatedAlgorithms.java b/src/main/java/net/schmizz/sshj/transport/NegotiatedAlgorithms.java index be78566d..b1c97b53 100644 --- a/src/main/java/net/schmizz/sshj/transport/NegotiatedAlgorithms.java +++ b/src/main/java/net/schmizz/sshj/transport/NegotiatedAlgorithms.java @@ -15,12 +15,10 @@ */ package net.schmizz.sshj.transport; -import java.util.List; - public final class NegotiatedAlgorithms { private final String kex; - private final List availableSigs; + private final String sig; private final String c2sCipher; private final String s2cCipher; private final String c2sMAC; @@ -28,24 +26,27 @@ public final class NegotiatedAlgorithms { private final String c2sComp; private final String s2cComp; - NegotiatedAlgorithms(String kex, List availableSigs, String c2sCipher, String s2cCipher, String c2sMAC, String s2cMAC, - String c2sComp, String s2cComp) { + private final boolean rsaSHA2Support; + + NegotiatedAlgorithms(String kex, String sig, String c2sCipher, String s2cCipher, String c2sMAC, String s2cMAC, + String c2sComp, String s2cComp, boolean rsaSHA2Support) { this.kex = kex; - this.availableSigs = availableSigs; + this.sig = sig; this.c2sCipher = c2sCipher; this.s2cCipher = s2cCipher; this.c2sMAC = c2sMAC; this.s2cMAC = s2cMAC; this.c2sComp = c2sComp; this.s2cComp = s2cComp; + this.rsaSHA2Support = rsaSHA2Support; } public String getKeyExchangeAlgorithm() { return kex; } - public List getSignatureAlgorithms() { - return availableSigs; + public String getSignatureAlgorithm() { + return sig; } public String getClient2ServerCipherAlgorithm() { @@ -72,17 +73,22 @@ public final class NegotiatedAlgorithms { return s2cComp; } + public boolean getRSASHA2Support() { + return rsaSHA2Support; + } + @Override public String toString() { return ("[ " + "kex=" + kex + "; " + - "availableSigs=" + availableSigs + "; " + + "sig=" + sig + "; " + "c2sCipher=" + c2sCipher + "; " + "s2cCipher=" + s2cCipher + "; " + "c2sMAC=" + c2sMAC + "; " + "s2cMAC=" + s2cMAC + "; " + "c2sComp=" + c2sComp + "; " + - "s2cComp=" + s2cComp + + "s2cComp=" + s2cComp + "; " + + "rsaSHA2Support=" + rsaSHA2Support + " ]"); } diff --git a/src/main/java/net/schmizz/sshj/transport/Proposal.java b/src/main/java/net/schmizz/sshj/transport/Proposal.java index a4b0b69f..1176ff9d 100644 --- a/src/main/java/net/schmizz/sshj/transport/Proposal.java +++ b/src/main/java/net/schmizz/sshj/transport/Proposal.java @@ -15,6 +15,7 @@ */ package net.schmizz.sshj.transport; +import com.hierynomus.sshj.key.KeyAlgorithms; import net.schmizz.sshj.Config; import net.schmizz.sshj.common.Buffer; import net.schmizz.sshj.common.Factory; @@ -91,7 +92,7 @@ class Proposal { return kex; } - public List getSignatureAlgorithms() { + public List getHostKeyAlgorithms() { return sig; } @@ -127,13 +128,14 @@ class Proposal { throws TransportException { return new NegotiatedAlgorithms( firstMatch(this.getKeyExchangeAlgorithms(), other.getKeyExchangeAlgorithms()), - allMatch(this.getSignatureAlgorithms(), other.getSignatureAlgorithms()), + firstMatch(this.getHostKeyAlgorithms(), other.getHostKeyAlgorithms()), firstMatch(this.getClient2ServerCipherAlgorithms(), other.getClient2ServerCipherAlgorithms()), firstMatch(this.getServer2ClientCipherAlgorithms(), other.getServer2ClientCipherAlgorithms()), firstMatch(this.getClient2ServerMACAlgorithms(), other.getClient2ServerMACAlgorithms()), firstMatch(this.getServer2ClientMACAlgorithms(), other.getServer2ClientMACAlgorithms()), firstMatch(this.getClient2ServerCompressionAlgorithms(), other.getClient2ServerCompressionAlgorithms()), - firstMatch(this.getServer2ClientCompressionAlgorithms(), other.getServer2ClientCompressionAlgorithms()) + firstMatch(this.getServer2ClientCompressionAlgorithms(), other.getServer2ClientCompressionAlgorithms()), + other.getHostKeyAlgorithms().containsAll(KeyAlgorithms.SSH_RSA_SHA2_ALGORITHMS) ); } diff --git a/src/main/java/net/schmizz/sshj/transport/Transport.java b/src/main/java/net/schmizz/sshj/transport/Transport.java index 6ae9f8f1..a04073af 100644 --- a/src/main/java/net/schmizz/sshj/transport/Transport.java +++ b/src/main/java/net/schmizz/sshj/transport/Transport.java @@ -238,5 +238,6 @@ public interface Transport */ void die(Exception e); - KeyAlgorithm getKeyAlgorithm(KeyType keyType) throws TransportException; + KeyAlgorithm getHostKeyAlgorithm(); + KeyAlgorithm getClientKeyAlgorithm(KeyType keyType) throws TransportException; } diff --git a/src/main/java/net/schmizz/sshj/transport/TransportImpl.java b/src/main/java/net/schmizz/sshj/transport/TransportImpl.java index 50e31d53..148feef2 100644 --- a/src/main/java/net/schmizz/sshj/transport/TransportImpl.java +++ b/src/main/java/net/schmizz/sshj/transport/TransportImpl.java @@ -16,6 +16,7 @@ package net.schmizz.sshj.transport; import com.hierynomus.sshj.key.KeyAlgorithm; +import com.hierynomus.sshj.key.KeyAlgorithms; import com.hierynomus.sshj.transport.IdentificationStringParser; import net.schmizz.concurrent.ErrorDeliveryUtil; import net.schmizz.concurrent.Event; @@ -89,7 +90,9 @@ public final class TransportImpl private final Decoder decoder; - private List keyAlgorithms; + private KeyAlgorithm hostKeyAlgorithm; + + private boolean rsaSHA2Support; private final Event serviceAccept; @@ -657,18 +660,30 @@ public final class TransportImpl return connInfo; } + public void setHostKeyAlgorithm(KeyAlgorithm keyAlgorithm) { + this.hostKeyAlgorithm = keyAlgorithm; + } + @Override - public KeyAlgorithm getKeyAlgorithm(KeyType keyType) throws TransportException { - for (KeyAlgorithm ka : keyAlgorithms) { - if (ka.getKeyFormat().equals(keyType)) { - return ka; - } + public KeyAlgorithm getHostKeyAlgorithm() { + return this.hostKeyAlgorithm; + } + + public void setRSASHA2Support(boolean rsaSHA2Support) { + this.rsaSHA2Support = rsaSHA2Support; + } + + @Override + public KeyAlgorithm getClientKeyAlgorithm(KeyType keyType) throws TransportException { + if (keyType != KeyType.RSA || !rsaSHA2Support) { + return Factory.Named.Util.create(getConfig().getKeyAlgorithms(), keyType.toString()); } + List> factories = getConfig().getKeyAlgorithms(); + if (factories != null) + for (Factory.Named f : factories) + if (f.getName().equals("ssh-rsa") || KeyAlgorithms.SSH_RSA_SHA2_ALGORITHMS.contains(f.getName())) + return f.create(); throw new TransportException("Cannot find an available KeyAlgorithm for type " + keyType); } - - public void setKeyAlgorithms(List keyAlgorithms) { - this.keyAlgorithms = keyAlgorithms; - } } diff --git a/src/main/java/net/schmizz/sshj/transport/kex/AbstractDHG.java b/src/main/java/net/schmizz/sshj/transport/kex/AbstractDHG.java index f82d45dd..ed307e4a 100644 --- a/src/main/java/net/schmizz/sshj/transport/kex/AbstractDHG.java +++ b/src/main/java/net/schmizz/sshj/transport/kex/AbstractDHG.java @@ -79,7 +79,7 @@ public abstract class AbstractDHG extends AbstractDH H = digest.digest(); - Signature signature = trans.getKeyAlgorithm(KeyType.fromKey(hostKey)).newSignature(); + Signature signature = trans.getHostKeyAlgorithm().newSignature(); signature.initVerify(hostKey); signature.update(H, 0, H.length); if (!signature.verify(sig)) diff --git a/src/main/java/net/schmizz/sshj/transport/kex/AbstractDHGex.java b/src/main/java/net/schmizz/sshj/transport/kex/AbstractDHGex.java index 8a62beb4..4c6bb8ec 100644 --- a/src/main/java/net/schmizz/sshj/transport/kex/AbstractDHGex.java +++ b/src/main/java/net/schmizz/sshj/transport/kex/AbstractDHGex.java @@ -85,7 +85,7 @@ public abstract class AbstractDHGex extends AbstractDH { .putMPInt(k); digest.update(buf.array(), buf.rpos(), buf.available()); H = digest.digest(); - KeyAlgorithm keyAlgorithm = trans.getKeyAlgorithm(KeyType.fromKey(hostKey)); + KeyAlgorithm keyAlgorithm = trans.getHostKeyAlgorithm(); Signature signature = keyAlgorithm.newSignature(); signature.initVerify(hostKey); signature.update(H, 0, H.length); diff --git a/src/main/java/net/schmizz/sshj/userauth/method/KeyedAuthMethod.java b/src/main/java/net/schmizz/sshj/userauth/method/KeyedAuthMethod.java index e03eb9ff..38ae608e 100644 --- a/src/main/java/net/schmizz/sshj/userauth/method/KeyedAuthMethod.java +++ b/src/main/java/net/schmizz/sshj/userauth/method/KeyedAuthMethod.java @@ -50,7 +50,7 @@ public abstract class KeyedAuthMethod // public key as 2 strings: [ key type | key blob ] KeyType keyType = KeyType.fromKey(key); try { - KeyAlgorithm ka = params.getTransport().getKeyAlgorithm(keyType); + KeyAlgorithm ka = params.getTransport().getClientKeyAlgorithm(keyType); reqBuf.putString(ka.getKeyAlgorithm()) .putString(new Buffer.PlainBuffer().putPublicKey(key).getCompactData()); return reqBuf; @@ -71,7 +71,7 @@ public abstract class KeyedAuthMethod final KeyType kt = KeyType.fromKey(key); Signature signature; try { - signature = params.getTransport().getKeyAlgorithm(kt).newSignature(); + signature = params.getTransport().getClientKeyAlgorithm(kt).newSignature(); } catch (TransportException e) { throw new UserAuthException("No KeyAlgorithm configured for key " + kt); }