From 2ce3a45f4d9ada7f05441a690a766ddf79f086f0 Mon Sep 17 00:00:00 2001 From: Chris Smith <1979423+chris13524@users.noreply.github.com> Date: Wed, 17 Apr 2024 12:49:38 -0400 Subject: [PATCH] fix: string slice panic & automated smart contract tests (#76) --- .github/workflows/ci.yaml | 9 +- Cargo.toml | 3 + README.md | 4 + blockchain_api/Cargo.toml | 5 +- contracts/Eip1271Mock.sol | 81 ++++++++++++++ relay_client/Cargo.toml | 3 + relay_rpc/Cargo.toml | 5 + relay_rpc/src/auth/cacao.rs | 4 + .../src/auth/cacao/signature/eip1271/mod.rs | 103 +++++++++++++++++- relay_rpc/src/auth/cacao/signature/eip191.rs | 80 ++++++++++++-- relay_rpc/src/auth/cacao/signature/mod.rs | 9 +- .../src/auth/cacao/signature/test_helpers.rs | 79 ++++++++++++++ relay_rpc/src/jwt.rs | 21 +++- 13 files changed, 385 insertions(+), 21 deletions(-) create mode 100644 contracts/Eip1271Mock.sol create mode 100644 relay_rpc/src/auth/cacao/signature/test_helpers.rs diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index de9a1c6..312e617 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -35,7 +35,7 @@ jobs: rust: nightly - name: "Tests" cmd: nextest - args: run --workspace --all-features --features cacao --retries 3 + args: run --workspace --all-features rust: stable - name: "Documentation Tests" cmd: test @@ -54,6 +54,13 @@ jobs: profile: default override: true + - name: Install Foundry + uses: foundry-rs/foundry-toolchain@v1 + + # pre-build contracts to avoid race condition installing solc during `forge create` in tests + - name: Build contracts + run: forge build -C contracts --cache-path=target/.forge/cache --out=target/.forge/out + - uses: Swatinem/rust-cache@v2 - uses: taiki-e/install-action@v1 diff --git a/Cargo.toml b/Cargo.toml index 64927a2..7857ebc 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -41,3 +41,6 @@ required-features = ["client", "rpc"] [[example]] name = "webhook" required-features = ["client", "rpc"] + +[lints.clippy] +indexing_slicing = "deny" diff --git a/README.md b/README.md index d004292..973fd77 100644 --- a/README.md +++ b/README.md @@ -15,6 +15,10 @@ The core Relay client. Provides access to all available Relay RPC methods to bui Provides all of the Relay domain types (e.g. `ClientId`, `ProjectId` etc.) as well as auth token generation and validation functionality. +### Test dependencies + +Foundry is required to be installed to your system for testing: + # License [Apache License (Version 2.0)](LICENSE) diff --git a/blockchain_api/Cargo.toml b/blockchain_api/Cargo.toml index c7f6515..e11aa42 100644 --- a/blockchain_api/Cargo.toml +++ b/blockchain_api/Cargo.toml @@ -4,9 +4,12 @@ version = "0.1.0" edition = "2021" [dependencies] -relay_rpc = { path = "../relay_rpc" } +relay_rpc = { path = "../relay_rpc", features = ["cacao"] } reqwest = { version = "0.12.2", features = ["json"] } serde = "1.0" tokio = { version = "1.0", features = ["test-util", "macros"] } tracing = "0.1.40" url = "2" + +[lints.clippy] +indexing_slicing = "deny" diff --git a/contracts/Eip1271Mock.sol b/contracts/Eip1271Mock.sol new file mode 100644 index 0000000..6a50659 --- /dev/null +++ b/contracts/Eip1271Mock.sol @@ -0,0 +1,81 @@ +pragma solidity ^0.8.25; + +// https://eips.ethereum.org/EIPS/eip-1271#reference-implementation + +contract Eip1271Mock { + address owner; + + constructor() { + owner = msg.sender; + } + + /** + * @notice Verifies that the signer is the owner of the signing contract. + */ + function isValidSignature( + bytes32 _hash, + bytes calldata _signature + ) external view returns (bytes4) { + // Validate signatures + if (recoverSigner(_hash, _signature) == owner) { + return 0x1626ba7e; + } else { + return 0xffffffff; + } + } + + /** + * @notice Recover the signer of hash, assuming it's an EOA account + * @dev Only for EthSign signatures + * @param _hash Hash of message that was signed + * @param _signature Signature encoded as (bytes32 r, bytes32 s, uint8 v) + */ + function recoverSigner( + bytes32 _hash, + bytes memory _signature + ) internal pure returns (address signer) { + require(_signature.length == 65, "SignatureValidator#recoverSigner: invalid signature length"); + + // Variables are not scoped in Solidity. + uint8 v = uint8(_signature[64]); + bytes32 r; + bytes32 s; + assembly { + // Slice the signature into r and s components + r := mload(add(_signature, 32)) + s := mload(add(_signature, 64)) + } + + // EIP-2 still allows signature malleability for ecrecover(). Remove this possibility and make the signature + // unique. Appendix F in the Ethereum Yellow paper (https://ethereum.github.io/yellowpaper/paper.pdf), defines + // the valid range for s in (281): 0 < s < secp256k1n ÷ 2 + 1, and for v in (282): v ∈ {27, 28}. Most + // signatures from current libraries generate a unique signature with an s-value in the lower half order. + // + // If your library generates malleable signatures, such as s-values in the upper range, calculate a new s-value + // with 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFEBAAEDCE6AF48A03BBFD25E8CD0364141 - s1 and flip v from 27 to 28 or + // vice versa. If your library also generates signatures with 0/1 for v instead 27/28, add 27 to v to accept + // these malleable signatures as well. + // + // Source OpenZeppelin + // https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/cryptography/ECDSA.sol + + if (uint256(s) > 0x7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF5D576E7357A4501DDFE92F46681B20A0) { + revert("SignatureValidator#recoverSigner: invalid signature 's' value"); + } + + if (v != 27 && v != 28) { + revert("SignatureValidator#recoverSigner: invalid signature 'v' value"); + } + + // Recover ECDSA signer + signer = ecrecover(_hash, v, r, s); + + // Prevent signer from being 0x0 + require( + signer != address(0x0), + "SignatureValidator#recoverSigner: INVALID_SIGNER" + ); + + return signer; + } +} diff --git a/relay_client/Cargo.toml b/relay_client/Cargo.toml index aae75a1..3451cf3 100644 --- a/relay_client/Cargo.toml +++ b/relay_client/Cargo.toml @@ -28,3 +28,6 @@ tokio-tungstenite = "0.21.0" futures-channel = "0.3" tokio-stream = "0.1" tokio-util = "0.7" + +[lints.clippy] +indexing_slicing = "deny" diff --git a/relay_rpc/Cargo.toml b/relay_rpc/Cargo.toml index 0306fb8..a60bcc8 100644 --- a/relay_rpc/Cargo.toml +++ b/relay_rpc/Cargo.toml @@ -15,6 +15,7 @@ cacao = [ "dep:alloy-json-abi", "dep:alloy-sol-types", "dep:alloy-primitives", + "dep:alloy-node-bindings" ] [dependencies] @@ -48,6 +49,7 @@ alloy-transport = { git = "https://github.com/alloy-rs/alloy.git", rev = "e6f98e alloy-transport-http = { git = "https://github.com/alloy-rs/alloy.git", rev = "e6f98e1", optional = true } alloy-rpc-types = { git = "https://github.com/alloy-rs/alloy.git", rev = "e6f98e1", optional = true } alloy-json-rpc = { git = "https://github.com/alloy-rs/alloy.git", rev = "e6f98e1", optional = true } +alloy-node-bindings = { git = "https://github.com/alloy-rs/alloy.git", rev = "e6f98e1", optional = true } alloy-json-abi = { version = "0.6.2", optional = true } alloy-sol-types = { version = "0.6.2", optional = true } alloy-primitives = { version = "0.6.2", optional = true } @@ -55,3 +57,6 @@ strum = { version = "0.26", features = ["strum_macros", "derive"] } [dev-dependencies] tokio = { version = "1.35.1", features = ["test-util", "macros"] } + +[lints.clippy] +indexing_slicing = "deny" diff --git a/relay_rpc/src/auth/cacao.rs b/relay_rpc/src/auth/cacao.rs index babe697..e215716 100644 --- a/relay_rpc/src/auth/cacao.rs +++ b/relay_rpc/src/auth/cacao.rs @@ -4,6 +4,7 @@ use { payload::Payload, signature::{eip1271::get_rpc_url::GetRpcUrl, Signature}, }, + alloy_primitives::hex::FromHexError, core::fmt::Debug, serde::{Deserialize, Serialize}, serde_json::value::RawValue, @@ -32,6 +33,9 @@ pub enum CacaoError { #[error("Invalid address")] AddressInvalid, + #[error("Address not EIP-191")] + AddressNotEip191(FromHexError), + #[error("EIP-1271 signatures not supported")] Eip1271NotSupported, diff --git a/relay_rpc/src/auth/cacao/signature/eip1271/mod.rs b/relay_rpc/src/auth/cacao/signature/eip1271/mod.rs index 50125ba..99eed65 100644 --- a/relay_rpc/src/auth/cacao/signature/eip1271/mod.rs +++ b/relay_rpc/src/auth/cacao/signature/eip1271/mod.rs @@ -56,8 +56,13 @@ pub async fn verify_eip1271( } })?; - if result[..4] == MAGIC_VALUE.to_be_bytes().to_vec() { - Ok(true) + let magic = result.get(..4); + if let Some(magic) = magic { + if magic == MAGIC_VALUE.to_be_bytes().to_vec() { + Ok(true) + } else { + Err(CacaoError::Verification) + } } else { Err(CacaoError::Verification) } @@ -67,8 +72,13 @@ pub async fn verify_eip1271( mod test { use { super::*, - crate::auth::cacao::signature::{eip191::eip191_bytes, strip_hex_prefix}, + crate::auth::cacao::signature::{ + eip191::eip191_bytes, + strip_hex_prefix, + test_helpers::{deploy_contract, message_hash, sign_message, spawn_anvil}, + }, alloy_primitives::address, + k256::ecdsa::SigningKey, sha3::{Digest, Keccak256}, }; @@ -76,7 +86,7 @@ mod test { // function #[tokio::test] #[ignore] - async fn test_eip1271() { + async fn test_eip1271_manual() { let address = address!("AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"); let signature = "xxx"; let signature = data_encoding::HEXLOWER_PERMISSIVE @@ -94,4 +104,89 @@ mod test { .await .unwrap()); } + + #[tokio::test] + async fn test_eip1271_pass() { + let (_anvil, rpc_url, private_key) = spawn_anvil().await; + let contract_address = deploy_contract(&rpc_url, &private_key).await; + + let message = "xxx"; + let signature = sign_message(message, &private_key); + + assert!( + verify_eip1271(signature, contract_address, &message_hash(message), rpc_url) + .await + .unwrap() + ); + } + + #[tokio::test] + async fn test_eip1271_wrong_signature() { + let (_anvil, rpc_url, private_key) = spawn_anvil().await; + let contract_address = deploy_contract(&rpc_url, &private_key).await; + + let message = "xxx"; + let mut signature = sign_message(message, &private_key); + *signature.first_mut().unwrap() = signature.first().unwrap().wrapping_add(1); + + assert!(matches!( + verify_eip1271(signature, contract_address, &message_hash(message), rpc_url).await, + Err(CacaoError::Verification) + )); + } + + #[tokio::test] + async fn test_eip1271_fail_wrong_signer() { + let (anvil, rpc_url, private_key) = spawn_anvil().await; + let contract_address = deploy_contract(&rpc_url, &private_key).await; + + let message = "xxx"; + let signature = sign_message( + message, + &SigningKey::from_bytes(&anvil.keys().get(1).unwrap().to_bytes()).unwrap(), + ); + + assert!(matches!( + verify_eip1271(signature, contract_address, &message_hash(message), rpc_url).await, + Err(CacaoError::Verification) + )); + } + + #[tokio::test] + async fn test_eip1271_fail_wrong_contract_address() { + let (_anvil, rpc_url, private_key) = spawn_anvil().await; + let mut contract_address = deploy_contract(&rpc_url, &private_key).await; + + *contract_address.0.first_mut().unwrap() = + contract_address.0.first().unwrap().wrapping_add(1); + + let message = "xxx"; + let signature = sign_message(message, &private_key); + + assert!(matches!( + verify_eip1271(signature, contract_address, &message_hash(message), rpc_url).await, + Err(CacaoError::Verification) + )); + } + + #[tokio::test] + async fn test_eip1271_wrong_message() { + let (_anvil, rpc_url, private_key) = spawn_anvil().await; + let contract_address = deploy_contract(&rpc_url, &private_key).await; + + let message = "xxx"; + let signature = sign_message(message, &private_key); + + let message2 = "yyy"; + assert!(matches!( + verify_eip1271( + signature, + contract_address, + &message_hash(message2), + rpc_url + ) + .await, + Err(CacaoError::Verification) + )); + } } diff --git a/relay_rpc/src/auth/cacao/signature/eip191.rs b/relay_rpc/src/auth/cacao/signature/eip191.rs index 4cb180d..ba782df 100644 --- a/relay_rpc/src/auth/cacao/signature/eip191.rs +++ b/relay_rpc/src/auth/cacao/signature/eip191.rs @@ -1,6 +1,7 @@ use { super::CacaoError, crate::auth::cacao::signature::strip_hex_prefix, + alloy_primitives::Address, sha3::{Digest, Keccak256}, }; @@ -15,25 +16,88 @@ pub fn eip191_bytes(message: &str) -> Vec { .into() } -pub fn verify_eip191(signature: &[u8], address: &str, hash: Keccak256) -> Result { +pub fn verify_eip191( + signature: &[u8], + address: &Address, + hash: Keccak256, +) -> Result { use k256::ecdsa::{RecoveryId, Signature as Sig, VerifyingKey}; - let sig = Sig::try_from(&signature[..64]).map_err(|_| CacaoError::Verification)?; - let recovery_id = - RecoveryId::try_from(&signature[64] % 27).map_err(|_| CacaoError::Verification)?; + let sig = Sig::try_from(signature.get(..64).ok_or(CacaoError::Verification)?) + .map_err(|_| CacaoError::Verification)?; + let recovery_id = RecoveryId::try_from(signature.get(64).ok_or(CacaoError::Verification)? % 27) + .map_err(|_| CacaoError::Verification)?; let recovered_key = VerifyingKey::recover_from_digest(hash, &sig, recovery_id) .map_err(|_| CacaoError::Verification)?; - let add = &Keccak256::default() - .chain_update(&recovered_key.to_encoded_point(false).as_bytes()[1..]) - .finalize()[12..]; + let hash = Keccak256::default() + .chain_update( + recovered_key + .to_encoded_point(false) + .as_bytes() + .get(1..) + .ok_or(CacaoError::Verification)?, + ) + .finalize(); + let add = hash.get(12..).ok_or(CacaoError::Verification)?; let address_encoded = data_encoding::HEXLOWER_PERMISSIVE.encode(add); - if address_encoded.to_lowercase() != strip_hex_prefix(address).to_lowercase() { + if address_encoded.to_lowercase() != strip_hex_prefix(&address.to_string()).to_lowercase() { Err(CacaoError::Verification) } else { Ok(true) } } + +#[cfg(test)] +mod tests { + use { + crate::auth::cacao::signature::{ + eip191::verify_eip191, + test_helpers::{message_hash_internal, sign_message}, + }, + alloy_primitives::Address, + k256::ecdsa::SigningKey, + }; + + #[test] + fn test_eip191() { + let private_key = SigningKey::random(&mut rand::thread_rng()); + let message = "xxx"; + let signature = sign_message(message, &private_key); + let address = Address::from_private_key(&private_key); + assert!(verify_eip191(&signature, &address, message_hash_internal(message)).unwrap()); + } + + #[test] + fn test_eip191_wrong_signature() { + let private_key = SigningKey::random(&mut rand::thread_rng()); + let message = "xxx"; + let mut signature = sign_message(message, &private_key); + *signature.first_mut().unwrap() = signature.first().unwrap().wrapping_add(1); + let address = Address::from_private_key(&private_key); + assert!(verify_eip191(&signature, &address, message_hash_internal(message)).is_err()); + } + + #[test] + fn test_eip191_wrong_address() { + let private_key = SigningKey::random(&mut rand::thread_rng()); + let message = "xxx"; + let signature = sign_message(message, &private_key); + let mut address = Address::from_private_key(&private_key); + *address.0.first_mut().unwrap() = address.0.first().unwrap().wrapping_add(1); + assert!(verify_eip191(&signature, &address, message_hash_internal(message)).is_err()); + } + + #[test] + fn test_eip191_wrong_message() { + let private_key = SigningKey::random(&mut rand::thread_rng()); + let message = "xxx"; + let signature = sign_message(message, &private_key); + let address = Address::from_private_key(&private_key); + let message2 = "yyy"; + assert!(verify_eip191(&signature, &address, message_hash_internal(message2)).is_err()); + } +} diff --git a/relay_rpc/src/auth/cacao/signature/mod.rs b/relay_rpc/src/auth/cacao/signature/mod.rs index 89ff7a7..edec7b9 100644 --- a/relay_rpc/src/auth/cacao/signature/mod.rs +++ b/relay_rpc/src/auth/cacao/signature/mod.rs @@ -13,6 +13,9 @@ use { pub mod eip1271; pub mod eip191; +#[cfg(test)] +mod test_helpers; + #[derive(Clone, Debug, PartialEq, Eq, Deserialize, Serialize, Hash)] pub struct Signature { pub t: String, @@ -34,7 +37,11 @@ impl Signature { let hash = Keccak256::new_with_prefix(eip191_bytes(&cacao.siwe_message()?)); match self.t.as_str() { - EIP191 => verify_eip191(&signature, &address, hash), + EIP191 => verify_eip191( + &signature, + &address.parse().map_err(CacaoError::AddressNotEip191)?, + hash, + ), EIP1271 => { if let Some(provider) = provider { let chain_id = cacao.p.chain_id_reference()?; diff --git a/relay_rpc/src/auth/cacao/signature/test_helpers.rs b/relay_rpc/src/auth/cacao/signature/test_helpers.rs new file mode 100644 index 0000000..ac6ba36 --- /dev/null +++ b/relay_rpc/src/auth/cacao/signature/test_helpers.rs @@ -0,0 +1,79 @@ +use { + super::eip191::eip191_bytes, + alloy_node_bindings::{Anvil, AnvilInstance}, + alloy_primitives::Address, + k256::ecdsa::SigningKey, + regex::Regex, + sha2::Digest, + sha3::Keccak256, + std::process::Stdio, + tokio::process::Command, + url::Url, +}; + +pub async fn spawn_anvil() -> (AnvilInstance, Url, SigningKey) { + let anvil = Anvil::new().spawn(); + let provider = anvil.endpoint().parse().unwrap(); + let private_key = anvil.keys().first().unwrap().clone(); + ( + anvil, + provider, + SigningKey::from_bytes(&private_key.to_bytes()).unwrap(), + ) +} + +pub async fn deploy_contract(rpc_url: &Url, private_key: &SigningKey) -> Address { + let key_encoded = data_encoding::HEXLOWER_PERMISSIVE.encode(&private_key.to_bytes()); + let output = Command::new("forge") + .args([ + "create", + "--contracts=contracts", + "Eip1271Mock", + "--rpc-url", + rpc_url.as_str(), + "--private-key", + &key_encoded, + "--cache-path", + "target/.forge/cache", + "--out", + "target/.forge/out", + ]) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .spawn() + .unwrap() + .wait_with_output() + .await + .unwrap(); + println!("forge status: {:?}", output.status); + let stdout = String::from_utf8(output.stdout).unwrap(); + println!("forge stdout: {stdout:?}"); + let stderr = String::from_utf8(output.stderr).unwrap(); + println!("forge stderr: {stderr:?}"); + assert!(output.status.success()); + let (_, [contract_address]) = Regex::new("Deployed to: (0x[0-9a-fA-F]+)") + .unwrap() + .captures(&stdout) + .unwrap() + .extract(); + contract_address.parse().unwrap() +} + +pub fn sign_message(message: &str, private_key: &SigningKey) -> Vec { + let (signature, recovery): (k256::ecdsa::Signature, _) = private_key + .sign_digest_recoverable(message_hash_internal(message)) + .unwrap(); + let signature = signature.to_bytes(); + // need for +27 is mentioned in EIP-1271 reference implementation + [&signature[..], &[recovery.to_byte() + 27]].concat() +} + +pub fn message_hash_internal(message: &str) -> Keccak256 { + Keccak256::new_with_prefix(eip191_bytes(message)) +} + +pub fn message_hash(message: &str) -> [u8; 32] { + message_hash_internal(message).finalize()[..] + .try_into() + .unwrap() +} diff --git a/relay_rpc/src/jwt.rs b/relay_rpc/src/jwt.rs index d9a4bf4..120a99d 100644 --- a/relay_rpc/src/jwt.rs +++ b/relay_rpc/src/jwt.rs @@ -149,12 +149,17 @@ pub trait VerifyableClaims: Serialize + DeserializeOwned { // Decode header. data_encoding::BASE64URL_NOPAD - .decode_mut(header.as_bytes(), &mut output[..header_len]) + .decode_mut( + header.as_bytes(), + output.get_mut(..header_len).ok_or(JwtError::Encoding)?, + ) .map_err(|_| JwtError::Encoding)?; { - let header = serde_json::from_slice::(&output[..header_len]) - .map_err(JwtError::Serialization)?; + let header = serde_json::from_slice::( + output.get(..header_len).ok_or(JwtError::Encoding)?, + ) + .map_err(JwtError::Serialization)?; if !header.is_valid() { return Err(JwtError::Header); @@ -163,11 +168,15 @@ pub trait VerifyableClaims: Serialize + DeserializeOwned { // Decode claims. data_encoding::BASE64URL_NOPAD - .decode_mut(claims.as_bytes(), &mut output[..claims_len]) + .decode_mut( + claims.as_bytes(), + output.get_mut(..claims_len).ok_or(JwtError::Encoding)?, + ) .map_err(|_| JwtError::Encoding)?; - let claims = serde_json::from_slice::(&output[..claims_len]) - .map_err(JwtError::Serialization)?; + let claims = + serde_json::from_slice::(output.get(..claims_len).ok_or(JwtError::Encoding)?) + .map_err(JwtError::Serialization)?; let mut parts = data.rsplitn(2, JWT_DELIMITER);