Merge branch 'dderler/malicious-test' into 'master'

System test: ensure certifications with maliciously altered root hash are rejected

 

See merge request dfinity-lab/public/ic!10870

rs/config/src/config_sample.rs

@@ -332,6 +332,7 @@ pub const SAMPLE_CONFIG: &str = r#"
          maliciously_corrupt_own_state_at_heights: [],
          maliciously_disable_ingress_validation: false,
          maliciously_corrupt_ecdsa_dealings: false,
+         maliciously_alter_certified_hash: false,
        },
     },
 

rs/state_manager/src/lib.rs

@@ -100,6 +100,12 @@ const LABEL_COPY_CHUNKS: &str = "copy_chunks";
 const LABEL_PREALLOCATE: &str = "preallocate";
 const LABEL_STATE_SYNC_MAKE_CHECKPOINT: &str = "state_sync_make_checkpoint";
 
+/// Labels for slice validation metrics
+const LABEL_VERIFY_SIG: &str = "verify";
+const LABEL_CMP_HASH: &str = "compare";
+const LABEL_VALUE_SUCCESS: &str = "success";
+const LABEL_VALUE_FAILURE: &str = "failure";
+
 #[derive(Clone)]
 pub struct StateManagerMetrics {
     state_manager_error_count: IntCounterVec,
@@ -118,6 +124,7 @@ pub struct StateManagerMetrics {
     checkpoint_metrics: CheckpointMetrics,
     manifest_metrics: ManifestMetrics,
     tip_handler_queue_length: IntGauge,
+    decode_slice_status: IntCounterVec,
 }
 
 #[derive(Clone)]
@@ -293,6 +300,20 @@ impl StateManagerMetrics {
             "Length of TipChannel queue.",
         );
 
+        let decode_slice_status = metrics_registry.int_counter_vec(
+            "state_manager_decode_slice",
+            "Statuses of slice decoding.",
+            &["op", "status"],
+        );
+
+        // Initialize all `decode_slice_status` counters with zero, so they are all
+        // exported from process start (`IntCounterVec` is really a map).
+        for op in &[LABEL_VERIFY_SIG, LABEL_CMP_HASH] {
+            for status in &[LABEL_VALUE_SUCCESS, LABEL_VALUE_FAILURE] {
+                decode_slice_status.with_label_values(&[op, status]);
+            }
+        }
+
         Self {
             state_manager_error_count,
             checkpoint_op_duration,
@@ -310,8 +331,32 @@ impl StateManagerMetrics {
             checkpoint_metrics: CheckpointMetrics::new(metrics_registry),
             manifest_metrics: ManifestMetrics::new(metrics_registry),
             tip_handler_queue_length,
+            decode_slice_status,
         }
     }
+
+    /// Records a decode slice status for `label`.
+    fn observe_decode_slice(&self, operation: &str, success: bool) {
+        let status = if success {
+            LABEL_VALUE_SUCCESS
+        } else {
+            LABEL_VALUE_FAILURE
+        };
+        self.decode_slice_status
+            .with_label_values(&[operation, status])
+            .inc();
+    }
+
+    /// Records a decode slice status for a signature verification.
+    fn observe_decode_slice_signature_verification(&self, success: bool) {
+        self.observe_decode_slice(LABEL_VERIFY_SIG, success);
+    }
+
+    /// Records a decode slice status for a comparison of a tree's root
+    /// hash and the hash in the corresponding certification.
+    fn observe_decode_slice_hash_comparison(&self, success: bool) {
+        self.observe_decode_slice(LABEL_CMP_HASH, success);
+    }
 }
 
 impl ManifestMetrics {
@@ -3133,11 +3178,28 @@ impl CertifiedStreamStore for StateManagerImpl {
             certification: &Certification,
             registry_version: RegistryVersion,
             digest: Digest,
+            metrics: &StateManagerMetrics,
+            #[allow(unused_variables)] log: &ReplicaLogger,
+            #[allow(unused_variables)] malicious_flags: &MaliciousFlags,
         ) -> bool {
-            crypto_hash_of_partial_state(&digest) == certification.signed.content.hash
-                && verifier
-                    .validate(remote_subnet, certification, registry_version)
-                    .is_ok()
+            #[cfg(feature = "malicious_code")]
+            let certification = &maliciously_alter_certified_hash(
+                certification.clone(),
+                malicious_flags,
+                &remote_subnet,
+                log,
+            );
+
+            let hash_matches =
+                crypto_hash_of_partial_state(&digest) == certification.signed.content.hash;
+            let verification_status =
+                verifier.validate(remote_subnet, certification, registry_version);
+            let signature_verifies = verification_status.is_ok();
+
+            metrics.observe_decode_slice_hash_comparison(hash_matches);
+            metrics.observe_decode_slice_signature_verification(signature_verifies);
+
+            hash_matches && signature_verifies
         }
 
         let tree = stream_encoding::decode_labeled_tree(&certified_slice.payload)?;
@@ -3167,6 +3229,9 @@ impl CertifiedStreamStore for StateManagerImpl {
             &certified_slice.certification,
             registry_version,
             digest,
+            &self.metrics,
+            &self.log,
+            &self.malicious_flags,
         ) {
             return Err(DecodeStreamError::InvalidSignature(remote_subnet));
         }
@@ -3189,6 +3254,25 @@ impl CertifiedStreamStore for StateManagerImpl {
     }
 }
 
