Skip to content

Commit

Permalink
[SSHD-704] Fix DH KEX for curve25519 and enable curve25519 and curve448
Browse files Browse the repository at this point in the history 
Simplify MontgomeryCurve, take advantage of the fact that the key is
always the last 32 or 56 bytes of the X.509 DER encoding. Add extensive
comments about the "magic" constants. Be more strict about key data
with an extra leading zero byte.

Enable the new KEX algorithms in BaseBuilder.

At that point, curve25519 KEX worked only if both the client's and the
server's ephemeral public keys byte arrays did not have the most
significant bit in the first byte set. This uncovered a bug in DHGClient
and DHGServer: ECDH and XDH KEX encode Q_C and Q_S as plain byte arrays,
not as multi-precision integers. I.e., they _do not_ prefix a zero byte
if the high bit in the first byte of the value is set.

Fix this by introducing AbstractDH.putE(Buffer) and putF(Buffer)
methods, so that individual DH implementations can override as
appropriate. Override them for ECDH and XDH to write byte arrays, not
mpints.

* RFC 4253, section 8:[1] encodes e and f as mpint.
* RFC 5656, section 4:[2] Q_C and Q_S, which take the place of e and f,
  are written as "strings" (byte arrays).
* RFC 8731, section 3:[3] Public ephemeral keys are written as strings.

With these changes, KEX using curve25519 works, and KEX using one of
the other already existing algorithms also still works.

[1] https://tools.ietf.org/html/rfc4253#page-22
[2] https://tools.ietf.org/html/rfc5656#page-9
[3] https://tools.ietf.org/html/rfc8731#section-3
  • Loading branch information
@tomaswolf
tomaswolf committed Nov 7, 2021
1 parent 5c0c8ab commit 84a35b0
Show file tree
Hide file tree
Showing 9 changed files with 124 additions and 46 deletions.
2 changes: 1 addition & 1 deletion sshd-common/src/main/java/org/apache/sshd/common/util/buffer/BufferUtils.java
View file
Expand Up @@ -444,7 +444,7 @@ public static BigInteger fromMPIntBytes(byte[] mpInt) {
}

if ((mpInt[0] & 0x80) != 0) {
return new BigInteger(0, mpInt);
return new BigInteger(1, mpInt);
} else {
return new BigInteger(mpInt);
}
Expand Down
6 changes: 3 additions & 3 deletions sshd-core/src/main/java/org/apache/sshd/client/kex/DHGClient.java
View file
Expand Up @@ -102,7 +102,7 @@ public void init(byte[] v_s, byte[] v_c, byte[] i_s, byte[] i_c) throws Exceptio
log.debug("init({})[{}] Send SSH_MSG_KEXDH_INIT", this, s);
}
Buffer buffer = s.createBuffer(SshConstants.SSH_MSG_KEXDH_INIT, e.length + Integer.SIZE);
buffer.putMPInt(e);
dh.putE(buffer, e);

s.writePacket(buffer);
}
Expand Down Expand Up @@ -167,8 +167,8 @@ public boolean next(int cmd, Buffer buffer) throws Exception {
buffer.putBytes(i_c);
buffer.putBytes(i_s);
buffer.putBytes(k_s);
buffer.putMPInt(getE());
buffer.putMPInt(f);
dh.putE(buffer, getE());
dh.putF(buffer, f);
buffer.putMPInt(k);
hash.update(buffer.array(), 0, buffer.available());
h = hash.digest();
Expand Down
3 changes: 3 additions & 0 deletions sshd-core/src/main/java/org/apache/sshd/common/BaseBuilder.java
View file
Expand Up @@ -87,6 +87,9 @@ public class BaseBuilder<T extends AbstractFactoryManager, S extends BaseBuilder
*/
public static final List<BuiltinDHFactories> DEFAULT_KEX_PREFERENCE = Collections.unmodifiableList(
Arrays.asList(
BuiltinDHFactories.curve25519,
BuiltinDHFactories.curve25519_libssh,
BuiltinDHFactories.curve448,
BuiltinDHFactories.ecdhp521,
BuiltinDHFactories.ecdhp384,
BuiltinDHFactories.ecdhp256,
Expand Down
11 changes: 11 additions & 0 deletions sshd-core/src/main/java/org/apache/sshd/common/kex/AbstractDH.java
View file
Expand Up @@ -22,6 +22,7 @@

import org.apache.sshd.common.digest.Digest;
import org.apache.sshd.common.util.NumberUtils;
import org.apache.sshd.common.util.buffer.Buffer;

/**
* Base class for the Diffie-Hellman key agreement.
Expand Down Expand Up @@ -63,6 +64,16 @@ public byte[] getE() throws Exception {
return e_array;
}

public void putE(Buffer buffer, byte[] e) {
// RFC 4253, section 8: e and f are encoded as mpints.
buffer.putMPInt(e);
}

public void putF(Buffer buffer, byte[] f) {
// RFC 4253, section 8: e and f are encoded as mpints.
buffer.putMPInt(f);
}

public boolean isSharedSecretAvailable() {
return k_array != null;
}
Expand Down
15 changes: 15 additions & 0 deletions sshd-core/src/main/java/org/apache/sshd/common/kex/ECDH.java
View file
Expand Up @@ -32,6 +32,7 @@
import org.apache.sshd.common.config.keys.KeyUtils;
import org.apache.sshd.common.digest.Digest;
import org.apache.sshd.common.util.ValidateUtils;
import org.apache.sshd.common.util.buffer.Buffer;
import org.apache.sshd.common.util.security.SecurityUtils;

/**
Expand Down Expand Up @@ -100,6 +101,20 @@ public void setF(byte[] f) {
this.f = ECCurves.octetStringToEcPoint(f);
}

@Override
public void putE(Buffer buffer, byte[] e) {
// RFC 5656, section 4: Q_C and Q_S, which take the place of e and f, are written as "strings", i.e., byte
// arrays.
buffer.putBytes(e);
}

@Override
public void putF(Buffer buffer, byte[] f) {
// RFC 5656, section 4: Q_C and Q_S, which take the place of e and f, are written as "strings", i.e., byte
// arrays.
buffer.putBytes(f);
}

@Override
public Digest getHash() throws Exception {
if (curve == null) {
Expand Down
105 changes: 67 additions & 38 deletions sshd-core/src/main/java/org/apache/sshd/common/kex/MontgomeryCurve.java
View file
Expand Up @@ -16,12 +16,8 @@
* specific language governing permissions and limitations
* under the License.
*/

package org.apache.sshd.common.kex;

import java.io.IOException;
import java.io.StreamCorruptedException;
import java.io.UncheckedIOException;
import java.security.GeneralSecurityException;
import java.security.InvalidKeyException;
import java.security.KeyFactory;
Expand All @@ -30,6 +26,7 @@
import java.security.PublicKey;
import java.security.spec.InvalidKeySpecException;
import java.security.spec.X509EncodedKeySpec;
import java.util.Arrays;

import javax.crypto.KeyAgreement;

Expand All @@ -38,25 +35,66 @@
import org.apache.sshd.common.digest.Digest;
import org.apache.sshd.common.digest.DigestFactory;
import org.apache.sshd.common.keyprovider.KeySizeIndicator;
import org.apache.sshd.common.util.io.der.ASN1Object;
import org.apache.sshd.common.util.io.der.DERParser;
import org.apache.sshd.common.util.security.SecurityUtils;

/**
* Provides implementation details for Montgomery curves and their key exchange algorithms Curve25519/X25519 and
* Curve448/X448 specified in RFC 7748 and RFC 8731. Montgomery curves provide improved security and flexibility over
* Weierstrass curves used in ECDH.
*
* @see <a href="https://www.rfc-editor.org/info/rfc7748">RFC 7748</a>
* @see <a href="https://www.rfc-editor.org/info/rfc8731">RFC 8731</a>
* @see <a href="https://tools.ietf.org/html/rfc7748">RFC 7748</a>
* @see <a href="https://tools.ietf.org/html/rfc8731">RFC 8731</a>
*/
public enum MontgomeryCurve implements KeySizeIndicator, OptionalFeature {

/**
* The "magic" bytes below are the beginning of a DER encoding of the ASN.1 of the SubjectPublicKeyInfo as specified
* in <a href="https://tools.ietf.org/html/rfc8410">RFC 8410</a>, sections 3 and 4.
*
* <pre>
* AlgorithmIdentifier ::= SEQUENCE {
* algorithm OBJECT IDENTIFIER,
* parameters ANY DEFINED BY algorithm OPTIONAL -- absent for these keys
* }
* SubjectPublicKeyInfo ::= SEQUENCE {
* algorithm AlgorithmIdentifier,
* subjectPublicKey BIT STRING
* }
* </pre>
* <p>
* If we take it apart the first one for x25519:
* </p>
*
* <pre>
* 0x30 - SEQUENCE (start of SubjectPublicKeyInfo)
* 0x2a - of 42 bytes
* 0x30 - SEQUENCE (start of AlgorithmIdentifier)
* 0x05 - of 5 bytes
* 0x06 - OID
* 0x03 - of 3 bytes
* 0x2b - 1 3 (encoded as 1*40 + 3 = 43 = 0x2b)
* 0x65 - 101
* 0x6e - 110
* 0x03 - BIT STRING
* 0x21 - of 33 bytes
* 0x00 - NUL byte
* </pre>
* <p>
* If one appends now the 32 public key bytes, the DER encoding for the x25519 public key is complete. The NUL byte
* at the end ensures that the raw key bytes appended are always interpreted as unsigned, even if the most
* significant bit is set.
* </p>
* <p>
* The OID for x25519 is { 1 3 101 110 }, for x448 { 1 3 101 111 }.
* </p>
*/

/**
* X25519 uses Curve25519 and SHA-256 with a 32-byte key size.
*/
x25519("X25519", 32, BuiltinDigests.sha256,
new byte[] { 0x30, 0x2a, 0x30, 0x05, 0x06, 0x03, 0x2b, 0x65, 0x6e, 0x03, 0x21, 0x00 }),

/**
* X448 uses Curve448 and SHA-512 with a 56-byte key size.
*/
Expand All @@ -70,14 +108,12 @@ public enum MontgomeryCurve implements KeySizeIndicator, OptionalFeature {
private final KeyPairGenerator keyPairGenerator;
private final KeyFactory keyFactory;
private final byte[] encodedPublicKeyPrefix;
private final int encodedKeySize;

MontgomeryCurve(String algorithm, int keySize, DigestFactory digestFactory, byte[] encodedPublicKeyPrefix) {
this.algorithm = algorithm;
this.keySize = keySize;
this.digestFactory = digestFactory;
this.encodedPublicKeyPrefix = encodedPublicKeyPrefix;
encodedKeySize = keySize + encodedPublicKeyPrefix.length;
boolean supported;
KeyPairGenerator generator = null;
KeyFactory factory = null;
Expand Down Expand Up @@ -121,38 +157,31 @@ public KeyPair generateKeyPair() {
}

public byte[] encode(PublicKey key) throws InvalidKeyException {
return extractSubjectPublicKey(key.getEncoded());
// Per the ASN.1 of SubjectPublicKeyInfo, the key must be the last keySize bytes of the X.509 encoding
byte[] subjectPublicKeyInfo = key.getEncoded();
byte[] result = Arrays.copyOfRange(subjectPublicKeyInfo, subjectPublicKeyInfo.length - getKeySize(),
subjectPublicKeyInfo.length);
return result;
}

public PublicKey decode(byte[] key) throws InvalidKeySpecException {
if (key.length < getKeySize()) {
throw new InvalidKeySpecException("Provided key is too small for " + getAlgorithm());
int size = getKeySize();
int offset = key.length - size;
// We're lenient here and accept a key prefixed by a zero byte.
if (offset < 0 || offset > 1) {
throw new InvalidKeySpecException("Provided key has wrong length (" + key.length + " bytes) for " + getAlgorithm());
} else if (offset == 1) {
if (key[0] != 0) {
throw new InvalidKeySpecException(
"Provided key for " + getAlgorithm()
+ " has extra byte, but it's non-zero: 0x"
+ Integer.toHexString(key[0] & 0xFF));
}
}
// ideally, we'd just parse the key as a BigInteger and then create a XECPublicKeySpec in Java 11
// BouncyCastle supports a separate API, so we can use the generic X.509 encoding scheme supported by both
byte[] encoded = new byte[encodedKeySize];
System.arraycopy(encodedPublicKeyPrefix, 0, encoded, 0, encodedPublicKeyPrefix.length);
// note that key can be either the raw key data or it may be prefixed by a padding byte and the key length.
// these two bytes are already present as the last two bytes in encodedPublicKeyPrefix, thus there is no harm
// in potentially overwriting it
System.arraycopy(key, 0, encoded, encodedKeySize - key.length, key.length);
// Ideally, we'd just parse the key as a BigInteger and then create a XECPublicKeySpec in Java 11
// BouncyCastle supports a separate API, but we can use the generic X.509 encoding scheme supported by both
byte[] encoded = Arrays.copyOf(encodedPublicKeyPrefix, encodedPublicKeyPrefix.length + size);
System.arraycopy(key, offset, encoded, encodedPublicKeyPrefix.length, size);
return keyFactory.generatePublic(new X509EncodedKeySpec(encoded));
}

private static byte[] extractSubjectPublicKey(byte[] subjectPublicKeyInfo) throws InvalidKeyException {
try {
// SubjectPublicKeyInfo ::= SEQUENCE {
// algorithm AlgorithmIdentifier,
// subjectPublicKey BIT STRING }
ASN1Object spki = new DERParser(subjectPublicKeyInfo).readObject();
DERParser parser = spki.createParser();
parser.readObject();
return parser.readObject().getValue();
} catch (StreamCorruptedException e) {
throw new InvalidKeyException(e);
} catch (IOException e) {
throw new UncheckedIOException(e);
}
}

}
19 changes: 19 additions & 0 deletions sshd-core/src/main/java/org/apache/sshd/common/kex/XDH.java
View file
Expand Up @@ -23,6 +23,7 @@
import java.util.Objects;

import org.apache.sshd.common.digest.Digest;
import org.apache.sshd.common.util.buffer.Buffer;

/**
* Provides Diffie-Hellman SSH key exchange algorithms for the Montgomery curves specified in RFC 8731.
Expand Down Expand Up @@ -51,6 +52,24 @@ public void setF(byte[] f) {
this.f = Objects.requireNonNull(f, "No 'f' value provided");
}

@Override
public void putE(Buffer buffer, byte[] e) {
// RFC 5656, section 4: Q_C and Q_S, which take the place of e and f, are written as "strings", i.e., byte
// arrays.
// RFC 8731, section 3: Public ephemeral keys are encoded for transmission as standard SSH strings. (Q_C and Q_S
// are the client's and server's ephemeral public keys.)
buffer.putBytes(e);
}

@Override
public void putF(Buffer buffer, byte[] f) {
// RFC 5656, section 4: Q_C and Q_S, which take the place of e and f, are written as "strings", i.e., byte
// arrays.
// RFC 8731, section 3: Public ephemeral keys are encoded for transmission as standard SSH strings. (Q_C and Q_S
// are the client's and server's ephemeral public keys.)
buffer.putBytes(f);
}

@Override
protected byte[] calculateK() throws Exception {
Objects.requireNonNull(f, "Missing 'f' value");
Expand Down
6 changes: 3 additions & 3 deletions sshd-core/src/main/java/org/apache/sshd/server/kex/DHGServer.java
View file
Expand Up @@ -121,9 +121,9 @@ public boolean next(int cmd, Buffer buffer) throws Exception {
buffer.putBytes(i_c);
buffer.putBytes(i_s);
buffer.putBytes(k_s);
buffer.putMPInt(e);
dh.putE(buffer, e);
byte[] f = getF();
buffer.putMPInt(f);
dh.putF(buffer, f);
buffer.putMPInt(k);

hash.update(buffer.array(), 0, buffer.available());
Expand All @@ -149,7 +149,7 @@ public boolean next(int cmd, Buffer buffer) throws Exception {

buffer = session.prepareBuffer(SshConstants.SSH_MSG_KEXDH_REPLY, BufferUtils.clear(buffer));
buffer.putBytes(k_s);
buffer.putBytes(f);
dh.putF(buffer, f);
buffer.putBytes(sigH);
session.writePacket(buffer);
return true;
Expand Down
3 changes: 2 additions & 1 deletion sshd-core/src/test/java/org/apache/sshd/DefaultSetupTestSupport.java
View file
Expand Up @@ -75,7 +75,8 @@ public void testNoDeprecatedCiphers() {
@Test
public void testDefaultKeyExchangeList() {
assertSameNamedResourceListNames(KeyExchange.class.getSimpleName(),
BaseBuilder.DEFAULT_KEX_PREFERENCE, factory.getKeyExchangeFactories());
BaseBuilder.DEFAULT_KEX_PREFERENCE.stream().filter(dh -> dh.isSupported()).collect(Collectors.toList()),
factory.getKeyExchangeFactories());
}

@Test // SSHD-1004
Expand Down

0 comments on commit 84a35b0

Please sign in to comment.