Skip to content

Commit a7fbbd6

Browse files
ShuoWangNSLShuoWangNSL
committed
test: [MR-403] ensure malicious state sync chunks are rejected
1 parent 7812f96 commit a7fbbd6

File tree

17 files changed

+576
-53
lines changed

17 files changed

+576
-53
lines changed

‎rs/config/src/config_sample.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -324,6 +324,7 @@ pub const SAMPLE_CONFIG: &str = r#"
324324
maliciously_disable_ingress_validation: false,
325325
maliciously_corrupt_ecdsa_dealings: false,
326326
maliciously_alter_certified_hash: false,
327+
maliciously_alter_state_sync_chunk_sending_side: false,
327328
},
328329
},
329330

‎rs/p2p/state_sync_manager/tests/common.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -436,6 +436,7 @@ fn state_sync_artifact(id: StateSyncArtifactId) -> StateSyncMessage {
436436
manifest,
437437
meta_manifest: Arc::new(meta_manifest),
438438
state_sync_file_group: Default::default(),
439+
malicious_flags: Default::default(),
439440
}
440441
}
441442

‎rs/state_manager/src/lib.rs

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -118,6 +118,9 @@ const LABEL_COPY_FILES: &str = "copy_files";
118118
const LABEL_COPY_CHUNKS: &str = "copy_chunks";
119119
const LABEL_PREALLOCATE: &str = "preallocate";
120120
const LABEL_STATE_SYNC_MAKE_CHECKPOINT: &str = "state_sync_make_checkpoint";
121+
const LABEL_FETCH_META_MANIFEST_CHUNK: &str = "fetch_meta_manifest_chunk";
122+
const LABEL_FETCH_MANIFEST_CHUNK: &str = "fetch_manifest_chunk";
123+
const LABEL_FETCH_STATE_CHUNK: &str = "fetch_state_chunk";
121124

