|
I know this is on hold pending the pqc-forum discussion, so this is for whenever it gets picked back up. The issues below apply to any SP 800-230 support regardless of the final parameter choices.
Thanks for taking this on. Adding the SP 800-230 limited-signature SLH-DSA sets (the "-24" family, capped at 2^24 signatures per key) is worth having in BC, and the parameter tuples are numerically right where they land: 128-24 = (16,4,1,24,6,22), 192-24 = (24,8,1,25,9,21), 256-24 = (32,4,1,25,12,21) as (n,w,d,a,k,h), with SHA2 and SHAKE matching per level, w=4 for the 128/256 sets and w=8 for 192, and d=1 throughout. Those reproduce the correct signature sizes (3856 / 7752 / 14944), so a size check passes. I ran the sets against the spec math and the existing engine code, and there are a few blocking issues before this can merge.
1. WOTS+ checksum / base-w decode is wrong for 128-24 and 192-24 (blocker)
File: core/src/main/java/org/bouncycastle/pqc/crypto/slhdsa/WotsPlus.java. This code is unchanged from base, but the new sets exercise it with lg_w values it was never written for. There are two distinct root causes, and only 256-24 escapes both.
128-24 (w=4, lg_w=2): checksum decodes to a constant zero. In the checksum encode (lines 84-91), the left-shift is csum << (8 - ((WOTS_LEN2 * WOTS_LOGW) % 8)). The SPHINCS+ reference implementation this code is ported from wraps that shift in an outer mod 8: csum << ((8 - ((len2*lg_w) % 8)) % 8) (wots.c, wots_checksum). For 128-24, len2*lg_w = 4*2 = 8, so the correct shift is 0 but this code shifts by 8, one whole byte too far. Combined with len_2_bytes = 1 and the read at offset 4 - len_2_bytes = 3, base_w then reads the vacated zero byte, so the checksum digits are [0,0,0,0] for every message. I reproduced this over 200 messages using the exact engine-constant and WotsPlus checksum code: the 128-24 checksum encodes to [0,0,0,0] every time (two messages with different real checksums encode identically), while the standard w=16 set varies correctly as a control. The omitted mod 8 was latent because len2*lg_w is never a multiple of 8 for any w=16 set; 128-24 is the first set to hit it. A constant WOTS+ checksum removes the mechanism that stops an attacker from advancing every message chain forward, so I would treat this as a correctness and security defect, not a size or interop nit.
192-24 (w=8, lg_w=3): base_w mis-decodes both the message and the checksum. WotsPlus.base_w refills total one byte at a time (total = X[XOff++]) and does bits -= WOTS_LOGW. That single-byte-replace form is only correct when lg_w divides 8. lg_w=3 does not, so bits goes negative, the read straddles bytes, and Java signed-byte sign-extension compounds it. On sample inputs the checksum decodes wrong: a true checksum of 448 comes out as digits [7,0,1] instead of [7,0,0], 447 as [6,7,1] instead of [6,7,7], and small values collapse (csum=1 gives [0,0,0]). The message base-w digits themselves are wrong too. The correct general routine already exists in this codebase: Fors.java (lines 149-161) uses an accumulating base_2b (total = total.shiftLeft(8).add(...)) precisely because the FORS a values (6, 9, 12, 14) also do not divide 8. WOTS+ should use that same accumulating form for lg_w that does not divide 8.
Because sign() and pkFromSig() encode the checksum identically for these parameters, BC still verifies its own signatures, so an internal round-trip test stays green and does not catch this. Of the three sets, only 256-24 encodes the WOTS+ layer correctly (0 of 200 message and checksum errors in the same test); 128-24 and 192-24 do not.
2. Sets landed only on the deprecated tree
The six sets are added at core/src/main/java/org/bouncycastle/pqc/crypto/slhdsa/SLHDSAParameters.java:56-77, whose class javadoc reads @deprecated use org.bouncycastle.crypto.params.SLHDSAParameters. The modern crypto/params/SLHDSAParameters.java is byte-identical to base and still holds only the 24 standard w=16 sets. New standards-track functionality on the deprecated path only is not reachable by the consumers the project points forward to.
3. Modern engine rejects w=4/w=8
If you move the sets to the modern tree (the right fix), they fail at construction: core/src/main/java/org/bouncycastle/crypto/signers/slhdsa/SLHDSAEngine.java accepts only w==16 or w==256 and otherwise throws "wots_w assumed 16 or 256" (line 82). The w=4/w=8 branches were added only to the deprecated pqc engine. Moving the params means also widening this guard and porting the LEN math, after fixing #1 in WotsPlus.
4. No JCA provider or public spec entry point
prov/src/main/java/org/bouncycastle/jcajce/provider/asymmetric/SLHDSA.java and jcajce/spec/SLHDSAParameterSpec.java register only the 24 standard sets. There is no *-24 name, so an application cannot select these sets through KeyPairGenerator / Signature / ParameterSpec. The feature is not wired end to end.
5. Diff is not reviewable
The PR reports 264,657 additions / 122,102 deletions across 3,807 files, while the real change is roughly 40 lines. The head branch has diverged substantially from base, which pulls in thousands of unrelated files plus some stray ones (CLAUDE.md, build/CI scripts, a rewritten build.gradle). Please rebase onto current base so the diff shows only the intended change, and drop the stray files.
6. No test vectors
core/src/test/java/org/bouncycastle/pqc/crypto/test/SLHDSATest.java has no coverage for any of the six sets: they are absent from parametersMap, from PARAMETER_SETS (the round-trip driver behind testConsistency), and from every KAT harness. Since SP 800-230 is an Initial Public Draft with no published NIST reference vectors yet, at minimum add a self-consistency sign/verify round-trip per set (mirroring testConsistency). A test that decodes the WOTS+ checksum and asserts it is not constant-zero would have caught #1.
Minor notes
- No OIDs / ASN.1 for the new sets. This is inherent to the IPD (it assigns none), so it is not something to fix here, but it does mean no X.509 / PKCS#8 / CMS path until NIST allocates identifiers. Worth a code comment so it is not read as an oversight.
- In the added w=4/w=8 branches,
WOTS_LEN2 is computed with a runtime float log, (int)(Math.log(WOTS_LEN1*(w-1))/Math.log(w))+1, rather than the integer tables the standard sets use. It gives the correct 4/3/5 here with a wide margin, so there is no bug, but an integer computation would match the surrounding style.
Objective checks you can run
Signature size: sig = (1 + k*(1+a) + h + d*len) * n, with len = len1 + len2, len1 = ceil(8n / lg_w), len2 = floor(log2(len1*(w-1)) / lg_w) + 1. For these sets that gives 241*16 = 3856, 323*24 = 7752, 467*32 = 14944. The stronger check is to sign a fixed message, decode the WOTS+ checksum digits back out, and confirm they are not constant-zero and that they track the message hash. That is what exposes #1.
What would make this mergeable
- Fix the WOTS+ base-w / checksum encoding for w=4 and w=8 in
WotsPlus.java (missing outer mod 8 in the checksum shift for 128-24; accumulating base_2b for the lg_w=3 case of 192-24), and confirm 128-24's checksum is no longer constant-zero.
- Add the six sets to the modern
crypto/params tree and widen the core SLHDSAEngine w-guard with the w=4/w=8 LEN math so they are constructible on the supported path.
- Wire the JCA provider and a public
SLHDSAParameterSpec so the sets are selectable through the standard API.
- Add a round-trip test per set, and external KATs once NIST publishes them.
- Rebase onto current base and drop the unrelated files so the diff is only the intended change.
The parameter values are right, so the foundation is sound once the encoding and the plumbing are fixed. Happy to look again once the checksum fix and a round-trip test are in place.
|
