Merge pull request #199 from asacamano/master

Fix DSA Signature Verification

README.md

@@ -21,6 +21,7 @@ Quick Links
 
 - [Discussion Group](http://groups.google.com/group/keyczar-discuss)
 - [Design Document (PDF)](http://keyczar.googlecode.com/files/keyczar05b.pdf)
+- [Known Security Issues](Known-Security-Issues)
 
 Why Keyczar?
 ------------
@@ -65,3 +66,51 @@ Get involved
 Interested in getting involved? We encourage open source developers to
 contribute to the Keyczar project. Please join us on the Keyczar
 project and subscribe to the Keyczar discussion group.
+
+
+Known Security Issues
+---------------------
+The following section lists known security issues.
+
+There are probably others that have not been identified.
+
+
+Signed Session Encryption Re-signing
+-----------------------------
+Keyczar signed session enctyption does not include the key ID of the signing key inside
+the encrypted plaintext. This makes is possible for an attacker to strip the signature
+from a message, and re-sign it using their private key, making it look like they sent
+the original message.
+
+
+DSA Signature Malleability
+--------------------------
+DSA signatures are basically two variable length ints. So some DSA signatures are shorter
+than others.
+
+There was a bug in KeyCzar (fixed
+[here](https://github.com/google/keyczar/commit/fb019ba4c5ed7002b93e632e85c5bb95af860711))
+which essentially padded (right padding with 0) all DSA signatures to their maximum length.
+
+Some crypto libraries - including many JCE implementations - stop checking the signature
+after finding both ints, which means that they will verify signature that have extra
+data. This is why keyczar did not discover the extra data in DSA signatures.
+
+However, this can be a problem for specific crypto applications that compute fingerprints
+of data that includes a message and its signature. See the
+[CVN](https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2014-8275),
+[OpenSSL's comments](https://www.openssl.org/news/vulnerabilities.html#2014-8275)
+and [problem](and https://en.bitcoin.it/wiki/Transaction_Malleability)
+this causes for BitCoint.
+
+Some new JCE implementations are more strict - and will reject DSA signatures with extra
+data. In order for older (improperly padded DSA signatures) to be acceptible even when
+running KeyCzar on such new JCE implementations - the KeyCzar Java DSA verifier function
+trims any extra data from the signature.
+
+Note that this means you should not use this implementation for such applications - such as
+bitcoin - without setting the "keyczar.strict\_dsa\_verification" system property.
+
+As other underlying crypto libraries make this strict - it is probable that other language
+implementations may have this issue.
+

java/code/src/org/keyczar/DsaPublicKey.java

@@ -48,6 +48,15 @@ public class DsaPublicKey extends KeyczarPublicKey {
   private static final String SIG_ALGORITHM = "SHA1withDSA";
   private static final int DSA_DIGEST_SIZE = 48;
 
+  // See DsaVerifyingStream.verify
+  private static boolean strictVerification = Boolean.valueOf(
+      System.getProperty("keyczar.strict_dsa_verification", "false"));
+
+  // visible for testing
+  public static void setStrictVerificationForTest(boolean strictVerification) {
+    DsaPublicKey.strictVerification = strictVerification;
+  }
+
   private DSAPublicKey jcePublicKey;
   private final byte[] hash = new byte[Keyczar.KEY_HASH_SIZE];
   final String y;
@@ -137,7 +146,7 @@ protected Stream getStream() throws KeyczarException {
     Stream cachedStream = cachedStreams.poll();
     if (cachedStream != null) {
       return cachedStream;
-    } 
+    }
     return new DsaVerifyingStream();
   }
 
@@ -150,7 +159,7 @@ public KeyType getType() {
   public byte[] hash() {
     return hash;
   }
-  
+
   @Override
   public Iterable<byte[]> fallbackHash() {
     return super.fallbackHash();
@@ -237,8 +246,58 @@ public void updateVerify(ByteBuffer input) throws KeyczarException {
     @Override
     public boolean verify(ByteBuffer sig) throws KeyczarException {
       try {
-        return signature.verify(sig.array(), sig.position(), sig.limit()
+        // Copy the signature so that it can be safely modified
+        ByteBuffer signatureToVerify = ByteBuffer.allocate(sig.limit() - sig.position());
+        signatureToVerify.put(sig.array(), sig.position(), sig.limit()
             - sig.position());
+        if (!strictVerification) {
+          // A DSA signature is a DER sequence tag, following by a varint length that indicates
+          // length of the rest of the signature. This code truncates the signature to the indicated
+          // length.
+
+          // This is necessary since older versions of KeyCzar had a bug that included extra bytes
+          // at the end of the signature.
+
+          // if at any point we find something that is not as expected, stop processing and let it
+          // just fall through the the JCE.
+
+
+          // Start at the beginning and assume that we will truncate - set the flag to false if
+          // anything happens that is unexpected.
+          signatureToVerify.position(0);
+          boolean truncateSignature = true;
+          // Look for a DER sequence tag
+          if (signatureToVerify.get() != 0x30) {
+            truncateSignature = false;
+          } else {
+            // Now look for the DER length of the signature, which can be at most 48 bytes, so will
+            // only be 1 byte in DER varint.
+            int coefficientLength = signatureToVerify.get() & 0x00FF;
+            if (coefficientLength >= 0x80) {
+              // Nope - this means it's not a DSA 1024 SHA 1 signature
+              truncateSignature = false;
+            } else if (
+                signatureToVerify.position() + coefficientLength > signatureToVerify.limit()) {
+              // Nope - this is also not a well formed DSA 1024 SHA 1 signature
+              truncateSignature = false;
+            } else {
+              // Advance to the first byte past the signature
+              signatureToVerify.position(signatureToVerify.position() + coefficientLength);
+            }
+          }
+          if (truncateSignature) {
+            int bytesToTruncate = signatureToVerify.limit() - signatureToVerify.position();
+            if (bytesToTruncate > 0) {
+              signatureToVerify.limit(signatureToVerify.position());
+            }
+          }
+        }
+
+        // Now do the actual verify
+        signatureToVerify.position(0);
+        return signature.verify(signatureToVerify.array(),
+            signatureToVerify.position(),
+            signatureToVerify.limit() - signatureToVerify.position());
       } catch (GeneralSecurityException e) {
         throw new KeyczarException(e);
       }

java/code/src/org/keyczar/Verifier.java

@@ -179,15 +179,6 @@ boolean rawVerify(KeyczarKey key, final ByteBuffer data, final ByteBuffer hidden
       // The signed data is terminated with the current Keyczar format 
       stream.updateVerify(ByteBuffer.wrap(FORMAT_BYTES));
 
-      // There was a bug in Signer which allowed extra bytes to be included at the end of signatures
-      // in some cases.  There are some crypto libraries which reject signatures with extra bytes at
-      // the end. So this change trims the signature to the expected length so that existing
-      // signatures will not break. Note that if you don't use one of the strict crypto libraries,
-      // then you won't have this problem - but there is no way to identify them.
-      int trimmedSignatureLength =
-          Math.min(stream.digestSize(), signature.limit() - signature.position());
-      ByteBuffer trimmedSignature = ByteBuffer.allocate(trimmedSignatureLength);
-      trimmedSignature.put(signature.array(), signature.position(), trimmedSignatureLength);
       boolean result = stream.verify(signature);
       key.addStreamToCacheForReuse(stream);
       return result;

java/code/tests/org/keyczar/VerifierBackwardsCompatilityTest.java

@@ -1,10 +1,11 @@
 package org.keyczar;
 
-import java.nio.ByteBuffer;
+import java.math.BigInteger;
+import java.util.HashSet;
+import java.util.Set;
 import junit.framework.TestCase;
 import org.junit.Test;
 import org.keyczar.exceptions.KeyczarException;
-import org.keyczar.util.Base64Coder;
 
 /**
  * This test makes sure that the verification of both proper length signatures and signatures with
@@ -14,35 +15,76 @@ public class VerifierBackwardsCompatilityTest extends TestCase {
   private static final String TEST_DATA = "./testdata";
   private Signer privateKeySigner;
   private Verifier publicKeyVerifier;
-  private byte[] data;
 
   @Override
   protected void setUp() throws Exception {
     super.setUp();
     privateKeySigner = new Signer(TEST_DATA + "/dsa");
     publicKeyVerifier = new Verifier(TEST_DATA + "/dsa.public");
-    data = Base64Coder.decodeWebSafe(
-        "U3VjY2VzcyEgWW91J3ZlIG1hbmFnZWQgdG8gaW5maWx0cmF0ZSBDb21tYW5kZXIgTGFtYmRhJ3MgZXZpbCBvcmdhbm"
-        + "l6YXRpb24sIGFuZCBmaW5hbGx5IGVhcm5lZCB5b3Vyc2VsZiBhbiBlbnRyeS1sZXZlbCBwb3NpdGlvbiBhcyBhIE"
-        + "1pbmlvbiBvbiBoZXIgc3BhY2Ugc3RhdGlvbi4gRnJvbSBoZXJlLCB5b3UganVzdCBtaWdodCBiZSBhYmxlIHRvIH"
-        + "N1YnZlcnQgaGVyIHBsYW5zIHRvIHVzZSB0aGUgTEFNQkNIT1AgZG9vbXNkYXkgZGV2aWNlIHRvIGRlc3Ryb3kgQn"
-        + "VubnkgUGxhbmV0LiBQcm9ibGVtIGlzLCBNaW5pb25zIGFyZSB0aGUgbG93ZXN0IG9mIHRoZSBsb3cgaW4gdGhlIE"
-        + "xhbWJkYSBoaWVyYXJjaHkuIEJldHRlciBidWNrIHVwIGFuZCBnZXQgd29ya2luZywgb3IgeW91J2xsIG5ldmVyIG"
-        + "1ha2UgaXQgdG8gdGhlIHRvcC4uLg=="
-        );
   }
 
   @Test
-  public final void testVerifyWithExtraBytes() throws KeyczarException {
-    byte[] signature = privateKeySigner.sign(data);
-    for (int cruftSize = 0; cruftSize < 16; cruftSize++) {
-      ByteBuffer cruftySignature = ByteBuffer.allocate(signature.length + cruftSize);
-      cruftySignature.put(signature);
-      for (int i = 0; i < cruftSize; i++) {
-        cruftySignature.put((byte)i);
+  public final void testVariableLengthSignaturesWithExtraBytes() throws KeyczarException {
+    // This is (I'm so sorry) a random test - because the signing process includes a random element.
+    // However, generally all these three sizes are found quite quickly, so this should not be too
+    // slow, nor flaky (if it flakes, buy a lottery ticket, and yell at me).
+    Set<Integer> remainingSizes = new HashSet<Integer>();
+    remainingSizes.add(50);
+    remainingSizes.add(51);
+    remainingSizes.add(52);
+
+    int limit = 10000;
+    Boolean jceDoesStrictVerification = null;
+    for (int i = 0; i < limit; i++) {
+      byte[] plaintext = BigInteger.valueOf(i).toByteArray();
+      byte[] signature = privateKeySigner.sign(plaintext);
+      int signatureSize = signature.length;
+      // Is this is a size that still needs to be tested
+      if (remainingSizes.contains(signatureSize)) {
+        for (int totalLength = signatureSize; totalLength <= 52; totalLength++) {
+          String error = "for " + i + ":" + signatureSize + ":" + totalLength;
+          byte[] signatureWithExtraBytes = new byte[totalLength];
+          System.arraycopy(signature, 0, signatureWithExtraBytes, 0, signature.length);
+          int extraBytesLength = totalLength - signature.length;
+
+          // Start by testing without strict verification - even with extra bytes at the end,
+          // the signatures should verify.
+          DsaPublicKey.setStrictVerificationForTest(false);
+          assertTrue("Invalid signature without strict verification " + error,
+              publicKeyVerifier.verify(plaintext, signatureWithExtraBytes));
+
+          // Now test without keyzar stripping the extra bytes.
+          DsaPublicKey.setStrictVerificationForTest(true);
+
+          // If there is no extra bytes, the signaure should verify
+          if (extraBytesLength == 0) {
+            assertTrue("Invalid signature with strict verification " + error,
+                publicKeyVerifier.verify(plaintext, signatureWithExtraBytes));
+          } else {
+            // This is dependant on the behavior of the JCE - so the first time through, use the
+            // behavior to decide whether the JCE has strict DSA verification - and then after that
+            // require that the behavior stays the same
+            boolean jceStrictVerificationDemonstrated =
+                !publicKeyVerifier.verify(plaintext, signatureWithExtraBytes);
+            if (jceDoesStrictVerification == null) {
+              jceDoesStrictVerification = jceStrictVerificationDemonstrated;
+              System.err.println("JCE strict DSA verification : " + jceDoesStrictVerification);
+            } else {
+              assertEquals("Invalid signature with strict verification " + error,
+                  jceStrictVerificationDemonstrated, jceDoesStrictVerification.booleanValue());
+            }
+          }
+        }
+
+        // Have we checked all the sizes we need to check - if so, stop
+        remainingSizes.remove(signatureSize);
+        if (remainingSizes.isEmpty()) {
+          return;
+        }
       }
-      assertTrue("failed with " + cruftSize + " bytes of cruft",
-          publicKeyVerifier.verify(data, cruftySignature.array()));
     }
+    // If we goes here, buy a lottery ticket because you're doing some crazy odds today.
+    fail("Failed after " + limit + " attempts to get 3 different signature sizes");
   }
-}
+}
+