122125
/// Labels for slice validation metrics
123126
const LABEL_VERIFY_SIG: &str = "verify";
@@ -533,12 +536,18 @@ impl StateSyncMetrics {
533536

534537
let corrupted_chunks = metrics_registry.int_counter_vec(
535538
"state_sync_corrupted_chunks",
536-
"Number of chunks not copied during state sync due to hash mismatch by source ('fetch', copy_files', 'copy_chunks')",
539+
"Number of chunks not copied/applied during state sync due to hash mismatch by source ('copy_files', 'copy_chunks', 'fetch_meta_manifest_chunk', 'fetch_manifest_chunk', 'fetch_state_chunk')",
537540
&["source"],
538541
);
539542

540543
// Note [Metrics preallocation]
541-
for source in &[LABEL_FETCH, LABEL_COPY_FILES, LABEL_COPY_CHUNKS] {
544+
for source in &[
545+
LABEL_COPY_FILES,
546+
LABEL_COPY_CHUNKS,
547+
LABEL_FETCH_META_MANIFEST_CHUNK,
548+
LABEL_FETCH_MANIFEST_CHUNK,
549+
LABEL_FETCH_STATE_CHUNK,
550+
] {
542551
corrupted_chunks.with_label_values(&[*source]);
543552
}
544553

‎rs/state_manager/src/state_sync.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -87,6 +87,7 @@ impl StateSync {
8787
meta_manifest,
8888
manifest: manifest.clone(),
8989
state_sync_file_group,
90+
malicious_flags: self.state_manager.malicious_flags.clone(),
9091
})
9192
} else {
9293
None
@@ -137,6 +138,7 @@ impl StateSync {
137138
manifest: manifest.clone(),
138139
meta_manifest,
139140
state_sync_file_group: Default::default(),
141+
malicious_flags: self.state_manager.malicious_flags.clone(),
140142
};
141143
Some(StateSyncArtifactId {
142144
height: msg.height,

‎rs/state_manager/src/state_sync/chunkable.rs

Lines changed: 72 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ use crate::{
77
},
88
StateManagerMetrics, StateSyncMetrics, StateSyncRefs,
99
CRITICAL_ERROR_STATE_SYNC_CORRUPTED_CHUNKS, LABEL_COPY_CHUNKS, LABEL_COPY_FILES, LABEL_FETCH,
10+
LABEL_FETCH_MANIFEST_CHUNK, LABEL_FETCH_META_MANIFEST_CHUNK, LABEL_FETCH_STATE_CHUNK,
1011
LABEL_PREALLOCATE, LABEL_STATE_SYNC_MAKE_CHECKPOINT,
1112
};
1213
use ic_interfaces::p2p::state_sync::{
@@ -785,6 +786,7 @@ impl IncompleteState {
785786
// `state_sync_file_group` and `checkpoint_root` are not included in the integrity hash of this artifact.
786787
// Therefore it is OK to pass a default value here as it is only used when fetching chunks.
787788
state_sync_file_group: Default::default(),
789+
malicious_flags: Default::default(),
788790
}
789791
}
790792

@@ -1122,6 +1124,62 @@ impl IncompleteState {
11221124
}
11231125
}
11241126

1127+
#[cfg(feature = "malicious_code")]
1128+
fn maliciously_alter_chunk_data(
1129+
mut chunk: Chunk,
1130+
chunk_id: ChunkId,
1131+
malicious_flags: &mut MaliciousFlags,
1132+
) -> Chunk {
1133+
let allowance = match malicious_flags
1134+
.maliciously_alter_state_sync_chunk_receiving_side
1135+
.as_mut()
1136+
{
1137+
Some(allowance) => allowance,
1138+
None => {
1139+
return chunk;
1140+
}
1141+
};
1142+
1143+
let payload = chunk.clone();
1144+
1145+
let ix = chunk_id.get();
1146+
match state_sync_chunk_type(ix) {
1147+
StateSyncChunk::MetaManifestChunk => {
1148+
if allowance.meta_manifest_chunk_error_allowance == 0 {
1149+
return chunk;
1150+
}
1151+
allowance.meta_manifest_chunk_error_allowance -= 1;
1152+
let meta_manifest = match decode_meta_manifest(&payload) {
1153+
Ok(meta_manifest) => meta_manifest,
1154+
Err(_) => {
1155+
return chunk;
1156+
}
1157+
};
1158+
chunk = crate::state_sync::types::maliciously_alter_meta_manifest(meta_manifest);
1159+
}
1160+
StateSyncChunk::ManifestChunk(_) => {
1161+
if allowance.manifest_chunk_error_allowance == 0 {
1162+
return chunk;
1163+
}
1164+
allowance.manifest_chunk_error_allowance -= 1;
1165+
chunk = crate::state_sync::types::maliciously_alter_chunk_payload(payload);
1166+
}
1167+
_ => {
1168+
if allowance.state_chunk_error_allowance == 0 {
1169+
return chunk;
1170+
}
1171+
allowance.state_chunk_error_allowance -= 1;
1172+
chunk = crate::state_sync::types::maliciously_alter_chunk_payload(payload);
1173+
}
1174+
}
1175+
// Sleep for 15 seconds to allow the replica connecting to more peers for state sync.
1176+
// Otherwise, the first invalid chunk in the very beginning will immediately fail the state sync
1177+
// if there is only one peer connected.
1178+
// Note that this is only an issue for tests not the real system.
1179+
std::thread::sleep(std::time::Duration::from_secs(15));
1180+
chunk
1181+
}
1182+
11251183
impl Chunkable<StateSyncMessage> for IncompleteState {
11261184
fn chunks_to_download(&self) -> Box<dyn Iterator<Item = ChunkId>> {
11271185
match self.state {
@@ -1157,6 +1215,9 @@ impl Chunkable<StateSyncMessage> for IncompleteState {
11571215
chunk_id: ChunkId,
11581216
chunk: Chunk,
11591217
) -> Result<StateSyncMessage, ArtifactErrorCode> {
1218+
#[cfg(feature = "malicious_code")]
1219+
let chunk = maliciously_alter_chunk_data(chunk, chunk_id, &mut self.malicious_flags);
1220+
11601221
let ix = chunk_id.get();
11611222
let payload = &chunk;
11621223
match &mut self.state {
@@ -1183,6 +1244,11 @@ impl Chunkable<StateSyncMessage> for IncompleteState {
11831244
crate::manifest::validate_meta_manifest(&meta_manifest, &self.root_hash)
11841245
.map_err(|err| {
11851246
warn!(self.log, "Received invalid meta-manifest: {}", err);
1247+
self.metrics
1248+
.state_sync_metrics
1249+
.corrupted_chunks
1250+
.with_label_values(&[LABEL_FETCH_META_MANIFEST_CHUNK])
1251+
.inc();
11861252
ChunkVerificationFailed
11871253
})?;
11881254
let manifest_chunks_len = meta_manifest.sub_manifest_hashes.len();
@@ -1248,6 +1314,11 @@ impl Chunkable<StateSyncMessage> for IncompleteState {
12481314
)
12491315
.map_err(|err| {
12501316
warn!(self.log, "Received invalid sub-manifest: {}", err);
1317+
self.metrics
1318+
.state_sync_metrics
1319+
.corrupted_chunks
1320+
.with_label_values(&[LABEL_FETCH_MANIFEST_CHUNK])
1321+
.inc();
12511322
ChunkVerificationFailed
12521323
})?;
12531324
manifest_in_construction.insert(ix, payload.clone());
@@ -1442,7 +1513,7 @@ impl Chunkable<StateSyncMessage> for IncompleteState {
14421513
metrics
14431514
.state_sync_metrics
14441515
.corrupted_chunks
1445-
.with_label_values(&[LABEL_FETCH])
1516+
.with_label_values(&[LABEL_FETCH_STATE_CHUNK])
14461517
.inc();
14471518
ChunkVerificationFailed
14481519
})?;

‎rs/state_manager/src/state_sync/chunkable/cache/tests.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,7 @@ fn fake_complete() -> DownloadState {
8181
manifest,
8282
meta_manifest: Arc::new(meta_manifest),
8383
state_sync_file_group: Default::default(),
84+
malicious_flags: Default::default(),
8485
};
8586
DownloadState::Complete(Box::new(artifact))
8687
}

‎rs/state_manager/src/state_sync/types.rs

Lines changed: 50 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -82,7 +82,7 @@ pub mod proto;
8282
use ic_interfaces::p2p::state_sync::{Chunk, ChunkId};
8383
use ic_protobuf::{proxy::ProtoProxy, state::sync::v1 as pb};
8484
use ic_types::state_sync::StateSyncVersion;
85-
use ic_types::{CryptoHashOfState, Height};
85+
use ic_types::{malicious_flags::MaliciousFlags, CryptoHashOfState, Height};
8686
use serde::{Deserialize, Serialize};
8787
use std::{
8888
collections::BTreeMap,
@@ -436,6 +436,35 @@ pub struct StateSyncMessage {
436436
/// The manifest containing the summary of the content.
437437
pub manifest: Manifest,
438438
pub state_sync_file_group: Arc<FileGroupChunks>,
439+
pub malicious_flags: MaliciousFlags,
440+
}
441+
442+
#[cfg(feature = "malicious_code")]
443+
pub(crate) fn maliciously_alter_chunk_payload(mut payload: Vec<u8>) -> Vec<u8> {
444+
match payload.last_mut() {
445+
Some(last) => {
446+
// Alter the last element of chunk payload.
447+
*last = last.wrapping_add(1);
448+
}
449+
None => {
450+
// The chunk payload is empty. Set it to some non-empty value.
451+
payload = vec![1; 100];
452+
}
453+
}
454+
payload
455+
}
456+
457+
#[cfg(feature = "malicious_code")]
458+
pub(crate) fn maliciously_alter_meta_manifest(mut meta_manifest: MetaManifest) -> Vec<u8> {
459+
match meta_manifest.sub_manifest_hashes.last_mut() {
460+
Some(last) => {
461+
last[0] = last[0].wrapping_add(1);
462+
}
463+
None => {
464+
meta_manifest.sub_manifest_hashes.push([1; 32]);
465+
}
466+
}
467+
encode_meta_manifest(&meta_manifest)
439468
}
440469

441470
impl StateSyncMessage {
@@ -499,6 +528,26 @@ impl StateSyncMessage {
499528
}
500529
}
501530

531+
#[cfg(feature = "malicious_code")]
532+
{
533+
if self
534+
.malicious_flags
535+
.maliciously_alter_state_sync_chunk_sending_side
536+
{
537+
match state_sync_chunk_type(chunk_id.get()) {
538+
StateSyncChunk::MetaManifestChunk => {
539+
// If the chunk is for the meta-manifest, we alter its inner content and then encode it.
540+
payload =
541+
maliciously_alter_meta_manifest((*self.meta_manifest).clone());
542+
}
543+
_ => {
544+
// Otherwise, we alter the raw payload directly.
545+
payload = maliciously_alter_chunk_payload(payload);
546+
}
547+
}
548+
}
549+
}
550+
502551
Some(payload)
503552
}
504553
}

‎rs/tests/Cargo.toml

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -558,6 +558,10 @@ path = "crypto/rpc_csp_vault_reconnection_test.rs"
558558
name = "ic-systest-xnet-malicious-slices"
559559
path = "message_routing/xnet/xnet_malicious_slices.rs"
560560

561+
[[bin]]
562+
name = "ic-systest-state-sync-malicious-chunk-test"
563+
path = "message_routing/state_sync_malicious_chunk_test.rs"
564+
561565
[[bin]]
562566
name = "ic-systest-canister-global-reboot-test"
563567
path = "message_routing/global_reboot_test.rs"

‎rs/tests/message_routing/BUILD.bazel

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,3 +36,16 @@ system_test(
3636
runtime_deps = GUESTOS_RUNTIME_DEPS + GRAFANA_RUNTIME_DEPS + NNS_CANISTER_RUNTIME_DEPS + STATESYNC_TEST_CANISTER_RUNTIME_DEPS,
3737
deps = DEPENDENCIES + ["//rs/tests"],
3838
)
39+
40+
system_test(
41+
name = "state_sync_malicious_chunk_test",
42+
malicious = True,
43+
proc_macro_deps = MACRO_DEPENDENCIES,
44+
tags = [
45+
"system_test_nightly",
46+
],
47+
target_compatible_with = ["@platforms//os:linux"], # requires libssh that does not build on Mac OS
48+
test_timeout = "eternal",
49+
runtime_deps = ["//ic-os:scripts/build-bootstrap-config-image.sh"] + GRAFANA_RUNTIME_DEPS + NNS_CANISTER_RUNTIME_DEPS + STATESYNC_TEST_CANISTER_RUNTIME_DEPS,
50+
deps = DEPENDENCIES + ["//rs/tests"],
51+
)

‎rs/tests/message_routing/state_sync_malicious_chunk_test.rs

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
#[rustfmt::skip]
2+
3+
use anyhow::Result;
4+
5+
use ic_tests::driver::group::SystemTestGroup;
6+
use ic_tests::message_routing::state_sync_malicious_chunk::Config;
7+
use ic_tests::systest;
8+
use std::time::Duration;
9+
const PER_TASK_TIMEOUT: Duration = Duration::from_secs(3600 * 2);
10+
const OVERALL_TIMEOUT: Duration = Duration::from_secs(3600 * 2);
11+
const NUM_NODES: usize = 7;
12+
13+
fn main() -> Result<()> {
14+
let config = Config::new(NUM_NODES);
15+
let test = config.clone().test();
16+
SystemTestGroup::new()
17+
.with_setup(config.build())
18+
.add_test(systest!(test))
19+
.with_timeout_per_test(PER_TASK_TIMEOUT)
20+
.with_overall_timeout(OVERALL_TIMEOUT)
21+
.execute_from_args()?;
22+
Ok(())
23+
}

‎rs/tests/src/driver/ic.rs

Lines changed: 14 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -616,15 +616,23 @@ impl Subnet {
616616
}
617617

618618
pub fn add_malicious_nodes(
619-
mut self,
619+
self,
620620
no_of_nodes: usize,
621621
malicious_behaviour: MaliciousBehaviour,
622622
) -> Self {
623-
for _ in 0..no_of_nodes {
624-
let node = Node::new().with_malicious_behaviour(malicious_behaviour.clone());
625-
self.nodes.push(node);
626-
}
627-
self
623+
(0..no_of_nodes).fold(self, |subnet, _| {
624+
let default_vm_resources = subnet.default_vm_resources;
625+
let vm_allocation = subnet.vm_allocation.clone();
626+
let required_host_features = subnet.required_host_features.clone();
627+
subnet.add_node(
628+
Node::new_with_settings(
629+
default_vm_resources,
630+
vm_allocation,
631+
required_host_features,
632+
)
633+
.with_malicious_behaviour(malicious_behaviour.clone()),
634+
)
635+
})
628636
}
629637

630638
pub fn add_node_with_ipv4(self, ipv4_config: Ipv4Config) -> Self {

‎rs/tests/src/message_routing/mod.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ pub mod global_reboot_test;
22
pub mod malicious_slices;
33
pub mod rejoin_test;
44
pub mod rejoin_test_large_state;
5+
pub mod state_sync_malicious_chunk;
56
pub mod xnet_slo_test;
67

78
mod common {

0 commit comments

Comments
 (0)