feat: Low-s normalization for ecdsa secp256r1 signing #9738

frumioj · 2021-07-20T20:04:17Z

Description

Closes: #9723

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

confirmed the correct type prefix in the PR title
confirmed ! in the type prefix if API or client breaking change
confirmed all author checklist items have been addressed
reviewed state machine logic
reviewed API design and naming
reviewed documentation is accurate
reviewed tests and test coverage
manually tested (if applicable)

…k/low-s

codecov · 2021-07-20T20:15:38Z

Codecov Report

Merging #9738 (1371772) into master (f4d0682) will increase coverage by 0.00%.
The diff coverage is 62.97%.

@@           Coverage Diff            @@
##           master    #9738    +/-   ##
========================================
  Coverage   63.47%   63.48%            
========================================
  Files         566      566            
  Lines       52797    53065   +268     
========================================
+ Hits        33515    33687   +172     
- Misses      17379    17471    +92     
- Partials     1903     1907     +4

Impacted Files	Coverage Δ
types/events.go	`82.77% <ø> (ø)`
x/auth/client/cli/query.go	`2.15% <6.41%> (+2.15%)`	⬆️
x/auth/ante/sigverify.go	`68.07% <38.00%> (-5.86%)`	⬇️
crypto/keys/internal/ecdsa/privkey.go	`84.21% <88.00%> (+2.39%)`	⬆️
crypto/keys/internal/ecdsa/pubkey.go	`77.08% <100.00%> (+9.43%)`	⬆️
x/auth/client/testutil/suite.go	`96.31% <100.00%> (+0.40%)`	⬆️

aaronc · 2021-07-20T20:25:35Z

@tarcieri are you able to take a look at this?

crypto/keys/internal/ecdsa/privkey.go

frumioj · 2021-07-21T19:06:54Z

@alexanderbez @robert-zaremba any comments?

…r the check for sig string length)

…k/low-s

tarcieri

I think it might still be good to either make functions like p256Order and p256OrderDiv2 parameterized over curves or just use vars for them instead, but otherwise this looks good to me.

frumioj · 2021-07-21T23:17:25Z

I think it might still be good to either make functions like p256Order and p256OrderDiv2 parameterized over curves or just use vars for them instead, but otherwise this looks good to me.

I agree with you, but I think I'm going to create a new (bigger) issue which will focus on refactoring all of the ECDSA signature code so we don't have essentially 3 different code paths all using roughly the same code but in 3 different places, and all slightly differently. Then I'd want to get the Curve.Params() from the key passed in, and work with that.

crypto/keys/internal/ecdsa/pubkey.go

…ion to fail after the valid signature was mutated by extracting and scalar negating its s value

crypto/keys/internal/ecdsa/privkey_internal_test.go

…k/low-s

amaury1093

I only checked the test in crypto/keys/internal/ecdsa/privkey_internal_test.go , and it lgtm. Not super familiar with the rest of the changes.

crypto/keys/internal/ecdsa/privkey_internal_test.go

…the publicKey

robert-zaremba

Approving, however I prefer to reuse and test a private function rather than copying it and have a test coverage miss.

crypto/keys/internal/ecdsa/privkey.go

crypto/keys/internal/ecdsa/privkey_internal_test.go

robert-zaremba · 2021-07-26T22:30:58Z

Added backport to 0.42

alexanderbez · 2021-07-27T13:26:27Z

I think @robert-zaremba left some good comments. I'll approve once addressed 🚀

robert-zaremba

Approving. Had a chat with John to advance on this. Let's add a missing tests and go through comments to resolve them before merging.

…k/low-s

…ed tests to improve code coverage

aaronc · 2021-07-27T18:09:59Z

Thanks @robert-zaremba. @frumioj feel free to merge when you're ready.

orijbot · 2021-07-27T18:14:16Z

Visit https://dashboard.github.orijtech.com?pr=9738&repo=cosmos%2Fcosmos-sdk to see benchmark details.

* added low-s normalization to ecdsa secp256r1 signing * go fmt fixes * removed else block as golint required * implement raw signature encoding for secp256r1 * move the creation of signature to after the check for sig string length * fake commit to re-run checks? (move the creation of signature to after the check for sig string length) * added a signature test for high s signature that requires sig validation to fail after the valid signature was mutated by extracting and scalar negating its s value * reordered code to prevent mutated message from being used in sig verify * added test for successful high_s signature with the ecdsa portion of the publicKey * Remove comment for self-explanatory code. Co-authored-by: Robert Zaremba <robert@zaremba.ch> * Missing quote Co-authored-by: Robert Zaremba <robert@zaremba.ch> * Apply minor suggestions from code review Co-authored-by: Robert Zaremba <robert@zaremba.ch> * normalize comments for godoc * refactored p256Order functions as private vars * Div -> Rsh optimizing time for division * resolve two code coverage issues; fix some small review issues * test using private signatureRaw function instead of copying code. Added tests to improve code coverage Co-authored-by: Aaron Craelius <aaron@regen.network> Co-authored-by: Robert Zaremba <robert@zaremba.ch> Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com> (cherry picked from commit aa37ae9)

