added additional checking to XMSS BDS tree parsing. Failures now mostly cause IOException

dghgit · dghgit · commit 4092ede58da5 · 2018-03-03T14:06:34.000+11:00
diff --git a/core/src/main/java/org/bouncycastle/pqc/crypto/gmss/GMSSKeyPairGenerator.java b/core/src/main/java/org/bouncycastle/pqc/crypto/gmss/GMSSKeyPairGenerator.java
@@ -178,26 +178,19 @@ private AsymmetricCipherKeyPair genKeyPair()
         // from bottom up to the root
         for (int h = numLayer - 1; h >= 0; h--)
         {
-            GMSSRootCalc tree = new GMSSRootCalc(this.heightOfTrees[h], this.K[h], digestProvider);
-            try
-            {
-                // on lowest layer no lower root is available, so just call
-                // the method with null as first parameter
-                if (h == numLayer - 1)
-                {
-                    tree = this.generateCurrentAuthpathAndRoot(null, currentStack[h], seeds[h], h);
-                }
-                else
-                // otherwise call the method with the former computed root
-                // value
-                {
-                    tree = this.generateCurrentAuthpathAndRoot(currentRoots[h + 1], currentStack[h], seeds[h], h);
-                }
+            GMSSRootCalc tree;
 
+            // on lowest layer no lower root is available, so just call
+            // the method with null as first parameter
+            if (h == numLayer - 1)
+            {
+                tree = this.generateCurrentAuthpathAndRoot(null, currentStack[h], seeds[h], h);
             }
-            catch (Exception e1)
+            else
+            // otherwise call the method with the former computed root
+            // value
             {
-                e1.printStackTrace();
+                tree = this.generateCurrentAuthpathAndRoot(currentRoots[h + 1], currentStack[h], seeds[h], h);
             }
 
             // set initial values needed for the private key construction
diff --git a/core/src/main/java/org/bouncycastle/pqc/crypto/rainbow/RainbowParameters.java b/core/src/main/java/org/bouncycastle/pqc/crypto/rainbow/RainbowParameters.java
@@ -44,37 +44,30 @@ public RainbowParameters()
     public RainbowParameters(int[] vi)
     {
         this.vi = vi;
-        try
-        {
-            checkParams();
-        }
-        catch (Exception e)
-        {
-            e.printStackTrace();
-        }
+
+        checkParams();
     }
 
     private void checkParams()
-        throws Exception
     {
         if (vi == null)
         {
-            throw new Exception("no layers defined.");
+            throw new IllegalArgumentException("no layers defined.");
         }
         if (vi.length > 1)
         {
             for (int i = 0; i < vi.length - 1; i++)
             {
                 if (vi[i] >= vi[i + 1])
                 {
-                    throw new Exception(
+                    throw new IllegalArgumentException(
                         "v[i] has to be smaller than v[i+1]");
                 }
             }
         }
         else
         {
-            throw new Exception(
+            throw new IllegalArgumentException(
                 "Rainbow needs at least 1 layer, such that v1 < v2.");
         }
     }
diff --git a/core/src/main/java/org/bouncycastle/pqc/crypto/xmss/XMSSMTPrivateKeyParameters.java b/core/src/main/java/org/bouncycastle/pqc/crypto/xmss/XMSSMTPrivateKeyParameters.java
@@ -68,21 +68,21 @@ private XMSSMTPrivateKeyParameters(Builder builder)
 			/* import BDS state */
             byte[] bdsStateBinary = XMSSUtil.extractBytesAtOffset(privateKey, position, privateKey.length - position);
 
-            BDSStateMap bdsImport = null;
             try
             {
-                bdsImport = (BDSStateMap)XMSSUtil.deserialize(bdsStateBinary);
+                BDSStateMap bdsImport = (BDSStateMap)XMSSUtil.deserialize(bdsStateBinary, BDSStateMap.class);
+
+                bdsImport.setXMSS(builder.xmss);
+                bdsState = bdsImport;
             }
             catch (IOException e)
             {
-                e.printStackTrace();
+                throw new IllegalArgumentException(e.getMessage(), e);
             }
             catch (ClassNotFoundException e)
             {
-                e.printStackTrace();
+                throw new IllegalArgumentException(e.getMessage(), e);
             }
-            bdsImport.setXMSS(builder.xmss);
-            bdsState = bdsImport;
         }
         else
         {
@@ -260,17 +260,14 @@ public byte[] toByteArray()
 		/* copy root */
         XMSSUtil.copyBytesAtOffset(out, root, position);
 		/* concatenate bdsState */
-        byte[] bdsStateOut = null;
         try
         {
-            bdsStateOut = XMSSUtil.serialize(bdsState);
+            return Arrays.concatenate(out, XMSSUtil.serialize(bdsState));
         }
         catch (IOException e)
         {
-            e.printStackTrace();
-            throw new RuntimeException("error serializing bds state");
+            throw new IllegalStateException("error serializing bds state: " + e.getMessage(), e);
         }
-        return Arrays.concatenate(out, bdsStateOut);
     }
 
     public long getIndex()
diff --git a/core/src/main/java/org/bouncycastle/pqc/crypto/xmss/XMSSPrivateKeyParameters.java b/core/src/main/java/org/bouncycastle/pqc/crypto/xmss/XMSSPrivateKeyParameters.java
@@ -86,26 +86,25 @@ private XMSSPrivateKeyParameters(Builder builder)
             position += rootSize;
 			/* import BDS state */
             byte[] bdsStateBinary = XMSSUtil.extractBytesAtOffset(privateKey, position, privateKey.length - position);
-            BDS bdsImport = null;
             try
             {
-                bdsImport = (BDS)XMSSUtil.deserialize(bdsStateBinary);
+                BDS bdsImport = (BDS)XMSSUtil.deserialize(bdsStateBinary, BDS.class);
+                bdsImport.setXMSS(builder.xmss);
+                bdsImport.validate();
+                if (bdsImport.getIndex() != index)
+                {
+                    throw new IllegalStateException("serialized BDS has wrong index");
+                }
+                bdsState = bdsImport;
             }
             catch (IOException e)
             {
-                e.printStackTrace();
+                throw new IllegalArgumentException(e.getMessage(), e);
             }
             catch (ClassNotFoundException e)
             {
-                e.printStackTrace();
-            }
-            bdsImport.setXMSS(builder.xmss);
-            bdsImport.validate();
-            if (bdsImport.getIndex() != index)
-            {
-                throw new IllegalStateException("serialized BDS has wrong index");
+                throw new IllegalArgumentException(e.getMessage(), e);
             }
-            bdsState = bdsImport;
         }
         else
         {
diff --git a/core/src/main/java/org/bouncycastle/pqc/crypto/xmss/XMSSUtil.java b/core/src/main/java/org/bouncycastle/pqc/crypto/xmss/XMSSUtil.java
@@ -321,12 +321,25 @@ public static byte[] serialize(Object obj)
         return out.toByteArray();
     }
 
-    public static Object deserialize(byte[] data)
+    public static Object deserialize(byte[] data, Class clazz)
         throws IOException, ClassNotFoundException
     {
         ByteArrayInputStream in = new ByteArrayInputStream(data);
         ObjectInputStream is = new ObjectInputStream(in);
-        return is.readObject();
+        Object obj = is.readObject();
+
+        if (is.available() != 0)
+        {
+            throw new IOException("unexpected data found at end of ObjectInputStream");
+        }
+        if (clazz.isInstance(obj))
+        {
+            return obj;
+        }
+        else
+        {
+            throw new IOException("unexpected class found in ObjectInputStream");
+        }
     }
 
     public static int calculateTau(int index, int height)
diff --git a/core/src/main/java/org/bouncycastle/pqc/math/linearalgebra/GF2nField.java b/core/src/main/java/org/bouncycastle/pqc/math/linearalgebra/GF2nField.java
@@ -155,16 +155,9 @@ protected final GF2Polynomial[] invertMatrix(GF2Polynomial[] matrix)
         // initialize a as a copy of matrix and inv as E(inheitsmatrix)
         for (i = 0; i < mDegree; i++)
         {
-            try
-            {
-                a[i] = new GF2Polynomial(matrix[i]);
-                inv[i] = new GF2Polynomial(mDegree);
-                inv[i].setBit(mDegree - 1 - i);
-            }
-            catch (RuntimeException BDNEExc)
-            {
-                BDNEExc.printStackTrace();
-            }
+            a[i] = new GF2Polynomial(matrix[i]);
+            inv[i] = new GF2Polynomial(mDegree);
+            inv[i].setBit(mDegree - 1 - i);
         }
         // construct triangle matrix so that for each a[i] the first i bits are
         // zero
diff --git a/core/src/test/java/org/bouncycastle/pqc/crypto/test/XMSSMTPrivateKeyTest.java b/core/src/test/java/org/bouncycastle/pqc/crypto/test/XMSSMTPrivateKeyTest.java
@@ -5,19 +5,61 @@
 
 import junit.framework.TestCase;
 import org.bouncycastle.crypto.digests.SHA256Digest;
+import org.bouncycastle.pqc.crypto.xmss.XMSS;
 import org.bouncycastle.pqc.crypto.xmss.XMSSMT;
 import org.bouncycastle.pqc.crypto.xmss.XMSSMTParameters;
+import org.bouncycastle.pqc.crypto.xmss.XMSSParameters;
+import org.bouncycastle.pqc.crypto.xmss.XMSSPrivateKeyParameters;
 import org.bouncycastle.util.Arrays;
+import org.bouncycastle.util.encoders.Base64;
 
 /**
  * Test cases for XMSSMTPrivateKey class.
  */
 public class XMSSMTPrivateKeyTest
     extends TestCase
 {
+    public void testPrivateKeySerialisation()
+        throws Exception
+    {
+        String stream = "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAArO0ABXNyACJzdW4ucm1pLnNlcnZlci5BY3RpdmF0aW9uR3JvdXBJbXBsT+r9SAwuMqcCAARaAA1ncm91cEluYWN0aXZlTAAGYWN0aXZldAAVTGphdmEvdXRpbC9IYXNodGFibGU7TAAHZ3JvdXBJRHQAJ0xqYXZhL3JtaS9hY3RpdmF0aW9uL0FjdGl2YXRpb25Hcm91cElEO0wACWxvY2tlZElEc3QAEExqYXZhL3V0aWwvTGlzdDt4cgAjamF2YS5ybWkuYWN0aXZhdGlvbi5BY3RpdmF0aW9uR3JvdXCVLvKwBSnVVAIAA0oAC2luY2FybmF0aW9uTAAHZ3JvdXBJRHEAfgACTAAHbW9uaXRvcnQAJ0xqYXZhL3JtaS9hY3RpdmF0aW9uL0FjdGl2YXRpb25Nb25pdG9yO3hyACNqYXZhLnJtaS5zZXJ2ZXIuVW5pY2FzdFJlbW90ZU9iamVjdEUJEhX14n4xAgADSQAEcG9ydEwAA2NzZnQAKExqYXZhL3JtaS9zZXJ2ZXIvUk1JQ2xpZW50U29ja2V0RmFjdG9yeTtMAANzc2Z0AChMamF2YS9ybWkvc2VydmVyL1JNSVNlcnZlclNvY2tldEZhY3Rvcnk7eHIAHGphdmEucm1pLnNlcnZlci5SZW1vdGVTZXJ2ZXLHGQcSaPM5+wIAAHhyABxqYXZhLnJtaS5zZXJ2ZXIuUmVtb3RlT2JqZWN002G0kQxhMx4DAAB4cHcSABBVbmljYXN0U2VydmVyUmVmeAAAFbNwcAAAAAAAAAAAcHAAcHBw";
+
+        XMSSParameters params = new XMSSParameters(10, new SHA256Digest());
+
+        byte[] output = Base64.decode(new String(stream).getBytes("UTF-8"));
+
+
+        //Simple Exploit
+
+        try
+        {
+            new XMSSPrivateKeyParameters.Builder(params).withPrivateKey(output, params).build();
+        }
+        catch (IllegalArgumentException e)
+        {
+            assertTrue(e.getCause() instanceof IOException);
+        }
+
+        //Same Exploit other method
+
+        XMSS xmss2 = new XMSS(params, new SecureRandom());
+
+        xmss2.generateKeys();
+
+        byte[] publicKey = xmss2.exportPublicKey();
+
+        try
+        {
+            xmss2.importState(output, publicKey);
+        }
+        catch (IllegalArgumentException e)
+        {
+            assertTrue(e.getCause() instanceof IOException);
+        }
+    }
 
     public void testPrivateKeyParsingSHA256()
-        throws IOException, ClassNotFoundException
+        throws Exception
     {
         XMSSMTParameters params = new XMSSMTParameters(20, 10, new SHA256Digest());
         XMSSMT mt = new XMSSMT(params, new SecureRandom());
diff --git a/docs/releasenotes.html b/docs/releasenotes.html
@@ -28,6 +28,7 @@ <h3>2.1.1 Version</h3>
 <h3>2.1.2 Defects Fixed</h3>
 <ul>
 <li>Base64/UrlBase64 would throw an exception on a zero length string. This has been fixed.</li>
+<li>XMSS applies further validation to deserialisation of the BDS tree so that failure occurs as soon as tampering is detected.</li>
 </ul>
 <h3>2.1.3 Additional Features and Functionality</h3>
 <ul>
diff --git a/prov/src/main/java/org/bouncycastle/pqc/jcajce/provider/xmss/BCXMSSMTPrivateKey.java b/prov/src/main/java/org/bouncycastle/pqc/jcajce/provider/xmss/BCXMSSMTPrivateKey.java
@@ -52,7 +52,7 @@ public BCXMSSMTPrivateKey(PrivateKeyInfo keyInfo)
 
             if (xmssMtPrivateKey.getBdsState() != null)
             {
-                keyBuilder.withBDSState((BDSStateMap)XMSSUtil.deserialize(xmssMtPrivateKey.getBdsState()));
+                keyBuilder.withBDSState((BDSStateMap)XMSSUtil.deserialize(xmssMtPrivateKey.getBdsState(), BDSStateMap.class));
             }
 
             this.keyParams = keyBuilder.build();
diff --git a/prov/src/main/java/org/bouncycastle/pqc/jcajce/provider/xmss/BCXMSSPrivateKey.java b/prov/src/main/java/org/bouncycastle/pqc/jcajce/provider/xmss/BCXMSSPrivateKey.java
@@ -51,7 +51,7 @@ public BCXMSSPrivateKey(PrivateKeyInfo keyInfo)
 
             if (xmssPrivateKey.getBdsState() != null)
             {
-                keyBuilder.withBDSState((BDS)XMSSUtil.deserialize(xmssPrivateKey.getBdsState()));
+                keyBuilder.withBDSState((BDS)XMSSUtil.deserialize(xmssPrivateKey.getBdsState(), BDS.class));
             }
 
             this.keyParams = keyBuilder.build();

Original file line number	Diff line number	Diff line change
`@@ -44,37 +44,30 @@ public RainbowParameters()`
`44`	`44`	`public RainbowParameters(int[] vi)`
`45`	`45`	`{`
`46`	`46`	`this.vi = vi;`
`47`		`- try`
`48`		`- {`
`49`		`- checkParams();`
`50`		`- }`
`51`		`- catch (Exception e)`
`52`		`- {`
`53`		`- e.printStackTrace();`
`54`		`- }`
	`47`	`+`
	`48`	`+ checkParams();`
`55`	`49`	`}`
`56`	`50`
`57`	`51`	`private void checkParams()`
`58`		`- throws Exception`
`59`	`52`	`{`
`60`	`53`	`if (vi == null)`
`61`	`54`	`{`
`62`		`- throw new Exception("no layers defined.");`
	`55`	`+ throw new IllegalArgumentException("no layers defined.");`
`63`	`56`	`}`
`64`	`57`	`if (vi.length > 1)`
`65`	`58`	`{`
`66`	`59`	`for (int i = 0; i < vi.length - 1; i++)`
`67`	`60`	`{`
`68`	`61`	`if (vi[i] >= vi[i + 1])`
`69`	`62`	`{`
`70`		`- throw new Exception(`
	`63`	`+ throw new IllegalArgumentException(`
`71`	`64`	`"v[i] has to be smaller than v[i+1]");`
`72`	`65`	`}`
`73`	`66`	`}`
`74`	`67`	`}`
`75`	`68`	`else`
`76`	`69`	`{`
`77`		`- throw new Exception(`
	`70`	`+ throw new IllegalArgumentException(`
`78`	`71`	`"Rainbow needs at least 1 layer, such that v1 < v2.");`
`79`	`72`	`}`
`80`	`73`	`}`
Original file line number	Diff line number	Diff line change
`@@ -68,21 +68,21 @@ private XMSSMTPrivateKeyParameters(Builder builder)`
`68`	`68`	`/* import BDS state */`
`69`	`69`	`byte[] bdsStateBinary = XMSSUtil.extractBytesAtOffset(privateKey, position, privateKey.length - position);`
`70`	`70`
`71`		`- BDSStateMap bdsImport = null;`
`72`	`71`	`try`
`73`	`72`	`{`
`74`		`- bdsImport = (BDSStateMap)XMSSUtil.deserialize(bdsStateBinary);`
	`73`	`+ BDSStateMap bdsImport = (BDSStateMap)XMSSUtil.deserialize(bdsStateBinary, BDSStateMap.class);`
	`74`	`+`
	`75`	`+ bdsImport.setXMSS(builder.xmss);`
	`76`	`+ bdsState = bdsImport;`
`75`	`77`	`}`
`76`	`78`	`catch (IOException e)`
`77`	`79`	`{`
`78`		`- e.printStackTrace();`
	`80`	`+ throw new IllegalArgumentException(e.getMessage(), e);`
`79`	`81`	`}`
`80`	`82`	`catch (ClassNotFoundException e)`
`81`	`83`	`{`
`82`		`- e.printStackTrace();`
	`84`	`+ throw new IllegalArgumentException(e.getMessage(), e);`
`83`	`85`	`}`
`84`		`- bdsImport.setXMSS(builder.xmss);`
`85`		`- bdsState = bdsImport;`
`86`	`86`	`}`
`87`	`87`	`else`
`88`	`88`	`{`
`@@ -260,17 +260,14 @@ public byte[] toByteArray()`
`260`	`260`	`/* copy root */`
`261`	`261`	`XMSSUtil.copyBytesAtOffset(out, root, position);`
`262`	`262`	`/* concatenate bdsState */`
`263`		`- byte[] bdsStateOut = null;`
`264`	`263`	`try`
`265`	`264`	`{`
`266`		`- bdsStateOut = XMSSUtil.serialize(bdsState);`
	`265`	`+ return Arrays.concatenate(out, XMSSUtil.serialize(bdsState));`
`267`	`266`	`}`
`268`	`267`	`catch (IOException e)`
`269`	`268`	`{`
`270`		`- e.printStackTrace();`
`271`		`- throw new RuntimeException("error serializing bds state");`
	`269`	`+ throw new IllegalStateException("error serializing bds state: " + e.getMessage(), e);`
`272`	`270`	`}`
`273`		`- return Arrays.concatenate(out, bdsStateOut);`
`274`	`271`	`}`
`275`	`272`
`276`	`273`	`public long getIndex()`
Original file line number	Diff line number	Diff line change
`@@ -86,26 +86,25 @@ private XMSSPrivateKeyParameters(Builder builder)`
`86`	`86`	`position += rootSize;`
`87`	`87`	`/* import BDS state */`
`88`	`88`	`byte[] bdsStateBinary = XMSSUtil.extractBytesAtOffset(privateKey, position, privateKey.length - position);`
`89`		`- BDS bdsImport = null;`
`90`	`89`	`try`
`91`	`90`	`{`
`92`		`- bdsImport = (BDS)XMSSUtil.deserialize(bdsStateBinary);`
	`91`	`+ BDS bdsImport = (BDS)XMSSUtil.deserialize(bdsStateBinary, BDS.class);`
	`92`	`+ bdsImport.setXMSS(builder.xmss);`
	`93`	`+ bdsImport.validate();`
	`94`	`+ if (bdsImport.getIndex() != index)`
	`95`	`+ {`
	`96`	`+ throw new IllegalStateException("serialized BDS has wrong index");`
	`97`	`+ }`
	`98`	`+ bdsState = bdsImport;`
`93`	`99`	`}`
`94`	`100`	`catch (IOException e)`
`95`	`101`	`{`
`96`		`- e.printStackTrace();`
	`102`	`+ throw new IllegalArgumentException(e.getMessage(), e);`
`97`	`103`	`}`
`98`	`104`	`catch (ClassNotFoundException e)`
`99`	`105`	`{`
`100`		`- e.printStackTrace();`
`101`		`- }`
`102`		`- bdsImport.setXMSS(builder.xmss);`
`103`		`- bdsImport.validate();`
`104`		`- if (bdsImport.getIndex() != index)`
`105`		`- {`
`106`		`- throw new IllegalStateException("serialized BDS has wrong index");`
	`106`	`+ throw new IllegalArgumentException(e.getMessage(), e);`
`107`	`107`	`}`
`108`		`- bdsState = bdsImport;`
`109`	`108`	`}`
`110`	`109`	`else`
`111`	`110`	`{`
Original file line number	Diff line number	Diff line change
`@@ -52,7 +52,7 @@ public BCXMSSMTPrivateKey(PrivateKeyInfo keyInfo)`
`52`	`52`
`53`	`53`	`if (xmssMtPrivateKey.getBdsState() != null)`
`54`	`54`	`{`
`55`		`- keyBuilder.withBDSState((BDSStateMap)XMSSUtil.deserialize(xmssMtPrivateKey.getBdsState()));`
	`55`	`+ keyBuilder.withBDSState((BDSStateMap)XMSSUtil.deserialize(xmssMtPrivateKey.getBdsState(), BDSStateMap.class));`
`56`	`56`	`}`
`57`	`57`
`58`	`58`	`this.keyParams = keyBuilder.build();`
Original file line number	Diff line number	Diff line change
`@@ -51,7 +51,7 @@ public BCXMSSPrivateKey(PrivateKeyInfo keyInfo)`
`51`	`51`
`52`	`52`	`if (xmssPrivateKey.getBdsState() != null)`
`53`	`53`	`{`
`54`		`- keyBuilder.withBDSState((BDS)XMSSUtil.deserialize(xmssPrivateKey.getBdsState()));`
	`54`	`+ keyBuilder.withBDSState((BDS)XMSSUtil.deserialize(xmssPrivateKey.getBdsState(), BDS.class));`
`55`	`55`	`}`
`56`	`56`
`57`	`57`	`this.keyParams = keyBuilder.build();`