Merge pull request #29 from Christewart/der_encoding_bug

Der encoding bug

src/main/scala/org/bitcoins/core/crypto/DERSignatureUtil.scala

@@ -13,6 +13,7 @@ trait DERSignatureUtil extends BitcoinSLogger {
   /**
    * Checks if this signature is encoded to DER correctly
    * https://crypto.stackexchange.com/questions/1795/how-can-i-convert-a-der-ecdsa-signature-to-asn-1
+   * NOTE: This will fail if this signature contains the hash type appended to the end of it
    * @return boolean representing if the signature is a valid
    */
   def isDEREncoded(signature : ECDigitalSignature) : Boolean = isDEREncoded(signature.bytes)
@@ -20,22 +21,22 @@ trait DERSignatureUtil extends BitcoinSLogger {
   /**
    * Checks if the bytes are encoded to DER correctly
    * https://crypto.stackexchange.com/questions/1795/how-can-i-convert-a-der-ecdsa-signature-to-asn-1
+   * This will fail if this signature contains the hash type appended to the end of it
    * @return boolean representing if the signature is a valid
    */
   def isDEREncoded(bytes : Seq[Byte]) : Boolean = {
     //signature is trivially valid if the signature is empty
-
-    if (!bytes.isEmpty && bytes.size < 9) false
-    else if (!bytes.isEmpty) {
+    if (bytes.nonEmpty && bytes.size < 9) false
+    else if (bytes.nonEmpty) {
       //first byte must be 0x30
       val firstByteIs0x30 = bytes.head == 0x30
       logger.debug("firstByteIs0x30: " + firstByteIs0x30)
       //second byte must indicate the length of the remaining byte array
       val signatureSize = bytes(1).toLong
       logger.debug("Encoded Sisgnature Size: " + signatureSize)
-      logger.debug("Actual Signature Size: " + bytes.slice(3,bytes.size).size)
+      logger.debug("Actual Signature Size: " + bytes.slice(2,bytes.size).size)
       //checks to see if the signature length is the same as the signatureSize val
-      val signatureLengthIsCorrect = signatureSize == bytes.slice(3,bytes.size).size
+      val signatureLengthIsCorrect = signatureSize == bytes.slice(2,bytes.size).size
       logger.debug("Signature length is correct: " + signatureLengthIsCorrect)
       //third byte must be 0x02
       val thirdByteIs0x02 = bytes(2) == 0x02
@@ -114,14 +115,15 @@ trait DERSignatureUtil extends BitcoinSLogger {
 
   /**
    * This functions implements the strict der encoding rules that were created in BIP66
-   * https://github.com/bitcoin/bips/blob/master/bip-0066.mediawiki
+   * [[https://github.com/bitcoin/bips/blob/master/bip-0066.mediawiki]]
+   * [[https://github.com/bitcoin/bitcoin/blob/master/src/script/interpreter.cpp#L98]]
    * @param signature the signature to check if they are strictly der encoded
    * @return boolean indicating whether the signature was der encoded or not
    */
-  def isStrictDEREncoding(signature : ECDigitalSignature) : Boolean = {
+  def isValidSignatureEncoding(signature : ECDigitalSignature) : Boolean = {
     signature match {
       case EmptyDigitalSignature => true
-      case signature : ECDigitalSignature => isStrictDEREncoding(signature.bytes)
+      case signature : ECDigitalSignature => isValidSignatureEncoding(signature.bytes)
     }
   }
 
@@ -129,10 +131,11 @@ trait DERSignatureUtil extends BitcoinSLogger {
   /**
    * This functions implements the strict der encoding rules that were created in BIP66
    * https://github.com/bitcoin/bips/blob/master/bip-0066.mediawiki
+   * [[https://github.com/bitcoin/bitcoin/blob/master/src/script/interpreter.cpp#L98]]
    * @param bytes the bytes to check if they are strictly der encoded
    * @return boolean indicating whether the bytes were der encoded or not
    */
-  def isStrictDEREncoding(bytes : Seq[Byte]) : Boolean = {
+  def isValidSignatureEncoding(bytes : Seq[Byte]) : Boolean = {
     // Format: 0x30 [total-length] 0x02 [R-length] [R] 0x02 [S-length] [S] [sighash]
     // * total-length: 1-byte length descriptor of everything that follows,
     //   excluding the sighash byte.
@@ -150,9 +153,9 @@ trait DERSignatureUtil extends BitcoinSLogger {
     if (bytes.size == 0) return true
 
     //check if the bytes are ATLEAST der encoded
-    val isDerEncoded = isDEREncoded(bytes)
+/*    val isDerEncoded = isDEREncoded(bytes)
     if (!isDerEncoded) return false
-    logger.debug("Signature is at minimum DER encoded")
+    logger.debug("Signature is at minimum DER encoded")*/
 
     if (bytes.size < 9) return false
     logger.debug("signature is the minimum size for strict der encoding")
@@ -222,26 +225,34 @@ trait DERSignatureUtil extends BitcoinSLogger {
   /**
    * Requires the S value in signatures to be the low version of the S value
    * https://github.com/bitcoin/bips/blob/master/bip-0062.mediawiki#low-s-values-in-signatures
- *
    * @param signature
    * @return if the S value is the low version
    */
-  def isLowDerSignature(signature : ECDigitalSignature) : Boolean = isLowDerSignature(signature.bytes)
+  def isLowS(signature : ECDigitalSignature) : Boolean = isLowS(signature.bytes)
 
   /**
    * Requires the S value in signatures to be the low version of the S value
    * https://github.com/bitcoin/bips/blob/master/bip-0062.mediawiki#low-s-values-in-signatures
- *
    * @param signature
    * @return if the S value is the low version
    */
-  def isLowDerSignature(signature : Seq[Byte]) : Boolean = {
-    if (!isDEREncoded(signature)) false
-    else {
+  def isLowS(signature : Seq[Byte]) : Boolean = {
+    val result = Try {
       val (r,s) = decodeSignature(signature)
-      val upperBound = BigInt("7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF5D576E7357A4501DDFE92F46681B20A0",16)
-      s >= 0x1 && s <= upperBound
+      s.bigInteger.compareTo(CryptoParams.halfCurveOrder) <= 0
     }
+    result match {
+      case Success(bool) => bool
+      case Failure(_) => false
+    }
+  }
+
+  /** Checks if the given digital signature uses a low s value, if it does not it converts it to a low s value and returns it */
+  def lowS(signature: ECDigitalSignature): ECDigitalSignature = {
+    val sigLowS = if (isLowS(signature)) signature
+    else ECDigitalSignature(signature.r,CryptoParams.curve.getN().subtract(signature.s.bigInteger))
+    require(DERSignatureUtil.isLowS(sigLowS))
+    sigLowS
   }
 }
 

src/main/scala/org/bitcoins/core/crypto/ECDigitalSignature.scala

@@ -17,17 +17,21 @@ sealed trait ECDigitalSignature extends BitcoinSLogger {
   /**
    * Checks if this signature is encoded to DER correctly
    * https://crypto.stackexchange.com/questions/1795/how-can-i-convert-a-der-ecdsa-signature-to-asn-1
- *
-   * @return boolean representing if the signature is a valid
+    *
+    * @return boolean representing if the signature is a valid
    */
   def isDEREncoded : Boolean = DERSignatureUtil.isDEREncoded(this)
 
 
+  /** Checks if the signature is strictly der encoded as per BIP66
+    * [[https://github.com/bitcoin/bips/blob/master/bip-0066.mediawiki]]
+    * */
+  def isStrictEncoded: Boolean = DERSignatureUtil.isValidSignatureEncoding(this)
   /**
    * Decodes the digital signature into it's r and s points
    * throws an exception if the given sequence of bytes is not a DER encoded signature
- *
-   * @return the (r,s) values for the elliptic curve digital signature
+    *
+    * @return the (r,s) values for the elliptic curve digital signature
    */
   def decodeSignature : (BigInt,BigInt) = DERSignatureUtil.decodeSignature(this)
 
@@ -40,8 +44,8 @@ sealed trait ECDigitalSignature extends BitcoinSLogger {
 
   /**
    * Represents the s value found in a elliptic curve digital signature
- *
-   * @return
+    *
+    * @return
    */
   def s = decodeSignature._2
 
@@ -61,7 +65,13 @@ object ECDigitalSignature extends Factory[ECDigitalSignature] {
     //this represents the empty signature
     if (bytes.size == 1 && bytes.head == 0x0) EmptyDigitalSignature
     else if (bytes.size == 0) EmptyDigitalSignature
-    else ECDigitalSignatureImpl(bytes)
+    else {
+      //make sure the signature follows BIP62's low-s value
+      //https://github.com/bitcoin/bips/blob/master/bip-0062.mediawiki#Low_S_values_in_signatures
+      //bitcoinj implementation
+      //https://github.com/bitcoinj/bitcoinj/blob/1e66b9a8e38d9ad425507bf5f34d64c5d3d23bb8/core/src/main/java/org/bitcoinj/core/ECKey.java#L551
+      ECDigitalSignatureImpl(bytes)
+    }
   }
 
   def apply(r : BigInt, s : BigInt) = fromRS(r,s)
@@ -69,6 +79,7 @@ object ECDigitalSignature extends Factory[ECDigitalSignature] {
     * Takes in the r and s component of a digital signature and gives back a ECDigitalSignature object
     * The ECDigitalSignature object complies with strict der encoding as per BIP62
     * note: That the hash type for the signature CANNOT be added to the digital signature
+    *
     * @param r the r component of the digital signature
     * @param s the s component of the digital signature
     * @return
@@ -80,4 +91,12 @@ object ECDigitalSignature extends Factory[ECDigitalSignature] {
       r.toByteArray.toSeq ++ Seq(0x2.toByte, s.toByteArray.size.toByte) ++ s.toByteArray.toSeq
     fromBytes(bytes)
   }
+
+
+  /** Checks if the given digital signature uses a low s value,
+    * if it does not it converts it to a low s value and returns it */
+  private def lowS(signature: ECDigitalSignature): ECDigitalSignature = {
+    if (signature.s.bigInteger.compareTo(CryptoParams.halfCurveOrder) <= 0) signature
+    else ECDigitalSignature(signature.r,CryptoParams.curve.getN().subtract(signature.s.bigInteger))
+  }
 }

src/main/scala/org/bitcoins/core/crypto/TransactionSignatureChecker.scala

@@ -36,10 +36,10 @@ trait TransactionSignatureChecker extends BitcoinSLogger {
                      pubKey: ECPublicKey, signature : ECDigitalSignature, flags : Seq[ScriptFlag]) : TransactionSignatureCheckerResult = {
     logger.info("Signature: " + signature)
     val pubKeyEncodedCorrectly = BitcoinScriptUtil.checkPubKeyEncoding(pubKey,flags)
-    if (ScriptFlagUtil.requiresStrictDerEncoding(flags) && !DERSignatureUtil.isStrictDEREncoding(signature)) {
+    if (ScriptFlagUtil.requiresStrictDerEncoding(flags) && !DERSignatureUtil.isValidSignatureEncoding(signature)) {
       logger.error("Signature was not stricly encoded der: " + signature.hex)
       SignatureValidationFailureNotStrictDerEncoding
-    } else if (ScriptFlagUtil.requireLowSValue(flags) && !DERSignatureUtil.isLowDerSignature(signature)) {
+    } else if (ScriptFlagUtil.requireLowSValue(flags) && !DERSignatureUtil.isLowS(signature)) {
       logger.error("Signature did not have a low s value")
       ScriptValidationFailureHighSValue
     } else if (ScriptFlagUtil.requireStrictEncoding(flags) && signature.bytes.nonEmpty &&

src/main/scala/org/bitcoins/core/crypto/TransactionSignatureCreator.scala

@@ -9,7 +9,6 @@ trait TransactionSignatureCreator {
 
   /**
     * Creates a signature from a tx signature component
- *
     * @param txSignatureComponent contains the tx, inputIndex which specify which input we are creating a sig for
     * @param privateKey the private key which we are signing the hash with
     * @param hashType the procedure to use for hashing to transaction
@@ -19,7 +18,9 @@ trait TransactionSignatureCreator {
     val hash = TransactionSignatureSerializer.hashForSignature(txSignatureComponent, hashType)
     val signature = privateKey.sign(hash)
     //append 1 byte hash type onto the end
-    ECDigitalSignature(signature.bytes ++ Seq(hashType.byte))
+    val sig = ECDigitalSignature(signature.bytes ++ Seq(hashType.byte))
+    require(sig.isStrictEncoded, "We did not create a signature that is strictly encoded, got: " + sig)
+    sig
   }
 }
 

src/main/scala/org/bitcoins/core/script/crypto/CryptoInterpreter.scala

@@ -93,7 +93,7 @@ trait CryptoInterpreter extends ControlOperationsInterpreter with BitcoinSLogger
           val signature = ECDigitalSignature(executionInProgressScriptProgram.stack.tail.head.bytes)
 
           if (ScriptFlagUtil.requiresStrictDerEncoding(executionInProgressScriptProgram.flags) &&
-            !DERSignatureUtil.isStrictDEREncoding(signature)) {
+            !DERSignatureUtil.isValidSignatureEncoding(signature)) {
             //this means all of the signatures must encoded according to BIP66 strict dersig
             //https://github.com/bitcoin/bips/blob/master/bip-0066.mediawiki
             //script verification fails since the sig is not strictly der encoded

src/main/scala/org/bitcoins/core/serializers/RawBitcoinSerializer.scala

@@ -1,12 +1,12 @@
 package org.bitcoins.core.serializers
 
-import org.bitcoins.core.util.BitcoinSUtil
+import org.bitcoins.core.util.{BitcoinSLogger, BitcoinSUtil}
 
 /**
  * Created by chris on 1/11/16.
  * A common trait for reading/writing bitcoin objects to/from bytes/hex
  */
-trait RawBitcoinSerializer[T] extends RawBitcoinSerializerHelper {
+trait RawBitcoinSerializer[T] extends RawBitcoinSerializerHelper with BitcoinSLogger {
 
   /**
    * Reads a hexadecimal value and transforms it into the native

src/test/scala/org/bitcoins/core/crypto/DERSignatureUtilTest.scala

@@ -11,17 +11,20 @@ class DERSignatureUtilTest extends FlatSpec with MustMatchers {
   val p2shSignature = ECDigitalSignature("304402205b7d2c2f177ae76cfbbf14d589c113b0b35db753d305d5562dd0b61cbf366cfb02202e56f93c4f08a27f986cd424ffc48a462c3202c4902104d4d0ff98ed28f4bf8001")
   val p2pkhSignature = ECDigitalSignature("3044022016ffdbb7c57634903c5e018fcfc48d59f4e37dc4bc3bbc9ba4e6ee39150bca030220119c2241a931819bc1a75d3596e4029d803d1cd6de123bf8a1a1a2c3665e1fac01")
   val p2pkSignature = ECDigitalSignature("304402200a5c6163f07b8d3b013c4d1d6dba25e780b39658d79ba37af7057a3b7f15ffa102201fd9b4eaa9943f734928b99a83592c2e7bf342ea2680f6a2bb705167966b742001")
+
   "DERSignatureUtil" must "say that a signature taken from a p2sh transaction is a valid DER encoded signature" in  {
-    DERSignatureUtil.isDEREncoded(p2shSignature) must be (true)
+    DERSignatureUtil.isValidSignatureEncoding(p2shSignature) must be (true)
   }
 
   it must "say that signature taken from a p2pkh transaction is a valid DER encoded signature" in  {
-    DERSignatureUtil.isDEREncoded(p2pkhSignature) must be (true)
+    //need to remove the hash type byte to check for der encoding
+    val hashTypeByteRemoved = p2pkhSignature.bytes.slice(0,p2pkhSignature.bytes.size-1)
+    DERSignatureUtil.isDEREncoded(hashTypeByteRemoved) must be (true)
   }
 
   it must "say that a signature taken from a p2pk transaction is a valid DER encoded signature" in {
-
-    DERSignatureUtil.isDEREncoded(p2pkSignature) must be (true)
+    val hashTypeByteRemoved = p2pkSignature.bytes.slice(0,p2pkSignature.bytes.size-1)
+    DERSignatureUtil.isDEREncoded(hashTypeByteRemoved) must be (true)
   }
 
   it must "retrieve the (r,s) values for a p2sh signature in bitcoin" in {
@@ -43,32 +46,32 @@ class DERSignatureUtilTest extends FlatSpec with MustMatchers {
   }
 
   it must "say that a signature taken from a p2sh transaction is a valid stirctly DER encoded signature" in {
-    DERSignatureUtil.isStrictDEREncoding(p2shSignature) must be (true)
+    DERSignatureUtil.isValidSignatureEncoding(p2shSignature) must be (true)
   }
 
   it must "say that signature taken from a p2pkh transaction is a valid strictly DER encoded signature" in  {
-    DERSignatureUtil.isStrictDEREncoding(p2pkhSignature) must be (true)
+    DERSignatureUtil.isValidSignatureEncoding(p2pkhSignature) must be (true)
   }
 
   it must "say that a signature taken from a p2pk transaction is a valid strictly DER encoded signature" in {
 
-    DERSignatureUtil.isStrictDEREncoding(p2pkSignature) must be (true)
+    DERSignatureUtil.isValidSignatureEncoding(p2pkSignature) must be (true)
   }
 
   it must "say that the empty signature is a valid strictly encoded DER signature" in {
-    DERSignatureUtil.isStrictDEREncoding(ECDigitalSignature("")) must be (true)
-    DERSignatureUtil.isStrictDEREncoding(EmptyDigitalSignature) must be (true)
+    DERSignatureUtil.isValidSignatureEncoding(ECDigitalSignature("")) must be (true)
+    DERSignatureUtil.isValidSignatureEncoding(EmptyDigitalSignature) must be (true)
   }
 
 
   it must "say that an overly long signature is NOT strict der encoded" in {
     val sig = ECDigitalSignature("0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000")
-    DERSignatureUtil.isStrictDEREncoding(sig) must be (false)
+    DERSignatureUtil.isValidSignatureEncoding(sig) must be (false)
   }
 
   it must "determine if a signature is encoded with a low s value" in {
     val highS = ECDigitalSignature("304502203e4516da7253cf068effec6b95c41221c0cf3a8e6ccb8cbf1725b562e9afde2c022100ab1e3da73d67e32045a20e0b999e049978ea8d6ee5480d485fcf2ce0d03b2ef001".toLowerCase)
-    DERSignatureUtil.isLowDerSignature(highS) must be (false)
+    DERSignatureUtil.isLowS(highS) must be (false)
   }
 }
 

src/test/scala/org/bitcoins/core/crypto/ECDigitalSignatureTest.scala

@@ -15,17 +15,17 @@ class ECDigitalSignatureTest extends FlatSpec with MustMatchers {
   }
 
   it must "say that a signature taken from a p2sh transaction is a valid DER encoded signature" in  {
-    val signature = ECDigitalSignature("304402205b7d2c2f177ae76cfbbf14d589c113b0b35db753d305d5562dd0b61cbf366cfb02202e56f93c4f08a27f986cd424ffc48a462c3202c4902104d4d0ff98ed28f4bf8001")
+    val signature = ECDigitalSignature("304402205b7d2c2f177ae76cfbbf14d589c113b0b35db753d305d5562dd0b61cbf366cfb02202e56f93c4f08a27f986cd424ffc48a462c3202c4902104d4d0ff98ed28f4bf80")
     signature.isDEREncoded must be (true)
   }
 
   it must "say that signature taken from a p2pkh transaction is a valid DER encoded signature" in  {
-    val signature = ECDigitalSignature("3044022016ffdbb7c57634903c5e018fcfc48d59f4e37dc4bc3bbc9ba4e6ee39150bca030220119c2241a931819bc1a75d3596e4029d803d1cd6de123bf8a1a1a2c3665e1fac01")
+    val signature = ECDigitalSignature("3044022016ffdbb7c57634903c5e018fcfc48d59f4e37dc4bc3bbc9ba4e6ee39150bca030220119c2241a931819bc1a75d3596e4029d803d1cd6de123bf8a1a1a2c3665e1fac")
     signature.isDEREncoded must be (true)
   }
 
   it must "say that a signature taken from a p2pk transaction is a valid DER encoded signature" in {
-    val signature = ECDigitalSignature("304402200a5c6163f07b8d3b013c4d1d6dba25e780b39658d79ba37af7057a3b7f15ffa102201fd9b4eaa9943f734928b99a83592c2e7bf342ea2680f6a2bb705167966b742001")
+    val signature = ECDigitalSignature("304402200a5c6163f07b8d3b013c4d1d6dba25e780b39658d79ba37af7057a3b7f15ffa102201fd9b4eaa9943f734928b99a83592c2e7bf342ea2680f6a2bb705167966b7420")
     signature.isDEREncoded must be (true)
   }
 

src/test/scala/org/bitcoins/core/crypto/TransactionSignatureCreatorTest.scala

@@ -46,6 +46,7 @@ class TransactionSignatureCreatorTest extends FlatSpec with MustMatchers with Bi
     txSignature.s must be (expectedSig.s)
     txSignature.hex must be (expectedSig.hex)
   }
+
   it must "create a p2pk scriptPubKey, create a crediting tx for scriptPubKey, " +
     "then create spending tx and make sure it evaluates to true in the interpreter" in {
     val privateKey = ECPrivateKey()