From 53595784e129632dbf4d9e680691837edef09490 Mon Sep 17 00:00:00 2001
From: Daniel Cousens <github@dcousens.com>
Date: Fri, 20 Jun 2014 15:25:23 +1000
Subject: [PATCH] ECSignature: fixes for canonical signatures

---
 src/ecsignature.js             | 33 +++++++++++++++++++++++++++------
 test/bitcoin.core.js           | 29 +++++++++++++++++++++++++++++
 test/fixtures/ecsignature.json | 30 +++++++++++++++++++++++-------
 3 files changed, 79 insertions(+), 13 deletions(-)

diff --git a/src/ecsignature.js b/src/ecsignature.js
index 9bd345f..e64cb02 100644
--- a/src/ecsignature.js
+++ b/src/ecsignature.js
@@ -1,5 +1,4 @@
 var assert = require('assert')
-
 var BigInteger = require('bigi')
 
 function ECSignature(r, s) {
@@ -34,28 +33,50 @@ ECSignature.parseCompact = function(buffer) {
 ECSignature.fromDER = function(buffer) {
   assert.equal(buffer.readUInt8(0), 0x30, 'Not a DER sequence')
   assert.equal(buffer.readUInt8(1), buffer.length - 2, 'Invalid sequence length')
-
   assert.equal(buffer.readUInt8(2), 0x02, 'Expected a DER integer')
+
   var rLen = buffer.readUInt8(3)
-  var rB = buffer.slice(4, 4 + rLen)
+  assert(rLen > 0, 'R length is zero')
 
   var offset = 4 + rLen
   assert.equal(buffer.readUInt8(offset), 0x02, 'Expected a DER integer (2)')
-  var sLen = buffer.readUInt8(1 + offset)
-  var sB = buffer.slice(2 + offset)
+
+  var sLen = buffer.readUInt8(offset + 1)
+  assert(sLen > 0, 'R length is zero')
+
+  var rB = buffer.slice(4, offset)
+  var sB = buffer.slice(offset + 2)
   offset += 2 + sLen
 
+  if (rLen > 1 && rB.readUInt8(0) === 0x00) {
+    assert(rB.readUInt8(1) & 0x80, 'R value excessively padded')
+  }
+
+  if (sLen > 1 && sB.readUInt8(0) === 0x00) {
+    assert(sB.readUInt8(1) & 0x80, 'S value excessively padded')
+  }
+
   assert.equal(offset, buffer.length, 'Invalid DER encoding')
   var r = BigInteger.fromDERInteger(rB)
   var s = BigInteger.fromDERInteger(sB)
 
+  assert(r.signum() >= 0, 'R value is negative')
+  assert(s.signum() >= 0, 'S value is negative')
+
   return new ECSignature(r, s)
 }
 
+// FIXME: 0x00, 0x04, 0x80 are SIGHASH_* boundary constants, importing Transaction causes a circular dependency
 ECSignature.parseScriptSignature = function(buffer) {
+  var hashType = buffer.readUInt8(buffer.length - 1)
+  var hashTypeMod = hashType & ~0x80
+
+  assert(hashTypeMod > 0x00, 'Invalid hashType')
+  assert(hashTypeMod < 0x04, 'Invalid hashType')
+
   return {
     signature: ECSignature.fromDER(buffer.slice(0, -1)),
-    hashType: buffer.readUInt8(buffer.length - 1)
+    hashType: hashType
   }
 }
 
diff --git a/test/bitcoin.core.js b/test/bitcoin.core.js
index b4f393b..94e1ff3 100644
--- a/test/bitcoin.core.js
+++ b/test/bitcoin.core.js
@@ -6,6 +6,7 @@ var networks = require('../src/networks')
 var Address = require('../src/address')
 var BigInteger = require('bigi')
 var ECKey = require('../src/eckey')
+var ECSignature = require('../src/ecsignature')
 var Transaction = require('../src/transaction')
 var Script = require('../src/script')
 
@@ -197,4 +198,32 @@ describe('Bitcoin-core', function() {
       })
     })
   })
+
+  describe('ECSignature', function() {
+    sig_canonical.forEach(function(hex) {
+      var buffer = new Buffer(hex, 'hex')
+
+      it('can parse ' + hex, function() {
+        var parsed = ECSignature.parseScriptSignature(buffer)
+        var actual = parsed.signature.toScriptSignature(parsed.hashType)
+        assert.equal(actual.toString('hex'), hex)
+      })
+    })
+
+    sig_noncanonical.forEach(function(hex, i) {
+      if (i === 0) return
+      if (i % 2 !== 0) return
+
+      var description = sig_noncanonical[i - 1].slice(0, -1)
+      if (description === 'too long') return // we support non secp256k1 signatures
+
+      var buffer = new Buffer(hex, 'hex')
+
+      it('throws on ' + description, function() {
+        assert.throws(function() {
+          ECSignature.parseScriptSignature(buffer)
+        })
+      })
+    })
+  })
 })
diff --git a/test/fixtures/ecsignature.json b/test/fixtures/ecsignature.json
index 814fea6..ae00f30 100644
--- a/test/fixtures/ecsignature.json
+++ b/test/fixtures/ecsignature.json
@@ -103,8 +103,8 @@
         "i": 1
       },
       "scriptSignature": {
-        "hex": "3045022100cde1302d83f8dd835d89aef803c74a119f561fbaef3eb9129e45f30de86abbf9022006ce643f5049ee1f27890467b77a6a8e11ec4661cc38cd8badf90115fbd03cefff",
-        "hashType": 255
+        "hex": "3045022100cde1302d83f8dd835d89aef803c74a119f561fbaef3eb9129e45f30de86abbf9022006ce643f5049ee1f27890467b77a6a8e11ec4661cc38cd8badf90115fbd03cef81",
+        "hashType": 129
       },
       "DER": "3045022100cde1302d83f8dd835d89aef803c74a119f561fbaef3eb9129e45f30de86abbf9022006ce643f5049ee1f27890467b77a6a8e11ec4661cc38cd8badf90115fbd03cef",
       "signature": {
@@ -131,23 +131,39 @@
     "DER": [
       {
         "exception": "Invalid sequence length",
-        "hex": "30ff0204ffffffff0204ffffffff"
+        "hex": "30ff020400ffffff020400ffffff"
       },
       {
         "exception": "Invalid sequence length",
-        "hex": "300c0304ffffffff0304ffffffff0000"
+        "hex": "300c030400ffffff030400ffffff0000"
       },
       {
         "exception": "Expected a DER integer",
-        "hex": "300cff04ffffffff0204ffffffff"
+        "hex": "300cff0400ffffff020400ffffff"
       },
       {
         "exception": "Expected a DER integer \\(2\\)",
-        "hex": "300c0202ffffffff0204ffffffff"
+        "hex": "300c020200ffffff020400ffffff"
       },
       {
         "exception": "Invalid DER encoding",
-        "hex": "300c0204ffffffff0202ffffffff"
+        "hex": "300c020400ffffff020200ffffff"
+      },
+      {
+        "exception": "R value is negative",
+        "hex": "300c0204ffffffff020400ffffff"
+      },
+      {
+        "exception": "S value is negative",
+        "hex": "300c020400ffffff0204ffffffff"
+      },
+      {
+        "exception": "R value excessively padded",
+        "hex": "300c02040000ffff020400ffffff"
+      },
+      {
+        "exception": "S value excessively padded",
+        "hex": "300c020400ffffff02040000ffff"
       }
     ]
   }