From 519c200997d824f34ef03487b12c3eb84d412372 Mon Sep 17 00:00:00 2001 From: Andrew Sacamano Date: Mon, 7 Nov 2016 17:17:11 -0700 Subject: [PATCH] Following more reviews from quannguyen@google.com, and bleichen@google.com - make the previous fix to the DSA verification code actually work. --- README.md | 49 +++++++++++ java/code/src/org/keyczar/DsaPublicKey.java | 65 +++++++++++++- java/code/src/org/keyczar/Verifier.java | 9 -- .../VerifierBackwardsCompatilityTest.java | 86 ++++++++++++++----- 4 files changed, 175 insertions(+), 34 deletions(-) diff --git a/README.md b/README.md index 68cbb37a..0abd0182 100644 --- a/README.md +++ b/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. + diff --git a/java/code/src/org/keyczar/DsaPublicKey.java b/java/code/src/org/keyczar/DsaPublicKey.java index 92432e05..3fa71ca1 100644 --- a/java/code/src/org/keyczar/DsaPublicKey.java +++ b/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 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); } diff --git a/java/code/src/org/keyczar/Verifier.java b/java/code/src/org/keyczar/Verifier.java index 90f1b908..9dfd6580 100644 --- a/java/code/src/org/keyczar/Verifier.java +++ b/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; diff --git a/java/code/tests/org/keyczar/VerifierBackwardsCompatilityTest.java b/java/code/tests/org/keyczar/VerifierBackwardsCompatilityTest.java index d035f744..3c968071 100644 --- a/java/code/tests/org/keyczar/VerifierBackwardsCompatilityTest.java +++ b/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 remainingSizes = new HashSet(); + 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"); } -} \ No newline at end of file +} +