+#[cfg(feature = "malicious_code")]
+fn maliciously_alter_certified_hash(
+    mut certification: Certification,
+    malicious_flags: &MaliciousFlags,
+    remote_subnet: &SubnetId,
+    log: &ReplicaLogger,
+) -> Certification {
+    if malicious_flags.maliciously_alter_certified_hash {
+        info!(
+            log,
+            "[MALICIOUS] Corrupting root hash of certification at height {} in stream slice from {}",
+            certification.height,
+            remote_subnet
+        );
+        certification.signed.content.hash = CryptoHashOfPartialState::from(CryptoHash(vec![0; 32]));
+    }
+    certification
+}
+
 #[derive(Clone, Debug, PartialEq, Eq, Hash)]
 pub enum CheckpointError {
     /// Wraps a stringified `std::io::Error`, a message and the path of the

rs/tests/BUILD.bazel

@@ -499,6 +499,18 @@ system_test(
     deps = DEPENDENCIES + [":tests"],
 )
 
+system_test(
+    name = "xnet_malicious_slices",
+    proc_macro_deps = MACRO_DEPENDENCIES,
+    tags = [
+        "system_test_nightly",
+    ],
+    target_compatible_with = ["@platforms//os:linux"],  # requires libssh that does not build on Mac OS
+    test_timeout = "long",
+    runtime_deps = GUESTOS_MALICIOUS_RUNTIME_DEPS + GRAFANA_RUNTIME_DEPS + ["//rs/rust_canisters/xnet_test:xnet-test-canister"],
+    deps = DEPENDENCIES + [":tests"],
+)
+
 system_test(
     name = "sr_app_same_nodes_test",
     proc_macro_deps = MACRO_DEPENDENCIES,

rs/tests/Cargo.toml

@@ -481,6 +481,10 @@ path = "bin/update_workload_large_payload.rs"
 name = "ic-systest-canister-sig-verification-cache-test"
 path = "bin/canister_sig_verification_cache_test.rs"
 
+[[bin]]
+name = "ic-systest-xnet-malicious-slices"
+path = "bin/xnet_malicious_slices.rs"
+
 [[bin]]
 name = "ic-systest-canister-global-reboot-test"
 path = "bin/global_reboot_test.rs"

rs/tests/bin/xnet_malicious_slices.rs

@@ -0,0 +1,23 @@
+#[rustfmt::skip]
+
+use anyhow::Result;
+
+use ic_tests::driver::new::group::SystemTestGroup;
+use ic_tests::message_routing::malicious_slices::Config;
+use ic_tests::systest;
+use std::time::Duration;
+
+const PER_TASK_TIMEOUT: Duration = Duration::from_secs(5 * 60);
+const OVERALL_TIMEOUT: Duration = Duration::from_secs(10 * 60);
+
+fn main() -> Result<()> {
+    let config = Config::new();
+    let test = config.clone().test();
+    SystemTestGroup::new()
+        .with_setup(config.build())
+        .add_test(systest!(test))
+        .with_timeout_per_test(PER_TASK_TIMEOUT) // each task (including the setup function) may take up to `per_task_timeout`.
+        .with_overall_timeout(OVERALL_TIMEOUT) // the entire group may take up to `overall_timeout`.
+        .execute_from_args()?;
+    Ok(())
+}