k256: Do not normalize the argument in FieldElementImpl::is_odd()
#530
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tarcieri
reviewed
Mar 14, 2022
|
@fjarri that looks like a legitimate failure to me. One thing that's special about the platform it's failing on is that it's 32-bit. Perhaps it's an issue with the 32-bit field backend? |
tarcieri
reviewed
Mar 14, 2022
k256/src/arithmetic/field.rs
Outdated
| @@ -542,6 +542,29 @@ mod tests { | |||
| assert_eq!(four.sqrt().unwrap().normalize(), two.normalize()); | |||
| } | |||
|
|
|||
| #[test] | |||
| #[should_panic(expected = "assertion failed: self.normalized")] | |||
There was a problem hiding this comment.
Another potential issue is that test will only succeed (i.e. is_odd will only panic) when debug_assertions are enabled.
Perhaps the test, or at least the should_panic, should be gated on that?
Suggested change
| #[should_panic(expected = "assertion failed: self.normalized")] | |
| #[cfg_attr(debug_assertions, should_panic(expected = "assertion failed: self.normalized"))] |
tarcieri
approved these changes
Mar 14, 2022
|
FYI: I opened #531 with a proposal to encapsulate concerns around normalizing field elements using type safety |
tarcieri
added a commit
that referenced
this pull request
Mar 14, 2022
This release backports #530. Since `master` is already v0.11 prereleases, this commit just contains the release notes. The released code can be found via the `k256/v0.10.3` tag: https://github.com/RustCrypto/elliptic-curves/tree/k256/v0.10.3
tarcieri
added a commit
that referenced
this pull request
Mar 14, 2022
This release backports #530. Since `master` is already v0.11 prereleases, this commit just contains the release notes. The released code can be found via the `k256/v0.10.3` tag: https://github.com/RustCrypto/elliptic-curves/tree/k256/v0.10.3
dfinity-bot
pushed a commit
to dfinity/ic
that referenced
this pull request
Mar 15, 2022
CRP-1445: Upgrade k256 crate to v0.10.3 to fix point serialization bug Upgrades the `k256` crate used in `ic-crypto-internal-threshold-sig-ecdsa` to v0.10.3. This version contains a [fix](RustCrypto/elliptic-curves#530) for the point (de)serialization bug that we reported in [k256#529](RustCrypto/elliptic-curves#529). This bug caused our tests to fail sporadically because elliptic curve points were different before and after a (de)serialization round trip. See merge request dfinity-lab/public/ic!3752
tarcieri
pushed a commit
that referenced
this pull request
Mar 15, 2022
…530) - `FieldElementImpl::is_odd()` now checks for normalization instead of performing it - `AffinePoint::decompress()` normalizes `y` coordinate (`beta` in the code) before checking if it is odd - `Scalar::try_sign_prehashed` normalizes `R.y` before checking if it is odd
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #529
FieldElementImpl::is_odd()(which was only compiled in debug version) normalized its argument, wherein in release mode it was bypassed in favor of the specific 32- or 64-bit implementation. This caused an inconsistency between the release and debug versions, and lead to uncaught issues where an un-normalized field element could be checked foris_odd()(the exact situationFieldElementImplwas designed to detect).In this PR:
FieldElementImpl::is_odd()now checks for normalization instead of performing itAffinePoint::decompress()normalizesycoordinate (betain the code) before checking if it is oddScalar::try_sign_prehashednormalizesR.ybefore checking if it is oddI added a regression test for the first change; the other two are already tested by
arithmetic::affine::tests::compressed_round_trip,arithmetic::affine::tests::compressed_to_uncompressedandecdsa::recoverable::tests::public_key_recovery- those tests only passed because they were always run in debug mode where force normalization was on.