CVE-2022-41340: Improper Verification of Cryptographic Signature (CWE-347)
value: The secp256k1-js package before 1.1.0 for Node.js implements ECDSA without required r and s validation, leading to signature forgery.
CWE description: The product does not verify, or incorrectly verifies, the cryptographic signature for data.
CVSS v3 base score: 7.5
Published: 2022-09-24T19:15Z
Repository: https://github.com/lionello/secp256k1-js
Language: JSON
Files changed: package-lock.json, package.json, src/secp256k1.js, test/test.js
Commit date: 2022-09-23 16:35:54 -0700
Commit message: Fix #11: add parameter validation

Diff: diff --git a/package-lock.json b/package-lock.json
index 4975e82..3410569 100644
--- a/package-lock.json
+++ b/package-lock.json
@@ -1,8 +1,8 @@
 {
   "name": "@enumatech/secp256k1-js",
-  "version": "1.0.0",
+  "version": "1.0.1",
   "lockfileVersion": 1,
   "requires": true,
   "dependencies": {
     "@ungap/promise-all-settled": {
       "version": "1.1.2",
diff --git a/package.json b/package.json
index dc98aaa..3723462 100644
--- a/package.json
+++ b/package.json
@@ -1,8 +1,8 @@
 {
   "name": "@enumatech/secp256k1-js",
-  "version": "1.0.0",
+  "version": "1.0.1",
   "description": "Pure JS implementation of secp256k1 signing, verification, recovery ECDSA.",
   "engines": {
     "node": ">=14.0.0"
   },
   "main": "src/secp256k1.js",
diff --git a/src/secp256k1.js b/src/secp256k1.js
index b668e8e..5194675 100644
--- a/src/secp256k1.js
+++ b/src/secp256k1.js
@@ -68,11 +68,19 @@
         const GinJ = AtoJ(GX, GY)
         const PUBinJ = ecmul(GinJ, k)
         return JtoA(PUBinJ)
     }
 
+    function assert(cond, msg) {
+        if (!cond) {
+            throw Error("assertion failed: " + msg)
+        }
+    }
+
     function ecsign(d, z) {
+        assert(d != 0, "d must not be 0")
+        assert(z != 0, "z must not be 0")
         while (true) {
             const k = rnd(P)
             const R = mulG(k)
             if (R[0] == 0) continue
             const s = mulmod(invmod(k, N), addmod(z, mulmod(R[0], d, N), N), N)
@@ -180,10 +188,13 @@
         const p = mulG(pk)
         return {x: toHex(p[0]), y: toHex(p[1])}
     }
 
     function ecrecover(recId, sigr, sigs, message) {
+        assert(recId >= 0 && recId <= 3, "recId must be 0..3")
+        assert(sigr != 0, "sigr must not be 0")
+        assert(sigs != 0, "sigs must not be 0")
         // 1.0 For j from 0 to h   (h == recId here and the loop is outside this function)
         //   1.1 Let x = r + jn
         const x = addmod(uint256(sigr), P.muln(recId >> 1), P)
         //   1.2. Convert the integer x to an octet string X of length mlen using the conversion routine
         //        specified in Section 2.3.7, where mlen = ⌈(log2 p)/8⌉ or mlen = ⌈m/8⌉.
@@ -225,10 +236,13 @@
         const p = JtoA(qinJ)
         return {x: toHex(p[0]), y: toHex(p[1])}
     }
 
     function ecverify (Qx, Qy, sigr, sigs, z) {
+        if (sigs == 0 || sigr == 0) {
+            return false
+        }
         const w = invmod(sigs, N)
         const u1 = mulmod(z, w, N)
         const u2 = mulmod(sigr, w, N)
         const Q = AtoJ(Qx, Qy)
         const G = AtoJ(GX, GY)
diff --git a/test/test.js b/test/test.js
index 494d61c..7c8ea95 100644
--- a/test/test.js
+++ b/test/test.js
@@ -49,45 +49,62 @@ describe('secp256k1', () => {
         const sig = Secp256k1.ecsign(d, z)
         Assert.ok(/^[0-9a-f]{64}$/.test(sig.r), 'sig.r is not a hex string')
         Assert.ok(/^[0-9a-f]{64}$/.test(sig.s), 'sig.s is not a hex string')
         Assert.ok(sig.v===0 || sig.v===1, 'sig.v is not a 0 or 1')
         if (Secp256k1Node) {
-            const success = Secp256k1Node.verify(B(z), Buffer.concat([B(sig.r), B(sig.s)]), Buffer.concat([Buffer('\04'), B(pubX), B(pubY)]))
+            const success = Secp256k1Node.verify(B(z), Buffer.concat([B(sig.r), B(sig.s)]), Buffer.concat([Buffer.from('\04'), B(pubX), B(pubY)]))
             Assert.ok(success, JSON.stringify(sig))
         }
     })
 
     it('has recovery bit', () => {
         const sig = Secp256k1.ecsign(d, z)
         if (Secp256k1Node) {
-            const success = Secp256k1Node.verify(B(z), Buffer.concat([B(sig.r), B(sig.s)]), Buffer.concat([Buffer('\04'), B(pubX), B(pubY)]))
+            const success = Secp256k1Node.verify(B(z), Buffer.concat([B(sig.r), B(sig.s)]), Buffer.concat([Buffer.from('\04'), B(pubX), B(pubY)]))
             Assert.ok(success, JSON.stringify(sig))
             const Q = Secp256k1Node.recover(B(z), Buffer.concat([B(sig.r), B(sig.s)]), sig.v, false)
             Assert.deepStrictEqual({x: Q.toString('hex').substr(2,64), y: Q.toString('hex').slice(-64)}, {x: pubX.toString(16), y: pubY.toString(16)})
         }
     })
 
-    it('can verify self', () =>  {
+    it('can verify ours', () =>  {
         const sig = Secp256k1.ecsign(d, z)
         Assert.ok(Secp256k1.ecverify(pubX, pubY, Secp256k1.uint256(sig.r,16), Secp256k1.uint256(sig.s,16), z))
     })
 
+    it('can verify known sig', () =>  {
+        Assert.ok(Secp256k1.ecverify(pubX, pubY, Secp256k1.uint256(r,16), Secp256k1.uint256(s,16), z))
+    })
+
     it('can verify fff...', () =>  {
         const z = Secp256k1.uint256("ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff", 16)
         const sig = Secp256k1.ecsign(d, z)
         Assert.ok(Secp256k1.ecverify(pubX, pubY, Secp256k1.uint256(sig.r,16), Secp256k1.uint256(sig.s,16), z))
     })
 
+    it('cannot sign 000...', () =>  {
+        const z = Secp256k1.uint256("0000000000000000000000000000000000000000000000000000000000000000", 16)
+        Assert.throws(() => Secp256k1.ecsign(d, z), "assertion failed: z must not be 0")
+    })
+
     it('can verify other sig', () => {
         if (Secp256k1Node) {
             const sig = Secp256k1Node.sign(B(z), B(d))
             r = sig.signature.toString('hex').substr(0,64)
             s = sig.signature.toString('hex').slice(-64)
         }
         Assert.ok(Secp256k1.ecverify(pubX, pubY, Secp256k1.uint256(r,16), Secp256k1.uint256(s,16), z))
     })
 
+    it('verify fails if r=0', () => {
+        Assert.isFalse(Secp256k1.ecverify(pubX, pubY, Secp256k1.uint256(0), Secp256k1.uint256(s,16), z))
+    })
+
+    it('verify fails if s=0', () => {
+        Assert.isFalse(Secp256k1.ecverify(pubX, pubY, Secp256k1.uint256(r,16), Secp256k1.uint256(0), z))
+    })
+
     it('can recover other sig', () => {
         if (Secp256k1Node) {
             const sig = Secp256k1Node.sign(B(z), B(d))
             r = sig.signature.toString('hex').substr(0,64)
             s = sig.signature.toString('hex').slice(-64)
@@ -95,11 +112,32 @@ describe('secp256k1', () => {
         }
         const Q = Secp256k1.ecrecover(v, Secp256k1.uint256(r,16), Secp256k1.uint256(s,16), z)
         Assert.deepStrictEqual(Q, {x: pubX.toString(16), y: pubY.toString(16)})
     })
 
-    it('can recover self', () =>  {
+    it('can recover ours', () =>  {
         const sig = Secp256k1.ecsign(d, z)
         const Q = Secp256k1.ecrecover(sig.v, Secp256k1.uint256(sig.r,16), Secp256k1.uint256(sig.s,16), z)
         Assert.deepStrictEqual(Q, {x: pubX.toString(16), y: pubY.toString(16)})
     })
+
+    it('can recover known sig', () =>  {
+        const Q = Secp256k1.ecrecover(v, Secp256k1.uint256(r,16), Secp256k1.uint256(s,16), z)
+        Assert.deepStrictEqual(Q, {x: pubX.toString(16), y: pubY.toString(16)})
+    })
+
+    it('recover fails if r=0', () => {
+        Assert.throws(() => Secp256k1.ecrecover(v, Secp256k1.uint256(0), Secp256k1.uint256(s,16), z), "assertion failed: sigr must not be 0")
+    })
+
+    it('recover fails if s=0', () => {
+        Assert.throws(() => Secp256k1.ecrecover(v, Secp256k1.uint256(r,16), Secp256k1.uint256(0), z), "assertion failed: sigs must not be 0")
+    })
+
+    it('recover fails if recId<0', () => {
+        Assert.throws(() => Secp256k1.ecrecover(-1, Secp256k1.uint256(r,16), Secp256k1.uint256(s,16), z), "assertion failed: recId must be 0..3")
+    })
+
+    it('recover fails if recId>3', () => {
+        Assert.throws(() => Secp256k1.ecrecover(4, Secp256k1.uint256(r,16), Secp256k1.uint256(s,16), z), "assertion failed: recId must be 0..3")
+    })
 })

Vulnerable code:   "version": "1.0.0",
  "version": "1.0.0",
            const success = Secp256k1Node.verify(B(z), Buffer.concat([B(sig.r), B(sig.s)]), Buffer.concat([Buffer('\04'), B(pubX), B(pubY)]))
            const success = Secp256k1Node.verify(B(z), Buffer.concat([B(sig.r), B(sig.s)]), Buffer.concat([Buffer('\04'), B(pubX), B(pubY)]))
    it('can verify self', () =>  {
    it('can recover self', () =>  {
Fixed code:   "version": "1.0.1",
  "version": "1.0.1",
    function assert(cond, msg) {
        if (!cond) {
            throw Error("assertion failed: " + msg)
        }
    }

        assert(d != 0, "d must not be 0")
        assert(z != 0, "z must not be 0")
        assert(recId >= 0 && recId <= 3, "recId must be 0..3")
        assert(sigr != 0, "sigr must not be 0")
        assert(sigs != 0, "sigs must not be 0")
        if (sigs == 0 || sigr == 0) {
            return false
        }
            const success = Secp256k1Node.verify(B(z), Buffer.concat([B(sig.r), B(sig.s)]), Buffer.concat([Buffer.from('\04'), B(pubX), B(pubY)]))
            const success = Secp256k1Node.verify(B(z), Buffer.concat([B(sig.r), B(sig.s)]), Buffer.concat([Buffer.from('\04'), B(pubX), B(pubY)]))
    it('can verify ours', () =>  {
    it('can verify known sig', () =>  {
        Assert.ok(Secp256k1.ecverify(pubX, pubY, Secp256k1.uint256(r,16), Secp256k1.uint256(s,16), z))
    })

    it('cannot sign 000...', () =>  {
        const z = Secp256k1.uint256("0000000000000000000000000000000000000000000000000000000000000000", 16)
        Assert.throws(() => Secp256k1.ecsign(d, z), "assertion failed: z must not be 0")
    })

    it('verify fails if r=0', () => {
        Assert.isFalse(Secp256k1.ecverify(pubX, pubY, Secp256k1.uint256(0), Secp256k1.uint256(s,16), z))
    })

    it('verify fails if s=0', () => {
        Assert.isFalse(Secp256k1.ecverify(pubX, pubY, Secp256k1.uint256(r,16), Secp256k1.uint256(0), z))
    })

    it('can recover ours', () =>  {

    it('can recover known sig', () =>  {
        const Q = Secp256k1.ecrecover(v, Secp256k1.uint256(r,16), Secp256k1.uint256(s,16), z)
        Assert.deepStrictEqual(Q, {x: pubX.toString(16), y: pubY.toString(16)})
    })

    it('recover fails if r=0', () => {
        Assert.throws(() => Secp256k1.ecrecover(v, Secp256k1.uint256(0), Secp256k1.uint256(s,16), z), "assertion failed: sigr must not be 0")
    })

    it('recover fails if s=0', () => {
        Assert.throws(() => Secp256k1.ecrecover(v, Secp256k1.uint256(r,16), Secp256k1.uint256(0), z), "assertion failed: sigs must not be 0")
    })

    it('recover fails if recId<0', () => {
        Assert.throws(() => Secp256k1.ecrecover(-1, Secp256k1.uint256(r,16), Secp256k1.uint256(s,16), z), "assertion failed: recId must be 0..3")
    })

    it('recover fails if recId>3', () => {
        Assert.throws(() => Secp256k1.ecrecover(4, Secp256k1.uint256(r,16), Secp256k1.uint256(s,16), z), "assertion failed: recId must be 0..3")
    })