Reject a field-zero divisor or multiplier in ShamirSplitSecret.divide and multiple, since the GF(256) reduction makes any multiple of 256 the field's zero element and would overwrite every share, in the base and jdk1.4 copies, and cover it with a regression test.

Author: dghgit

core/src/main/java/org/bouncycastle/crypto/threshold/ShamirSplitSecret.java

@@ -43,6 +43,15 @@ public ShamirSplitSecretShare[] getSecretShares()
     public ShamirSplitSecret multiple(int mul)
         throws IOException
     {
+        // gfMul reduces the multiplier modulo 256, so any mul with (mul & 0xFF) == 0 is the field's
+        // zero element, which would rewrite every share in place to zero and irreversibly destroy the
+        // secret. The multiplier is a caller parameter rather than secret material, so rejecting it up
+        // front leaks nothing.
+        if ((mul & 0xFF) == 0)
+        {
+            throw new IllegalArgumentException("Invalid input: the multiplier cannot be zero.");
+        }
+
         byte[] ss;
         for (int i = 0; i < secretShares.length; ++i)
         {
@@ -59,10 +68,12 @@ public ShamirSplitSecret multiple(int mul)
     public ShamirSplitSecret divide(int div)
         throws IOException
     {
-        // division by zero is undefined in the field, and every share is rewritten in place, so
-        // accepting it would silently overwrite the whole set with zeroes. The divisor is a caller
-        // parameter rather than secret material, so rejecting it up front leaks nothing.
-        if (div == 0)
+        // gfDiv/gfMul reduce the operand modulo 256, so any divisor with (div & 0xFF) == 0 - zero, or
+        // a multiple of 256 - is the field's zero element. Division by zero is undefined, and every
+        // share is rewritten in place, so accepting it would silently overwrite the whole set with
+        // zeroes. The divisor is a caller parameter rather than secret material, so rejecting it up
+        // front leaks nothing.
+        if ((div & 0xFF) == 0)
         {
             throw new IllegalArgumentException("Invalid input: the divisor cannot be zero.");
         }

core/src/main/jdk1.4/org/bouncycastle/crypto/threshold/ShamirSplitSecret.java

@@ -43,6 +43,15 @@ public SecretShare[] getSecretShares()
     public ShamirSplitSecret multiple(int mul)
         throws IOException
     {
+        // gfMul reduces the multiplier modulo 256, so any mul with (mul & 0xFF) == 0 is the field's
+        // zero element, which would rewrite every share in place to zero and irreversibly destroy the
+        // secret. The multiplier is a caller parameter rather than secret material, so rejecting it up
+        // front leaks nothing.
+        if ((mul & 0xFF) == 0)
+        {
+            throw new IllegalArgumentException("Invalid input: the multiplier cannot be zero.");
+        }
+
         byte[] ss;
         for (int i = 0; i < secretShares.length; ++i)
         {
@@ -59,10 +68,12 @@ public ShamirSplitSecret multiple(int mul)
     public ShamirSplitSecret divide(int div)
         throws IOException
     {
-        // division by zero is undefined in the field, and every share is rewritten in place, so
-        // accepting it would silently overwrite the whole set with zeroes. The divisor is a caller
-        // parameter rather than secret material, so rejecting it up front leaks nothing.
-        if (div == 0)
+        // gfDiv/gfMul reduce the operand modulo 256, so any divisor with (div & 0xFF) == 0 - zero, or
+        // a multiple of 256 - is the field's zero element. Division by zero is undefined, and every
+        // share is rewritten in place, so accepting it would silently overwrite the whole set with
+        // zeroes. The divisor is a caller parameter rather than secret material, so rejecting it up
+        // front leaks nothing.
+        if ((div & 0xFF) == 0)
         {
             throw new IllegalArgumentException("Invalid input: the divisor cannot be zero.");
         }

core/src/test/java/org/bouncycastle/crypto/threshold/test/ShamirSecretSplitterTest.java

@@ -32,6 +32,7 @@ public void performTest()
     {
         testShamirSecretResplit();
         testShamirSecretMultipleDivide();
+        testMultipleDivideRejectFieldZero();
         testShamirSecretSplitterSplitAround();
         testPolynomial();
         testPolynomialModeEquivalence();
@@ -437,6 +438,70 @@ public void testShamirSecretMultipleDivide()
         assertFalse(Arrays.areEqual(secret1, secret3));
     }
 
+    /**
+     * divide() and multiple() rewrite every share in place, so a zero divisor or multiplier would
+     * silently overwrite the whole set with zeroes and irreversibly destroy the secret. gfMul reduces
+     * its operand modulo 256, so the field's zero element is any value with (v & 0xFF) == 0 - not just
+     * an exact 0 - and both methods reject it up front. An in-range re-scale must still be accepted.
+     */
+    public void testMultipleDivideRejectFieldZero()
+        throws IOException
+    {
+        int l = 9, m = 3, n = 9;
+        SecureRandom random = new SecureRandom();
+        ShamirSecretSplitter.Algorithm algorithm = ShamirSecretSplitter.Algorithm.AES;
+
+        int[] fieldZeros = new int[]{0, 256, 512};
+
+        for (int z = 0; z != fieldZeros.length; z++)
+        {
+            ShamirSecretSplitter splitter = ShamirSecretSplitter.getInstance(algorithm, l, random);
+
+            try
+            {
+                ((ShamirSplitSecret)splitter.split(m, n)).divide(fieldZeros[z]);
+                fail("divide accepted the field-zero divisor " + fieldZeros[z]);
+            }
+            catch (IllegalArgumentException e)
+            {
+                assertEquals("Invalid input: the divisor cannot be zero.", e.getMessage());
+            }
+
+            try
+            {
+                ((ShamirSplitSecret)splitter.split(m, n)).multiple(fieldZeros[z]);
+                fail("multiple accepted the field-zero multiplier " + fieldZeros[z]);
+            }
+            catch (IllegalArgumentException e)
+            {
+                assertEquals("Invalid input: the multiplier cannot be zero.", e.getMessage());
+            }
+        }
+
+        // an in-range multiplier is still accepted, and multiple(k) followed by divide(k) leaves every
+        // share byte unchanged - the guard must not reject a legitimate re-scale.
+        ShamirSecretSplitter splitter = ShamirSecretSplitter.getInstance(algorithm, l, random);
+        ShamirSplitSecret splitSecret = (ShamirSplitSecret)splitter.split(m, n);
+
+        ShamirSplitSecretShare[] originalShares = (ShamirSplitSecretShare[])splitSecret.getSecretShares();
+        byte[][] before = new byte[originalShares.length][];
+        for (int i = 0; i != originalShares.length; i++)
+        {
+            before[i] = Arrays.clone(originalShares[i].getEncoded());
+        }
+
+        int k = random.nextInt(255) + 1;    // 1..255, a non-zero field element
+        splitSecret.multiple(k).divide(k);
+
+        ShamirSplitSecretShare[] after = (ShamirSplitSecretShare[])splitSecret.getSecretShares();
+        assertEquals("share count changed", before.length, after.length);
+        for (int i = 0; i != before.length; i++)
+        {
+            assertTrue("multiple(" + k + ").divide(" + k + ") altered share " + i,
+                Arrays.areEqual(before[i], after[i].getEncoded()));
+        }
+    }
+
     public void testShamirSecretSplitterSplitAround()
         throws IOException
     {

docs/releasenotes.html

@@ -107,6 +107,7 @@ <h3>2.1.2 Defects Fixed</h3>
 <li>Neither copy of PKIXCertPathReviewer - org.bouncycastle.pkix.jcajce.PKIXCertPathReviewer nor the legacy org.bouncycastle.x509.PKIXCertPathReviewer - applied X.509 name constraints to the end-entity certificate. checkNameConstraints() walked the path with a loop bound of index &gt; 0, which is the bound the CA-only steps want, but index 0 is the target certificate under the standard CertPath ordering, so the permitted and excluded subtree checks of RFC 5280 sec. 6.1.3 (b) and (c) never ran against the leaf's subject DN or its subjectAltName. A chain whose leaf violated a NameConstraints extension imposed by its own issuing CA therefore reported isValidCertPath() true with an empty error list, while CertPathValidator.getInstance("PKIX", "BC") - which shares no code with the reviewer and iterates index &gt;= 0 through RFC3280CertPathUtilities.processCertBC - rejected the identical chain against the identical trust anchor. An application using the reviewer to make the trust decision, rather than for its per-certificate diagnostics alongside a real validation, accepted a certificate the constrained CA was never authorised to issue. Both copies now check every certificate in the path including the target, waive the sec. 4.2.1.10 self-issued exemption for the final certificate as sec. 6.1.3 requires, and skip the sec. 6.1.4 (g) constraint-accumulation step for the target, where there is no next certificate to accumulate for.</li>
 <li>The MLS (RFC 9420) external-commit path let a joiner remove an arbitrary existing group member. Validation of an external commit's proposal list, org.bouncycastle.mls.protocol.Group.validateExternalCachedProposals, counted the proposals by type and bounded the removed leaf index, but never established that the removed leaf had anything to do with the joiner. RFC 9420 sec. 12.2 permits "at most one Remove proposal, with which the joiner removes an old version of themselves", and requires that where one is present the LeafNode in the commit's path field meet the criteria it would have to meet in an Update for the removed leaf - in particular that its credential present identifiers acceptable for the removed participant. The ordinary proposal-list validator's self-remove rule is deliberately not applied on this path, because a resync commit legitimately removes a leaf the joiner owns, but nothing was put in its place, so any party holding the group's public GroupInfo - which is precisely what an external joiner is meant to be given - could commit a Remove naming any member's LeafIndex and have every member apply it, evicting that member and taking over their slot in the ratchet tree. The credential check that should have prevented this existed only in the gRPC interop harness (MLSClientImpl.externalJoinImpl), not in Group itself, so it protected no other caller of the public Group.externalJoin / Group.handle API. An external commit carrying a Remove is now accepted only when the removed leaf's credential is identical to the one in the joiner's own new leaf, on both the sending and receiving side. Credentials are compared by their encoding rather than by Credential.getIdentity(), which is populated only for the basic credential type and would have matched any two x509 credentials against each other.</li>
 <li>The high-level OpenPGP message API offered no way to bound how far a compressed data packet expands. A compressed data packet declares no decompressed length, so a small packet can expand into an arbitrarily large amount of data; PGPCompressedData has long carried getDataStream(long) for exactly this, which stops the decompression itself once the limit is passed, but org.bouncycastle.openpgp.api.OpenPGPMessageInputStream always called the unbounded getDataStream() overload and OpenPGPPolicy exposed no corresponding property, so a caller on the recommended OpenPGPMessageProcessor path had no way to set one. OpenPGPMessageInputStream.MAX_RECURSION bounds how many compression layers may nest, which is a separate limit and says nothing about how much any one layer may produce; and a consumer counting bytes off the returned stream cannot help itself, because the decompression producing those bytes has already happened by the time it can act. OpenPGPPolicy has gained getMaximumDecompressedDataSize(), settable on OpenPGPDefaultPolicy via setMaximumDecompressedDataSize(long), which OpenPGPMessageInputStream now applies per compressed data packet; passing it raises a StreamOverflowException out of the message input stream. As on the low-level pair the default is unbounded, so existing behaviour is unchanged and choosing a limit remains the application's decision. OpenPGPMessageProcessor also gained an internal accessor for the policy it was configured with, since the two-argument constructor's policy is not the one getImplementation().policy() returns.</li>
+<li>org.bouncycastle.crypto.threshold.ShamirSplitSecret.divide(int) and multiple(int), which re-scale an existing share set in place, could silently overwrite every share with zeroes and irreversibly destroy the secret. GF(256) multiplication reduces its operand modulo 256, so a divisor or multiplier whose low eight bits are zero is the field's zero element: divide() rejected only an exactly-zero divisor (div == 0) and so let a non-zero multiple of 256 through to act as division by zero, and multiple() performed no such check at all, so multiple(0) - or any multiple of 256 - rewrote each share byte to zero through gfMul(x, 0). Both methods now reject a divisor or multiplier with (value &amp; 0xFF) == 0 up front with an IllegalArgumentException; the value is a caller parameter rather than secret material, so the check leaks nothing. In-range re-scaling is unaffected.</li>
 </ul>
 
 <h3>2.1.3 Additional Features and Functionality</h3>