From c3f7d637be870b42cb76b5b281af95961158f6ff Mon Sep 17 00:00:00 2001 From: sydhds Date: Wed, 9 Oct 2024 17:41:05 +0200 Subject: [PATCH 1/7] Add additional verification for v & s value in evm_signature_verify --- massa-execution-worker/src/interface_impl.rs | 30 +++++++++++++++++-- massa-execution-worker/src/tests/interface.rs | 26 ++++++++++++++++ 2 files changed, 53 insertions(+), 3 deletions(-) diff --git a/massa-execution-worker/src/interface_impl.rs b/massa-execution-worker/src/interface_impl.rs index 1ccd389f3d4..a4bfdd298f5 100644 --- a/massa-execution-worker/src/interface_impl.rs +++ b/massa-execution-worker/src/interface_impl.rs @@ -885,7 +885,7 @@ impl Interface for InterfaceImpl { // parse the public key let public_key = libsecp256k1::PublicKey::parse_slice( public_key_, - Some(libsecp256k1::PublicKeyFormat::Raw), + None, )?; // build the message @@ -899,9 +899,33 @@ impl Interface for InterfaceImpl { // r is the R.x value of the signature's R point (32 bytes) // s is the signature proof for R.x (32 bytes) // v is a recovery parameter used to ease the signature verification (1 byte) - // we ignore the recovery parameter here // see test_evm_verify for an example of its usage - let signature = libsecp256k1::Signature::parse_standard_slice(&signature_[..64])?; + let recovery_id: u8 = libsecp256k1::RecoveryId::parse_rpc(signature_[64])? + .into(); + // Note: parse_rpc returns p - 27 and allow for 27, 28, 29, 30 + // restrict to only 27 & 28 (=> 0 & 1) + if recovery_id != 0 && recovery_id != 1 { + // Note: + // The v value in an EVM signature serves as a recovery ID, + // aiding in the recovery of the public key from the signature. + // Typically, v should be either 27 or 28 + // (or sometimes 0 or 1, depending on the implementation). + // Ensuring that v is within the expected range is crucial + // for correctly recovering the public key. + // the Ethereum yellow paper specifies only 27 and 28, requiring additional checks. + return Err(anyhow!("invalid recovery id value (v = {recovery_id}) in evm_signature_verify")); + } + + let mut signature = libsecp256k1::Signature::parse_standard_slice(&signature_[..64])?; + + // Note: + // The s value in an EVM signature should be in the lower half of the elliptic curve + // in order to prevent malleability attacks. + // If s is in the high-order range, it can be converted to its low-order equivalent, + // which should be enforced during signature verification. + if signature.s.is_high() { + signature.s = -signature.s; + } // verify the signature Ok(libsecp256k1::verify(&message, &signature, &public_key)) diff --git a/massa-execution-worker/src/tests/interface.rs b/massa-execution-worker/src/tests/interface.rs index 506ab937f7a..dba9c9c1b8c 100644 --- a/massa-execution-worker/src/tests/interface.rs +++ b/massa-execution-worker/src/tests/interface.rs @@ -17,3 +17,29 @@ fn test_hash_sha256() { &hex!("3fc9b689459d738f8c88a3a48aa9e33542016b7a4052e001aaa536fca74813cb")[..]; assert_eq!(actual_hash, expected_hash); } + +#[test] +fn test_evm_signature_verify() { + + let interface = InterfaceImpl::new_default( + Address::from_str("AU12cMW9zRKFDS43Z2W88VCmdQFxmHjAo54XvuVV34UzJeXRLXW9M").unwrap(), + None, + ); + + let _address = hex!("807a7bb5193edf9898b9092c1597bb966fe52514"); + let message_ = b"test"; + let mut signature_ = hex!("d0d05c35080635b5e865006c6c4f5b5d457ec342564d8fc67ce40edc264ccdab3f2f366b5bd1e38582538fed7fa6282148e86af97970a10cb3302896f5d68ef51b"); + let private_key_ = hex!("ed6602758bdd68dc9df67a6936ed69807a74b8cc89bdc18f3939149d02db17f3"); + + // build original public key + let private_key = libsecp256k1::SecretKey::parse_slice(&private_key_).unwrap(); + let public_key = libsecp256k1::PublicKey::from_secret_key(&private_key); + + let result = interface.evm_signature_verify(message_, &signature_, &public_key.serialize()); + assert!(result.is_ok()); + + // Invalid v + signature_[64] ^= 1; + let result = interface.evm_signature_verify(message_, &signature_, &public_key.serialize()); + assert!(result.is_err()); +} From 0ec21cb94d2c17b4df5be7abd2fc89b896be046f Mon Sep 17 00:00:00 2001 From: sydhds Date: Wed, 9 Oct 2024 17:44:23 +0200 Subject: [PATCH 2/7] Cargo fmt pass --- massa-execution-worker/src/interface_impl.rs | 12 +++++------- massa-execution-worker/src/tests/interface.rs | 1 - 2 files changed, 5 insertions(+), 8 deletions(-) diff --git a/massa-execution-worker/src/interface_impl.rs b/massa-execution-worker/src/interface_impl.rs index a4bfdd298f5..775044b3b3a 100644 --- a/massa-execution-worker/src/interface_impl.rs +++ b/massa-execution-worker/src/interface_impl.rs @@ -883,10 +883,7 @@ impl Interface for InterfaceImpl { } // parse the public key - let public_key = libsecp256k1::PublicKey::parse_slice( - public_key_, - None, - )?; + let public_key = libsecp256k1::PublicKey::parse_slice(public_key_, None)?; // build the message let prefix = format!("\x19Ethereum Signed Message:\n{}", message_.len()); @@ -900,8 +897,7 @@ impl Interface for InterfaceImpl { // s is the signature proof for R.x (32 bytes) // v is a recovery parameter used to ease the signature verification (1 byte) // see test_evm_verify for an example of its usage - let recovery_id: u8 = libsecp256k1::RecoveryId::parse_rpc(signature_[64])? - .into(); + let recovery_id: u8 = libsecp256k1::RecoveryId::parse_rpc(signature_[64])?.into(); // Note: parse_rpc returns p - 27 and allow for 27, 28, 29, 30 // restrict to only 27 & 28 (=> 0 & 1) if recovery_id != 0 && recovery_id != 1 { @@ -913,7 +909,9 @@ impl Interface for InterfaceImpl { // Ensuring that v is within the expected range is crucial // for correctly recovering the public key. // the Ethereum yellow paper specifies only 27 and 28, requiring additional checks. - return Err(anyhow!("invalid recovery id value (v = {recovery_id}) in evm_signature_verify")); + return Err(anyhow!( + "invalid recovery id value (v = {recovery_id}) in evm_signature_verify" + )); } let mut signature = libsecp256k1::Signature::parse_standard_slice(&signature_[..64])?; diff --git a/massa-execution-worker/src/tests/interface.rs b/massa-execution-worker/src/tests/interface.rs index dba9c9c1b8c..6e3c82e6acb 100644 --- a/massa-execution-worker/src/tests/interface.rs +++ b/massa-execution-worker/src/tests/interface.rs @@ -20,7 +20,6 @@ fn test_hash_sha256() { #[test] fn test_evm_signature_verify() { - let interface = InterfaceImpl::new_default( Address::from_str("AU12cMW9zRKFDS43Z2W88VCmdQFxmHjAo54XvuVV34UzJeXRLXW9M").unwrap(), None, From ab49cb3e99198e24172ac3f19225a3026b485d0d Mon Sep 17 00:00:00 2001 From: sydhds Date: Thu, 10 Oct 2024 11:18:52 +0200 Subject: [PATCH 3/7] Add additional verification for v & s value in evm_get_pubkey_from_signature --- massa-execution-worker/src/interface_impl.rs | 25 +++++++++-- massa-execution-worker/src/tests/interface.rs | 44 ++++++++++++++++++- 2 files changed, 64 insertions(+), 5 deletions(-) diff --git a/massa-execution-worker/src/interface_impl.rs b/massa-execution-worker/src/interface_impl.rs index 775044b3b3a..44d3cf4d0ab 100644 --- a/massa-execution-worker/src/interface_impl.rs +++ b/massa-execution-worker/src/interface_impl.rs @@ -963,16 +963,33 @@ impl Interface for InterfaceImpl { } // parse the message - let message = libsecp256k1::Message::parse_slice(hash_).unwrap(); + let message = libsecp256k1::Message::parse_slice(hash_)?; // parse the signature as being (r, s, v) use only r and s - let signature = libsecp256k1::Signature::parse_standard_slice(&signature_[..64]).unwrap(); + let signature = libsecp256k1::Signature::parse_standard_slice(&signature_[..64])?; + + // Note: + // See evm_signature_verify explanation + if signature.s.is_high() { + return Err(anyhow!( + "High-Order s Value are prohibited in evm_get_pubkey_from_signature" + )); + } // parse v as a recovery id - let recovery_id = libsecp256k1::RecoveryId::parse_rpc(signature_[64]).unwrap(); + let recovery_id = libsecp256k1::RecoveryId::parse_rpc(signature_[64])?; + + let recovery_id_: u8 = recovery_id.into(); + if recovery_id_ != 0 && recovery_id_ != 1 { + // Note: + // See evm_signature_verify explanation + return Err(anyhow!( + "invalid recovery id value (v = {recovery_id_}) in evm_get_pubkey_from_signature" + )); + } // recover the public key - let recovered = libsecp256k1::recover(&message, &signature, &recovery_id).unwrap(); + let recovered = libsecp256k1::recover(&message, &signature, &recovery_id)?; // return its serialized value Ok(recovered.serialize().to_vec()) diff --git a/massa-execution-worker/src/tests/interface.rs b/massa-execution-worker/src/tests/interface.rs index 6e3c82e6acb..35fb0688aa3 100644 --- a/massa-execution-worker/src/tests/interface.rs +++ b/massa-execution-worker/src/tests/interface.rs @@ -1,11 +1,15 @@ // Copyright (c) 2022 MASSA LABS +use std::str::FromStr; + use hex_literal::hex; +use sha2::Digest; + use massa_models::address::Address; use massa_sc_runtime::Interface; -use std::str::FromStr; use crate::interface_impl::InterfaceImpl; + #[test] fn test_hash_sha256() { let interface = InterfaceImpl::new_default( @@ -42,3 +46,41 @@ fn test_evm_signature_verify() { let result = interface.evm_signature_verify(message_, &signature_, &public_key.serialize()); assert!(result.is_err()); } + +#[test] +fn test_evm_get_pubkey_from_signature() { + let interface = InterfaceImpl::new_default( + Address::from_str("AU12cMW9zRKFDS43Z2W88VCmdQFxmHjAo54XvuVV34UzJeXRLXW9M").unwrap(), + None, + ); + + // let _address = hex!("807a7bb5193edf9898b9092c1597bb966fe52514"); + let message_ = b"test"; + let mut signature_ = hex!("d0d05c35080635b5e865006c6c4f5b5d457ec342564d8fc67ce40edc264ccdab3f2f366b5bd1e38582538fed7fa6282148e86af97970a10cb3302896f5d68ef51b"); + let private_key_ = hex!("ed6602758bdd68dc9df67a6936ed69807a74b8cc89bdc18f3939149d02db17f3"); + + // build original public key + let private_key = libsecp256k1::SecretKey::parse_slice(&private_key_).unwrap(); + let public_key = libsecp256k1::PublicKey::from_secret_key(&private_key); + + // build the hash + let prefix = format!("\x19Ethereum Signed Message:\n{}", message_.len()); + let to_hash = [prefix.as_bytes(), message_].concat(); + let full_hash = sha3::Keccak256::digest(to_hash); + + let result = interface.evm_get_pubkey_from_signature(&full_hash, &signature_); + assert!(result.is_ok()); + assert_eq!(public_key.serialize(), result.unwrap().as_ref()); + + // Invalid s + let mut signature_2 = libsecp256k1::Signature::parse_standard_slice(&signature_[..64]).unwrap(); + signature_2.s = -signature_2.s; + let result = interface.evm_get_pubkey_from_signature(&full_hash, &signature_); + assert!(result.is_err()); + + // Invalid v + signature_[64] ^= 1; + let result = interface.evm_get_pubkey_from_signature(&full_hash, &signature_); + assert!(result.is_err()); + +} From 6e1f7b28146bb71b9b7b76228fd9ddd05be2151a Mon Sep 17 00:00:00 2001 From: sydhds Date: Thu, 10 Oct 2024 11:19:26 +0200 Subject: [PATCH 4/7] Cargo fmt pass --- massa-execution-worker/src/tests/interface.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/massa-execution-worker/src/tests/interface.rs b/massa-execution-worker/src/tests/interface.rs index 35fb0688aa3..bc5005c55b0 100644 --- a/massa-execution-worker/src/tests/interface.rs +++ b/massa-execution-worker/src/tests/interface.rs @@ -82,5 +82,4 @@ fn test_evm_get_pubkey_from_signature() { signature_[64] ^= 1; let result = interface.evm_get_pubkey_from_signature(&full_hash, &signature_); assert!(result.is_err()); - } From fe854aedb7378850b913e6612f7f9683202b5f40 Mon Sep 17 00:00:00 2001 From: sydhds Date: Thu, 10 Oct 2024 11:29:44 +0200 Subject: [PATCH 5/7] Isolate some tests --- massa-execution-worker/src/tests/interface.rs | 35 ++++++++++++------- 1 file changed, 23 insertions(+), 12 deletions(-) diff --git a/massa-execution-worker/src/tests/interface.rs b/massa-execution-worker/src/tests/interface.rs index bc5005c55b0..c91e8b19370 100644 --- a/massa-execution-worker/src/tests/interface.rs +++ b/massa-execution-worker/src/tests/interface.rs @@ -31,7 +31,7 @@ fn test_evm_signature_verify() { let _address = hex!("807a7bb5193edf9898b9092c1597bb966fe52514"); let message_ = b"test"; - let mut signature_ = hex!("d0d05c35080635b5e865006c6c4f5b5d457ec342564d8fc67ce40edc264ccdab3f2f366b5bd1e38582538fed7fa6282148e86af97970a10cb3302896f5d68ef51b"); + let signature_ = hex!("d0d05c35080635b5e865006c6c4f5b5d457ec342564d8fc67ce40edc264ccdab3f2f366b5bd1e38582538fed7fa6282148e86af97970a10cb3302896f5d68ef51b"); let private_key_ = hex!("ed6602758bdd68dc9df67a6936ed69807a74b8cc89bdc18f3939149d02db17f3"); // build original public key @@ -42,9 +42,13 @@ fn test_evm_signature_verify() { assert!(result.is_ok()); // Invalid v - signature_[64] ^= 1; - let result = interface.evm_signature_verify(message_, &signature_, &public_key.serialize()); - assert!(result.is_err()); + { + let mut signature_2_ = signature_.clone(); + signature_2_[64] ^= 1; + let result = + interface.evm_signature_verify(message_, &signature_2_, &public_key.serialize()); + assert!(result.is_err()); + } } #[test] @@ -56,7 +60,7 @@ fn test_evm_get_pubkey_from_signature() { // let _address = hex!("807a7bb5193edf9898b9092c1597bb966fe52514"); let message_ = b"test"; - let mut signature_ = hex!("d0d05c35080635b5e865006c6c4f5b5d457ec342564d8fc67ce40edc264ccdab3f2f366b5bd1e38582538fed7fa6282148e86af97970a10cb3302896f5d68ef51b"); + let signature_ = hex!("d0d05c35080635b5e865006c6c4f5b5d457ec342564d8fc67ce40edc264ccdab3f2f366b5bd1e38582538fed7fa6282148e86af97970a10cb3302896f5d68ef51b"); let private_key_ = hex!("ed6602758bdd68dc9df67a6936ed69807a74b8cc89bdc18f3939149d02db17f3"); // build original public key @@ -73,13 +77,20 @@ fn test_evm_get_pubkey_from_signature() { assert_eq!(public_key.serialize(), result.unwrap().as_ref()); // Invalid s - let mut signature_2 = libsecp256k1::Signature::parse_standard_slice(&signature_[..64]).unwrap(); - signature_2.s = -signature_2.s; - let result = interface.evm_get_pubkey_from_signature(&full_hash, &signature_); - assert!(result.is_err()); + { + let mut signature_2 = + libsecp256k1::Signature::parse_standard_slice(&signature_[..64]).unwrap(); + signature_2.s = -signature_2.s; + assert!(signature_2.s.is_high()); + let result = interface.evm_get_pubkey_from_signature(&full_hash, &signature_2.serialize()); + assert!(result.is_err()); + } // Invalid v - signature_[64] ^= 1; - let result = interface.evm_get_pubkey_from_signature(&full_hash, &signature_); - assert!(result.is_err()); + { + let mut signature_2_ = signature_.clone(); + signature_2_[64] ^= 1; + let result = interface.evm_get_pubkey_from_signature(&full_hash, &signature_2_); + assert!(result.is_err()); + } } From c75e8d8268955556356235276a5bbffca58017ef Mon Sep 17 00:00:00 2001 From: sydhds Date: Thu, 10 Oct 2024 15:21:11 +0200 Subject: [PATCH 6/7] Cargo clippy fixes --- massa-execution-worker/src/tests/interface.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/massa-execution-worker/src/tests/interface.rs b/massa-execution-worker/src/tests/interface.rs index c91e8b19370..6ada4d6a134 100644 --- a/massa-execution-worker/src/tests/interface.rs +++ b/massa-execution-worker/src/tests/interface.rs @@ -43,7 +43,7 @@ fn test_evm_signature_verify() { // Invalid v { - let mut signature_2_ = signature_.clone(); + let mut signature_2_ = signature_; signature_2_[64] ^= 1; let result = interface.evm_signature_verify(message_, &signature_2_, &public_key.serialize()); @@ -88,7 +88,7 @@ fn test_evm_get_pubkey_from_signature() { // Invalid v { - let mut signature_2_ = signature_.clone(); + let mut signature_2_ = signature_; signature_2_[64] ^= 1; let result = interface.evm_get_pubkey_from_signature(&full_hash, &signature_2_); assert!(result.is_err()); From 3478bbf511af81bed4c37481c188927c1bf07b6e Mon Sep 17 00:00:00 2001 From: sydhds Date: Thu, 10 Oct 2024 17:06:46 +0200 Subject: [PATCH 7/7] Reject signature if signature.s is high order --- massa-execution-worker/src/interface_impl.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/massa-execution-worker/src/interface_impl.rs b/massa-execution-worker/src/interface_impl.rs index 44d3cf4d0ab..65ae41b08ae 100644 --- a/massa-execution-worker/src/interface_impl.rs +++ b/massa-execution-worker/src/interface_impl.rs @@ -914,7 +914,7 @@ impl Interface for InterfaceImpl { )); } - let mut signature = libsecp256k1::Signature::parse_standard_slice(&signature_[..64])?; + let signature = libsecp256k1::Signature::parse_standard_slice(&signature_[..64])?; // Note: // The s value in an EVM signature should be in the lower half of the elliptic curve @@ -922,7 +922,9 @@ impl Interface for InterfaceImpl { // If s is in the high-order range, it can be converted to its low-order equivalent, // which should be enforced during signature verification. if signature.s.is_high() { - signature.s = -signature.s; + return Err(anyhow!( + "High-Order s Value are prohibited in evm_get_pubkey_from_signature" + )); } // verify the signature