k256 MPrime missing strip()

[collincusce]

Related to issue: indutny/elliptic#191 (comment)

@fanatid said he saw the same issue as the one below where strip() is missing. I'm posting it here in hopes for a minor patch with a workaround.

 FAIL  tests/apis/ava/keychain.test.ts (5.154s)
  AssetsKeyPair
    ✕ Creation Empty (403ms)

  ● AssetsKeyPair › Creation Empty

    TypeError: r.strip is not a function



      at K256.ireduce (node_modules/elliptic/node_modules/bn.js/lib/bn.js:2975:9)
      at Red.imod (node_modules/elliptic/node_modules/bn.js/lib/bn.js:3147:39)
      at Red.mul (node_modules/elliptic/node_modules/bn.js/lib/bn.js:3211:17)
      at BN.redMul (node_modules/bn.js/lib/bn.js:2984:21)
      at JPoint.eqXToP (node_modules/elliptic/lib/elliptic/curve/short.js:909:36)
      at EC.verify (node_modules/elliptic/lib/elliptic/ec/index.js:191:12)
      at AssetsKeyPair.verify (src/apis/ava/keychain.ts:2440:17)
      at Object.<anonymous> (tests/apis/ava/keychain.test.ts:15:19)

from code:

/**
     * Verifies that the private key associated with the provided public key produces the signature associated with the given message.
     * 
     * @param msg The message associated with the signature
     * @param sig The signature of the signed message
     * 
     * @returns True on success, false on failure
     */
    verify = (msg:Buffer, sig:Buffer):boolean => { 
        let sigObj:elliptic.ec.SignatureOptions = this._sigFromSigBuffer(sig);
        return ec.verify(msg, sigObj, this.keypair);
    }

    /**
     * @ignore
     */
    protected _sigFromSigBuffer = (sig:Buffer):elliptic.ec.SignatureOptions => {
        let r:BN = new BN(bintools.copyFrom(sig, 0, 32));
        let s:BN = new BN(bintools.copyFrom(sig, 32, 64));
        let recoveryParam:number = bintools.copyFrom(sig, 64, 65).readUIntBE(0, 1);
        let sigOpt = {
            r:r,
            s:s,
            recoveryParam:recoveryParam
        };
        return sigOpt;
    }

[holgerd77]

We have likely a similar problem in ethereumjs/ethereumjs-wallet#121

[holgerd77]

Update: tested a downgrade of the BN.js version from v5.0.x to v4.11.8 on our side ethereumjs/ethereumjs-wallet#122 and can confirm that this fixes the problem (which were triggered in our browser test suite in GitHub actions CI).

[collincusce]

Yeah that's our solution as well in http://github.com/ava-labs/slopes ... until this is patched, we can't upgrade BN.js.

[holgerd77]

Debugged this a bit more. This is triggered by having mixed versions of BN.js in the dependency stack, so v4 and v5 running on different libraries and coming into collision.

Couldn't completely trace this but in our case the strip() function was called in a mixed context in MPrime.prototype.ireduce here. The strip() function moved from being public (and therefore called strip(), see respective code part) in v4 to being private (renamed to _strip()) in v5. In our case a v4 num instance was passed along a v5 BN context where r._strip() was therefore failing cause the v4 instance just had a r.strip() function.

@fanatid not sure, a "fix" here would likely be to keep respectively reintroduce a r.strip() alias to r._strip(), would understand if you hesitate though. But might be the most pragmatic thing to do, otherwise this will likely cause substantial unrest here and there.

[holgerd77]

On a second thought: I would say it's worth to reintroduce r.strip(). This was I assume just a side clean-up, if this significantly breaks version interoperability it's likely not worth it.

[holgerd77]

We did a longer investigation here along this PR ethereumjs/ethereumjs-wallet#121 on the wallet library.

This is indeed an interoperability issue between BN.js v4 and v5 instances interacting in a shared context in the MPrime.prototype.ireduce() function here.

In our case this is even happening without having any production dependencies on v5. The v5 instance got in by the crypto-browserify -> browserify-sign dependency used along transpilation for the browser tests, see the following dependency tree for the BN.js versions:

ethereumjs-wallet@0.6.3 [PATH]/ethereumjs-wallet
├─┬ ethereumjs-util@7.0.1
│ ├── bn.js@4.11.8
│ └─┬ rlp@2.2.4
│   └── bn.js@4.11.8  deduped
├─┬ ethers@4.0.47
│ ├── bn.js@4.11.8  deduped
│ └─┬ elliptic@6.5.2
│   └── bn.js@4.11.8  deduped
├─┬ hdkey@1.1.2
│ └─┬ secp256k1@3.8.0
│   └── bn.js@4.11.8  deduped
└─┬ karma-typescript@5.0.2
  └─┬ crypto-browserify@3.12.0
    ├─┬ browserify-sign@4.1.0
    │ ├── bn.js@5.1.1    <--- BN v5 dependency injected here!
    │ ├─┬ browserify-rsa@4.0.1
    │ │ └── bn.js@4.11.8  deduped
    │ └─┬ parse-asn1@5.1.5
    │   └─┬ asn1.js@4.10.1
    │     └── bn.js@4.11.8  deduped
    ├─┬ create-ecdh@4.0.3
    │ └── bn.js@4.11.8  deduped
    ├─┬ diffie-hellman@5.0.3
    │ ├── bn.js@4.11.8  deduped
    │ └─┬ miller-rabin@4.0.1
    │   └── bn.js@4.11.8  deduped
    └─┬ public-encrypt@4.0.3
      └── bn.js@4.11.8  deduped

This led to a BN v5 instance created by ripemd160 (which is in our context a dependency of crypto-browserify) which was then passed as num to MPrime.prototype.ireduce = function ireduce (num) in the context of a v4 instance. Since the v4 code expects an r.strip() function to be present on a passed in BN.js instance here (which wasn't present due to a v5 instance passed in just having a r._strip() method) this broke.

This will likely lead to more problems within the community once more people upgrade to v5. The good thing is that there is an easy fix for this by adding a simple conditional check to the respective line of code:

if (r.strip !== undefined) {
   r.strip();
} else {
   r._strip();
}

This solution wouldn't affect or revert any breaking changes but just improve interoperability between the versions. Fix would need to be applied and released on both versions though, since this can actually happen in both directions (a v4 instance passed to a v5 ireduce function).

@fanatid would you be ok with this fix? Then I would open two PRs here on this. Could you create a new releases/v4.x branch from the v4.11.8 tag state so I can do the v4 patch towards that branch?

[fanatid]

Sorry that missed previous messages. I like idea of patching 4.x version, looks like this will resolve strip problem!
I created branch v4.x, will you submit PR?

[holgerd77]

No problem 👍, yes, will do directly!

[holgerd77]

Ok, have submitted, please note that this need two PRs (#247 and #248), one for a v4.x and one for a v5.x patch, since the error can occur in two ways around.

CI on #247 stopped, not sure what happened.

[fanatid]

Thanks @holgerd77!