Skip to content

Commit 74ed427

Browse files
Dfinity-skaestleDfinity-skaestle
committed
Merge branch 'composite-query/sec-review' into 'master'
FOLLOW-1027: Composite query: Disallow some syscalls from non-relicated reply + reject callbacks See merge request dfinity-lab/public/ic!11894
2 parents 5f875bb + 70b4dee commit 74ed427

File tree

5 files changed

+203
-24
lines changed

5 files changed

+203
-24
lines changed

‎rs/execution_environment/src/query_handler/tests.rs

Lines changed: 83 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1494,3 +1494,86 @@ fn composite_query_chained_calls() {
14941494
.unwrap();
14951495
assert_eq!(result, WasmResult::Reply(b));
14961496
}
1497+
1498+
#[test]
1499+
fn composite_query_syscalls_from_reply_reject_callback() {
1500+
// In this test canister 0 calls canisters 1 and attempts syscalls from reply callback.
1501+
let mut test = ExecutionTestBuilder::new().with_composite_queries().build();
1502+
1503+
// Install two universal canisters
1504+
let mut canisters = vec![];
1505+
for _ in 0..2 {
1506+
canisters.push(test.universal_canister_with_cycles(CYCLES_BALANCE).unwrap());
1507+
}
1508+
1509+
let reply = wasm().reply_data(&[1]).build();
1510+
let reject = wasm().reject().build();
1511+
1512+
let syscalls = vec![
1513+
(wasm().msg_cycles_available().build(), "cycles_available"),
1514+
(wasm().msg_cycles_refunded().build(), "cycles_refunded"),
1515+
(wasm().msg_cycles_accept(42).build(), "cycles_accept"),
1516+
(wasm().api_global_timer_set(0).build(), "global_timer_set"),
1517+
(wasm().call_cycles_add(0).build(), "call_cycles_add"),
1518+
(
1519+
wasm().call_cycles_add128(0, 0).build(),
1520+
"call_cycles_add128",
1521+
),
1522+
(
1523+
wasm().msg_cycles_available128().build(),
1524+
"cycles_available128",
1525+
),
1526+
(
1527+
wasm().msg_cycles_refunded128().build(),
1528+
"cycles_refunded128",
1529+
),
1530+
(
1531+
wasm().msg_cycles_accept128(4, 2).build(),
1532+
"cycles_accept128",
1533+
),
1534+
(
1535+
wasm().certified_data_set(&[42]).build(),
1536+
"certified_data_set",
1537+
),
1538+
];
1539+
1540+
for (other_side, callback_type) in vec![(reply, "reply"), (reject, "reject")] {
1541+
for (syscall, label) in &syscalls {
1542+
let canister_0 = wasm().composite_query(
1543+
canisters[1],
1544+
call_args()
1545+
.other_side(other_side.clone())
1546+
.on_reply(syscall.clone())
1547+
.on_reject(syscall.clone()),
1548+
);
1549+
1550+
let output = test.query(
1551+
UserQuery {
1552+
source: user_test_id(2),
1553+
receiver: canisters[0],
1554+
method_name: "composite_query".to_string(),
1555+
method_payload: canister_0.build(),
1556+
ingress_expiry: 0,
1557+
nonce: None,
1558+
},
1559+
Arc::new(test.state().clone()),
1560+
vec![],
1561+
);
1562+
match output {
1563+
Ok(_) => {
1564+
unreachable!(
1565+
"{} call should not be allowed from a composite query {} callback",
1566+
label, callback_type
1567+
)
1568+
}
1569+
Err(err) => assert_eq!(
1570+
err.code(),
1571+
ErrorCode::CanisterContractViolation,
1572+
"Incorrect return code for {} {}",
1573+
label,
1574+
callback_type
1575+
),
1576+
}
1577+
}
1578+
}
1579+
}

‎rs/interfaces/src/execution_environment.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -273,7 +273,7 @@ impl ops::Div<i64> for SubnetAvailableMemory {
273273
}
274274
}
275275