* added low-s normalization to ecdsa secp256r1 signing * go fmt fixes * removed else block as golint required * implement raw signature encoding for secp256r1 * move the creation of signature to after the check for sig string length * fake commit to re-run checks? (move the creation of signature to after the check for sig string length) * added a signature test for high s signature that requires sig validation to fail after the valid signature was mutated by extracting and scalar negating its s value * reordered code to prevent mutated message from being used in sig verify * added test for successful high_s signature with the ecdsa portion of the publicKey * Remove comment for self-explanatory code. Co-authored-by: Robert Zaremba <robert@zaremba.ch> * Missing quote Co-authored-by: Robert Zaremba <robert@zaremba.ch> * Apply minor suggestions from code review Co-authored-by: Robert Zaremba <robert@zaremba.ch> * normalize comments for godoc * refactored p256Order functions as private vars * Div -> Rsh optimizing time for division * resolve two code coverage issues; fix some small review issues * test using private signatureRaw function instead of copying code. Added tests to improve code coverage Co-authored-by: Aaron Craelius <aaron@regen.network> Co-authored-by: Robert Zaremba <robert@zaremba.ch> Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com> (cherry picked from commit aa37ae9) # Conflicts: # crypto/keys/internal/ecdsa/privkey.go # crypto/keys/internal/ecdsa/privkey_internal_test.go # crypto/keys/internal/ecdsa/pubkey.go

* added low-s normalization to ecdsa secp256r1 signing * go fmt fixes * removed else block as golint required * implement raw signature encoding for secp256r1 * move the creation of signature to after the check for sig string length * fake commit to re-run checks? (move the creation of signature to after the check for sig string length) * added a signature test for high s signature that requires sig validation to fail after the valid signature was mutated by extracting and scalar negating its s value * reordered code to prevent mutated message from being used in sig verify * added test for successful high_s signature with the ecdsa portion of the publicKey * Remove comment for self-explanatory code. Co-authored-by: Robert Zaremba <robert@zaremba.ch> * Missing quote Co-authored-by: Robert Zaremba <robert@zaremba.ch> * Apply minor suggestions from code review Co-authored-by: Robert Zaremba <robert@zaremba.ch> * normalize comments for godoc * refactored p256Order functions as private vars * Div -> Rsh optimizing time for division * resolve two code coverage issues; fix some small review issues * test using private signatureRaw function instead of copying code. Added tests to improve code coverage Co-authored-by: Aaron Craelius <aaron@regen.network> Co-authored-by: Robert Zaremba <robert@zaremba.ch> Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com> (cherry picked from commit aa37ae9) Co-authored-by: John Kemp <frumioj@users.noreply.github.com>

…osmos#9793) * added low-s normalization to ecdsa secp256r1 signing * go fmt fixes * removed else block as golint required * implement raw signature encoding for secp256r1 * move the creation of signature to after the check for sig string length * fake commit to re-run checks? (move the creation of signature to after the check for sig string length) * added a signature test for high s signature that requires sig validation to fail after the valid signature was mutated by extracting and scalar negating its s value * reordered code to prevent mutated message from being used in sig verify * added test for successful high_s signature with the ecdsa portion of the publicKey * Remove comment for self-explanatory code. Co-authored-by: Robert Zaremba <robert@zaremba.ch> * Missing quote Co-authored-by: Robert Zaremba <robert@zaremba.ch> * Apply minor suggestions from code review Co-authored-by: Robert Zaremba <robert@zaremba.ch> * normalize comments for godoc * refactored p256Order functions as private vars * Div -> Rsh optimizing time for division * resolve two code coverage issues; fix some small review issues * test using private signatureRaw function instead of copying code. Added tests to improve code coverage Co-authored-by: Aaron Craelius <aaron@regen.network> Co-authored-by: Robert Zaremba <robert@zaremba.ch> Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com> (cherry picked from commit aa37ae9) Co-authored-by: John Kemp <frumioj@users.noreply.github.com>

frumioj and others added 2 commits July 20, 2021 12:03

added low-s normalization to ecdsa secp256r1 signing

5400da0

Merge branch 'master' into johnk/low-s

85ccef3

frumioj changed the title ~~Low-s normalization for ecdsa secp256r1 signing~~ feat: Low-s normalization for ecdsa secp256r1 signing Jul 20, 2021

frumioj added 2 commits July 20, 2021 13:10

go fmt fixes

07592ec

Merge branch 'johnk/low-s' of github.com:frumioj/cosmos-sdk into john…

1718254

…k/low-s

removed else block as golint required

6e62695

tarcieri reviewed Jul 20, 2021

View reviewed changes

crypto/keys/internal/ecdsa/privkey.go Outdated Show resolved Hide resolved

tarcieri reviewed Jul 20, 2021

View reviewed changes

crypto/keys/internal/ecdsa/privkey.go Outdated Show resolved Hide resolved

frumioj added 2 commits July 21, 2021 11:54

implement raw signature encoding for secp256r1

b4b66fa

move the creation of signature to after the check for sig string length

1969404

