Merge pull request #48 from bitcoinjs/addStrict

Fix discrepancies between JS and Native implementations

Author: junderw

js.js

@@ -110,12 +110,14 @@ function pointAddScalar (p, tweak, __compressed) {
   return getEncoded(uu, compressed)
 }
 
-function pointCompress (p, compressed) {
+function pointCompress (p, __compressed) {
   if (!isPoint(p)) throw new TypeError(THROW_BAD_POINT)
 
   const pp = decodeFrom(p)
   if (pp.isInfinity()) throw new TypeError(THROW_BAD_POINT)
 
+  const compressed = assumeCompression(__compressed, p)
+
   return getEncoded(pp, compressed)
 }
 
@@ -215,7 +217,7 @@ function __sign (hash, x, addData) {
   return buffer
 }
 
-function verify (hash, q, signature) {
+function verify (hash, q, signature, strict) {
   if (!isScalar(hash)) throw new TypeError(THROW_BAD_HASH)
   if (!isPoint(q)) throw new TypeError(THROW_BAD_POINT)
 
@@ -226,6 +228,10 @@ function verify (hash, q, signature) {
   const r = fromBuffer(signature.slice(0, 32))
   const s = fromBuffer(signature.slice(32, 64))
 
+  if (strict && s.cmp(nDiv2) > 0) {
+    return false
+  }
+
   // 1.4.1 Enforce r and s are both integers in the interval [1, n − 1] (2, enforces '> 0')
   if (r.gtn(0) <= 0 /* || r.compareTo(n) >= 0 */) return false
   if (s.gtn(0) <= 0 /* || s.compareTo(n) >= 0 */) return false

native/addon.cpp

@@ -77,8 +77,9 @@ namespace {
 
 	template <size_t index, typename I, typename A>
 	unsigned int assumeCompression (const I& info, const A& p) {
-		if (info.Length() <= index) return __isPointCompressed(p) ? SECP256K1_EC_COMPRESSED : SECP256K1_EC_UNCOMPRESSED;
-		if (info[index]->IsUndefined()) return SECP256K1_EC_COMPRESSED;
+		if (info.Length() <= index || info[index]->IsUndefined()) {
+			return __isPointCompressed(p) ? SECP256K1_EC_COMPRESSED : SECP256K1_EC_UNCOMPRESSED;
+		}
 #if (NODE_MODULE_VERSION > NODE_11_0_MODULE_VERSION)
 		return info[index]->BooleanValue(v8::Isolate::GetCurrent()) ? SECP256K1_EC_COMPRESSED : SECP256K1_EC_UNCOMPRESSED;
 #else

package.json

@@ -1,6 +1,6 @@
 {
   "name": "tiny-secp256k1",
-  "version": "1.1.4",
+  "version": "1.1.5",
   "description": "A tiny secp256k1 native/JS wrapper",
   "main": "index.js",
   "gypfile": true,

tests/ecdsa.js

@@ -93,7 +93,10 @@ function test (binding) {
           binding.verify(m, Q, signature)
         }, new RegExp(f.exception), `${f.description} throws ${f.exception}`)
       } else {
-        t.equal(binding.verify(m, Q, signature), false, `verify(${f.signature}) is rejected`)
+        t.equal(binding.verify(m, Q, signature, f.strict), false, `verify(${f.signature}) is rejected`)
+        if (f.strict === true) {
+          t.equal(binding.verify(m, Q, signature, false), true, `verify(${f.signature}) is OK without strict`)
+        }
       }
     })
 

tests/fixtures/ecdsa.json

@@ -10301,6 +10301,13 @@
         "Q": "0279be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798",
         "m": "0000000000000000000000000000000000000000000000000000000000000003",
         "signature": "0000000000000000000000000000000000000000000000000000000000000001fffffffffffffffffffffffffffffffebaaedce6af48a03bbfd25e8cd0364141"
+      },
+      {
+        "description": "Fails verification if strict set to truthy value with high s value",
+        "strict": true,
+        "Q": "034cd032cdc72820498aeb0fdd25712aa4f6d263997cf7df140b8c42d44626b08c",
+        "m": "df8e1382d17bdac18f3c854815071226385803e85f06114129ebbcfcef991278",
+        "signature": "ec8cd8d7dcda4ff770c2858bdd93dd64806e218b11bb1971a4a09b065760b040c1095cf554dab161b2b2c46386fab2c81e57035be714b760ef507981f8f70073"
       }
     ]
   },

tests/fixtures/points.json

@@ -10078,6 +10078,18 @@
         "P": "04d5747c593533e620dc7b576febd13fb758525670eeb42006166fcfbaf273bb416f32cb9ea9d5403e4bdb5ca38037e169e95004324c0786c5ab5ee32050cc0213",
         "compress": false,
         "expected": "04d5747c593533e620dc7b576febd13fb758525670eeb42006166fcfbaf273bb416f32cb9ea9d5403e4bdb5ca38037e169e95004324c0786c5ab5ee32050cc0213"
+      },
+      {
+        "P": "042f01e5e15cca351daff3843fb70f3c2f0a1bdd05e5af888a67784ef3e10a2a015c4da8a741539949293d082a132d13b4c2e213d6ba5b7617b5da2cb76cbde904",
+        "noarg": true,
+        "expected": "042f01e5e15cca351daff3843fb70f3c2f0a1bdd05e5af888a67784ef3e10a2a015c4da8a741539949293d082a132d13b4c2e213d6ba5b7617b5da2cb76cbde904",
+        "note": "Need to test if pointCompress keeps compressed status without any compressed arg"
+      },
+      {
+        "P": "042f01e5e15cca351daff3843fb70f3c2f0a1bdd05e5af888a67784ef3e10a2a015c4da8a741539949293d082a132d13b4c2e213d6ba5b7617b5da2cb76cbde904",
+        "noarg": false,
+        "expected": "042f01e5e15cca351daff3843fb70f3c2f0a1bdd05e5af888a67784ef3e10a2a015c4da8a741539949293d082a132d13b4c2e213d6ba5b7617b5da2cb76cbde904",
+        "note": "Need to test if pointCompress keeps compressed status when explicitly passing undefined. (This is why this fixture has no 'compressed')"
       }
     ],
     "pointFromScalar": [

tests/points.js

@@ -85,7 +85,11 @@ function test (binding) {
       const p = Buffer.from(f.P, 'hex')
       const expected = Buffer.from(f.expected, 'hex')
 
-      t.same(binding.pointCompress(p, f.compress), expected)
+      if (f.noarg) {
+        t.same(binding.pointCompress(p), expected)
+      } else {
+        t.same(binding.pointCompress(p, f.compress), expected)
+      }
     })
 
     fpoints.invalid.pointCompress.forEach((f) => {