RUSTSEC-2021-0076 bump libsecp256k1 #9391

trevor-crypto · 2021-07-20T07:45:08Z

libsecp256k1 allows overflowing signatures
https://rustsec.org/advisories/RUSTSEC-2021-0076

Changes were made to conform to libsecp256k1 version differences.

bkchr · 2021-07-20T09:31:48Z

@sorpaas please check if that is the right way. I remember that you said something about some migration.

sorpaas · 2021-07-20T09:33:57Z

primitives/core/src/ecdsa.rs

 		Ok((
-			secp256k1::Signature::parse_slice(&x.0[0..64]).expect("hardcoded to 64 bytes; qed"),
-			secp256k1::RecoveryId::parse(x.0[64]).map_err(|_| ())?,
+			libsecp256k1::Signature::parse_standard_slice(&x.0[0..64]).expect("hardcoded to 64 bytes; qed"),


Suggested change

libsecp256k1::Signature::parse_standard_slice(&x.0[0..64]).expect("hardcoded to 64 bytes; qed"),

libsecp256k1::Signature::parse_overflowing_slice(&x.0[0..64]).expect("hardcoded to 64 bytes; qed"),

We still need this as parse_overflowing_slice for now, then move to parse_standard_slice through a migration.

What's the reasoning behind this? (just for my own education)

Actually, I see. Some preexisting Signatures may be overflowing so we need to parse them with that

It would otherwise be an actual security advisory for Substrate.

Parsing overflowing slice is not dangerous and by itself is safe. The security advisory arises if there are multiple clients being maintained and all of them need to conform to a standard way.

If we change this to parse_standard_slice in Substrate now we need to mark it as a critical security upgrade for Substrate and ask everyone to upgrade, and during this period an attacker can split the chain. On the other hand, if we do it via a migration then it's completely safe, and we can do the migration in our own pace without any security concerns.

The drawback for the later, of course, is that parse_overflowing_slice will exist in Substrate at least for years to come (until when old blocks no longer need to be re-processed by anyone). However, given that the method is actually safe, but just non-standard, it shouldn't be of any problems.

What you mean by a migration?

Isn't the problem only the old runtimes? So we should introduce a new host function?

Yeah. We're on the same page.

primitives/core/src/ecdsa.rs

bkchr · 2021-07-20T14:21:45Z

@sorpaas shouldn't we directly introduce the new host function? Host functions support versioning and we already use this versioning for a similar problem with schnorrkel.

sorpaas · 2021-07-20T14:27:39Z

@bkchr Yeah we should do it.

bkchr · 2021-07-20T14:49:56Z

@trevor-crypto check this out https://github.com/paritytech/substrate/blob/master/primitives/io/src/lib.rs#L599-L600

@trevor-crypto We would need to do something similar here and use parse_overflowing_slice() in the version 1 and version 2 would use parse_standard_slice()

trevor-crypto · 2021-07-20T14:57:40Z

@bkchr thanks! Found my way there already by furiously grepping for 'host functions". I'm wondering how something like this will work with the TryFrom trait impl, since that is one place where parse_overflowing/standard_slice() needs to be used.

Edit: might be better to just remove the TryFrom impl and usage

bkchr · 2021-07-20T15:02:12Z

@trevor-crypto basically you need to revert c653284 because everything from now need to use the new function. Only the old host function needs to use the old function.

trevor-crypto · 2021-07-20T15:51:28Z

Added version 2 for ecdsa_verify. Please check carefully 🙏

bkchr

LGTM

primitives/core/src/ecdsa.rs

bkchr · 2021-07-20T19:01:10Z

@sorpaas please also check another time.

please review again

sorpaas · 2021-07-21T04:23:58Z

primitives/core/src/ecdsa.rs

+}
+
+#[cfg(feature = "full_crypto")]
+fn parse_signature(


Can we rename this to parse_standard_signature and parse_overflowing_signature and similarly for all the above verify_ public functions? The priority here should be make sure that all library users get to deliberately choose whether they want standard or overflowing, so we shouldn't be using the same old name.

Can't change the trait fn names, but I changed verify_deprecated to verify_overflowing

That doesn't do much unfortunately. If you can't change the other name for standard then I suggest putting the overflowing one back to deprecated.

I don't have huge problems keeping it as is, but we do need to be slightly careful here since misusing overflowing vs standard may be a source of trouble.

Alternatively, we can create a separate structure for the OverflowingSignature and rename the current one to StandardSignature. That would be the best for clarity, but just that it may be a lot of work -- so I guess it's optional.

@sorpaas ok thanks. I will leave it as verify_deprecated, since it also coincides with the names of the other versioned host functions in primitives/io/src/lib.rs.

trevor-crypto · 2021-07-21T06:43:04Z

Rebased with master hoping that the tests would pass 😖

girazoki · 2021-07-29T15:46:48Z

I wanted to jump in because I see that

substrate/primitives/io/src/lib.rs

Line 750 in 7dcc77b

fn secp256k1_ecdsa_recover(

also uses secp256k1::Signature::parse_slice. Is there a reason why it does not need to get this change?

sorpaas

Yeah @girazoki is right. This PR still misses a few version bumps.

trevor-crypto · 2021-07-30T05:30:45Z

Thanks @girazoki

I guess I missed other places as well like bumping libp2p since libp2p-core uses libsecp256k1 too.

bin/node/browser-testing/Cargo.toml

primitives/core/src/ecdsa.rs

bkchr · 2021-08-16T10:21:57Z

bot merge

ghost · 2021-08-16T10:22:01Z

Waiting for commit status.

trevor-crypto · 2021-08-16T10:23:58Z

@bkchr Thanks for applying those fixes!

ghost · 2021-08-16T10:39:51Z

Merge aborted: Checks failed for 00e976a

bkchr · 2021-08-16T14:17:05Z

bot merge force

ghost · 2021-08-16T14:17:09Z

Trying merge.

* RUSTSEC-2021-0076 bump libsecp256k1 libsecp256k1 allows overflowing signatures https://rustsec.org/advisories/RUSTSEC-2021-0076 Changes were made to conform to libsecp256k1 version differences. Closes paritytech#9356 * parse_standard_slice() -> parse_overflowing_slice() * Added v2 host function for ecdsa_verify * Add feature tag over helpers * Added ecdsa_verify v2 to test runner * PR feedback - Spaces -> tabs - renamed two helper functions * Fixed imports after rebasing * Bump rest of libsecp256k1 * Add version2 for ecdsa pubkey recovery * Update primitives/core/src/ecdsa.rs * Update primitives/core/src/ecdsa.rs * Update Cargo.lock Co-authored-by: Bastian Köcher <[email protected]>

* RUSTSEC-2021-0076 bump libsecp256k1 libsecp256k1 allows overflowing signatures https://rustsec.org/advisories/RUSTSEC-2021-0076 Changes were made to conform to libsecp256k1 version differences. Closes #9356 * parse_standard_slice() -> parse_overflowing_slice() * Added v2 host function for ecdsa_verify * Add feature tag over helpers * Added ecdsa_verify v2 to test runner * PR feedback - Spaces -> tabs - renamed two helper functions * Fixed imports after rebasing * Bump rest of libsecp256k1 (and libp2p) libp2p also uses libsecp256k1 so it is required to be bumped too, along with all the version difference changes. * Add version2 for ecdsa pubkey recovery * libp2p rebase master fixes * Fix test panic when non Behaviour event is returned * Update bin/node/browser-testing/Cargo.toml * Update primitives/core/src/ecdsa.rs * Update primitives/core/src/ecdsa.rs * Update Cargo.lock Co-authored-by: Bastian Köcher <[email protected]>

* RUSTSEC-2021-0076 bump libsecp256k1 libsecp256k1 allows overflowing signatures https://rustsec.org/advisories/RUSTSEC-2021-0076 Changes were made to conform to libsecp256k1 version differences. Closes paritytech#9356 * parse_standard_slice() -> parse_overflowing_slice() * Added v2 host function for ecdsa_verify * Add feature tag over helpers * Added ecdsa_verify v2 to test runner * PR feedback - Spaces -> tabs - renamed two helper functions * Fixed imports after rebasing * Bump rest of libsecp256k1 * Add version2 for ecdsa pubkey recovery * Update primitives/core/src/ecdsa.rs * Update primitives/core/src/ecdsa.rs * Update Cargo.lock Co-authored-by: Bastian Köcher <[email protected]>

* RUSTSEC-2021-0076 bump libsecp256k1 (#9391) * RUSTSEC-2021-0076 bump libsecp256k1 libsecp256k1 allows overflowing signatures https://rustsec.org/advisories/RUSTSEC-2021-0076 Changes were made to conform to libsecp256k1 version differences. Closes #9356 * parse_standard_slice() -> parse_overflowing_slice() * Added v2 host function for ecdsa_verify * Add feature tag over helpers * Added ecdsa_verify v2 to test runner * PR feedback - Spaces -> tabs - renamed two helper functions * Fixed imports after rebasing * Bump rest of libsecp256k1 (and libp2p) libp2p also uses libsecp256k1 so it is required to be bumped too, along with all the version difference changes. * Add version2 for ecdsa pubkey recovery * libp2p rebase master fixes * Fix test panic when non Behaviour event is returned * Update bin/node/browser-testing/Cargo.toml * Update primitives/core/src/ecdsa.rs * Update primitives/core/src/ecdsa.rs * Update Cargo.lock Co-authored-by: Bastian Köcher <[email protected]> * Use coherent prost crate versions (#9676) * Bump node-browser-testing deps on prost Co-authored-by: Trevor Arjeski <[email protected]> Co-authored-by: Bastian Köcher <[email protected]> Co-authored-by: Andreas Doerr <[email protected]>

* RUSTSEC-2021-0076 bump libsecp256k1 libsecp256k1 allows overflowing signatures https://rustsec.org/advisories/RUSTSEC-2021-0076 Changes were made to conform to libsecp256k1 version differences. Closes #9356 * parse_standard_slice() -> parse_overflowing_slice() * Added v2 host function for ecdsa_verify * Add feature tag over helpers * Added ecdsa_verify v2 to test runner * PR feedback - Spaces -> tabs - renamed two helper functions * Fixed imports after rebasing * Bump rest of libsecp256k1 (and libp2p) libp2p also uses libsecp256k1 so it is required to be bumped too, along with all the version difference changes. * Add version2 for ecdsa pubkey recovery * libp2p rebase master fixes * Fix test panic when non Behaviour event is returned * Update bin/node/browser-testing/Cargo.toml * Update primitives/core/src/ecdsa.rs * Update primitives/core/src/ecdsa.rs * Update Cargo.lock Co-authored-by: Bastian Köcher <[email protected]>

bkchr requested a review from sorpaas July 20, 2021 09:31

sorpaas reviewed Jul 20, 2021

View reviewed changes

primitives/core/src/ecdsa.rs Outdated Show resolved Hide resolved

sorpaas previously approved these changes Jul 20, 2021

View reviewed changes

trevor-crypto force-pushed the libsecp-rustsec branch 2 times, most recently from 1c6d9a5 to 82fc9da Compare July 20, 2021 16:41

trevor-crypto mentioned this pull request Jul 20, 2021

Bump libsecp256k1 to 0.5.0 paritytech/polkadot#3490

Open

bkchr approved these changes Jul 20, 2021

View reviewed changes

primitives/core/src/ecdsa.rs Outdated Show resolved Hide resolved

sorpaas reviewed Jul 21, 2021

View reviewed changes

trevor-crypto force-pushed the libsecp-rustsec branch from 421aedf to 5a169e0 Compare July 21, 2021 05:37

trevor-crypto requested a review from sorpaas July 21, 2021 13:59

trevor-crypto force-pushed the libsecp-rustsec branch from 5a169e0 to 093131b Compare July 21, 2021 15:45

sorpaas approved these changes Jul 29, 2021

View reviewed changes

sorpaas suggested changes Jul 29, 2021

View reviewed changes

tomaka approved these changes Aug 16, 2021

View reviewed changes

bkchr approved these changes Aug 16, 2021

View reviewed changes

bin/node/browser-testing/Cargo.toml Outdated Show resolved Hide resolved

primitives/core/src/ecdsa.rs Outdated Show resolved Hide resolved

primitives/core/src/ecdsa.rs Outdated Show resolved Hide resolved

bkchr added 3 commits August 16, 2021 12:21

Update bin/node/browser-testing/Cargo.toml

ebb8380

Update primitives/core/src/ecdsa.rs

7eb9175

Update primitives/core/src/ecdsa.rs

00e976a

Update Cargo.lock

a9414e1

ghost merged commit 08cac00 into paritytech:master Aug 16, 2021

xlc mentioned this pull request Aug 26, 2021

Fix ci open-web3-stack/open-runtime-module-library#593

Merged

This was referenced Sep 1, 2021

Dependency issue with crypto-mac package #9633

Open

snow which could resolve this conflict #9680

Closed

Neopallium mentioned this pull request Sep 2, 2021

MESH-1680 bump libsecp256k1 PolymeshAssociation/substrate#1

Merged

balqaasem mentioned this pull request Sep 6, 2021

Breaking - error: failed to select a version for snow Setheum-Labs/Setheum#548

Closed

chevdor mentioned this pull request Sep 7, 2021

Cherry Pick of PR #9676 + #9391 #9704

Merged

trevor-crypto deleted the libsecp-rustsec branch September 7, 2021 19:58

JoshOrndorff mentioned this pull request Sep 16, 2021

Update substrate/polkadot/cumulus from v0.9.9 to v0.9.10 moonbeam-foundation/moonbeam#823

Closed

tomaka mentioned this pull request Sep 20, 2021

Backport upgrade to libsecp256k1 paritytech/smoldot#1333

Merged

github-actions bot mentioned this pull request Oct 11, 2021

Update substrate/polkadot/cumulus from v0.9.10 to v0.9.11 moonbeam-foundation/moonbeam#892

Closed

Yithis mentioned this pull request Oct 29, 2021

Runtime_update using compact version does not work #10120

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RUSTSEC-2021-0076 bump libsecp256k1 #9391

RUSTSEC-2021-0076 bump libsecp256k1 #9391

trevor-crypto commented Jul 20, 2021

bkchr commented Jul 20, 2021

sorpaas Jul 20, 2021

trevor-crypto Jul 20, 2021

trevor-crypto Jul 20, 2021

sorpaas Jul 20, 2021 •

edited

Loading

bkchr Jul 20, 2021

sorpaas Jul 20, 2021

bkchr commented Jul 20, 2021

sorpaas commented Jul 20, 2021

bkchr commented Jul 20, 2021

trevor-crypto commented Jul 20, 2021 •

edited

Loading

bkchr commented Jul 20, 2021

trevor-crypto commented Jul 20, 2021

bkchr left a comment

bkchr commented Jul 20, 2021

sorpaas Jul 21, 2021

trevor-crypto Jul 21, 2021

sorpaas Jul 21, 2021 •

edited

Loading

trevor-crypto Jul 21, 2021

trevor-crypto commented Jul 21, 2021

girazoki commented Jul 29, 2021

sorpaas left a comment

trevor-crypto commented Jul 30, 2021

bkchr commented Aug 16, 2021

ghost commented Aug 16, 2021

trevor-crypto commented Aug 16, 2021

ghost commented Aug 16, 2021

bkchr commented Aug 16, 2021

ghost commented Aug 16, 2021

	libsecp256k1::Signature::parse_standard_slice(&x.0[0..64]).expect("hardcoded to 64 bytes; qed"),
	libsecp256k1::Signature::parse_overflowing_slice(&x.0[0..64]).expect("hardcoded to 64 bytes; qed"),

RUSTSEC-2021-0076 bump libsecp256k1 #9391

RUSTSEC-2021-0076 bump libsecp256k1 #9391

Conversation

trevor-crypto commented Jul 20, 2021

bkchr commented Jul 20, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sorpaas Jul 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkchr commented Jul 20, 2021

sorpaas commented Jul 20, 2021

bkchr commented Jul 20, 2021

trevor-crypto commented Jul 20, 2021 • edited Loading

bkchr commented Jul 20, 2021

trevor-crypto commented Jul 20, 2021

bkchr left a comment

Choose a reason for hiding this comment

bkchr commented Jul 20, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sorpaas Jul 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trevor-crypto commented Jul 21, 2021

girazoki commented Jul 29, 2021

sorpaas left a comment

Choose a reason for hiding this comment

trevor-crypto commented Jul 30, 2021

bkchr commented Aug 16, 2021

ghost commented Aug 16, 2021

trevor-crypto commented Aug 16, 2021

ghost commented Aug 16, 2021

bkchr commented Aug 16, 2021

ghost commented Aug 16, 2021

sorpaas Jul 20, 2021 •

edited

Loading

trevor-crypto commented Jul 20, 2021 •

edited

Loading

sorpaas Jul 21, 2021 •

edited

Loading