Add additional verification for v & s value in evm_signature_verify (#…

…4762)

* Add additional verification for v & s value in evm_signature_verify

* Cargo fmt pass

* Add additional verification for v & s value in evm_get_pubkey_from_signature

* Cargo fmt pass

* Isolate some tests

* Cargo clippy fixes

* Reject signature if signature.s is high order

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_,
-            Some(libsecp256k1::PublicKeyFormat::Raw),
-        )?;
+        let public_key = libsecp256k1::PublicKey::parse_slice(public_key_, None)?;
 
         // build the message
         let prefix = format!("\x19Ethereum Signed Message:\n{}", message_.len());
@@ -899,10 +896,37 @@ 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 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 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() {
+            return Err(anyhow!(
+                "High-Order s Value are prohibited in evm_get_pubkey_from_signature"
+            ));
+        }
+
         // verify the signature
         Ok(libsecp256k1::verify(&message, &signature, &public_key))
     }
@@ -941,16 +965,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())

massa-execution-worker/src/tests/interface.rs

@@ -1,11 +1,15 @@
 // Copyright (c) 2022 MASSA LABS <[email protected]>
 
+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(
@@ -17,3 +21,76 @@ 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 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
+    {
+        let mut signature_2_ = signature_;
+        signature_2_[64] ^= 1;
+        let result =
+            interface.evm_signature_verify(message_, &signature_2_, &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 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;
+        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
+    {
+        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());
+    }
+}