From 20a10ae0ae88e264fce930524e08d86d9c67f16e Mon Sep 17 00:00:00 2001 From: zzz Date: Sat, 25 Apr 2026 15:22:21 -0400 Subject: [PATCH] Crypto: Change RFC 7748 DH error to a checked exception so things are cleaned up properly --- .../noise/crypto/x25519/Curve25519.java | 7 ++-- .../java/src/net/i2p/crypto/KeyGenerator.java | 12 +++++-- .../src/net/i2p/crypto/x25519/X25519DH.java | 8 ++++- .../noise/protocol/Curve25519DHState.java | 5 +-- .../southernstorm/noise/protocol/DHState.java | 8 +++-- .../noise/protocol/HandshakeState.java | 11 ++++-- .../noise/protocol/MLKEMDHState.java | 36 +++++++++---------- .../crypto/ratchet/ECIESAEADEngine.java | 2 +- .../i2p/router/crypto/ratchet/RatchetSKM.java | 19 ++++++++-- 9 files changed, 72 insertions(+), 36 deletions(-) diff --git a/core/java/src/com/southernstorm/noise/crypto/x25519/Curve25519.java b/core/java/src/com/southernstorm/noise/crypto/x25519/Curve25519.java index 082ff4953..816119432 100644 --- a/core/java/src/com/southernstorm/noise/crypto/x25519/Curve25519.java +++ b/core/java/src/com/southernstorm/noise/crypto/x25519/Curve25519.java @@ -22,6 +22,7 @@ package com.southernstorm.noise.crypto.x25519; +import java.security.GeneralSecurityException; import java.util.Arrays; /** @@ -471,9 +472,9 @@ public final class Curve25519 { * @param publicKey The public key to use in the evaluation, or null * MUST have MSB high bit cleared, i.e. publicKey[31] & 0x80 == 0 * if the base point of the curve should be used. - * @throws IllegalArgumentException on low-order input see RFC 7748 + * @throws GeneralSecurityException on low-order input see RFC 7748 */ - public static void eval(byte[] result, int offset, byte[] privateKey, byte[] publicKey) + public static void eval(byte[] result, int offset, byte[] privateKey, byte[] publicKey) throws GeneralSecurityException { Curve25519 state = new Curve25519(); try { @@ -530,7 +531,7 @@ public final class Curve25519 { b |= result[offset + index]; } if (b == 0) - throw new IllegalArgumentException("low order input RFC 7748"); + throw new GeneralSecurityException("low order input RFC 7748"); } finally { // Clean up all temporary state before we exit. state.destroy(); diff --git a/core/java/src/net/i2p/crypto/KeyGenerator.java b/core/java/src/net/i2p/crypto/KeyGenerator.java index d7364f416..3ea98b285 100644 --- a/core/java/src/net/i2p/crypto/KeyGenerator.java +++ b/core/java/src/net/i2p/crypto/KeyGenerator.java @@ -206,7 +206,11 @@ public final class KeyGenerator { // worth doing? } while (bpriv[31] == 0); byte[] bpub = new byte[32]; - Curve25519.eval(bpub, 0, bpriv, null); + try { + Curve25519.eval(bpub, 0, bpriv, null); + } catch (GeneralSecurityException gse) { + throw new IllegalArgumentException("Bad keys", gse); + } pub = new PublicKey(type, bpub); priv = new PrivateKey(type, bpriv, pub); break; @@ -245,7 +249,11 @@ public final class KeyGenerator { case MLKEM768_X25519: case MLKEM1024_X25519: data = new byte[32]; - Curve25519.eval(data, 0, priv.getData(), null); + try { + Curve25519.eval(data, 0, priv.getData(), null); + } catch (GeneralSecurityException gse) { + throw new IllegalArgumentException("Bad keys", gse); + } break; default: diff --git a/core/java/src/net/i2p/crypto/x25519/X25519DH.java b/core/java/src/net/i2p/crypto/x25519/X25519DH.java index 7f3fd5de8..e397318f3 100644 --- a/core/java/src/net/i2p/crypto/x25519/X25519DH.java +++ b/core/java/src/net/i2p/crypto/x25519/X25519DH.java @@ -1,5 +1,7 @@ package net.i2p.crypto.x25519; +import java.security.GeneralSecurityException; + import com.southernstorm.noise.crypto.x25519.Curve25519; import net.i2p.crypto.EncType; @@ -30,7 +32,11 @@ public class X25519DH { if (priv.getType() != TYPE || pub.getType() != TYPE) throw new IllegalArgumentException(); byte[] rv = new byte[32]; - Curve25519.eval(rv, 0, priv.getData(), pub.getData()); + try { + Curve25519.eval(rv, 0, priv.getData(), pub.getData()); + } catch (GeneralSecurityException gse) { + throw new IllegalArgumentException("Bad keys", gse); + } return new SessionKey(rv); } diff --git a/router/java/src/com/southernstorm/noise/protocol/Curve25519DHState.java b/router/java/src/com/southernstorm/noise/protocol/Curve25519DHState.java index 1a3d7fb01..b1d676426 100644 --- a/router/java/src/com/southernstorm/noise/protocol/Curve25519DHState.java +++ b/router/java/src/com/southernstorm/noise/protocol/Curve25519DHState.java @@ -22,6 +22,7 @@ package com.southernstorm.noise.protocol; +import java.security.GeneralSecurityException; import java.util.Arrays; import com.southernstorm.noise.crypto.x25519.Curve25519; @@ -112,7 +113,7 @@ class Curve25519DHState implements DHState, Cloneable { */ @Deprecated @Override - public void setPrivateKey(byte[] key, int offset) { + public void setPrivateKey(byte[] key, int offset) throws GeneralSecurityException { System.arraycopy(key, offset, privateKey, 0, 32); Curve25519.eval(publicKey, 0, privateKey, null); mode = 0x03; @@ -200,7 +201,7 @@ class Curve25519DHState implements DHState, Cloneable { } @Override - public void calculate(byte[] sharedKey, int offset, DHState publicDH) { + public void calculate(byte[] sharedKey, int offset, DHState publicDH) throws GeneralSecurityException { if (!(publicDH instanceof Curve25519DHState)) throw new IllegalArgumentException("Incompatible DH algorithms"); Curve25519.eval(sharedKey, offset, privateKey, ((Curve25519DHState)publicDH).publicKey); diff --git a/router/java/src/com/southernstorm/noise/protocol/DHState.java b/router/java/src/com/southernstorm/noise/protocol/DHState.java index 62b135a3f..b81a76712 100644 --- a/router/java/src/com/southernstorm/noise/protocol/DHState.java +++ b/router/java/src/com/southernstorm/noise/protocol/DHState.java @@ -22,6 +22,8 @@ package com.southernstorm.noise.protocol; +import java.security.GeneralSecurityException; + /** * Interface to a Diffie-Hellman algorithm for the Noise protocol. */ @@ -96,10 +98,11 @@ public interface DHState extends Destroyable, Cloneable { * If this object previously held only a public key, then * this function will change it into a key pair. * + * @throws GeneralSecurityException on bad keys * @deprecated use setKeys() */ @Deprecated - void setPrivateKey(byte[] key, int offset); + void setPrivateKey(byte[] key, int offset) throws GeneralSecurityException; /** * Sets the private and public keys for this object. @@ -178,8 +181,9 @@ public interface DHState extends Destroyable, Cloneable { * * @throws IllegalArgumentException The publicDH object is not the same * type as this object, or one of the objects does not contain a valid key. + * @throws GeneralSecurityException on bad keys */ - void calculate(byte[] sharedKey, int offset, DHState publicDH); + void calculate(byte[] sharedKey, int offset, DHState publicDH) throws GeneralSecurityException; /** * Copies the key values from another DH object of the same type. diff --git a/router/java/src/com/southernstorm/noise/protocol/HandshakeState.java b/router/java/src/com/southernstorm/noise/protocol/HandshakeState.java index dd23518d5..c12b11c6c 100644 --- a/router/java/src/com/southernstorm/noise/protocol/HandshakeState.java +++ b/router/java/src/com/southernstorm/noise/protocol/HandshakeState.java @@ -22,6 +22,7 @@ package com.southernstorm.noise.protocol; +import java.security.GeneralSecurityException; import java.security.NoSuchAlgorithmException; import java.util.Arrays; @@ -503,7 +504,7 @@ public class HandshakeState implements Destroyable, Cloneable { * @param local Local private key object. * @param remote Remote public key object. */ - private void mixDH(DHState local, DHState remote) + private void mixDH(DHState local, DHState remote) throws GeneralSecurityException { if (local == null || remote == null) throw new IllegalStateException("Pattern definition error"); @@ -552,10 +553,12 @@ public class HandshakeState implements Destroyable, Cloneable { * @throws ShortBufferException The message buffer does not have * enough space for the handshake message. * + * @throws GeneralSecurityException on bad keys for DH + * * @see #getAction() * @see #readMessage(byte[], int, int, byte[], int) */ - public int writeMessage(byte[] message, int messageOffset, byte[] payload, int payloadOffset, int payloadLength) throws ShortBufferException + public int writeMessage(byte[] message, int messageOffset, byte[] payload, int payloadOffset, int payloadLength) throws ShortBufferException, GeneralSecurityException { int messagePosn = messageOffset; boolean success = false; @@ -772,10 +775,12 @@ public class HandshakeState implements Destroyable, Cloneable { * @throws BadPaddingException A MAC value in the message failed * to verify. * + * @throws GeneralSecurityException on bad keys for DH + * * @see #getAction() * @see #writeMessage(byte[], int, byte[], int, int) */ - public int readMessage(byte[] message, int messageOffset, int messageLength, byte[] payload, int payloadOffset) throws ShortBufferException, BadPaddingException + public int readMessage(byte[] message, int messageOffset, int messageLength, byte[] payload, int payloadOffset) throws ShortBufferException, BadPaddingException, GeneralSecurityException { boolean success = false; int messageEnd = messageOffset + messageLength; diff --git a/router/java/src/com/southernstorm/noise/protocol/MLKEMDHState.java b/router/java/src/com/southernstorm/noise/protocol/MLKEMDHState.java index 4fef81d61..d4b8ff35c 100644 --- a/router/java/src/com/southernstorm/noise/protocol/MLKEMDHState.java +++ b/router/java/src/com/southernstorm/noise/protocol/MLKEMDHState.java @@ -236,31 +236,27 @@ class MLKEMDHState implements DHState, Cloneable { * Side effect: If we are Bob, copies the ciphertext to our public key * so it may be written out in the message. * - * @throws IllegalArgumentException on bad public key modulus + * @throws GeneralSecurityException on bad public key modulus */ @Override - public void calculate(byte[] sharedKey, int offset, DHState publicDH) { + public void calculate(byte[] sharedKey, int offset, DHState publicDH) throws GeneralSecurityException { if (!(publicDH instanceof MLKEMDHState)) throw new IllegalArgumentException("Incompatible DH algorithms"); - try { - if (hasPrivateKey()) { - // we are Alice - byte[] sk = MLKEM.decaps(type, ((MLKEMDHState)publicDH).publicKey, privateKey); - System.arraycopy(sk, 0, sharedKey, offset, sk.length); - } else if (!hasPublicKey()) { - // we are Bob - byte[][] rv = MLKEM.encaps(type, ((MLKEMDHState)publicDH).publicKey); - byte[] ct = rv[0]; - byte[] sk = rv[1]; - System.arraycopy(sk, 0, sharedKey, offset, sk.length); - setPublicKey(ct, 0); - } else { - throw new IllegalStateException(); - } - //System.out.println("Calculated shared PQ key: " + net.i2p.data.Base64.encode(sharedKey, offset, 32)); - } catch (GeneralSecurityException gse) { - throw new IllegalArgumentException(gse); + if (hasPrivateKey()) { + // we are Alice + byte[] sk = MLKEM.decaps(type, ((MLKEMDHState)publicDH).publicKey, privateKey); + System.arraycopy(sk, 0, sharedKey, offset, sk.length); + } else if (!hasPublicKey()) { + // we are Bob + byte[][] rv = MLKEM.encaps(type, ((MLKEMDHState)publicDH).publicKey); + byte[] ct = rv[0]; + byte[] sk = rv[1]; + System.arraycopy(sk, 0, sharedKey, offset, sk.length); + setPublicKey(ct, 0); + } else { + throw new IllegalStateException(); } + //System.out.println("Calculated shared PQ key: " + net.i2p.data.Base64.encode(sharedKey, offset, 32)); } @Override diff --git a/router/java/src/net/i2p/router/crypto/ratchet/ECIESAEADEngine.java b/router/java/src/net/i2p/router/crypto/ratchet/ECIESAEADEngine.java index 1bdf4df5b..2772df973 100644 --- a/router/java/src/net/i2p/router/crypto/ratchet/ECIESAEADEngine.java +++ b/router/java/src/net/i2p/router/crypto/ratchet/ECIESAEADEngine.java @@ -1424,7 +1424,7 @@ public final class ECIESAEADEngine { return enc; } - static final PrivateKey doDH(PrivateKey privkey, PublicKey pubkey) { + static final PrivateKey doDH(PrivateKey privkey, PublicKey pubkey) throws GeneralSecurityException { byte[] dh = new byte[KEYLEN]; Curve25519.eval(dh, 0, privkey.getData(), pubkey.getData()); return new PrivateKey(EncType.ECIES_X25519, dh); diff --git a/router/java/src/net/i2p/router/crypto/ratchet/RatchetSKM.java b/router/java/src/net/i2p/router/crypto/ratchet/RatchetSKM.java index ee40feeda..04188714e 100644 --- a/router/java/src/net/i2p/router/crypto/ratchet/RatchetSKM.java +++ b/router/java/src/net/i2p/router/crypto/ratchet/RatchetSKM.java @@ -3,6 +3,7 @@ package net.i2p.router.crypto.ratchet; import java.io.IOException; import java.io.Serializable; import java.io.Writer; +import java.security.GeneralSecurityException; import java.util.ArrayList; import java.util.Collections; import java.util.Comparator; @@ -1200,7 +1201,14 @@ public class RatchetSKM extends SessionKeyManager implements SessionTagListener // create new OB TS, delete old one PrivateKey priv = nextKeys.getPrivate(); - PrivateKey sharedSecret = ECIESAEADEngine.doDH(priv, key); + PrivateKey sharedSecret; + try { + sharedSecret = ECIESAEADEngine.doDH(priv, key); + } catch (GeneralSecurityException gse) { + if (_log.shouldWarn()) + _log.warn("Bad nextkey DH", gse); + return; + } byte[] sk = new byte[32]; _hkdf.calculate(sharedSecret.getData(), ZEROLEN, INFO_7, sk); SessionKey ssk = new SessionKey(sk); @@ -1294,7 +1302,14 @@ public class RatchetSKM extends SessionKeyManager implements SessionTagListener } _hisOBKey = receivedKey; - PrivateKey sharedSecret = ECIESAEADEngine.doDH(_myIBKeys.getPrivate(), key); + PrivateKey sharedSecret; + try { + sharedSecret = ECIESAEADEngine.doDH(_myIBKeys.getPrivate(), key); + } catch (GeneralSecurityException gse) { + if (_log.shouldWarn()) + _log.warn("Bad nextkey DH", gse); + return; + } int newtsID = oldtsID + 1; _currentIBTagSetID = newtsID; _myIBKeySendCount = 0;