Append the LMS private key tree cache as optional trailing data under an unchanged version 0 rather than a version 1 encoding, and cache the top six levels rather than seven, so releases predating the cache still read the key and it costs half the bytes, relates to github #2365.

Author: dghgit

core/src/main/java/org/bouncycastle/pqc/crypto/lms/LMSPrivateKeyParameters.java

@@ -17,7 +17,7 @@ public class LMSPrivateKeyParameters
     implements LMSContextBasedSigner
 {
     private static CacheKey T1 = new CacheKey(1);
-    private static CacheKey[] internedKeys = new CacheKey[129];
+    private static CacheKey[] internedKeys = new CacheKey[64];
 
     static
     {
@@ -109,10 +109,9 @@ else if (src instanceof DataInputStream)
              */
 
 
-            int version = dIn.readInt();
-            if (version != 0 && version != 1)
+            if (dIn.readInt() != 0)
             {
-                throw new IllegalStateException("expected version 0 or 1 lms private key");
+                throw new IllegalStateException("expected version 0 lms private key");
             }
 
             int sigType = dIn.readInt();
@@ -147,11 +146,15 @@ else if (src instanceof DataInputStream)
             LMSPrivateKeyParameters key = new LMSPrivateKeyParameters(parameter, otsParameter, q, I, maxQ, masterSecret);
 
             //
-            // A version 1 encoding appends a cache of the top of the Merkle tree (see getEncoded).
-            // Priming it here means the first signature made after the key is decoded does not have
-            // to rebuild the whole tree, which otherwise costs about as much as key generation.
+            // Anything after the master secret is a cache of the top of the Merkle tree (see
+            // getEncoded). Priming it here means the first signature made after the key is decoded
+            // does not have to rebuild the whole tree, which otherwise costs about as much as key
+            // generation. The cache is optional trailing data rather than a new version so that
+            // releases predating it still read the key - they stop at the master secret and ignore
+            // what follows - at the cost of it being absent rather than malformed when a stream
+            // supplies no more bytes.
             //
-            if (version == 1)
+            if (dIn.available() > 0)
             {
                 int cacheCount = dIn.readInt();
                 if (cacheCount < 0 || cacheCount >= internedKeys.length)
@@ -512,29 +515,31 @@ public byte[] getEncoded()
         // It is implementation dependent.
         //
         // Format:
-        //     version u32                 (1; version 0 - without the tree cache - is still accepted on read)
+        //     version u32                 (0)
         //     type u32
         //     otstype u32
         //     I u8x16
         //     q u32
         //     maxQ u32
         //     master secret Length u32
         //     master secret u8[]
-        //     tree cache node count u32   (n; the top-of-tree nodes 1..n)
-        //     tree cache nodes u8[]       (n * getSigParameters().getM() bytes)
+        //     tree cache node count u32   (n; the top-of-tree nodes 1..n) - optional
+        //     tree cache nodes u8[]       (n * getSigParameters().getM() bytes) - optional
         //
         // The tree cache carries the top of the Merkle tree so that the first signature made after
         // the key is decoded does not have to rebuild the whole tree - which otherwise costs about
         // as much as key generation (see github #2365). The nodes are a deterministic function of I,
         // the master secret and the parameters and are independent of q, so persisting them leaks
-        // nothing the (already encoded) master secret does not. A key written by this method cannot
-        // be read by releases before 1.86; those releases reject the version field.
+        // nothing the (already encoded) master secret does not. The cache is appended after the
+        // master secret rather than announced by a new version number, so a key written by this
+        // method is still readable by releases that predate it: their decoder returns at the end of
+        // the master secret and never looks at the trailing bytes.
         //
 
         int cacheTop = Math.min(internedKeys.length, maxCacheR);
 
         Composer composer = Composer.compose()
-            .u32str(1) // version
+            .u32str(0) // version
             .u32str(parameters.getType()) // type
             .u32str(otsParameters.getType()) // ots type
             .bytes(I) // I at 16 bytes

core/src/test/java/org/bouncycastle/pqc/crypto/lms/LMSTests.java

@@ -1,9 +1,11 @@
 package org.bouncycastle.pqc.crypto.lms;
 
 import java.io.IOException;
+import java.security.SecureRandom;
 
 import junit.framework.TestCase;
 import org.bouncycastle.util.Arrays;
+import org.bouncycastle.util.Pack;
 import org.bouncycastle.util.encoders.Hex;
 
 public class LMSTests
@@ -195,11 +197,13 @@ public void testTreeCachePersistence()
 
         byte[] enc = privateKey.getEncoded();
 
-        // version 1: 72 byte legacy body + u32 node count + (cacheTop - 1) nodes of m bytes each.
+        // 72 byte body + u32 node count + (cacheTop - 1) nodes of m bytes each. The version stays 0
+        // and the cache is appended as trailing data, so releases that predate it still read the
+        // key - they stop at the master secret - and simply do not see the cache.
         assertEquals(0, enc[0]);
         assertEquals(0, enc[1]);
         assertEquals(0, enc[2]);
-        assertEquals(1, enc[3]);
+        assertEquals(0, enc[3]);
         assertEquals(72 + 4 + (cacheTop - 1) * m, enc.length);
 
         // Decoding primes the cache - this is the fix; without it the decoded key's cache is empty
@@ -214,7 +218,8 @@ public void testTreeCachePersistence()
         LMSPrivateKeyParameters fresh = LMS.generateKeys(sigParams, otsParams, 0, I, seed);
         assertTrue(Arrays.areEqual(sigFromDecoded.getEncoded(), LMS.generateSign(fresh, msg).getEncoded()));
 
-        // Legacy version 0 encodings (no cache) must still decode and sign correctly.
+        // An encoding with no trailing cache - what an older release writes - must still decode
+        // and sign correctly.
         byte[] legacyEnc = Composer.compose()
             .u32str(0)
             .u32str(sigParams.getType())
@@ -228,7 +233,7 @@ public void testTreeCachePersistence()
         assertEquals(72, legacyEnc.length);
 
         LMSPrivateKeyParameters legacy = LMSPrivateKeyParameters.getInstance(legacyEnc);
-        assertFalse("version 0 encoding carries no cache", legacy.isTreeCachePrimed());
+        assertFalse("encoding without trailing data carries no cache", legacy.isTreeCachePrimed());
         assertTrue(LMS.verifySignature(publicKey, LMS.generateSign(legacy, msg), msg));
     }
 
@@ -292,11 +297,20 @@ public void testMalformedPrivateKeyTreeCache()
 
         LMSigParameters sigParams = LMSigParameters.lms_sha256_n32_h10;
         LMOtsParameters otsParams = LMOtsParameters.sha256_n32_w4;
-        int cacheCountLimit = 128;
         int m = sigParams.getM();
 
+        //
+        // The number of nodes cached is capped by LMSPrivateKeyParameters' interned-key table, so
+        // read it off a freshly generated key of the same parameters rather than hard-coding it -
+        // the limit moves if that table is resized.
+        //
+        LMSKeyPairGenerator limitGen = new LMSKeyPairGenerator();
+        limitGen.init(new LMSKeyGenerationParameters(new LMSParameters(sigParams, otsParams), new SecureRandom()));
+        byte[] sampleEnc = ((LMSPrivateKeyParameters)limitGen.generateKeyPair().getPrivate()).getEncoded();
+        int cacheCountLimit = Pack.bigEndianToInt(sampleEnc, 40 + m);
+
         byte[] atLimit = Composer.compose()
-            .u32str(1)
+            .u32str(0)
             .u32str(sigParams.getType())
             .u32str(otsParams.getType())
             .bytes(I)
@@ -310,7 +324,7 @@ public void testMalformedPrivateKeyTreeCache()
         assertTrue(LMSPrivateKeyParameters.getInstance(atLimit).isTreeCachePrimed());
 
         byte[] beyondLimit = Composer.compose()
-            .u32str(1)
+            .u32str(0)
             .u32str(sigParams.getType())
             .u32str(otsParams.getType())
             .bytes(I)
@@ -331,7 +345,7 @@ public void testMalformedPrivateKeyTreeCache()
         }
 
         byte[] truncated = Composer.compose()
-            .u32str(1)
+            .u32str(0)
             .u32str(sigParams.getType())
             .u32str(otsParams.getType())
             .bytes(I)

docs/releasenotes.html

@@ -76,7 +76,7 @@ <h3>2.1.2 Defects Fixed</h3>
 <li>ArmoredInputStream.read() returned read(), calling itself, on reaching the ASCII-armor checksum line. The running CRC-24 is not reset between lines and the checksum was not required to be the last thing before the armor tail, so repeating one line whose value matches - "=twTO", the base64 of the CRC-24 initial value, which needs no data bytes at all - cost a stack frame per line and threw StackOverflowError from about 360KB of plain ASCII. Being an Error it escaped the catch (IOException) callers put around PGP parsing, and PGPUtil.getDecoderStream reaches it on the first read of any untrusted armored message. A second checksum is now rejected, per RFC 9580 sec. 6.2, which places it once immediately before the tail; the armor tail still clears the flag, so concatenated messages are unaffected.</li>
 <li>X500Name.hashCode() threw a NullPointerException for a name containing an RDN decoded from an empty SET, which any peer can encode: AbstractX500NameStyle.calculateHashCode took the RDN's first AttributeTypeAndValue without a null check, while its two siblings for the same case - areEqual for equals(), and IETFUtils.appendRDN for toString() - both already had one, so hashCode was the only one of the three that failed. An empty RDN now contributes nothing to the hash, matching what toString renders for it. X500Name.hashCode() was additionally marking the value calculated before computing it, so once a style had thrown, every later call quietly returned 0 - a single loud failure degrading into a silent source of hash collisions; the flag is now set after the value is in hand, matching the j2me copy of the class, which had not drifted.</li>
 <li>HSSSigner.init and LMSSigner.init assigned only the key for the mode being set and left the other one from a previous init in place, so a signer initialised for verification still held the private key from an earlier signing init and would sign with it, and one initialised for signing still held the public key and would verify. Both keys are now cleared on every init, and calling the signer in the mode it was not initialised for raises IllegalStateException naming which init is missing rather than working off the stale key or failing later as a NullPointerException; the verification check sits ahead of the existing catch-all so a missing init is reported rather than folded into "signature did not verify". The two lightweight tests that asserted an exhausted key shard were reaching generateSignature through a signer left in verify mode and now re-initialise for signing first, which is what they meant to test. Note the same init pattern remains in the other older signers under pqc.crypto - XMSS, XMSS^MT, Falcon, Picnic, Rainbow, Dilithium, SPHINCS-256 and SPHINCS+ - while every signer added since (AIMer, Faest, HAETAE, Hawk, Mayo, MQOM, QRUOV, SDitH, SLH-DSA, Snova, SQIsign, UOV) already clears both.</li>
-<li>LMS private-key encodings now preserve a bounded top-of-tree cache alongside the existing seed material, so a decoded LMS/HSS private key avoids rebuilding the Merkle tree before its first signature while continuing to accept the previous version-0 encoding. LMS private-key decoding also now rejects unknown LMS and LM-OTS type codes with IOException rather than leaking a NullPointerException. The persisted cache is deterministic from the already-encoded master secret and is capped to the existing LMS tree-cache bound (github #2365).</li>
+<li>LMS private-key encodings now preserve a bounded top-of-tree cache alongside the existing seed material, so a decoded LMS/HSS private key avoids rebuilding the Merkle tree before its first signature - measured at about thirty times faster for the h10 and h15 parameter sets. The cache is appended after the master secret rather than announced by a new version number, so the encoding stays version 0 and remains readable by releases that predate it: their decoder stops at the master secret and never looks at the trailing bytes, giving them a working key without the cache. An encoding carrying no cache is still accepted, and a truncated or over-large one is still rejected. The cache holds the top six levels of the tree, which takes an sha256_n32 private key from 72 to 2092 bytes. LMS private-key decoding also now rejects unknown LMS and LM-OTS type codes with IOException rather than leaking a NullPointerException. The persisted cache is deterministic from the already-encoded master secret, so it discloses nothing that secret does not (github #2365).</li>
 <li>XMSSSigner.init and XMSSMTSigner.init assigned only the key for the mode being set. generateSignature already refused to run unless the signer had been initialised for signing, but verifySignature carried no equivalent check, so a signer re-initialised for signing still verified against the public key left behind by an earlier verification init - and returned true rather than failing. The public key is now cleared on a signing init, and verifySignature opens with a single check that reports an IllegalStateException both for that wrong mode and for a signer never initialised at all - the latter previously returned false for XMSS, because the NullPointerException the absent public key raised was swallowed by the catch that keeps a malformed signature from propagating, and threw NullPointerException for XMSS^MT. The private key is deliberately left in place on a verification init, unlike the HSSSigner and LMSSigner change above: getUpdatedPrivateKey() synchronizes on it, so clearing it would turn sign, then verify, then collect the advanced key state - a legitimate sequence for these stateful schemes, and the one the JCA layer uses - into a NullPointerException. Falcon and SPHINCS-256 were checked and are unaffected, holding a single key field that a re-init overwrites rather than separate private and public fields.</li>
 <li>The PQC PrivateKeyFactory (org.bouncycastle.pqc.crypto.util) chose its algorithm branch with a subtree test on the algorithm identifier's OID arc, algOID.on(arc), which matches every leaf below the arc - including leaves the corresponding Utils parameters table has no entry for. The branch then passed the null lookup result straight into a *PrivateKeyParameters constructor, so a PKCS#8 PrivateKeyInfo naming such an OID escaped createKey with a NullPointerException past its declared throws IOException for fourteen of the eighteen arcs, and for the other four - Classic McEliece, NTRU LPRime, Streamlined NTRU Prime and SNOVA, whose constructors only store the parameters - createKey returned a key object carrying null parameters, deferring the failure to whatever used the key. Every arc was reachable with an arbitrary undeclared leaf; concretely, the OQS interop arc 1.3.9999.6 carries sixteen hybrid (ECDSA-P256 / RSA-3072 / P-384 / P-521 combined with SPHINCS+) OIDs that are declared in BCObjectIdentifiers but deliberately not implemented, for example p256_sphincs_sha2_128f_simple (1.3.9999.6.4.14). Each branch now dispatches on the parameters table itself, Utils.&lt;alg&gt;Params.containsKey(algOID) - the predicate the same method already used for SLH-DSA and MQOM - so an OID with no parameters falls through to the unrecognised-algorithm terminal, which now throws IOException rather than RuntimeException, matching PublicKeyFactory whose terminal has always been an IOException. On the public-key side the sixteen hybrid OIDs are not in the converter table at all and were already rejected cleanly, but five registrations there route to a converter whose parameters lookup cannot succeed - the bare SPHINCS+ arc, the OQS round-3 OID 1.3.9999.6.4.10, and the vestigial Kyber-AES OIDs - and those leaked the same NullPointerException; the SPHINCS+ and ML-KEM converters now reject an unmapped OID with IOException. Keys naming a mapped OID are unaffected, and the jdk1.4 and jdk1.1 legacy overlays of both factories carry the same fix.</li>
 <li>EdEC KeyFactorySpi.engineGeneratePublic could read the algorithm discriminator byte from a malformed X509EncodedKeySpec before checking the encoding length, so very short Ed25519/Ed448/X25519/X448 public-key encodings leaked ArrayIndexOutOfBoundsException, and malformed full-length encodings could leak runtime exceptions from the key-parameter constructors instead of the KeyFactory contract's InvalidKeySpecException. The base implementation and the jdk1.4, jdk1.11 and jdk1.15 legacy/multi-release overlays now guard the fixed-offset fast path and wrap malformed EdEC public-key decode failures as InvalidKeySpecException while preserving valid key import.</li>