For contract verification: don't rely on Wasm hash in metadata (#1820)

* For contract verification: don't rely on Wasm hash in metadata

* Improve user output

* Check if reference contract `source.hash` matches `hash(source.wasm)`

* Adapt existing test to new user output format

* Update changelog

* Apply `cargo fmt`

CHANGELOG.md

@@ -10,6 +10,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Fix "chain configuration not found" error - [#1786](https://github.com/paritytech/cargo-contract/pull/1786)
 - Fix "chain configuration not found" error (Rust 1.79) - [#1821](https://github.com/paritytech/cargo-contract/pull/1821)
 - Validate externally passed Rust toolchain identifiers - [#1817](https://github.com/paritytech/cargo-contract/pull/1817)
+- For contract verification: don't rely on Wasm hash in metadata - [#1820](https://github.com/paritytech/cargo-contract/pull/1820)
 
 ## [5.0.0-alpha]
 

crates/cargo-contract/src/cmd/verify.rs

@@ -22,6 +22,7 @@ use colored::Colorize;
 use contract_build::{
     code_hash,
     execute,
+    util::decode_hex,
     verbose_eprintln,
     BuildArtifacts,
     BuildInfo,
@@ -248,8 +249,21 @@ impl VerifyCommand {
         let build_result = execute(args)?;
 
         // 4. Grab the code hash from the built contract and compare it with the reference
-        //    one.
-        let reference_code_hash = metadata.source.hash;
+        //    code hash.
+        //
+        //    We compute the hash of the reference code here, instead of relying on
+        //    the `source.hash` field in the metadata. This is because the `source.hash`
+        //    field could have been manipulated; we want to be sure that _the code_ of
+        //    both contracts is equal.
+        let reference_wasm_blob = decode_hex(
+            &metadata
+                .source
+                .wasm
+                .expect("no source.wasm field exists in metadata")
+                .to_string(),
+        )
+        .expect("decoding the source.wasm hex failed");
+        let reference_code_hash = CodeHash(code_hash(&reference_wasm_blob));
         let built_contract_path = if let Some(m) = build_result.metadata_result {
             m
         } else {
@@ -280,16 +294,33 @@ impl VerifyCommand {
         if reference_code_hash != target_code_hash {
             verbose_eprintln!(
                 verbosity,
-                "Expected Code Hash: '{}'\n\nGot Code Hash: `{}`",
+                "Expected code hash in reference contract ({}): {}\nGot Code Hash: {}\n",
+                &path.display(),
                 &reference_code_hash,
                 &target_code_hash
             );
             anyhow::bail!(format!(
-                "\nFailed to verify the authenticity of {} contract against the workspace \n\
-                found at {}.",
-                format!("`{}`", metadata.contract.name).bright_white(),
-                format!("{:?}", manifest_path.as_ref()).bright_white()).bright_red()
+                "\nFailed to verify `{}` against the workspace at `{}`: the hashed Wasm blobs are not matching.",
+                format!("{}", &path.display()).bright_white(),
+                format!("{}", manifest_path.as_ref().display()).bright_white()
+            )
+            .bright_red());
+        }
+
+        // check that the metadata hash is the same as reference_code_hash
+        if reference_code_hash != metadata.source.hash {
+            verbose_eprintln!(
+                verbosity,
+                "Expected code hash in reference metadata ({}): {}\nGot Code Hash: {}\n",
+                &path.display(),
+                &reference_code_hash,
+                &metadata.source.hash
             );
+            anyhow::bail!(format!(
+                "\nThe reference contract `{}` metadata is corrupt: the source.hash does not match the source.wasm hash.",
+                format!("{}", &path.display()).bright_white()
+            )
+            .bright_red());
         }
 
         Ok(VerificationResult {

crates/cargo-contract/tests/verify.rs

@@ -14,7 +14,15 @@
 // You should have received a copy of the GNU General Public License
 // along with cargo-contract.  If not, see <http://www.gnu.org/licenses/>.
 
-use std::path::Path;
+use serde_json::{
+    Map,
+    Value,
+};
+use std::path::{
+    Path,
+    PathBuf,
+};
+use tempfile::TempDir;
 
 /// Create a `cargo contract` command
 fn cargo_contract<P: AsRef<Path>>(path: P) -> assert_cmd::Command {
@@ -243,7 +251,8 @@ fn verify_different_contracts() {
         .expect("Failed to write wasm binary to the current dir!");
 
     // when
-    let output: &str = r#"Failed to verify the authenticity of `incrementer`"#;
+    let output: &str = "Failed to verify `reference.contract` against the workspace at \
+                        `Cargo.toml`: the hashed Wasm blobs are not matching.";
 
     // then
     cargo_contract(&project_dir)
@@ -267,3 +276,134 @@ fn verify_different_contracts() {
         .failure()
         .stderr(predicates::str::contains(output));
 }
+
+#[test]
+fn verify_must_fail_on_manipulated_wasm_code() {
+    // given
+    let tmp_dir = tempfile::Builder::new()
+        .prefix("cargo-contract.cli.test.")
+        .tempdir()
+        .expect("temporary directory creation failed");
+    let (project_dir, mut metadata_json) = create_and_compile_minimal_contract(&tmp_dir);
+
+    // when
+    // we change the `source.wasm` blob to a different contract bytecode, but the hash
+    // will remain the same as the one from our compiled minimal contract.
+    let source = metadata_json
+        .get_mut("source")
+        .expect("source field not found in metadata");
+    let wasm = source
+        .get_mut("wasm")
+        .expect("source.wasm field not found in metadata");
+    *wasm = Value::String(String::from("0x00"));
+
+    let contract_file =
+        project_dir.join("contract_with_mismatching_wasm_hash_and_code.contract");
+    let metadata = serde_json::to_string_pretty(&metadata_json)
+        .expect("failed converting metadata to json");
+    std::fs::write(contract_file, metadata)
+        .expect("Failed to write bundle contract to the current dir!");
+
+    // then
+    let output: &str = "Failed to verify `contract_with_mismatching_wasm_hash_and_code.contract` \
+                        against the workspace at `Cargo.toml`: the hashed Wasm blobs are not \
+                        matching.";
+    cargo_contract(&project_dir)
+        .arg("verify")
+        .arg("--contract")
+        .arg("contract_with_mismatching_wasm_hash_and_code.contract")
+        .arg("--output-json")
+        .assert()
+        .failure()
+        .stderr(predicates::str::contains(output));
+}
+
+#[test]
+fn verify_must_fail_on_corrupt_hash() {
+    // given
+    let tmp_dir = tempfile::Builder::new()
+        .prefix("cargo-contract.cli.test.")
+        .tempdir()
+        .expect("temporary directory creation failed");
+    let (project_dir, mut metadata_json) = create_and_compile_minimal_contract(&tmp_dir);
+
+    // when
+    // we change the `source.hash` value to a different hash
+    let source = metadata_json
+        .get_mut("source")
+        .expect("source field not found in metadata");
+    let wasm = source
+        .get_mut("hash")
+        .expect("source.hash field not found in metadata");
+    *wasm = Value::String(String::from(
+        "0x0000000000000000000000000000000000000000000000000000000000000000",
+    ));
+
+    let contract_file = project_dir.join("contract_with_corrupt_hash.contract");
+    let metadata = serde_json::to_string_pretty(&metadata_json)
+        .expect("failed converting metadata to json");
+    std::fs::write(contract_file, metadata)
+        .expect("Failed to write bundle contract to the current dir!");
+
+    // then
+    let output: &str = "The reference contract `contract_with_corrupt_hash.contract` \
+                        metadata is corrupt: the source.hash does not match the source.wasm hash.";
+    cargo_contract(&project_dir)
+        .arg("verify")
+        .arg("--contract")
+        .arg("contract_with_corrupt_hash.contract")
+        .arg("--output-json")
+        .assert()
+        .failure()
+        .stderr(predicates::str::contains(output));
+}
+
+// Creates a minimal contract in `tmp_dir` and compiles it.
+//
+// Returns a tuple of:
+//  * the workspace folder within `tmp_dir` and
+//  * the metadata contained in the `.contract` file that build.
+fn create_and_compile_minimal_contract(
+    tmp_dir: &TempDir,
+) -> (PathBuf, Map<String, Value>) {
+    let contract = r#"
+    #![cfg_attr(not(feature = "std"), no_std, no_main)]
+
+    #[ink::contract]
+    mod minimal {
+        #[ink(storage)]
+        pub struct Minimal {}
+
+        impl Minimal {
+            #[ink(constructor)]
+            pub fn new() -> Self {
+                Self { }
+            }
+
+            #[ink(message)]
+            pub fn void(&self) { }
+        }
+    }"#;
+    cargo_contract(tmp_dir.path())
+        .arg("new")
+        .arg("minimal")
+        .assert()
+        .success();
+    let project_dir = tmp_dir.path().to_path_buf().join("minimal");
+    let lib = project_dir.join("lib.rs");
+    std::fs::write(lib, contract).expect("Failed to write contract lib.rs");
+
+    tracing::debug!("Building contract in {}", project_dir.to_string_lossy());
+    cargo_contract(&project_dir)
+        .arg("build")
+        .arg("--release")
+        .assert()
+        .success();
+
+    let bundle_path = project_dir.join("target/ink/minimal.contract");
+    let bundle = std::fs::read(bundle_path)
+        .expect("Failed to read the content of the contract bundle!");
+    let metadata_json: Map<String, Value> = serde_json::from_slice(&bundle).unwrap();
+
+    (project_dir, metadata_json)
+}