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

address upper and lower S issues #1

Open
OR13 opened this issue Nov 8, 2021 · 7 comments
Open

address upper and lower S issues #1

OR13 opened this issue Nov 8, 2021 · 7 comments

Comments

@OR13
Copy link
Contributor

OR13 commented Nov 8, 2021

associated with secp256k1 ECDSA.

@OR13
Copy link
Contributor Author

OR13 commented Nov 8, 2021

Initially, our implementation failed to verify Microsoft issued, credentials, but then we updated it in this PR: transmute-industries/verifiable-data#115

99% of the PR is unrelated to the fix, but the core problem is "ECDSA verify vs ECDSA normalize and verify"... if the verify function expects the signature to be lower S (normalized) and it was not, the verify will fail (erroneously)... even in such cases, the exact same JWS can be safely transformed and verified, by applying the normalization process...

see these related links:

https://github.com/bitauth/libauth

https://github.com/bitauth/libauth/blob/13454f1f6b3d9e87b6361cc71f082950fe6abf3a/src/lib/crypto/secp256k1-types.ts#L190

https://bitcoin.stackexchange.com/questions/49767/openssl-and-secp256k1-differing-in-implementation-of-ecdsa-signatures

@brianorwhatever
Copy link

@OR13 I think I'm encountering this problem using the https://github.com/paulmillr/noble-secp256k1 library. Glad I remembered hearing you discuss this during the JWS meeting.. I will go through the links here and figure out how to normalize

@brianorwhatever
Copy link

Ok, I got my tests passing at least but I think my actual issue was unrelated so now i'm not sure if i solved this. I am currently using options.der = false and options.canonical = false from here do you think that's correct?
https://github.com/paulmillr/noble-secp256k1#signhash-privatekey

I'm going to start plugging my issuer into the JWS test suite to see if I'm verifiable

@OR13
Copy link
Contributor Author

OR13 commented Nov 29, 2021

@brianorwhatever I solve these issues by cross testing against "known good" implementations.... those are really implementations I didn't write, that were written by people I trust.... for JWS in javascript thats https://www.npmjs.com/package/jose but you may have other languages you prefer... a cross test should confirm:

  1. You can issue, they can verify.

  2. They can issue, you can verify

  3. You can issue and verify (I do these first generally, since you need them for 1 and 2)

  4. They can issue and verify (I do these first generally, since you need them for 1 and 2)

@clehner
Copy link

clehner commented Jan 20, 2022

I think I am encountering this issue, with the JWT at the following URL: https://identity.foundation/sidetree/spec/v1.0.0/#deactivate-2 (JWT in the signedData property). I'm unable to verify it using the Rust k256 crate, but it verifies using latchset/jose#90 (openssl). Edit: the k256 crate readme says it supports low-S normalization, so maybe this is something else. confirmed, it verifies after normalization.

@clehner
Copy link

clehner commented Jan 20, 2022

Here's the C source for the secp256k1_ecdsa_signature_normalize function from libsecp256k1, as used in libauth (as WASM/emscripten, exposed in the normalizeSignatureCompact function): https://github.com/bitauth/libauth-secp256k1/blob/2b8e4e6a95a7da9fde0d15fb5351b22bcc1863c5/src/secp256k1.c#L387-L404

Here's C++ code in bitcoin for normalizing while signing: https://github.com/bitcoin/bitcoin/blob/40d20412ff173e8eea2f456fd749663c8cabda18/src/key.cpp#L202-L227

Here's the relevant BIP: https://github.com/bitcoin/bips/blob/master/bip-0062.mediawiki#low-s-values-in-signatures

Here's Rust code for normalization in the ecdsa crate (used by k256 crate):
https://docs.rs/ecdsa/latest/ecdsa/struct.Signature.html#method.normalize_s
https://github.com/RustCrypto/signatures/blob/786b9bc158b2b9e0a8f0c07f811600957294880c/ecdsa/src/lib.rs#L221-L237

@OR13
Copy link
Contributor Author

OR13 commented Jan 20, 2022

@clehner thanks for linking directly to those... this issue will keep coming up as long as secp256k1 shall live.

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

No branches or pull requests

3 participants