From 68b924863e7116e1c9ccafd88351a625c1dc7e38 Mon Sep 17 00:00:00 2001 From: David Solin Date: Fri, 9 Sep 2016 14:39:21 -0500 Subject: [PATCH 1/3] Fail gracefully when reading an OpenSSH Known Hosts file that uses key types requiring BouncyCastle, but we're not including BouncyCastle. --- .../java/net/schmizz/sshj/common/Buffer.java | 14 ++++++++++++-- .../java/net/schmizz/sshj/common/KeyType.java | 19 +++++++++---------- .../verification/OpenSSHKnownHosts.java | 7 +------ 3 files changed, 22 insertions(+), 18 deletions(-) diff --git a/src/main/java/net/schmizz/sshj/common/Buffer.java b/src/main/java/net/schmizz/sshj/common/Buffer.java index 55a2dd42..b3b8c9e5 100644 --- a/src/main/java/net/schmizz/sshj/common/Buffer.java +++ b/src/main/java/net/schmizz/sshj/common/Buffer.java @@ -426,8 +426,18 @@ public class Buffer> { public PublicKey readPublicKey() throws BufferException { try { - final String type = readString(); - return KeyType.fromString(type).readPubKeyFromBuffer(type, this); + final KeyType type = KeyType.fromString(readString()); + switch(type) { + case RSA: + case DSA: + return type.readPubKeyFromBuffer(this); + default: + if (SecurityUtils.isBouncyCastleRegistered()) { + return type.readPubKeyFromBuffer(this); + } else { + throw new BufferException("BouncyCastle is required to read a key of type " + type); + } + } } catch (GeneralSecurityException e) { throw new SSHRuntimeException(e); } diff --git a/src/main/java/net/schmizz/sshj/common/KeyType.java b/src/main/java/net/schmizz/sshj/common/KeyType.java index a2458323..e30ecb96 100644 --- a/src/main/java/net/schmizz/sshj/common/KeyType.java +++ b/src/main/java/net/schmizz/sshj/common/KeyType.java @@ -46,7 +46,7 @@ public enum KeyType { /** SSH identifier for RSA keys */ RSA("ssh-rsa") { @Override - public PublicKey readPubKeyFromBuffer(String type, Buffer buf) + public PublicKey readPubKeyFromBuffer(Buffer buf) throws GeneralSecurityException { final BigInteger e, n; try { @@ -77,7 +77,7 @@ public enum KeyType { /** SSH identifier for DSA keys */ DSA("ssh-dss") { @Override - public PublicKey readPubKeyFromBuffer(String type, Buffer buf) + public PublicKey readPubKeyFromBuffer(Buffer buf) throws GeneralSecurityException { BigInteger p, q, g, y; try { @@ -114,7 +114,7 @@ public enum KeyType { private final Logger log = LoggerFactory.getLogger(getClass()); @Override - public PublicKey readPubKeyFromBuffer(String type, Buffer buf) + public PublicKey readPubKeyFromBuffer(Buffer buf) throws GeneralSecurityException { try { // final String algo = buf.readString(); it has been already read @@ -127,7 +127,7 @@ public enum KeyType { buf.readRawBytes(y); if(log.isDebugEnabled()) { log.debug(String.format("Key algo: %s, Key curve: %s, Key Len: %s, 0x04: %s\nx: %s\ny: %s", - type, + sType, curveName, keyLen, x04, @@ -176,14 +176,14 @@ public enum KeyType { ED25519("ssh-ed25519") { private final Logger log = LoggerFactory.getLogger(KeyType.class); @Override - public PublicKey readPubKeyFromBuffer(String type, Buffer buf) throws GeneralSecurityException { + public PublicKey readPubKeyFromBuffer(Buffer buf) throws GeneralSecurityException { try { final int keyLen = buf.readUInt32AsInt(); final byte[] p = new byte[keyLen]; buf.readRawBytes(p); if (log.isDebugEnabled()) { log.debug(String.format("Key algo: %s, Key curve: 25519, Key Len: %s\np: %s", - type, + sType, keyLen, Arrays.toString(p)) ); @@ -213,9 +213,9 @@ public enum KeyType { /** Unrecognized */ UNKNOWN("unknown") { @Override - public PublicKey readPubKeyFromBuffer(String type, Buffer buf) + public PublicKey readPubKeyFromBuffer(Buffer buf) throws GeneralSecurityException { - throw new UnsupportedOperationException("Don't know how to decode key:" + type); + throw new UnsupportedOperationException("Don't know how to decode key:" + sType); } @Override @@ -238,7 +238,7 @@ public enum KeyType { this.sType = type; } - public abstract PublicKey readPubKeyFromBuffer(String type, Buffer buf) + public abstract PublicKey readPubKeyFromBuffer(Buffer buf) throws GeneralSecurityException; public abstract void putPubKeyIntoBuffer(PublicKey pk, Buffer buf); @@ -263,5 +263,4 @@ public enum KeyType { public String toString() { return sType; } - } diff --git a/src/main/java/net/schmizz/sshj/transport/verification/OpenSSHKnownHosts.java b/src/main/java/net/schmizz/sshj/transport/verification/OpenSSHKnownHosts.java index dbd23aff..ee6eca16 100644 --- a/src/main/java/net/schmizz/sshj/transport/verification/OpenSSHKnownHosts.java +++ b/src/main/java/net/schmizz/sshj/transport/verification/OpenSSHKnownHosts.java @@ -207,7 +207,7 @@ public class OpenSSHKnownHosts if (type != KeyType.UNKNOWN) { final String sKey = split[i++]; - key = getKey(sKey); + key = new Buffer.PlainBuffer(Base64.decode(sKey)).readPublicKey(); } else if (isBits(sType)) { type = KeyType.RSA; // int bits = Integer.valueOf(sType); @@ -232,11 +232,6 @@ public class OpenSSHKnownHosts } } - private PublicKey getKey(String sKey) - throws IOException { - return new Buffer.PlainBuffer(Base64.decode(sKey)).readPublicKey(); - } - private boolean isBits(String type) { try { Integer.parseInt(type); From e420593fa9af3737531ab937eb6dd60d594c24e5 Mon Sep 17 00:00:00 2001 From: David Solin Date: Fri, 9 Sep 2016 16:03:45 -0500 Subject: [PATCH 2/3] Okay, more spaces Codacy, you win! --- .../java/net/schmizz/sshj/common/Buffer.java | 18 +++---- .../userauth/keyprovider/PKCS5KeyFile.java | 50 +++++++++---------- 2 files changed, 34 insertions(+), 34 deletions(-) diff --git a/src/main/java/net/schmizz/sshj/common/Buffer.java b/src/main/java/net/schmizz/sshj/common/Buffer.java index b3b8c9e5..aab13eb6 100644 --- a/src/main/java/net/schmizz/sshj/common/Buffer.java +++ b/src/main/java/net/schmizz/sshj/common/Buffer.java @@ -427,16 +427,16 @@ public class Buffer> { throws BufferException { try { final KeyType type = KeyType.fromString(readString()); - switch(type) { - case RSA: - case DSA: - return type.readPubKeyFromBuffer(this); - default: - if (SecurityUtils.isBouncyCastleRegistered()) { + switch (type) { + case RSA: + case DSA: return type.readPubKeyFromBuffer(this); - } else { - throw new BufferException("BouncyCastle is required to read a key of type " + type); - } + default: + if (SecurityUtils.isBouncyCastleRegistered()) { + return type.readPubKeyFromBuffer(this); + } else { + throw new BufferException("BouncyCastle is required to read a key of type " + type); + } } } catch (GeneralSecurityException e) { throw new SSHRuntimeException(e); diff --git a/src/main/java/net/schmizz/sshj/userauth/keyprovider/PKCS5KeyFile.java b/src/main/java/net/schmizz/sshj/userauth/keyprovider/PKCS5KeyFile.java index 8652c36f..8f31ca7e 100644 --- a/src/main/java/net/schmizz/sshj/userauth/keyprovider/PKCS5KeyFile.java +++ b/src/main/java/net/schmizz/sshj/userauth/keyprovider/PKCS5KeyFile.java @@ -194,31 +194,31 @@ public class PKCS5KeyFile throw new FormatException("PKCS5 header not found"); } ASN1Data asn = new ASN1Data(data = decrypt(Base64.decode(sb.toString()), cipher, iv)); - switch(type) { - case RSA: { - KeyFactory factory = KeyFactory.getInstance("RSA"); - asn.readNext(); - BigInteger modulus = asn.readNext(); - BigInteger pubExp = asn.readNext(); - BigInteger prvExp = asn.readNext(); - PublicKey pubKey = factory.generatePublic(new RSAPublicKeySpec(modulus, pubExp)); - PrivateKey prvKey = factory.generatePrivate(new RSAPrivateKeySpec(modulus, prvExp)); - return new KeyPair(pubKey, prvKey); - } - case DSA: { - KeyFactory factory = KeyFactory.getInstance("DSA"); - asn.readNext(); - BigInteger p = asn.readNext(); - BigInteger q = asn.readNext(); - BigInteger g = asn.readNext(); - BigInteger pub = asn.readNext(); - BigInteger prv = asn.readNext(); - PublicKey pubKey = factory.generatePublic(new DSAPublicKeySpec(pub, p, q, g)); - PrivateKey prvKey = factory.generatePrivate(new DSAPrivateKeySpec(prv, p, q, g)); - return new KeyPair(pubKey, prvKey); - } - default: - throw new IOException("Unrecognized PKCS5 key type: " + type); + switch (type) { + case RSA: { + KeyFactory factory = KeyFactory.getInstance("RSA"); + asn.readNext(); + BigInteger modulus = asn.readNext(); + BigInteger pubExp = asn.readNext(); + BigInteger prvExp = asn.readNext(); + PublicKey pubKey = factory.generatePublic(new RSAPublicKeySpec(modulus, pubExp)); + PrivateKey prvKey = factory.generatePrivate(new RSAPrivateKeySpec(modulus, prvExp)); + return new KeyPair(pubKey, prvKey); + } + case DSA: { + KeyFactory factory = KeyFactory.getInstance("DSA"); + asn.readNext(); + BigInteger p = asn.readNext(); + BigInteger q = asn.readNext(); + BigInteger g = asn.readNext(); + BigInteger pub = asn.readNext(); + BigInteger prv = asn.readNext(); + PublicKey pubKey = factory.generatePublic(new DSAPublicKeySpec(pub, p, q, g)); + PrivateKey prvKey = factory.generatePrivate(new DSAPrivateKeySpec(prv, p, q, g)); + return new KeyPair(pubKey, prvKey); + } + default: + throw new IOException("Unrecognized PKCS5 key type: " + type); } } catch (NoSuchAlgorithmException e) { throw new IOException(e); From 6185ac4db89fcf09c6b41e64545082b31f2b058e Mon Sep 17 00:00:00 2001 From: David Solin Date: Tue, 13 Sep 2016 08:04:15 -0500 Subject: [PATCH 3/3] Improvements per JVE. --- src/main/java/net/schmizz/sshj/common/Buffer.java | 13 +------------ src/main/java/net/schmizz/sshj/common/KeyType.java | 3 +++ .../transport/verification/OpenSSHKnownHosts.java | 5 ++++- 3 files changed, 8 insertions(+), 13 deletions(-) diff --git a/src/main/java/net/schmizz/sshj/common/Buffer.java b/src/main/java/net/schmizz/sshj/common/Buffer.java index aab13eb6..2c809774 100644 --- a/src/main/java/net/schmizz/sshj/common/Buffer.java +++ b/src/main/java/net/schmizz/sshj/common/Buffer.java @@ -426,18 +426,7 @@ public class Buffer> { public PublicKey readPublicKey() throws BufferException { try { - final KeyType type = KeyType.fromString(readString()); - switch (type) { - case RSA: - case DSA: - return type.readPubKeyFromBuffer(this); - default: - if (SecurityUtils.isBouncyCastleRegistered()) { - return type.readPubKeyFromBuffer(this); - } else { - throw new BufferException("BouncyCastle is required to read a key of type " + type); - } - } + return KeyType.fromString(readString()).readPubKeyFromBuffer(this); } catch (GeneralSecurityException e) { throw new SSHRuntimeException(e); } diff --git a/src/main/java/net/schmizz/sshj/common/KeyType.java b/src/main/java/net/schmizz/sshj/common/KeyType.java index e30ecb96..7d6f62fc 100644 --- a/src/main/java/net/schmizz/sshj/common/KeyType.java +++ b/src/main/java/net/schmizz/sshj/common/KeyType.java @@ -116,6 +116,9 @@ public enum KeyType { @Override public PublicKey readPubKeyFromBuffer(Buffer buf) throws GeneralSecurityException { + if (!SecurityUtils.isBouncyCastleRegistered()) { + throw new GeneralSecurityException("BouncyCastle is required to read a key of type " + sType); + } try { // final String algo = buf.readString(); it has been already read final String curveName = buf.readString(); diff --git a/src/main/java/net/schmizz/sshj/transport/verification/OpenSSHKnownHosts.java b/src/main/java/net/schmizz/sshj/transport/verification/OpenSSHKnownHosts.java index ee6eca16..eb1a7d0d 100644 --- a/src/main/java/net/schmizz/sshj/transport/verification/OpenSSHKnownHosts.java +++ b/src/main/java/net/schmizz/sshj/transport/verification/OpenSSHKnownHosts.java @@ -57,7 +57,7 @@ public class OpenSSHKnownHosts try { // Read in the file, storing each line as an entry String line; - while ((line = br.readLine()) != null) + while ((line = br.readLine()) != null) { try { HostEntry entry = entryFactory.parseEntry(line); if (entry != null) { @@ -65,7 +65,10 @@ public class OpenSSHKnownHosts } } catch (SSHException ignore) { log.debug("Bad line ({}): {} ", ignore.toString(), line); + } catch (SSHRuntimeException ignore) { + log.debug("Failed to process line ({}): {} ", ignore.toString(), line); } + } } finally { IOUtils.closeQuietly(br); }