Implemented (lax) signature parsing in Bitcoin-S (#1446)

* Implemented parseDERLax signature parsing and bumped bouncy castle version to most recent

* implemented using iterators

* Factored out common functionality and added comments, it is readable now! Deleted C-like version in place of this one after property based tests showed them to be equivalent

* Made compatible with scala 2.12

* Make compatible with scala 2.11

* Added tests for lax DER signatures

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

@@ -2,6 +2,7 @@ package org.bitcoins.crypto
 
 import org.bitcoins.core.util.NumberUtil
 import org.bitcoins.testkit.util.BitcoinSUnitTest
+import scodec.bits.ByteVector
 
 /**
   * Created by chris on 3/23/16.
@@ -95,4 +96,64 @@ class DERSignatureUtilTest extends BitcoinSUnitTest {
       "304502203e4516da7253cf068effec6b95c41221c0cf3a8e6ccb8cbf1725b562e9afde2c022100ab1e3da73d67e32045a20e0b999e049978ea8d6ee5480d485fcf2ce0d03b2ef001".toLowerCase)
     DERSignatureUtil.isLowS(highS) must be(false)
   }
+
+  it must "parse lax DER signatures" in {
+    // Copied from https://github.com/rust-bitcoin/rust-secp256k1/blob/a1842125a77cadaa8ac7d6ca794ddb1f4852c593/src/lib.rs#L940-L956 except for the first one
+    // which is from script_tests.json and the second one which is from tx_valid.json
+    val signatures = Vector(
+      "304402200060558477337b9022e70534f1fea71a318caf836812465a2509931c5e7c4987022078ec32bd50ac9e03a349ba953dfd9fe1c8d2dd8bdb1d38ddca844d3d5c78c11801",
+      "30440220ffda47bfc776bcd269da4832626ac332adfca6dd835e8ecd83cd1ebe7d709b0e022049cffa1cdc102a0b56e0e04913606c70af702a1149dc3b305ab9439288fee09001",
+      "304402204c2dd8a9b6f8d425fcd8ee9a20ac73b619906a6367eac6cb93e70375225ec0160220356878eff111ff3663d7e6bf08947f94443845e0dcc54961664d922f7660b80c",
+      "304402202ea9d51c7173b1d96d331bd41b3d1b4e78e66148e64ed5992abd6ca66290321c0220628c47517e049b3e41509e9d71e480a0cdc766f8cdec265ef0017711c1b5336f",
+      "3045022100bf8e050c85ffa1c313108ad8c482c4849027937916374617af3f2e9a881861c9022023f65814222cab09d5ec41032ce9c72ca96a5676020736614de7b78a4e55325a",
+      "3046022100839c1fbc5304de944f697c9f4b1d01d1faeba32d751c0f7acb21ac8a0f436a72022100e89bd46bb3a5a62adc679f659b7ce876d83ee297c7a5587b2011c4fcc72eab45",
+      "3046022100eaa5f90483eb20224616775891397d47efa64c68b969db1dacb1c30acdfc50aa022100cf9903bbefb1c8000cf482b0aeeb5af19287af20bd794de11d82716f9bae3db1",
+      "3045022047d512bc85842ac463ca3b669b62666ab8672ee60725b6c06759e476cebdc6c102210083805e93bd941770109bcc797784a71db9e48913f702c56e60b1c3e2ff379a60",
+      "3044022023ee4e95151b2fbbb08a72f35babe02830d14d54bd7ed1320e4751751d1baa4802206235245254f58fd1be6ff19ca291817da76da65c2f6d81d654b5185dd86b8acf"
+    ).map(ByteVector.fromValidHex(_))
+
+    signatures.foreach { sig =>
+      DERSignatureUtil.parseDERLax(sig) match {
+        case None => fail(s"Failed to parse $sig")
+        case Some((r, s)) =>
+          if (r == BigInt(0) || s == BigInt(0)) {
+            fail(s"Failed to parse $sig")
+          } else {
+            succeed
+          }
+      }
+    }
+  }
+
+  it must "fail to parse garbage signatures" in {
+    val signatures = Vector(
+      ("4402204c2dd8a9b6f8d425fcd8ee9a20ac73b619906a6367eac6cb93e70375225ec0160220356878eff111ff3663d7e6bf08947f94443845e0dcc54961664d922f7660b80c",
+       "no leading byte"),
+      ("204402202ea9d51c7173b1d96d331bd41b3d1b4e78e66148e64ed5992abd6ca66290321c0220628c47517e049b3e41509e9d71e480a0cdc766f8cdec265ef0017711c1b5336f",
+       "bad leading byte"),
+      ("30477022100bf8e050c85ffa1c313108ad8c482c4849027937916374617af3f2e9a881861c9022023f65814222cab09d5ec41032ce9c72ca96a5676020736614de7b78a4e55325a",
+       "long length byte"),
+      ("30462100839c1fbc5304de944f697c9f4b1d01d1faeba32d751c0f7acb21ac8a0f436a72022100e89bd46bb3a5a62adc679f659b7ce876d83ee297c7a5587b2011c4fcc72eab45",
+       "no r integer tag"),
+      ("3046022100eaa5f90483eb20224616775891397d47efa64c68b969db1dacb1c30acdfc50aa2100cf9903bbefb1c8000cf482b0aeeb5af19287af20bd794de11d82716f9bae3db1",
+       "no s integer tag"),
+      ("3045022847d512bc85842ac463ca3b669b62666ab8672ee60725b6c06759e476cebdc6c102210083805e93bd941770109bcc797784a71db9e48913f702c56e60b1c3e2ff379a60",
+       "long r length"),
+      ("3044022023ee4e95151b2fbbb08a72f35babe02830d14d54bd7ed1320e4751751d1baa4802286235245254f58fd1be6ff19ca291817da76da65c2f6d81d654b5185dd86b8acf",
+       "long s length")
+    ).map { case (sig, msg) => (ByteVector.fromValidHex(sig), msg) }
+
+    signatures.foreach {
+      case (sig, msg) =>
+        DERSignatureUtil.parseDERLax(sig) match {
+          case None => succeed
+          case Some((r, s)) =>
+            if (r == BigInt(0) || s == BigInt(0)) {
+              succeed
+            } else {
+              fail(s"Successfully parsed bad signature $sig : $msg")
+            }
+        }
+    }
+  }
 }

crypto/src/main/scala/org/bitcoins/crypto/BouncyCastleUtil.scala

@@ -103,9 +103,8 @@ object BouncyCastleUtil {
                                  java.math.BigInteger.valueOf(0),
                                  java.math.BigInteger.valueOf(0))
         case _: ECDigitalSignature =>
-          val rBigInteger: BigInteger = new BigInteger(signature.r.toString())
-          val sBigInteger: BigInteger = new BigInteger(signature.s.toString())
-          signer.verifySignature(data.toArray, rBigInteger, sBigInteger)
+          val (r, s) = signature.decodeSignature
+          signer.verifySignature(data.toArray, r.bigInteger, s.bigInteger)
       }
     }
     resultTry.getOrElse(false)

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

@@ -1,6 +1,5 @@
 package org.bitcoins.crypto
 
-import org.bouncycastle.asn1.{ASN1InputStream, ASN1Integer, DLSequence}
 import scodec.bits.ByteVector
 
 import scala.util.{Failure, Success, Try}
@@ -61,39 +60,10 @@ sealed abstract class DERSignatureUtil {
     * throws an exception if the given sequence of bytes is not a DER encoded signature
     */
   def decodeSignature(bytes: ByteVector): (BigInt, BigInt) = {
-    val asn1InputStream = new ASN1InputStream(bytes.toArray)
-    //TODO: this is nasty, is there any way to get rid of all this casting???
-    //TODO: Not 100% this is completely right for signatures that are incorrectly DER encoded
-    //the behavior right now is to return the defaults in the case the signature is not DER encoded
-    //https://stackoverflow.com/questions/2409618/how-do-i-decode-a-der-encoded-string-in-java
-    val seq: DLSequence = Try(
-      asn1InputStream.readObject.asInstanceOf[DLSequence]) match {
-      case Success(seq) => seq
-      case Failure(_)   => new DLSequence()
+    DERSignatureUtil.parseDERLax(bytes) match {
+      case Some((r, s)) => (r, s)
+      case None         => (0, 0)
     }
-    val default = new ASN1Integer(0)
-    val r: ASN1Integer = Try(seq.getObjectAt(0).asInstanceOf[ASN1Integer]) match {
-      case Success(r) =>
-        //this is needed for a bug inside of bouncy castle where zero length values throw an exception
-        //we need to treat these like zero
-        Try(r.getValue) match {
-          case Success(_) => r
-          case Failure(_) => default
-        }
-      case Failure(_) => default
-    }
-    val s: ASN1Integer = Try(seq.getObjectAt(1).asInstanceOf[ASN1Integer]) match {
-      case Success(s) =>
-        //this is needed for a bug inside of bouncy castle where zero length values throw an exception
-        //we need to treat these like zero
-        Try(s.getValue) match {
-          case Success(_) => s
-          case Failure(_) => default
-        }
-      case Failure(_) => default
-    }
-    asn1InputStream.close()
-    (r.getPositiveValue, s.getPositiveValue)
   }
 
   /**
@@ -235,6 +205,108 @@ sealed abstract class DERSignatureUtil {
     require(DERSignatureUtil.isLowS(sigLowS))
     sigLowS
   }
+
+  /** Scala implementation of https://github.com/bitcoin/bitcoin/blob/master/src/pubkey.cpp#L16-L165
+    *
+    * Parses correctly as well as poorly encoded DER signatures.
+    *
+    * "Supported violations include negative integers, excessive padding, garbage
+    * at the end, and overly long length descriptors."
+    *
+    * The signatures must still follow the following general format:
+    *
+    *     0x30 | total length | 0x02 | R length | [R] \ 0x02 | S length | [S]
+    *
+    * IMPORTANT: Do not use this without further validation when validating blocks
+    * because BIP 66 requires that new signatures are verified to be strict DER encodings.
+    */
+  def parseDERLax(input: ByteVector): Option[(BigInt, BigInt)] = {
+    val iterator = input.toIterable.iterator.buffered
+
+    // Can't use iterator.nextOption because it doesn't exist in scala 2.12
+    def nextOption(): Option[Byte] = {
+      if (iterator.hasNext) {
+        Some(iterator.next())
+      } else None
+    }
+
+    def nextByteMustBe(requiredByte: Byte): Option[Unit] = {
+      nextOption().flatMap { nextByte =>
+        if (nextByte == requiredByte) {
+          Some(())
+        } else None
+      }
+    }
+
+    def moveIterForward(steps: Int): Option[ByteVector] = {
+      (0 until steps)
+        .foldLeft(Option(ByteVector.empty)) {
+          case (bytesOpt, _) =>
+            bytesOpt.flatMap { bytesSoFar =>
+              nextOption().map(bytesSoFar.:+)
+            }
+        }
+    }
+
+    def processInteger(): Option[BigInt] = {
+      for {
+        // Check next byte exists and is 0x02
+        _ <- nextByteMustBe(0x02.toByte)
+
+        // Check next byte exists and process as integer length byte
+        lengthByteUnProcessed <- nextOption()
+        length <- {
+          if ((lengthByteUnProcessed & 0x80) != 0) {
+            var lenByte = lengthByteUnProcessed - 0x80
+
+            while (lenByte > 0 && iterator.hasNext && iterator.head == 0.toByte) {
+              iterator.next()
+              lenByte -= 1
+            }
+
+            if (lenByte >= 4) {
+              None
+            } else {
+              moveIterForward(lenByte).map(_.toInt(signed = false))
+            }
+          } else {
+            Some(lengthByteUnProcessed.toInt)
+          }
+        }
+
+        numBytes <- moveIterForward(length)
+
+        // Erase leading zeroes
+        numBytesWithoutLeadingZero = numBytes.dropWhile(_ == 0.toByte)
+
+        // If length > 32, then overflow
+        num <- if (numBytesWithoutLeadingZero.length <= 32) {
+          Some(BigInt(1, numBytesWithoutLeadingZero.toArray))
+        } else None
+      } yield num
+    }
+
+    for {
+      // Check first byte exists and is 0x30
+      _ <- nextByteMustBe(0x30.toByte)
+
+      // Check second byte exists and process as length byte
+      totalLengthByteUnProcessed <- nextOption()
+      _ <- {
+        if ((totalLengthByteUnProcessed & 0x80) != 0) {
+          val processedTotalLengthByte = totalLengthByteUnProcessed - 0x80
+          moveIterForward(processedTotalLengthByte)
+        } else {
+          Some(())
+        }
+      }
+
+      r <- processInteger()
+      s <- processInteger()
+    } yield {
+      (r, s)
+    }
+  }
 }
 
 object DERSignatureUtil extends DERSignatureUtil

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

@@ -34,7 +34,8 @@ sealed abstract class ECDigitalSignature {
     * 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
     */
-  def decodeSignature: (BigInt, BigInt) = DERSignatureUtil.decodeSignature(this)
+  lazy val decodeSignature: (BigInt, BigInt) =
+    DERSignatureUtil.decodeSignature(this)
 
   /** Represents the r value found in a elliptic curve digital signature */
   def r: BigInt = {

crypto/src/main/scala/org/bitcoins/crypto/ECKey.scala

@@ -210,7 +210,6 @@ sealed abstract class ECPublicKey extends BaseECKey {
       //transactions can have weird non strict der encoded digital signatures
       //bitcoin core implements this functionality here:
       //https://github.com/bitcoin/bitcoin/blob/master/src/pubkey.cpp#L16-L165
-      //TODO: Implement functionality in Bitcoin Core linked above
       verifyWithBouncyCastle(data, signature)
     } else result
   }

project/Deps.scala

@@ -3,7 +3,7 @@ import sbt._
 object Deps {
 
   object V {
-    val bouncyCastle = "1.55"
+    val bouncyCastle = "1.65"
     val logback = "1.2.3"
     val scalacheck = "1.14.3"
     val scalaTest = "3.1.2"