Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug: Some invalid signatures cause .verify to throw instead of returning false #20

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

voltrevo
Copy link
Contributor

This PR adds a failing test showing an example of the bug.

➜  hubble-bls git:(invalid-sig-bug) ✗ npm test

> @thehubbleproject/[email protected] test
> mocha -r ts-node/register test/**/*.test.ts



  Hash to Field
    ✓ expand message
    ✓ hash to point

  BLS raw API
    ✓ parse g1
    ✓ parse g2
    ✓ load and dumps Fr
    ✓ load and dumps G1
    ✓ load and dumps G2
    ✓ verify single signature
    ✓ verify aggregated signature

  BLS Signer
    ✓ verify single signature (78ms)
    ✓ verify aggregated signature (84ms)
    1) rejects invalid signature


  11 passing (396ms)
  1 failing

  1) BLS Signer
       rejects invalid signature:
     Error: err _wrapInput 1 0x059647ae964ea7a386767ab1e41aa47652265bf031ab9d05b70bf5ad1a770a3c 0x0d46b38b1f72d7df1f88786272f0b3fea1c79e1c8515a165fe19939e031ced9e
      at /Users/andrew/workspaces/ethf/hubble-bls/node_modules/mcl-wasm/src/mcl.js:145:22
      at exports.G1._setter (node_modules/mcl-wasm/src/mcl.js:274:19)
      at exports.G1.setStr (node_modules/mcl-wasm/src/mcl.js:484:14)
      at Object.parseG1 (src/mcl.ts:219:8)
      at BlsVerifier.verify (src/signer.ts:59:29)
      at Context.<anonymous> (test/signer.test.ts:68:33)
      at processImmediate (node:internal/timers:464:21)

@voltrevo voltrevo changed the title Bug: .verify sometimes throws instead of returning false on invalid signatures Bug: Some invalid signatures cause .verify to throw instead of returning false Oct 22, 2021
@ChihChengLiang
Copy link
Collaborator

Sorry, haven't got the bandwidth to investigate this.


// Change first hex digit of signature from 1 to 0 (to make it invalid)
const badSignature = signature.slice() as solG1;
badSignature[0] = `0x0${signature[0].slice(3)}`;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@voltrevo mcl-wasm will throw the err _wrapInput with malformed hex values, or values that are out of the supported range. See:

I ran into this same issue and used an openssl generated hex to resolve, see https://github.com/jzaki/bls-wallet/pull/92/files#r787376021 . I am unfortunately not cryptographically experienced enough to provide a more robust explanation.

Two ways I can think of that we can address:

  • Add more validation logic to values passed into this lib to throw more meaningful errors.
  • Add validation logic in consuming code (such as in bls-wallet-clients) to handle it a layer up.

Thoughts?

@voltrevo
Copy link
Contributor Author

Sorry, haven't got the bandwidth to investigate this.

👍😞

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants