fix: do not double hash in secp256r1-verify
#6763
Open
+132
−8
Conversation
This file contains hidden or 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
This is a consensus breaking change and will need to go out in a hard-fork with a new version of Clarity (Clarity5). In the existing implementation, the code was incorrectly hashing the passed in `message-hash`. Unfortunately, the test code was also double-hashing on the signing side as well, so this was not caught. The bug was discovered by @eric-stacks when attempting to generate signatures externally (e.g. with WebAuthn) and verify them in Clarity code.
Codecov Report❌ Patch coverage is
❌ Your project check has failed because the head coverage (72.67%) is below the target coverage (80.00%). You can increase the head coverage or adjust the target coverage.
Additional details and impacted files@@ Coverage Diff @@
## develop #6763 +/- ##
===========================================
- Coverage 78.18% 72.67% -5.51%
===========================================
Files 580 580
Lines 361096 361175 +79
===========================================
- Hits 282312 262488 -19824
- Misses 78784 98687 +19903
... and 264 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Contributor
|
I think there's some test vectors for secp256r1 produced by NIST -- see the test vectors section at the bottom here (https://csrc.nist.gov/Projects/cryptographic-algorithm-validation-program/Component-Testing): Not sure if these would have caught the fact that we're double hashing. |
Contributor
|
Yeah -- I think we probably would have caught this if we used the NIST test vectors: diff --git a/clarity/src/vm/tests/crypto.rs b/clarity/src/vm/tests/crypto.rs
index 05d9460320..842090421e 100644
--- a/clarity/src/vm/tests/crypto.rs
+++ b/clarity/src/vm/tests/crypto.rs
@@ -3,13 +3,81 @@ use clarity_types::VmExecutionError;
use proptest::prelude::*;
use stacks_common::types::chainstate::{StacksPrivateKey, StacksPublicKey};
use stacks_common::types::{PrivateKey, StacksEpochId};
-use stacks_common::util::hash::{to_hex, Sha256Sum};
+use stacks_common::util::hash::{Sha256Sum, hex_bytes, to_hex};
use stacks_common::util::secp256k1::MessageSignature as Secp256k1Signature;
-use stacks_common::util::secp256r1::{Secp256r1PrivateKey, Secp256r1PublicKey};
+use stacks_common::util::secp256r1::{MessageSignature, Secp256r1PrivateKey, Secp256r1PublicKey};
use crate::vm::types::{ResponseData, TypeSignature, Value};
use crate::vm::{execute_with_parameters, ClarityVersion};
+struct NistVector {
+ msg: &'static str,
+ d: &'static str,
+ q_x: &'static str,
+ q_y: &'static str,
+ r: &'static str,
+ s: &'static str,
+}
+
+static NIST_VECTORS: &[NistVector] = &[
+ NistVector {
+ msg: "44acf6b7e36c1342c2c5897204fe09504e1e2efb1a900377dbc4e7a6a133ec56",
+ d: "519b423d715f8b581f4fa8ee59f4771a5b44c8130b4e3eacca54a56dda72b464",
+ q_x: "1ccbe91c075fc7f4f033bfa248db8fccd3565de94bbfb12f3c59ff46c271bf83",
+ q_y: "ce4014c68811f9a21a1fdb2c0e6113e06db7ca93b7404e78dc7ccd5ca89a4ca9",
+ r: "f3ac8061b514795b8843e3d6629527ed2afd6b1f6a555a7acabb5e6f79c8c2ac",
+ s: "8bf77819ca05a6b2786c76262bf7371cef97b218e96f175a3ccdda2acc058903",
+ }
+];
+
+#[rstest]
+#[case(0)]
+fn secp256r1_verify_valid_signatures_nist(
+ #[case] nist_index: usize
+) {
+ let NistVector { msg, d, q_x, q_y, r, s } = NIST_VECTORS[nist_index];
+
+ let message_hash = hex_bytes(msg).unwrap();
+ let privk = Secp256r1PrivateKey::from_hex(d).unwrap();
+ let mut pubk = Secp256r1PublicKey::from_hex(&format!("04{q_x}{q_y}")).unwrap();
+ pubk.set_compressed(true);
+
+ let signature_dh = privk.sign(&message_hash).unwrap();
+ let signature_nist = MessageSignature::from_hex(&format!("{r}{s}")).unwrap();
+
+ let verified_double_hash = pubk.verify(&message_hash, &signature_dh).is_ok();
+ let verified_nist_sign = pubk.verify_digest(&message_hash, &signature_nist).is_ok();
+
+ info!(
+ "Signed with SK";
+ "nist_signature" => format!("{r}{s}"),
+ "double_hash" => to_hex(&signature_dh.0),
+ "pubk" => to_hex(&pubk.to_bytes()),
+ "verified_double_hash" => verified_double_hash,
+ "verified_nist_sign" => verified_nist_sign,
+ );
+
+ let program = format!(
+ "(secp256r1-verify {} {} {})",
+ buff_literal(&message_hash),
+ buff_literal(&signature_nist.0),
+ buff_literal(&pubk.to_bytes())
+ );
+
+ assert_eq!(
+ Value::Bool(true),
+ execute_with_parameters(
+ program.as_str(),
+ ClarityVersion::Clarity4,
+ StacksEpochId::Epoch33,
+ false
+ )
+ .expect("execution should succeed")
+ .expect("should return a value")
+ );
+}
+
+
fn secp256r1_vectors() -> (Vec<u8>, Vec<u8>, Vec<u8>) {
let privk = Secp256r1PrivateKey::from_seed(&[7u8; 32]);
let pubk = Secp256r1PublicKey::from_private(&privk);That test fails: But you can see that it would have passed with pre-hash verification. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
This is a consensus breaking change and will need to go out in a hard-fork with a new version of Clarity (Clarity5). In the existing implementation, the code was incorrectly hashing the passed in
message-hash. Unfortunately, the test code was also double-hashing on the signing side as well, so this was not caught. The bug was discovered by @eric-stacks when attempting to generate signatures externally (e.g. with WebAuthn) and verify them in Clarity code. The docs are updated to match the actual behavior and will need to be updated again once Clarity 5 is added.