frumioj marked this pull request as ready for review July 21, 2021 19:27

frumioj requested review from aaronc and alexanderbez as code owners July 21, 2021 19:27

frumioj added 2 commits July 21, 2021 12:44

fake commit to re-run checks? (move the creation of signature to afte…

e6f1089

…r the check for sig string length)

Merge branch 'johnk/low-s' of github.com:frumioj/cosmos-sdk into john…

40373c4

…k/low-s

tarcieri approved these changes Jul 21, 2021

View reviewed changes

amaury1093 reviewed Jul 22, 2021

View reviewed changes

crypto/keys/internal/ecdsa/pubkey.go Show resolved Hide resolved

frumioj and others added 2 commits July 22, 2021 10:26

added a signature test for high s signature that requires sig validat…

1c68028

…ion to fail after the valid signature was mutated by extracting and scalar negating its s value

Merge branch 'master' into johnk/low-s

a8073f5

frumioj requested a review from amaury1093 July 22, 2021 17:32

amaury1093 reviewed Jul 22, 2021

View reviewed changes

crypto/keys/internal/ecdsa/privkey_internal_test.go Outdated Show resolved Hide resolved

crypto/keys/internal/ecdsa/privkey_internal_test.go Outdated Show resolved Hide resolved

frumioj added 2 commits July 22, 2021 11:01

reordered code to prevent mutated message from being used in sig verify

6bffdb8

Merge branch 'johnk/low-s' of github.com:frumioj/cosmos-sdk into john…

2ca8996

…k/low-s

amaury1093 approved these changes Jul 22, 2021

View reviewed changes

crypto/keys/internal/ecdsa/privkey_internal_test.go Outdated Show resolved Hide resolved

frumioj and others added 2 commits July 22, 2021 17:14

added test for successful high_s signature with the ecdsa portion of …

1899373

…the publicKey

Merge branch 'master' into johnk/low-s

a70dfdf

robert-zaremba approved these changes Jul 26, 2021

View reviewed changes

robert-zaremba added backport/0.42.x (Stargate) labels Jul 26, 2021

frumioj and others added 2 commits July 26, 2021 20:46

Merge branch 'master' into johnk/low-s

36f7f53

Merge branch 'master' into johnk/low-s

4985c02

robert-zaremba approved these changes Jul 27, 2021

View reviewed changes

frumioj and others added 4 commits July 27, 2021 10:38

Merge branch 'master' into johnk/low-s

5f70929

resolve two code coverage issues; fix some small review issues

eb13a4b

Merge branch 'johnk/low-s' of github.com:frumioj/cosmos-sdk into john…

79be622

…k/low-s

test using private signatureRaw function instead of copying code. Add…

1371772

…ed tests to improve code coverage

frumioj added the A:automerge Automatically merge PR once all prerequisites pass. label Jul 27, 2021

frumioj merged commit aa37ae9 into cosmos:master Jul 27, 2021

mergify bot mentioned this pull request Jul 27, 2021

feat: Low-s normalization for ecdsa secp256r1 signing (backport #9738) #9793

Merged

mergify bot mentioned this pull request Jul 27, 2021

feat: Low-s normalization for ecdsa secp256r1 signing (backport #9738) #9794

Closed

aaronc removed the backport/0.42.x (Stargate) label Jul 27, 2021

fedekunze mentioned this pull request Nov 14, 2021

all: cherry pick olympus mons tharsis/cosmos-sdk#1

Closed

19 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Low-s normalization for ecdsa secp256r1 signing #9738

feat: Low-s normalization for ecdsa secp256r1 signing #9738

frumioj commented Jul 20, 2021 •

edited

Loading

codecov bot commented Jul 20, 2021 •

edited

Loading

aaronc commented Jul 20, 2021

frumioj commented Jul 21, 2021

tarcieri left a comment

frumioj commented Jul 21, 2021

amaury1093 left a comment

robert-zaremba left a comment

robert-zaremba commented Jul 26, 2021

alexanderbez commented Jul 27, 2021

robert-zaremba left a comment

aaronc commented Jul 27, 2021

orijbot commented Jul 27, 2021

feat: Low-s normalization for ecdsa secp256r1 signing #9738

feat: Low-s normalization for ecdsa secp256r1 signing #9738

Conversation

frumioj commented Jul 20, 2021 • edited Loading

Description

Author Checklist

Reviewers Checklist

codecov bot commented Jul 20, 2021 • edited Loading

Codecov Report

aaronc commented Jul 20, 2021

frumioj commented Jul 21, 2021

tarcieri left a comment

Choose a reason for hiding this comment

frumioj commented Jul 21, 2021

amaury1093 left a comment

Choose a reason for hiding this comment

robert-zaremba left a comment

Choose a reason for hiding this comment

robert-zaremba commented Jul 26, 2021

alexanderbez commented Jul 27, 2021

robert-zaremba left a comment

Choose a reason for hiding this comment

aaronc commented Jul 27, 2021

orijbot commented Jul 27, 2021

frumioj commented Jul 20, 2021 •

edited

Loading

codecov bot commented Jul 20, 2021 •

edited

Loading