-
Notifications
You must be signed in to change notification settings - Fork 22
improve ML-KEM algorithm handling and test coverage #1443
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -38,18 +38,22 @@ public MLKEMImpl(OpenJCEPlusProvider provider, String alg) { | |
| this.alg = alg; | ||
| } | ||
|
|
||
| private int getEncapsulationLength() { | ||
| private int getEncapsulationLength(String algorithm) { | ||
| int size = 0; | ||
|
|
||
| switch (this.alg) { | ||
| switch (algorithm) { | ||
| case "ML-KEM-512": | ||
| size = 768; | ||
| break; | ||
| case "ML-KEM-768": | ||
| size = 1088; | ||
| break; | ||
| default: | ||
| case "ML-KEM-1024": | ||
| size = 1568; | ||
| break; | ||
| default: | ||
| // If algorithm is generic "ML-KEM", default to ML-KEM-768 | ||
| size = 1088; | ||
| } | ||
| return size; | ||
| } | ||
|
|
@@ -72,8 +76,15 @@ public KEMSpi.EncapsulatorSpi engineNewEncapsulator(PublicKey publicKey, | |
|
|
||
| if (!(pubKey instanceof PQCPublicKey)) { | ||
| // Try and convert this key to a usage PQCPublicKey | ||
| // First verify it's an ML-KEM key | ||
| String keyAlgorithm = publicKey.getAlgorithm(); | ||
| if (keyAlgorithm == null || !keyAlgorithm.startsWith("ML-KEM")) { | ||
| throw new InvalidKeyException("unsupported key"); | ||
| } | ||
|
|
||
| // Use the key's actual algorithm, not the generic "ML-KEM" | ||
| try { | ||
| KeyFactory kf = KeyFactory.getInstance(this.alg, this.provider.getName()); | ||
| KeyFactory kf = KeyFactory.getInstance(keyAlgorithm, this.provider.getName()); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This can cause a algorithm miss match if they create this instance as ML-KEM-786, but pass in a ML-KEM-512 key that should not be allowed. |
||
| EncodedKeySpec publicKeySpec = new X509EncodedKeySpec(publicKey.getEncoded()); | ||
| pubKey = kf.generatePublic(publicKeySpec); | ||
|
|
||
|
|
@@ -105,7 +116,9 @@ class MLKEMEncapsulator implements KEMSpi.EncapsulatorSpi { | |
|
|
||
| @Override | ||
| public KEM.Encapsulated engineEncapsulate(int from, int to, String algorithm) { | ||
| int encapLen = getEncapsulationLength(); | ||
| // Get the actual algorithm from the public key | ||
| String keyAlgorithm = publicKey.getAlgorithm(); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same issue here. |
||
| int encapLen = getEncapsulationLength(keyAlgorithm); | ||
| byte[] encapsulation = new byte[encapLen]; | ||
| byte[] secret = new byte[SECRETSIZE]; | ||
|
|
||
|
|
@@ -130,7 +143,8 @@ public KEM.Encapsulated engineEncapsulate(int from, int to, String algorithm) { | |
|
|
||
| @Override | ||
| public int engineEncapsulationSize() { | ||
| return getEncapsulationLength(); | ||
| String keyAlgorithm = publicKey.getAlgorithm(); | ||
| return getEncapsulationLength(keyAlgorithm); | ||
| } | ||
|
|
||
| @Override | ||
|
|
@@ -155,9 +169,16 @@ public KEMSpi.DecapsulatorSpi engineNewDecapsulator(PrivateKey privateKey, | |
|
|
||
| if (!(privKey instanceof PQCPrivateKey)) { | ||
| // Try and convert this key to a usage PQCPrivateKey | ||
| // First verify it's an ML-KEM key | ||
| String keyAlgorithm = privateKey.getAlgorithm(); | ||
| if (keyAlgorithm == null || !keyAlgorithm.startsWith("ML-KEM")) { | ||
| throw new InvalidKeyException("unsupported key"); | ||
| } | ||
|
|
||
| // Use the key's actual algorithm, not the generic "ML-KEM" | ||
| byte[] encoding = null; | ||
| try { | ||
| KeyFactory kf = KeyFactory.getInstance(this.alg, this.provider.getName()); | ||
| KeyFactory kf = KeyFactory.getInstance(keyAlgorithm, this.provider.getName()); | ||
| encoding = privateKey.getEncoded(); | ||
| PKCS8EncodedKeySpec privateKeySpec = new PKCS8EncodedKeySpec(encoding); | ||
| privKey = kf.generatePrivate(privateKeySpec); | ||
|
|
@@ -197,6 +218,17 @@ public SecretKey engineDecapsulate(byte[] cipherText, int from, int to, String a | |
| if (algorithm == null || cipherText == null) { | ||
| throw new NullPointerException(); | ||
| } | ||
|
|
||
| // Validate encapsulation length matches the key's algorithm | ||
| String keyAlgorithm = privateKey.getAlgorithm(); | ||
| int expectedEncapLen = getEncapsulationLength(keyAlgorithm); | ||
| if (cipherText.length != expectedEncapLen) { | ||
| throw new DecapsulateException( | ||
| "Invalid key encapsulation message length: expected " + | ||
| expectedEncapLen + " bytes for " + keyAlgorithm + | ||
| ", but got " + cipherText.length + " bytes"); | ||
| } | ||
|
|
||
| try { | ||
| secret = OJPKEM.KEM_decapsulate(((PQCPrivateKey) this.privateKey).getPQCKey().getPKeyId(), | ||
| cipherText, provider); | ||
|
|
@@ -210,8 +242,8 @@ public SecretKey engineDecapsulate(byte[] cipherText, int from, int to, String a | |
|
|
||
| @Override | ||
| public int engineEncapsulationSize() { | ||
|
|
||
| return getEncapsulationLength(); | ||
| String keyAlgorithm = privateKey.getAlgorithm(); | ||
| return getEncapsulationLength(keyAlgorithm); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same type of issue here too. The algorithm the should be based on the one requested when this was created. |
||
| } | ||
|
|
||
| @Override | ||
|
|
@@ -222,6 +254,13 @@ public int engineSecretSize() { | |
|
|
||
| } | ||
|
|
||
| public static final class MLKEM extends MLKEMImpl { | ||
|
|
||
| public MLKEM(OpenJCEPlusProvider provider) { | ||
| super(provider, "ML-KEM"); | ||
| } | ||
| } | ||
|
|
||
| public static final class MLKEM512 extends MLKEMImpl { | ||
|
|
||
| public MLKEM512(OpenJCEPlusProvider provider) { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since, this is no longer using the Algorithm for this Implementation instance. Couldn't this cause issues with callers mix matching things easier?