From b0c3ce99d43d73a096268831d0d120ffc89eac7f Mon Sep 17 00:00:00 2001 From: David Hook Date: Sat, 15 Oct 2016 09:50:44 +1100 Subject: [PATCH] added length check for sequence in DSA signatures --- .../provider/asymmetric/dsa/DSASigner.java | 8 +- .../jce/provider/test/DSATest.java | 105 ++++++++++++++++++ 2 files changed, 110 insertions(+), 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 index 877b0aa519..0138efcf8f 100644 --- 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 index 81760f23c6..83a1cec4b6 100644 --- 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(