From 756d7740b367ab7aee3426ce00f71fcdbdd7406d Mon Sep 17 00:00:00 2001 From: Chris Stewart Date: Mon, 25 Apr 2016 09:56:03 -0500 Subject: [PATCH] Adding functionality to test pubkey encoding, fails the script with appropriate error if the pubkey is not encoded correctly --- .../scalacoin/crypto/DERSignatureUtil.scala | 4 +- .../crypto/TransactionSignatureChecker.scala | 11 ++-- .../TransactionSignatureCheckerResult.scala | 8 +++ .../script/crypto/CryptoInterpreter.scala | 10 ++-- .../script/flag/ScriptFlagUtil.scala | 7 +++ .../scalacoin/util/BitcoinScriptUtil.scala | 53 ++++++++++++++++++- .../util/BitcoinScriptUtilTest.scala | 8 +++ 7 files changed, 92 insertions(+), 9 deletions(-) diff --git a/src/main/scala/org/scalacoin/crypto/DERSignatureUtil.scala b/src/main/scala/org/scalacoin/crypto/DERSignatureUtil.scala index 621d2b41d1..ec50933bca 100644 --- a/src/main/scala/org/scalacoin/crypto/DERSignatureUtil.scala +++ b/src/main/scala/org/scalacoin/crypto/DERSignatureUtil.scala @@ -24,7 +24,9 @@ trait DERSignatureUtil extends BitcoinSLogger { */ def isDEREncoded(bytes : Seq[Byte]) : Boolean = { //signature is trivially valid if the signature is empty - if (!bytes.isEmpty) { + + if (!bytes.isEmpty && bytes.size < 9) false + else if (!bytes.isEmpty) { //first byte must be 0x30 val firstByteIs0x30 = bytes.head == 0x30 logger.debug("firstByteIs0x30: " + firstByteIs0x30) diff --git a/src/main/scala/org/scalacoin/crypto/TransactionSignatureChecker.scala b/src/main/scala/org/scalacoin/crypto/TransactionSignatureChecker.scala index b25c8ba0ec..33a9fb0e94 100644 --- a/src/main/scala/org/scalacoin/crypto/TransactionSignatureChecker.scala +++ b/src/main/scala/org/scalacoin/crypto/TransactionSignatureChecker.scala @@ -6,7 +6,7 @@ import org.scalacoin.protocol.transaction.{Transaction, TransactionInput} import org.scalacoin.script.{ScriptProgram} import org.scalacoin.script.crypto._ import org.scalacoin.script.flag.{ScriptFlagUtil, ScriptFlag, ScriptVerifyDerSig} -import org.scalacoin.util.{BitcoinSLogger, BitcoinSUtil} +import org.scalacoin.util.{BitcoinScriptUtil, BitcoinSLogger, BitcoinSUtil} import org.slf4j.LoggerFactory import scala.annotation.tailrec @@ -28,9 +28,13 @@ trait TransactionSignatureChecker extends BitcoinSLogger { */ def checkSignature(txSignatureComponent : TransactionSignatureComponent, pubKey: ECPublicKey, signature : ECDigitalSignature, flags : Seq[ScriptFlag]) : TransactionSignatureCheckerResult = { + val pubKeyEncodedCorrectly = BitcoinScriptUtil.checkPubKeyEncoding(pubKey,flags) if (ScriptFlagUtil.requiresStrictDerEncoding(flags) && !DERSignatureUtil.isStrictDEREncoding(signature)) { - logger.warn("Signature was not stricly encoded der: " + signature.hex) + logger.error("Signature was not stricly encoded der: " + signature.hex) SignatureValidationFailureNotStrictDerEncoding + } else if (!pubKeyEncodedCorrectly) { + logger.error("The public key given for signature checking was not encoded correctly") + SignatureValidationFailurePubKeyEncoding } else { //we need to check if the scriptSignature has a redeemScript //in that case, we need to pass the redeemScript to the TransactionSignatureChecker @@ -50,7 +54,6 @@ trait TransactionSignatureChecker extends BitcoinSLogger { val isValid = pubKey.verify(hashForSignature,signature) if (isValid) SignatureValidationSuccess else SignatureValidationFailureIncorrectSignatures } - } /** @@ -95,6 +98,8 @@ trait TransactionSignatureChecker extends BitcoinSLogger { SignatureValidationFailureNotStrictDerEncoding case SignatureValidationFailureSignatureCount => SignatureValidationFailureSignatureCount + case SignatureValidationFailurePubKeyEncoding => + SignatureValidationFailurePubKeyEncoding } } else if (sigs.isEmpty) { //means that we have checked all of the sigs against the public keys diff --git a/src/main/scala/org/scalacoin/crypto/TransactionSignatureCheckerResult.scala b/src/main/scala/org/scalacoin/crypto/TransactionSignatureCheckerResult.scala index 9d82c7becf..aa529d438f 100644 --- a/src/main/scala/org/scalacoin/crypto/TransactionSignatureCheckerResult.scala +++ b/src/main/scala/org/scalacoin/crypto/TransactionSignatureCheckerResult.scala @@ -46,3 +46,11 @@ case object SignatureValidationFailureIncorrectSignatures extends TransactionSig case object SignatureValidationFailureSignatureCount extends TransactionSignatureCheckerResult { def isValid = false } + +/** + * This indicates that the public key was not encoded correctly according to this function + * https://github.com/bitcoin/bitcoin/blob/master/src/script/interpreter.cpp#L202 + */ +case object SignatureValidationFailurePubKeyEncoding extends TransactionSignatureCheckerResult { + def isValid = false +} diff --git a/src/main/scala/org/scalacoin/script/crypto/CryptoInterpreter.scala b/src/main/scala/org/scalacoin/script/crypto/CryptoInterpreter.scala index a214a59685..affe7a6fe0 100644 --- a/src/main/scala/org/scalacoin/script/crypto/CryptoInterpreter.scala +++ b/src/main/scala/org/scalacoin/script/crypto/CryptoInterpreter.scala @@ -95,8 +95,6 @@ trait CryptoInterpreter extends ControlOperationsInterpreter with BitcoinSLogger ScriptProgram(program,ScriptErrorSigDer) } else { val restOfStack = program.stack.tail.tail - - val result = TransactionSignatureChecker.checkSignature(program.txSignatureComponent,pubKey, signature,program.flags) logger.debug("signature verification isValid: " + result) @@ -109,6 +107,9 @@ trait CryptoInterpreter extends ControlOperationsInterpreter with BitcoinSLogger ScriptProgram(program, ScriptFalse :: restOfStack,program.script.tail) case SignatureValidationFailureSignatureCount => ScriptProgram(program, ScriptFalse :: restOfStack,program.script.tail) + case SignatureValidationFailurePubKeyEncoding => + //means that a public key was not encoded correctly + ScriptProgram(program,ScriptErrorPubKeyType) } } } @@ -177,7 +178,7 @@ trait CryptoInterpreter extends ControlOperationsInterpreter with BitcoinSLogger (program.stack.tail.slice(0,nPossibleSignatures.num.toInt), program.stack.tail.slice(nPossibleSignatures.num.toInt,program.stack.tail.size)) - val pubKeys = pubKeysScriptTokens.map(key => ECFactory.publicKey(key.hex)) + val pubKeys = pubKeysScriptTokens.map(key => ECFactory.publicKey(key.bytes)) logger.debug("Public keys on the stack: " + pubKeys) logger.debug("mRequiredSignatures: " + mRequiredSignatures) @@ -224,6 +225,9 @@ trait CryptoInterpreter extends ControlOperationsInterpreter with BitcoinSLogger case SignatureValidationFailureSignatureCount => //means that we did not have enough signatures for OP_CHECKMULTISIG ScriptProgram(program, ScriptErrorSigCount) + case SignatureValidationFailurePubKeyEncoding => + //means that a public key was not encoded correctly + ScriptProgram(program,ScriptErrorPubKeyType) } } } diff --git a/src/main/scala/org/scalacoin/script/flag/ScriptFlagUtil.scala b/src/main/scala/org/scalacoin/script/flag/ScriptFlagUtil.scala index f283ca2ad3..1080822034 100644 --- a/src/main/scala/org/scalacoin/script/flag/ScriptFlagUtil.scala +++ b/src/main/scala/org/scalacoin/script/flag/ScriptFlagUtil.scala @@ -15,6 +15,13 @@ trait ScriptFlagUtil { flags.contains(ScriptVerifyDerSig) || flags.contains(ScriptVerifyStrictEnc) } + /** + * Checks if we are required to check for strict encoding + * @param flags + * @return + */ + def requireStrictEncoding(flags : Seq[ScriptFlag]) : Boolean = flags.contains(ScriptVerifyStrictEnc) + /** * Checks if the script flag for checklocktimeverify is enabled * @param flags diff --git a/src/main/scala/org/scalacoin/util/BitcoinScriptUtil.scala b/src/main/scala/org/scalacoin/util/BitcoinScriptUtil.scala index ade3a93ea8..ae6db78ae7 100644 --- a/src/main/scala/org/scalacoin/util/BitcoinScriptUtil.scala +++ b/src/main/scala/org/scalacoin/util/BitcoinScriptUtil.scala @@ -1,6 +1,7 @@ package org.scalacoin.util -import org.scalacoin.script.flag.ScriptFlagUtil +import org.scalacoin.crypto.ECPublicKey +import org.scalacoin.script.flag.{ScriptFlag, ScriptFlagUtil} import org.scalacoin.script.{ScriptProgram, ExecutionInProgressScriptProgram, ScriptSettings} import org.scalacoin.script.constant._ import org.scalacoin.script.crypto.{OP_CHECKMULTISIGVERIFY, OP_CHECKMULTISIG, OP_CHECKSIG, OP_CHECKSIGVERIFY} @@ -190,9 +191,57 @@ trait BitcoinScriptUtil { false } else true } else true - } + /** + * Checks the public key encoding according to bitcoin core's function + * https://github.com/bitcoin/bitcoin/blob/master/src/script/interpreter.cpp#L202 + * @param key the key whose encoding we are checking + * @param program the program whose flags which dictate the rules for the public keys encoding + * @return if the key is encoded correctly against the rules give in the flags parameter + */ + def checkPubKeyEncoding(key : ECPublicKey, program : ScriptProgram) : Boolean = checkPubKeyEncoding(key,program.flags) + + /** + * Checks the public key encoding according to bitcoin core's function + * https://github.com/bitcoin/bitcoin/blob/master/src/script/interpreter.cpp#L202 + * @param key the key whose encoding we are checking + * @param flags the flags which dictate the rules for the public keys encoding + * @return if the key is encoded correctly against the rules givein the flags parameter + */ + def checkPubKeyEncoding(key : ECPublicKey, flags : Seq[ScriptFlag]) : Boolean = { + if (ScriptFlagUtil.requireStrictEncoding(flags) && + !isCompressedOrUncompressedPubKey(key)) false else true + } + + + /** + * Returns true if the key is compressed or uncompressed, false otherwise + * https://github.com/bitcoin/bitcoin/blob/master/src/script/interpreter.cpp#L66 + * @param key the public key that is being checked + * @return true if the key is compressed/uncompressed otherwise false + */ + def isCompressedOrUncompressedPubKey(key : ECPublicKey) : Boolean = { + if (key.bytes.size < 33) { + // Non-canonical public key: too short + return false + } + if (key.bytes.head == 0x04) { + if (key.bytes.size != 65) { + // Non-canonical public key: invalid length for uncompressed key + return false + } + } else if (key.bytes.head == 0x02 || key.bytes.head == 0x03) { + if (key.bytes.size != 33) { + // Non-canonical public key: invalid length for compressed key + return false + } + } else { + // Non-canonical public key: neither compressed nor uncompressed + return false + } + return true + } } diff --git a/src/test/scala/org/scalacoin/util/BitcoinScriptUtilTest.scala b/src/test/scala/org/scalacoin/util/BitcoinScriptUtilTest.scala index 1892bbe1a1..fce372f08f 100644 --- a/src/test/scala/org/scalacoin/util/BitcoinScriptUtilTest.scala +++ b/src/test/scala/org/scalacoin/util/BitcoinScriptUtilTest.scala @@ -1,5 +1,6 @@ package org.scalacoin.util +import org.scalacoin.crypto.ECFactory import org.scalacoin.script.bitwise.OP_EQUALVERIFY import org.scalacoin.script.constant._ import org.scalacoin.script.crypto._ @@ -170,4 +171,11 @@ class BitcoinScriptUtilTest extends FlatSpec with MustMatchers { BitcoinScriptUtil.isShortestEncoding(ScriptConstantFactory.fromHex("ffff7f80")) must be (false) BitcoinScriptUtil.isShortestEncoding(ScriptConstantFactory.fromHex("ffff7f00")) must be (false) } + + it must "check a public key's encoding" in { + //pubkeys must be compressed or uncompressed or else that are not validly encoded + val key = ECFactory.publicKey("00") + val program = TestUtil.testProgram + BitcoinScriptUtil.checkPubKeyEncoding(key,program) must be (false) + } }