276-
#[derive(Clone, Debug, Deserialize, Serialize)]
276+
#[derive(Clone, Debug, Deserialize, Serialize, Eq, PartialEq)]
277277
pub enum ExecutionMode {
278278
Replicated,
279279
NonReplicated,

‎rs/system_api/src/lib.rs

Lines changed: 66 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -1006,22 +1006,30 @@ impl SystemApiImpl {
10061006
}
10071007
| ApiType::RejectCallback {
10081008
outgoing_request, ..
1009-
} => match outgoing_request {
1010-
None => Err(HypervisorError::ContractViolation(format!(
1011-
"{} called when no call is under construction.",
1012-
method_name
1013-
))),
1014-
Some(request) => {
1015-
self.sandbox_safe_system_state
1016-
.withdraw_cycles_for_transfer(
1017-
self.memory_usage.current_usage,
1018-
self.execution_parameters.compute_allocation,
1019-
amount,
1020-
)?;
1021-
request.add_cycles(amount);
1022-
Ok(())
1009+
} => {
1010+
// Reply and reject callbacks can be executed in non-replicated mode
1011+
// iff from within a composite query call. Always disallow in that case.
1012+
if self.execution_parameters.execution_mode == ExecutionMode::NonReplicated {
1013+
return Err(self.error_for(method_name));
10231014
}
1024-
},
1015+
1016+
match outgoing_request {
1017+
None => Err(HypervisorError::ContractViolation(format!(
1018+
"{} called when no call is under construction.",
1019+
method_name
1020+
))),
1021+
Some(request) => {
1022+
self.sandbox_safe_system_state
1023+
.withdraw_cycles_for_transfer(
1024+
self.memory_usage.current_usage,
1025+
self.execution_parameters.compute_allocation,
1026+
amount,
1027+
)?;
1028+
request.add_cycles(amount);
1029+
Ok(())
1030+
}
1031+
}
1032+
}
10251033
}
10261034
}
10271035

@@ -1062,9 +1070,17 @@ impl SystemApiImpl {
10621070
}
10631071
| ApiType::RejectCallback {
10641072
call_context_id, ..
1065-
} => Ok(self
1066-
.sandbox_safe_system_state
1067-
.msg_cycles_available(*call_context_id)),
1073+
} => {
1074+
if self.execution_parameters.execution_mode == ExecutionMode::NonReplicated {
1075+
// Non-replicated mode means we are handling a composite query.
1076+
// Access to this syscall not permitted.
1077+
Err(self.error_for(method_name))
1078+
} else {
1079+
Ok(self
1080+
.sandbox_safe_system_state
1081+
.msg_cycles_available(*call_context_id))
1082+
}
1083+
}
10681084
}
10691085
}
10701086

@@ -1084,7 +1100,15 @@ impl SystemApiImpl {
10841100
}
10851101
| ApiType::RejectCallback {
10861102
incoming_cycles, ..
1087-
} => Ok(*incoming_cycles),
1103+
} => {
1104+
if self.execution_parameters.execution_mode == ExecutionMode::NonReplicated {
1105+
// Execution callback in non-replicated mode means we are handling a composite query.
1106+
// Access to this syscall not permitted.
1107+
Err(self.error_for(method_name))
1108+
} else {
1109+
Ok(*incoming_cycles)
1110+
}
1111+
}
10881112
}
10891113
}
10901114

@@ -1118,9 +1142,17 @@ impl SystemApiImpl {
11181142
}
11191143
| ApiType::RejectCallback {
11201144
call_context_id, ..
1121-
} => Ok(self
1122-
.sandbox_safe_system_state
1123-
.msg_cycles_accept(*call_context_id, max_amount)),
1145+
} => {
1146+
if self.execution_parameters.execution_mode == ExecutionMode::NonReplicated {
1147+
// Non-replicated mode means we are handling a composite query.
1148+
// Access to this syscall not permitted.
1149+
Err(self.error_for(method_name))
1150+
} else {
1151+
Ok(self
1152+
.sandbox_safe_system_state
1153+
.msg_cycles_accept(*call_context_id, max_amount))
1154+
}
1155+
}
11241156
}
11251157
}
11261158

