diff --git a/core/src/main/java/com/google/bitcoin/core/ECKey.java b/core/src/main/java/com/google/bitcoin/core/ECKey.java index 8fe17944..fbd5fd09 100644 --- a/core/src/main/java/com/google/bitcoin/core/ECKey.java +++ b/core/src/main/java/com/google/bitcoin/core/ECKey.java @@ -475,57 +475,24 @@ public class ECKey implements Serializable { } /** - * Returns true if the given signature is in canonical form (ie will be accepted as standard by the reference client) + * Returns true if this pubkey is canonical, i.e. the correct length taking into account compression. */ - public static boolean isSignatureCanonical(byte[] signature) { - // See reference client's IsCanonicalSignature, https://bitcointalk.org/index.php?topic=8392.msg127623#msg127623 - // A canonical signature exists of: <30> <02> <02> - // Where R and S are not negative (their first byte has its highest bit not set), and not - // excessively padded (do not start with a 0 byte, unless an otherwise negative number follows, - // in which case a single 0 byte is necessary and even required). - if (signature.length < 9 || signature.length > 73) - return false; - - int hashType = signature[signature.length-1] & ((int)(~Transaction.SIGHASH_ANYONECANPAY_VALUE)); - if (hashType < (Transaction.SigHash.ALL.ordinal() + 1) || hashType > (Transaction.SigHash.SINGLE.ordinal() + 1)) - return false; - - // "wrong type" "wrong length marker" - if ((signature[0] & 0xff) != 0x30 || (signature[1] & 0xff) != signature.length-3) - return false; - - int lenR = signature[3] & 0xff; - if (5 + lenR >= signature.length || lenR == 0) - return false; - int lenS = signature[5+lenR] & 0xff; - if (lenR + lenS + 7 != signature.length || lenS == 0) - return false; - - // R value type mismatch R value negative - if (signature[4-2] != 0x02 || (signature[4] & 0x80) == 0x80) - return false; - if (lenR > 1 && signature[4] == 0x00 && (signature[4+1] & 0x80) != 0x80) - return false; // R value excessively padded - - // S value type mismatch S value negative - if (signature[6 + lenR - 2] != 0x02 || (signature[6 + lenR] & 0x80) == 0x80) - return false; - if (lenS > 1 && signature[6 + lenR] == 0x00 && (signature[6 + lenR + 1] & 0x80) != 0x80) - return false; // S value excessively padded - - return true; + public boolean isPubKeyCanonical() { + return isPubKeyCanonical(pub); } /** - * Returns true if the given pubkey is canonical (ie the correct length for its declared type) + * Returns true if the given pubkey is canonical, i.e. the correct length taking into account compression. */ public static boolean isPubKeyCanonical(byte[] pubkey) { if (pubkey.length < 33) return false; - if (pubkey[0] == 0x04) { // Uncompressed pubkey + if (pubkey[0] == 0x04) { + // Uncompressed pubkey if (pubkey.length != 65) return false; - } else if (pubkey[0] == 0x02 || pubkey[0] == 0x03) { // Compressed pubkey + } else if (pubkey[0] == 0x02 || pubkey[0] == 0x03) { + // Compressed pubkey if (pubkey.length != 33) return false; } else diff --git a/core/src/main/java/com/google/bitcoin/core/Transaction.java b/core/src/main/java/com/google/bitcoin/core/Transaction.java index 1b22a711..2fdbbc33 100644 --- a/core/src/main/java/com/google/bitcoin/core/Transaction.java +++ b/core/src/main/java/com/google/bitcoin/core/Transaction.java @@ -797,11 +797,7 @@ public class Transaction extends ChildMessage implements Serializable { // The anyoneCanPay feature isn't used at the moment. boolean anyoneCanPay = false; byte[] connectedPubKeyScript = input.getOutpoint().getConnectedPubKeyScript(); - Sha256Hash hash = hashTransactionForSignature(i, connectedPubKeyScript, hashType, anyoneCanPay); - - // Now calculate the signatures we need to prove we own this transaction and are authorized to claim the - // associated money. - signatures[i] = new TransactionSignature(key.sign(hash, aesKey), hashType, anyoneCanPay); + signatures[i] = calculateSignature(i, key, aesKey, connectedPubKeyScript, hashType, anyoneCanPay); } // Now we have calculated each signature, go through and create the scripts. Reminder: the script consists: @@ -828,6 +824,45 @@ public class Transaction extends ChildMessage implements Serializable { // Every input is now complete. } + /** + * Calculates a signature that is valid for being inserted into the input at the given position. This is simply + * a wrapper around calling {@link Transaction#hashForSignature(int, byte[], com.google.bitcoin.core.Transaction.SigHash, boolean)} + * followed by {@link ECKey#sign(Sha256Hash, org.spongycastle.crypto.params.KeyParameter)} and then returning + * a new {@link TransactionSignature}. + * + * @param inputIndex Which input to calculate the signature for, as an index. + * @param key The private key used to calculate the signature. + * @param aesKey If not null, this will be used to decrypt the key. + * @param connectedPubKeyScript Byte-exact contents of the scriptPubKey that is being satisified. + * @param hashType Signing mode, see the enum for documentation. + * @param anyoneCanPay Signing mode, see the SigHash enum for documentation. + * @return A newly calculated signature object that wraps the r, s and sighash components. + */ + public synchronized TransactionSignature calculateSignature(int inputIndex, ECKey key, KeyParameter aesKey, + byte[] connectedPubKeyScript, + SigHash hashType, boolean anyoneCanPay) { + Sha256Hash hash = hashForSignature(inputIndex, connectedPubKeyScript, hashType, anyoneCanPay); + return new TransactionSignature(key.sign(hash, aesKey), hashType, anyoneCanPay); + } + + /** + * Calculates a signature that is valid for being inserted into the input at the given position. This is simply + * a wrapper around calling {@link Transaction#hashForSignature(int, byte[], com.google.bitcoin.core.Transaction.SigHash, boolean)} + * followed by {@link ECKey#sign(Sha256Hash)} and then returning a new {@link TransactionSignature}. + * + * @param inputIndex Which input to calculate the signature for, as an index. + * @param key The private key used to calculate the signature. + * @param connectedPubKeyScript The scriptPubKey that is being satisified. + * @param hashType Signing mode, see the enum for documentation. + * @param anyoneCanPay Signing mode, see the SigHash enum for documentation. + * @return A newly calculated signature object that wraps the r, s and sighash components. + */ + public synchronized TransactionSignature calculateSignature(int inputIndex, ECKey key, Script connectedPubKeyScript, + SigHash hashType, boolean anyoneCanPay) { + Sha256Hash hash = hashForSignature(inputIndex, connectedPubKeyScript.getProgram(), hashType, anyoneCanPay); + return new TransactionSignature(key.sign(hash), hashType, anyoneCanPay); + } + /** *

Calculates a signature hash, that is, a hash of a simplified form of the transaction. How exactly the transaction * is simplified is specified by the type and anyoneCanPay parameters.

@@ -840,10 +875,10 @@ public class Transaction extends ChildMessage implements Serializable { * @param type Should be SigHash.ALL * @param anyoneCanPay should be false. */ - public synchronized Sha256Hash hashTransactionForSignature(int inputIndex, byte[] connectedScript, - SigHash type, boolean anyoneCanPay) { + public synchronized Sha256Hash hashForSignature(int inputIndex, byte[] connectedScript, + SigHash type, boolean anyoneCanPay) { byte sigHashType = (byte) TransactionSignature.calcSigHashValue(type, anyoneCanPay); - return hashTransactionForSignature(inputIndex, connectedScript, sigHashType); + return hashForSignature(inputIndex, connectedScript, sigHashType); } /** @@ -858,17 +893,17 @@ public class Transaction extends ChildMessage implements Serializable { * @param type Should be SigHash.ALL * @param anyoneCanPay should be false. */ - public synchronized Sha256Hash hashTransactionForSignature(int inputIndex, Script connectedScript, - SigHash type, boolean anyoneCanPay) { + public synchronized Sha256Hash hashForSignature(int inputIndex, Script connectedScript, + SigHash type, boolean anyoneCanPay) { int sigHash = TransactionSignature.calcSigHashValue(type, anyoneCanPay); - return hashTransactionForSignature(inputIndex, connectedScript.getProgram(), (byte) sigHash); + return hashForSignature(inputIndex, connectedScript.getProgram(), (byte) sigHash); } /** * This is required for signatures which use a sigHashType which cannot be represented using SigHash and anyoneCanPay * See transaction c99c49da4c38af669dea436d3e73780dfdb6c1ecf9958baa52960e8baee30e73, which has sigHashType 0 */ - public synchronized Sha256Hash hashTransactionForSignature(int inputIndex, byte[] connectedScript, byte sigHashType) { + public synchronized Sha256Hash hashForSignature(int inputIndex, byte[] connectedScript, byte sigHashType) { // The SIGHASH flags are used in the design of contracts, please see this page for a further understanding of // the purposes of the code in this method: // diff --git a/core/src/main/java/com/google/bitcoin/crypto/TransactionSignature.java b/core/src/main/java/com/google/bitcoin/crypto/TransactionSignature.java index 5a45c0ba..224d3718 100644 --- a/core/src/main/java/com/google/bitcoin/crypto/TransactionSignature.java +++ b/core/src/main/java/com/google/bitcoin/crypto/TransactionSignature.java @@ -18,6 +18,7 @@ package com.google.bitcoin.crypto; import com.google.bitcoin.core.ECKey; import com.google.bitcoin.core.Transaction; +import com.google.bitcoin.core.VerificationException; import java.io.ByteArrayOutputStream; import java.io.IOException; @@ -55,6 +56,53 @@ public class TransactionSignature extends ECKey.ECDSASignature { return sighashFlags; } + /** + * Returns true if the given signature is has canonical encoding, and will thus be accepted as standard by + * the reference client. DER and the SIGHASH encoding allow for quite some flexibility in how the same structures + * are encoded, and this can open up novel attacks in which a man in the middle takes a transaction and then + * changes its signature such that the transaction hash is different but it's still valid. This can confuse wallets + * and generally violates people's mental model of how Bitcoin should work, thus, non-canonical signatures are now + * not relayed by default. + */ + public static boolean isEncodingCanonical(byte[] signature) { + // See reference client's IsCanonicalSignature, https://bitcointalk.org/index.php?topic=8392.msg127623#msg127623 + // A canonical signature exists of: <30> <02> <02> + // Where R and S are not negative (their first byte has its highest bit not set), and not + // excessively padded (do not start with a 0 byte, unless an otherwise negative number follows, + // in which case a single 0 byte is necessary and even required). + if (signature.length < 9 || signature.length > 73) + return false; + + int hashType = signature[signature.length-1] & ((int)(~Transaction.SIGHASH_ANYONECANPAY_VALUE)); + if (hashType < (Transaction.SigHash.ALL.ordinal() + 1) || hashType > (Transaction.SigHash.SINGLE.ordinal() + 1)) + return false; + + // "wrong type" "wrong length marker" + if ((signature[0] & 0xff) != 0x30 || (signature[1] & 0xff) != signature.length-3) + return false; + + int lenR = signature[3] & 0xff; + if (5 + lenR >= signature.length || lenR == 0) + return false; + int lenS = signature[5+lenR] & 0xff; + if (lenR + lenS + 7 != signature.length || lenS == 0) + return false; + + // R value type mismatch R value negative + if (signature[4-2] != 0x02 || (signature[4] & 0x80) == 0x80) + return false; + if (lenR > 1 && signature[4] == 0x00 && (signature[4+1] & 0x80) != 0x80) + return false; // R value excessively padded + + // S value type mismatch S value negative + if (signature[6 + lenR - 2] != 0x02 || (signature[6 + lenR] & 0x80) == 0x80) + return false; + if (lenS > 1 && signature[6 + lenR] == 0x00 && (signature[6 + lenR + 1] & 0x80) != 0x80) + return false; // S value excessively padded + + return true; + } + /** Configures the sighashFlags field as appropriate. */ public void setSigHash(Transaction.SigHash mode, boolean anyoneCanPay) { sighashFlags = calcSigHashValue(mode, anyoneCanPay); @@ -93,13 +141,16 @@ public class TransactionSignature extends ECKey.ECDSASignature { * Returns a decoded signature. * @throws RuntimeException if the signature is invalid or unparseable in some way. */ - public static TransactionSignature decodeFromBitcoin(byte[] bytes) { + public static TransactionSignature decodeFromBitcoin(byte[] bytes, boolean requireCanonical) throws VerificationException { // Bitcoin encoding is DER signature + sighash byte. + if (requireCanonical && !isEncodingCanonical(bytes)) + throw new VerificationException("Signature encoding is not canonical."); ECKey.ECDSASignature sig = ECKey.ECDSASignature.decodeFromDER(bytes); if (sig == null) - throw new RuntimeException("Could not DER decode signature."); + throw new VerificationException("Could not decode DER component."); TransactionSignature tsig = new TransactionSignature(sig.r, sig.s); - // In Bitcoin, any value of the final byte is valid unfortunately. However it may not be "canonical". + // In Bitcoin, any value of the final byte is valid, but not necessarily canonical. See javadocs for + // isEncodingCanonical to learn more about this. tsig.sighashFlags = bytes[bytes.length - 1]; return tsig; } diff --git a/core/src/main/java/com/google/bitcoin/script/Script.java b/core/src/main/java/com/google/bitcoin/script/Script.java index ca508446..a5f2c17e 100644 --- a/core/src/main/java/com/google/bitcoin/script/Script.java +++ b/core/src/main/java/com/google/bitcoin/script/Script.java @@ -1061,8 +1061,8 @@ public class Script { // TODO: Use int for indexes everywhere, we can't have that many inputs/outputs boolean sigValid = false; try { - TransactionSignature sig = TransactionSignature.decodeFromBitcoin(sigBytes); - Sha256Hash hash = txContainingThis.hashTransactionForSignature(index, connectedScript, (byte)sig.sighashFlags); + TransactionSignature sig = TransactionSignature.decodeFromBitcoin(sigBytes, false); + Sha256Hash hash = txContainingThis.hashForSignature(index, connectedScript, (byte) sig.sighashFlags); sigValid = ECKey.verify(hash.getBytes(), sig, pubKey); } catch (Exception e1) { // There is (at least) one exception that could be hit here (EOFException, if the sig is too short) @@ -1131,8 +1131,8 @@ public class Script { // We could reasonably move this out of the loop, but because signature verification is significantly // more expensive than hashing, its not a big deal. try { - TransactionSignature sig = TransactionSignature.decodeFromBitcoin(sigs.getFirst()); - Sha256Hash hash = txContainingThis.hashTransactionForSignature(index, connectedScript, (byte)sig.sighashFlags); + TransactionSignature sig = TransactionSignature.decodeFromBitcoin(sigs.getFirst(), false); + Sha256Hash hash = txContainingThis.hashForSignature(index, connectedScript, (byte) sig.sighashFlags); if (ECKey.verify(hash.getBytes(), sig, pubKey)) sigs.pollFirst(); } catch (Exception e) { diff --git a/core/src/test/java/com/google/bitcoin/core/ECKeyTest.java b/core/src/test/java/com/google/bitcoin/core/ECKeyTest.java index 20addb62..1c631c5b 100644 --- a/core/src/test/java/com/google/bitcoin/core/ECKeyTest.java +++ b/core/src/test/java/com/google/bitcoin/core/ECKeyTest.java @@ -19,6 +19,7 @@ package com.google.bitcoin.core; import com.google.bitcoin.crypto.EncryptedPrivateKey; import com.google.bitcoin.crypto.KeyCrypter; import com.google.bitcoin.crypto.KeyCrypterScrypt; +import com.google.bitcoin.crypto.TransactionSignature; import com.google.bitcoin.params.MainNetParams; import com.google.bitcoin.params.TestNet3Params; import com.google.bitcoin.params.UnitTestParams; @@ -404,7 +405,7 @@ public class ECKeyTest { while (in.available() > 0 && (c = in.read()) != '"') sig.append((char)c); - assertTrue(ECKey.isSignatureCanonical(Hex.decode(sig.toString()))); + assertTrue(TransactionSignature.isEncodingCanonical(Hex.decode(sig.toString()))); } in.close(); } @@ -426,7 +427,7 @@ public class ECKeyTest { sig.append((char)c); try { - assertFalse(ECKey.isSignatureCanonical(Hex.decode(sig.toString()))); + assertFalse(TransactionSignature.isEncodingCanonical(Hex.decode(sig.toString()))); } catch (StringIndexOutOfBoundsException e) { } // Expected for non-hex strings in the JSON that we should ignore } in.close(); @@ -447,7 +448,7 @@ public class ECKeyTest { byte[] sigBytes = key.sign(new Sha256Hash(hash)).encodeToDER(); byte[] encodedSig = Arrays.copyOf(sigBytes, sigBytes.length + 1); encodedSig[sigBytes.length] = (byte) (Transaction.SigHash.ALL.ordinal() + 1); - if (!ECKey.isSignatureCanonical(encodedSig)) { + if (!TransactionSignature.isEncodingCanonical(encodedSig)) { log.error(Utils.bytesToHexString(sigBytes)); fail(); } diff --git a/core/src/test/java/com/google/bitcoin/core/FullBlockTestGenerator.java b/core/src/test/java/com/google/bitcoin/core/FullBlockTestGenerator.java index 8e7f2a14..9bff98e6 100644 --- a/core/src/test/java/com/google/bitcoin/core/FullBlockTestGenerator.java +++ b/core/src/test/java/com/google/bitcoin/core/FullBlockTestGenerator.java @@ -716,7 +716,7 @@ public class FullBlockTestGenerator { if (scriptSig == null) { // Exploit the SigHash.SINGLE bug to avoid having to make more than one signature - Sha256Hash hash = tx.hashTransactionForSignature(1, b39p2shScriptPubKey, SigHash.SINGLE, false); + Sha256Hash hash = tx.hashForSignature(1, b39p2shScriptPubKey, SigHash.SINGLE, false); // Sign input try { @@ -785,7 +785,7 @@ public class FullBlockTestGenerator { if (scriptSig == null) { // Exploit the SigHash.SINGLE bug to avoid having to make more than one signature - Sha256Hash hash = tx.hashTransactionForSignature(1, + Sha256Hash hash = tx.hashForSignature(1, b39p2shScriptPubKey, SigHash.SINGLE, false); // Sign input @@ -1565,7 +1565,7 @@ public class FullBlockTestGenerator { t.addInput(input); byte[] connectedPubKeyScript = prevOut.scriptPubKey.getProgram(); - Sha256Hash hash = t.hashTransactionForSignature(0, connectedPubKeyScript, SigHash.ALL, false); + Sha256Hash hash = t.hashForSignature(0, connectedPubKeyScript, SigHash.ALL, false); // Sign input try { diff --git a/core/src/test/java/com/google/bitcoin/core/FullPrunedBlockChainTest.java b/core/src/test/java/com/google/bitcoin/core/FullPrunedBlockChainTest.java index d81546ba..43ac8cdb 100644 --- a/core/src/test/java/com/google/bitcoin/core/FullPrunedBlockChainTest.java +++ b/core/src/test/java/com/google/bitcoin/core/FullPrunedBlockChainTest.java @@ -160,7 +160,7 @@ public class FullPrunedBlockChainTest { TransactionInput input = new TransactionInput(params, t, new byte[]{}, prevOut); t.addInput(input); - Sha256Hash hash = t.hashTransactionForSignature(0, prevOutScriptPubKey, SigHash.ALL, false); + Sha256Hash hash = t.hashForSignature(0, prevOutScriptPubKey, SigHash.ALL, false); // Sign input try {