added additional checking to XMSS BDS tree parsing. Failures now most…

…ly cause IOException
bcgit · Mar 3, 2018 · 4092ede · 4092ede
1 parent 0a702f1
commit 4092ede
Show file tree

Hide file tree

Showing 10 changed files with 97 additions and 66 deletions.
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();