@@ -2306,6 +2338,12 @@ impl SystemApi for SystemApiImpl {
23062338
| ApiType::PreUpgrade { .. }
23072339
| ApiType::ReplyCallback { .. }
23082340
| ApiType::RejectCallback { .. } => {
2341+
// Reply and reject callbacks can be executed in non-replicated mode
2342+
// iff from within a composite query call. Disallow in that case.
2343+
if self.execution_parameters.execution_mode == ExecutionMode::NonReplicated {
2344+
return Err(self.error_for("ic0_global_timer_set"));
2345+
}
2346+
23092347
let prev_time = self.sandbox_safe_system_state.global_timer().to_time();
23102348
self.sandbox_safe_system_state
23112349
.set_global_timer(CanisterTimer::from_time(time));
@@ -2672,6 +2710,12 @@ impl SystemApi for SystemApiImpl {
26722710
| ApiType::ReplyCallback { .. }
26732711
| ApiType::RejectCallback { .. }
26742712
| ApiType::PreUpgrade { .. } => {
2713+
// Reply and reject callbacks can be executed in non-replicated mode
2714+
// iff from within a composite query call. Disallow in that case.
2715+
if self.execution_parameters.execution_mode == ExecutionMode::NonReplicated {
2716+
return Err(self.error_for("ic0_certified_data_set"));
2717+
}
2718+
26752719
if size > CERTIFIED_DATA_MAX_LENGTH {
26762720
return Err(ContractViolation(format!(
26772721
"ic0_certified_data_set failed because the passed data must be \

‎rs/universal_canister/lib/src/lib.rs

Lines changed: 53 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ use universal_canister::Ops;
1717
/// `rs/universal_canister`.
1818
pub const UNIVERSAL_CANISTER_WASM: &[u8] = include_bytes!("universal-canister.wasm");
1919
pub const UNIVERSAL_CANISTER_WASM_SHA256: [u8; 32] =
20-
hex!("cf13dfde0aaf4139933651766943cdb8824b47fe37a099994ab7f524e75f7656");
20+
hex!("706a648d859d1a55b5722ebb3a35426928e8231a3c6177f4de6491e86231129b");
2121

2222
/// A succinct shortcut for creating a `PayloadBuilder`, which is used to encode
2323
/// instructions to be executed by the UC.
@@ -277,6 +277,19 @@ impl PayloadBuilder {
277277
self
278278
}
279279

280+
pub fn call_cycles_add128(mut self, high_amount: u64, low_amount: u64) -> Self {
281+
self = self.push_int64(high_amount);
282+
self = self.push_int64(low_amount);
283+
self.0.push(Ops::CallCyclesAdd128 as u8);
284+
self
285+
}
286+
287+
pub fn call_cycles_add(mut self, amount: u64) -> Self {
288+
self = self.push_int64(amount);
289+
self.0.push(Ops::CallCyclesAdd as u8);
290+
self
291+
}
292+
280293
pub fn message_payload(mut self) -> Self {
281294
self.0.push(Ops::MessagePayload as u8);
282295
self
@@ -433,6 +446,45 @@ impl PayloadBuilder {
433446
self
434447
}
435448

449+
pub fn msg_cycles_available(mut self) -> Self {
450+
self.0.push(Ops::CyclesAvailable as u8);
451+
self
452+
}
453+
454+
pub fn msg_cycles_available128(mut self) -> Self {
455+
self.0.push(Ops::CyclesAvailable128 as u8);
456+
self
457+
}
458+
459+
pub fn msg_cycles_refunded(mut self) -> Self {
460+
self.0.push(Ops::CyclesRefunded as u8);
461+
self
462+
}
463+
464+
pub fn msg_cycles_refunded128(mut self) -> Self {
465+
self.0.push(Ops::CyclesRefunded128 as u8);
466+
self
467+
}
468+
469+
pub fn msg_cycles_accept(mut self, max_amount: i64) -> Self {
470+
self = self.push_int64(max_amount as u64);
471+
self.0.push(Ops::AcceptCycles as u8);
472+
self
473+
}
474+
475+
pub fn msg_cycles_accept128(mut self, max_amount_hight: i64, max_amount_low: i64) -> Self {
476+
self = self.push_int64(max_amount_hight as u64);
477+
self = self.push_int64(max_amount_low as u64);
478+
self.0.push(Ops::AcceptCycles128 as u8);
479+
self
480+
}
481+
482+
pub fn certified_data_set(mut self, data: &[u8]) -> Self {
483+
self = self.push_bytes(data);
484+
self.0.push(Ops::CertifiedDataSet as u8);
485+
self
486+
}
487+
436488
/// Loops until the instruction counter is at least the specified amount.
437489
pub fn instruction_counter_is_at_least(mut self, amount: u64) -> Self {
438490
self = self.push_int64(amount);

‎rs/universal_canister/lib/src/universal-canister.wasm

29 Bytes
Binary file not shown.

0 commit comments

Comments
 (0)