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

Update ECDSA signer with bb signature once fixed #913

Closed
PhilWindle opened this issue Jun 26, 2023 · 3 comments · Fixed by #954 or #1345
Closed

Update ECDSA signer with bb signature once fixed #913

PhilWindle opened this issue Jun 26, 2023 · 3 comments · Fixed by #954 or #1345
Assignees

Comments

@PhilWindle
Copy link
Collaborator

No description provided.

@PhilWindle PhilWindle added this to A3 Jun 26, 2023
@PhilWindle PhilWindle converted this from a draft issue Jun 26, 2023
@PhilWindle
Copy link
Collaborator Author

PhilWindle commented Jun 26, 2023

The signing code for ecdsa signatures uses a third party package as the equivalent in bb does not currently work . Once bb is fixed this should be changed.

This code is in yarn-project/circuit.js/src/barretenberg/crypto/ecdsa/index.ts.

  /**
   * Constructs an ECDSA signature given a msg and a private key.
   * @param msg - Message over which the signature is constructed.
   * @param privateKey - The secp256k1 private key of the signer.
   * @returns An ECDSA signature of the form (r, s, v).
   */
  public constructSignature(msg: Uint8Array, privateKey: Uint8Array) {
    const mem = this.wasm.call('bbmalloc', msg.length);
    this.wasm.writeMemory(0, privateKey);
    this.wasm.writeMemory(mem, msg);
    this.wasm.call('ecdsa__construct_signature', mem, msg.length, 0, 32, 64, 96);

    // TODO: Understand why this doesn't work
    // const sig = new EcdsaSignature(
    //   Buffer.from(this.wasm.getMemorySlice(32, 64)),
    //   Buffer.from(this.wasm.getMemorySlice(64, 96)),
    //   Buffer.from(this.wasm.getMemorySlice(96, 97)),
    // );

    const signature = secp256k1.sign(msg, privateKey);
    return new EcdsaSignature(
      toBufferBE(signature.r, 32),
      toBufferBE(signature.s, 32),
      numToUInt32BE(signature.recovery!).subarray(3, 4),
    );
  }

@benesjan benesjan self-assigned this Jul 4, 2023
@benesjan benesjan moved this from Todo to In Progress in A3 Jul 4, 2023
@benesjan benesjan mentioned this issue Jul 4, 2023
6 tasks
@spalladino
Copy link
Collaborator

spalladino commented Jul 11, 2023

The issue we ran into with touchid verification was that Noir expected signatures to be just on one side of the curve, to prevent malleability attacks. So we had to manually invert the signature before passing it. This was the code snippet, which should work the same here as long as you change the prime to the one from the secp256k1 curve. Assuming this is the issue, of course.

// Your scalar value as a BigInt
let scalar = BigInt('Your_Scalar_Value_Here');

// The order 'n' of secp256r1 as a BigInt
let n = BigInt('115792089210356248762697446949407573529996955224135760342422259061068512044369');

// Compute (n - scalar) mod n
let negated = (n - scalar) % n;

console.log(negated.toString());

Here's more info about the malleability issue.

@github-project-automation github-project-automation bot moved this from In Progress to Done in A3 Jul 13, 2023
@spalladino
Copy link
Collaborator

Reopening issue since barretenberg ECDSA is still not verifying in Noir. Reason why #954 passed is because the ECDSA account contract tests were temporarily commented out, and when reenabling them in #1080 signature failed.

@spalladino spalladino reopened this Jul 14, 2023
@iAmMichaelConnor iAmMichaelConnor moved this from Done to In Progress in A3 Jul 31, 2023
suyash67 added a commit that referenced this issue Aug 2, 2023
# Description

resolves #913 

Detailed hackmd: https://hackmd.io/9QRzytElQE2sMLf4S7qR1A?view

TLDR: ECDSA signing in bberg and verification in noir has a minor
"difference". Signature construction and verification should operate
consistently on the message.

<img width="445" alt="image"
src="https://github.com/AztecProtocol/aztec-packages/assets/19621621/b845ab1c-44ca-49ea-932d-ed05ed51b204">

<img width="567" alt="image"
src="https://github.com/AztecProtocol/aztec-packages/assets/19621621/5ae6fd8d-4a64-4196-973c-ffc45bfd10cf">

The noir ecdsa verification takes in the hashed message $z$ as an
argument. So we need to hash the message before calling
`std::ecdsa_secp256k1::verify_signature`.

The reason this worked with the noble curves package was: in the noble
package, the message is never hashed. It is used as is for signing a
message. Therefore the noir-ecdsa-verify works as we don't need to hash
the message in verification.

# Checklist:

- [x] I have reviewed my diff in github, line by line.
- [x] Every change is related to the PR description.
- [x] I have
[linked](https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue)
this pull request to the issue(s) that it resolves.
- [x] There are no unexpected formatting changes, superfluous debug
logs, or commented-out code.
- [x] The branch has been merged or rebased against the head of its
merge target.
- [x] I'm happy for the PR to be merged at the reviewer's next
convenience.
@github-project-automation github-project-automation bot moved this from In Progress to Done in A3 Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
3 participants