added length check for sequence in DSA signatures

bcgit · Oct 14, 2016 · b0c3ce9 · b0c3ce9
1 parent 0049491
commit b0c3ce9
Show file tree

Hide file tree

Showing 2 changed files with 110 additions and 3 deletions.
diff --git a/prov/src/main/java/org/bouncycastle/jcajce/provider/asymmetric/dsa/DSASigner.java b/prov/src/main/java/org/bouncycastle/jcajce/provider/asymmetric/dsa/DSASigner.java
@@ -8,8 +8,6 @@
 import java.security.SecureRandom;
 import java.security.SignatureException;
 import java.security.SignatureSpi;
-import java.security.interfaces.DSAKey;
-import java.security.interfaces.DSAPublicKey;
 import java.security.spec.AlgorithmParameterSpec;
 
 import org.bouncycastle.asn1.ASN1Encoding;
@@ -18,7 +16,6 @@
 import org.bouncycastle.asn1.ASN1Sequence;
 import org.bouncycastle.asn1.DERSequence;
 import org.bouncycastle.asn1.pkcs.PKCSObjectIdentifiers;
-import org.bouncycastle.asn1.x509.SubjectPublicKeyInfo;
 import org.bouncycastle.asn1.x509.X509ObjectIdentifiers;
 import org.bouncycastle.crypto.CipherParameters;
 import org.bouncycastle.crypto.DSA;
@@ -179,6 +176,11 @@ private BigInteger[] derDecode(
         throws IOException
     {
         ASN1Sequence s = (ASN1Sequence)ASN1Primitive.fromByteArray(encoding);
+        if (s.size() != 2)
+        {
+            throw new IOException("malformed signature");
+        }
+
         return new BigInteger[]{
             ((ASN1Integer)s.getObjectAt(0)).getValue(),
             ((ASN1Integer)s.getObjectAt(1)).getValue()

diff --git a/prov/src/test/java/org/bouncycastle/jce/provider/test/DSATest.java b/prov/src/test/java/org/bouncycastle/jce/provider/test/DSATest.java
@@ -70,6 +70,110 @@ public class DSATest
 
     SecureRandom    random = new FixedSecureRandom(new byte[][] { k1, k2 });
 
+    // DSA modified signatures, courtesy of the Google security team
+    static final DSAPrivateKeySpec PRIVATE_KEY = new DSAPrivateKeySpec(
+        // x
+        new BigInteger(
+            "15382583218386677486843706921635237927801862255437148328980464126979"),
+        // p
+        new BigInteger(
+            "181118486631420055711787706248812146965913392568235070235446058914"
+            + "1170708161715231951918020125044061516370042605439640379530343556"
+            + "4101919053459832890139496933938670005799610981765220283775567361"
+            + "4836626483403394052203488713085936276470766894079318754834062443"
+            + "1033792580942743268186462355159813630244169054658542719322425431"
+            + "4088256212718983105131138772434658820375111735710449331518776858"
+            + "7867938758654181244292694091187568128410190746310049564097068770"
+            + "8161261634790060655580211122402292101772553741704724263582994973"
+            + "9109274666495826205002104010355456981211025738812433088757102520"
+            + "562459649777989718122219159982614304359"),
+        // q
+        new BigInteger(
+            "19689526866605154788513693571065914024068069442724893395618704484701"),
+        // g
+        new BigInteger(
+            "2859278237642201956931085611015389087970918161297522023542900348"
+            + "0877180630984239764282523693409675060100542360520959501692726128"
+            + "3149190229583566074777557293475747419473934711587072321756053067"
+            + "2532404847508798651915566434553729839971841903983916294692452760"
+            + "2490198571084091890169933809199002313226100830607842692992570749"
+            + "0504363602970812128803790973955960534785317485341020833424202774"
+            + "0275688698461842637641566056165699733710043802697192696426360843"
+            + "1736206792141319514001488556117408586108219135730880594044593648"
+            + "9237302749293603778933701187571075920849848690861126195402696457"
+            + "4111219599568903257472567764789616958430"));
+
+    static final DSAPublicKeySpec PUBLIC_KEY = new DSAPublicKeySpec(
+        new BigInteger(
+            "3846308446317351758462473207111709291533523711306097971550086650"
+            + "2577333637930103311673872185522385807498738696446063139653693222"
+            + "3528823234976869516765207838304932337200968476150071617737755913"
+            + "3181601169463467065599372409821150709457431511200322947508290005"
+            + "1780020974429072640276810306302799924668893998032630777409440831"
+            + "4314588994475223696460940116068336991199969153649625334724122468"
+            + "7497038281983541563359385775312520539189474547346202842754393945"
+            + "8755803223951078082197762886933401284142487322057236814878262166"
+            + "5072306622943221607031324846468109901964841479558565694763440972"
+            + "5447389416166053148132419345627682740529"),
+         PRIVATE_KEY.getP(),
+         PRIVATE_KEY.getQ(),
+         PRIVATE_KEY.getG());
+
+    // The following test vectors check for signature malleability and bugs. That means the test
+    // vectors are derived from a valid signature by modifying the ASN encoding. A correct
+    // implementation of DSA should only accept correct DER encoding and properly handle the others.
+    // Allowing alternative BER encodings is in many cases benign. An example where this kind of
+    // signature malleability was a problem: https://en.bitcoin.it/wiki/Transaction_Malleability
+    static final String[] MODIFIED_SIGNATURES  = {
+        "303e02811c1e41b479ad576905b960fe14eadb91b0ccf34843dab916173bb8c9cd021d00ade65988d237d30f9e"
+        + "f41dd424a4e1c8f16967cf3365813fe8786236",
+        "303f0282001c1e41b479ad576905b960fe14eadb91b0ccf34843dab916173bb8c9cd021d00ade65988d237d30f"
+        + "9ef41dd424a4e1c8f16967cf3365813fe8786236",
+        "303e021d001e41b479ad576905b960fe14eadb91b0ccf34843dab916173bb8c9cd021d00ade65988d237d30f9e"
+        + "f41dd424a4e1c8f16967cf3365813fe8786236",
+        "303e021c1e41b479ad576905b960fe14eadb91b0ccf34843dab916173bb8c9cd02811d00ade65988d237d30f9e"
+        + "f41dd424a4e1c8f16967cf3365813fe8786236",
+        "303f021c1e41b479ad576905b960fe14eadb91b0ccf34843dab916173bb8c9cd0282001d00ade65988d237d30f"
+        + "9ef41dd424a4e1c8f16967cf3365813fe8786236",
+        "303e021c1e41b479ad576905b960fe14eadb91b0ccf34843dab916173bb8c9cd021e0000ade65988d237d30f9e"
+        + "f41dd424a4e1c8f16967cf3365813fe8786236",
+        "30813d021c1e41b479ad576905b960fe14eadb91b0ccf34843dab916173bb8c9cd021d00ade65988d237d30f9e"
+        + "f41dd424a4e1c8f16967cf3365813fe8786236",
+        "3082003d021c1e41b479ad576905b960fe14eadb91b0ccf34843dab916173bb8c9cd021d00ade65988d237d30f"
+        + "9ef41dd424a4e1c8f16967cf3365813fe8786236",
+        "303d021c1e41b479ad576905b960fe14eadb91b0ccf34843dab916173bb8c9cd021d00ade65988d237d30f9ef4"
+        + "1dd424a4e1c8f16967cf3365813fe87862360000",
+        "3040021c57b10411b54ab248af03d8f2456676ebc6d3db5f1081492ac87e9ca8021d00942b117051d7d9d107fc42cac9c5a36a1fd7f0f8916ccca86cec4ed3040100"
+    };
+
+    private void testModified()
+        throws Exception
+    {
+        KeyFactory kFact = KeyFactory.getInstance("DSA", "BC");
+        PublicKey pubKey = kFact.generatePublic(PUBLIC_KEY);
+        Signature sig = Signature.getInstance("DSA", "BC");
+
+        for (int i = 0; i != MODIFIED_SIGNATURES.length; i++)
+        {
+            sig.initVerify(pubKey);
+
+            sig.update(Strings.toByteArray("Hello"));
+
+            boolean failed;
+
+            try
+            {
+                failed = !sig.verify(Hex.decode(MODIFIED_SIGNATURES[i]));
+            }
+            catch (SignatureException e)
+            {
+                failed = true;
+            }
+
+            isTrue("sig verified when shouldn't", failed);
+        }
+    }
+
     private void testCompat()
         throws Exception
     {
@@ -1223,6 +1327,7 @@ public void performTest()
         testDSA2Parameters();
         testNullParameters();
         testValidate();
+        testModified();
     }
 
     protected BigInteger[] derDecode(