Skip to content

Commit

Permalink
fix(core-processor): Force *_input syscalls to return an error on o…
Browse files Browse the repository at this point in the history
…ut of bounds `offset` and `len` params. (#4186)
  • Loading branch information
techraed authored Aug 28, 2024
1 parent 81fe12d commit d444bda
Show file tree
Hide file tree
Showing 8 changed files with 118 additions and 61 deletions.
12 changes: 12 additions & 0 deletions core-errors/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,16 @@ pub enum MessageError {
#[display(fmt = "Outgoing messages bytes limit exceeded")]
OutgoingMessagesBytesLimitExceeded = 311,

/// The error occurs when a wrong offset of the input buffer (currently executing message payload)
/// is provided.
#[display(fmt = "Offset value for the input payload is out of it's size bounds")]
OutOfBoundsInputSliceOffset = 312,

/// The error occurs when a too big length value to form a slice (range) of the input buffer
/// (currently executing message payload) is provided.
#[display(fmt = "Too big length value is set to form a slice (range) of the input buffer")]
OutOfBoundsInputSliceLength = 313,

// TODO: remove after delay refactoring is done
/// An error occurs in attempt to charge gas for dispatch stash hold.
#[display(fmt = "Not enough gas to hold dispatch message")]
Expand Down Expand Up @@ -268,6 +278,8 @@ impl ExtError {
309 => Some(MessageError::DuplicateReplyDeposit.into()),
310 => Some(MessageError::IncorrectMessageForReplyDeposit.into()),
311 => Some(MessageError::OutgoingMessagesBytesLimitExceeded.into()),
312 => Some(MessageError::OutOfBoundsInputSliceOffset.into()),
313 => Some(MessageError::OutOfBoundsInputSliceLength.into()),
399 => Some(MessageError::InsufficientGasForDelayedSending.into()),
//
500 => Some(ReservationError::InvalidReservationId.into()),
Expand Down
83 changes: 66 additions & 17 deletions core-processor/src/ext.rs
Original file line number Diff line number Diff line change
Expand Up @@ -949,7 +949,10 @@ impl<LP: LazyPagesInterface> Externalities for Ext<LP> {
offset: u32,
len: u32,
) -> Result<(), Self::FallibleError> {
let range = self.context.message_context.check_input_range(offset, len);
let range = self
.context
.message_context
.check_input_range(offset, len)?;

self.with_changes(|mutator| {
mutator.charge_gas_if_enough(
Expand Down Expand Up @@ -1085,7 +1088,7 @@ impl<LP: LazyPagesInterface> Externalities for Ext<LP> {
let range = mutator
.context
.message_context
.check_input_range(offset, len);
.check_input_range(offset, len)?;
mutator.charge_gas_if_enough(
mutator
.context
Expand Down Expand Up @@ -1753,7 +1756,12 @@ mod tests {
.build(),
);

let data = HandlePacket::default();
let res = ext
.context
.message_context
.payload_mut()
.try_extend_from_slice(&[1, 2, 3, 4, 5, 6]);
assert!(res.is_ok());

let fake_handle = 0;

Expand All @@ -1765,20 +1773,37 @@ mod tests {

let handle = ext.send_init().expect("Outgoing limit is u32::MAX");

let res = ext
.context
.message_context
.payload_mut()
.try_extend_from_slice(&[1, 2, 3, 4, 5, 6]);
assert!(res.is_ok());

let res = ext.send_push_input(handle, 2, 3);
assert!(res.is_ok());

let res = ext.send_push_input(handle, 8, 10);
let res = ext.send_push_input(handle, 5, 1);
assert!(res.is_ok());

let msg = ext.send_commit(handle, data, 0);
// Len too big
let res = ext.send_push_input(handle, 0, 7);
assert_eq!(
res.unwrap_err(),
FallibleExtError::Core(FallibleExtErrorCore::Message(
MessageError::OutOfBoundsInputSliceLength
))
);
let res = ext.send_push_input(handle, 5, 2);
assert_eq!(
res.unwrap_err(),
FallibleExtError::Core(FallibleExtErrorCore::Message(
MessageError::OutOfBoundsInputSliceLength
))
);

// Too big offset
let res = ext.send_push_input(handle, 6, 0);
assert_eq!(
res.unwrap_err(),
FallibleExtError::Core(FallibleExtErrorCore::Message(
MessageError::OutOfBoundsInputSliceOffset
))
);

let msg = ext.send_commit(handle, HandlePacket::default(), 0);
assert!(msg.is_ok());

let res = ext.send_push_input(handle, 0, 1);
Expand All @@ -1797,7 +1822,7 @@ mod tests {
.map(|(dispatch, _, _)| dispatch)
.expect("Send commit was ok");

assert_eq!(dispatch.message().payload_bytes(), &[3, 4, 5]);
assert_eq!(dispatch.message().payload_bytes(), &[3, 4, 5, 6]);
}

#[test]
Expand Down Expand Up @@ -1912,10 +1937,34 @@ mod tests {

let res = ext.reply_push_input(2, 3);
assert!(res.is_ok());

let res = ext.reply_push_input(8, 10);
let res = ext.reply_push_input(5, 1);
assert!(res.is_ok());

// Len too big
let res = ext.reply_push_input(0, 7);
assert_eq!(
res.unwrap_err(),
FallibleExtError::Core(FallibleExtErrorCore::Message(
MessageError::OutOfBoundsInputSliceLength
))
);
let res = ext.reply_push_input(5, 2);
assert_eq!(
res.unwrap_err(),
FallibleExtError::Core(FallibleExtErrorCore::Message(
MessageError::OutOfBoundsInputSliceLength
))
);

// Too big offset
let res = ext.reply_push_input(6, 0);
assert_eq!(
res.unwrap_err(),
FallibleExtError::Core(FallibleExtErrorCore::Message(
MessageError::OutOfBoundsInputSliceOffset
))
);

let msg = ext.reply_commit(ReplyPacket::auto());
assert!(msg.is_ok());

Expand All @@ -1935,7 +1984,7 @@ mod tests {
.map(|(dispatch, _, _)| dispatch)
.expect("Send commit was ok");

assert_eq!(dispatch.message().payload_bytes(), &[3, 4, 5]);
assert_eq!(dispatch.message().payload_bytes(), &[3, 4, 5, 6]);
}

// TODO: fix me (issue #3881)
Expand Down
27 changes: 15 additions & 12 deletions core/src/message/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -428,24 +428,27 @@ impl MessageContext {
/// limits. Result `CheckedRange` instance is accepted by
/// `send_push_input`/`reply_push_input` and has the method `len`
/// allowing to charge gas before the calls.
pub fn check_input_range(&self, offset: u32, len: u32) -> CheckedRange {
pub fn check_input_range(&self, offset: u32, len: u32) -> Result<CheckedRange, Error> {
let input = self.current.payload_bytes();
let offset = offset as usize;
let len = len as usize;

// Check `offset` is not out of bounds.
if offset >= input.len() {
return CheckedRange {
offset: 0,
excluded_end: 0,
};
return Err(Error::OutOfBoundsInputSliceOffset);
}

CheckedRange {
offset,
excluded_end: if len == 0 {
offset
} else {
offset.saturating_add(len as usize).min(input.len())
},
// Check `len` for the current `offset` doesn't refer to the slice out of intput bounds.
let available_len = input.len() - offset;
if len > available_len {
return Err(Error::OutOfBoundsInputSliceLength);
}

Ok(CheckedRange {
offset,
// guaranteed to be `<= input.len()`, because of the check upper
excluded_end: offset.saturating_add(len),
})
}

/// Send reply message.
Expand Down
19 changes: 10 additions & 9 deletions examples/proxy-relay/src/wasm.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ use gstd::msg::{self, MessageHandle};

static mut RELAY_CALL: Option<RelayCall> = None;

fn resend_push(resend_pushes: &[ResendPushData]) {
fn resend_push(resend_pushes: &[ResendPushData], size: usize) {
for data in resend_pushes {
let msg_handle = MessageHandle::init().expect("Failed to obtain new message handle");

Expand All @@ -35,7 +35,7 @@ fn resend_push(resend_pushes: &[ResendPushData]) {
match start.map(|s| s as usize) {
Some(s) => match end {
None => {
msg_handle.push_input(s..).expect("Push failed");
msg_handle.push_input(s..size).expect("Push failed");
}
Some((e, true)) => {
msg_handle.push_input(s..=e).expect("Push failed");
Expand All @@ -46,7 +46,7 @@ fn resend_push(resend_pushes: &[ResendPushData]) {
},
None => match end {
None => {
msg_handle.push_input(..).expect("Push failed");
msg_handle.push_input(..size).expect("Push failed");
}
Some((e, true)) => {
msg_handle.push_input(..=e).expect("Push failed");
Expand All @@ -67,26 +67,27 @@ fn resend_push(resend_pushes: &[ResendPushData]) {
extern "C" fn handle() {
use RelayCall::*;
let relay_call = unsafe { RELAY_CALL.as_ref().expect("Relay call is not initialized") };
let size = msg::size();

match relay_call {
Resend(d) => {
msg::send_input(*d, msg::value(), ..msg::size()).expect("Resend failed");
msg::send_input(*d, msg::value(), ..size).expect("Resend failed");
}
ResendWithGas(d, gas) => {
msg::send_input_with_gas(*d, *gas, msg::value(), ..).expect("Resend wgas failed");
msg::send_input_with_gas(*d, *gas, msg::value(), ..size).expect("Resend wgas failed");
}
ResendPush(data) => {
resend_push(data);
resend_push(data, size);
}
Rereply => {
msg::reply_input(msg::value(), 0..msg::size()).expect("Rereply failed");
msg::reply_input(msg::value(), 0..size).expect("Rereply failed");
}
RereplyPush => {
msg::reply_push_input(0..).expect("Push failed");
msg::reply_push_input(0..size).expect("Push failed");
msg::reply_commit(msg::value()).expect("Commit failed");
}
RereplyWithGas(gas) => {
msg::reply_input_with_gas(*gas, msg::value(), ..).expect("Rereply wgas failed");
msg::reply_input_with_gas(*gas, msg::value(), ..size).expect("Rereply wgas failed");
}
}
}
Expand Down
14 changes: 8 additions & 6 deletions examples/syscalls/src/wasm.rs
Original file line number Diff line number Diff line change
Expand Up @@ -102,8 +102,10 @@ fn process(syscall_kind: Kind) {
}
Kind::SendInput(gas_opt, expected_mid) => {
let actual_mid_res = match gas_opt {
Some(gas) => msg::send_input_with_gas_delayed(msg::source(), gas, 0, .., 0),
None => msg::send_input_delayed(msg::source(), 0, .., 0),
Some(gas) => {
msg::send_input_with_gas_delayed(msg::source(), gas, 0, ..msg::size(), 0)
}
None => msg::send_input_delayed(msg::source(), 0, ..msg::size(), 0),
};
assert_eq!(
Ok(expected_mid.into()),
Expand All @@ -120,7 +122,7 @@ fn process(syscall_kind: Kind) {

// check handle
handle
.push_input(0..)
.push_input(0..msg::size())
.expect("internal error: push_input failed");

let actual_mid_res = handle.commit_delayed(msg::source(), 0, 0);
Expand Down Expand Up @@ -212,8 +214,8 @@ fn process(syscall_kind: Kind) {
}
Kind::ReplyInput(gas_opt, expected_mid) => {
let actual_mid_res = match gas_opt {
Some(gas) => msg::reply_input_with_gas(gas, 0, ..),
None => msg::reply_input(0, ..),
Some(gas) => msg::reply_input_with_gas(gas, 0, ..msg::size()),
None => msg::reply_input(0, ..msg::size()),
};
assert_eq!(
Ok(expected_mid.into()),
Expand All @@ -222,7 +224,7 @@ fn process(syscall_kind: Kind) {
);
}
Kind::ReplyPushInput(expected_mid) => {
msg::reply_push_input(..).expect("internal error: reply_push_input failed");
msg::reply_push_input(..msg::size()).expect("internal error: reply_push_input failed");
let actual_mid_res = msg::reply_commit(0);
assert_eq!(
Ok(expected_mid.into()),
Expand Down
2 changes: 1 addition & 1 deletion gtest/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@
//! let prog = Program::current(&sys);
//!
//! // Provide user with some balance.
//! sys.min_to(USER_ID, EXISTENTIAL_DEPOSIT * 1000);
//! sys.mint_to(USER_ID, EXISTENTIAL_DEPOSIT * 1000);
//!
//! // Send an init message to the program.
//! let init_message_id = prog.send_bytes(USER_ID, b"Doesn't matter");
Expand Down
14 changes: 0 additions & 14 deletions pallets/gear/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13531,20 +13531,6 @@ fn relay_messages() {
payload: vec![],
},
),
(
RelayCall::ResendPush(vec![
// invalid range
ResendPushData {
destination: USER_3.into(),
start: Some(payload.len() as u32),
end: Some((0, false)),
},
]),
Expected {
user: USER_3,
payload: vec![],
},
),
];

for (call, expectation) in pairs {
Expand Down
8 changes: 6 additions & 2 deletions utils/wasm-gen/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -519,6 +519,8 @@ fn test_msg_value_ptr_dest() {
let params_config = SyscallsParamsConfig::new()
.with_default_regular_config()
.with_rule(RegularParamType::Gas, (0..=0).into())
.with_rule(RegularParamType::Offset, (0..=0).into())
.with_rule(RegularParamType::Length, (0..=1).into())
.with_ptr_rule(PtrParamAllowedValues::ActorIdWithValue {
actor_kind: dest_var.clone(),
range: REPLY_VALUE..=REPLY_VALUE,
Expand Down Expand Up @@ -896,7 +898,9 @@ fn precise_syscalls_works() {

let param_config = SyscallsParamsConfig::new()
.with_default_regular_config()
.with_rule(RegularParamType::Gas, (0..=0).into());
.with_rule(RegularParamType::Gas, (0..=0).into())
.with_rule(RegularParamType::Offset, (0..=0).into())
.with_rule(RegularParamType::Length, (0..=1).into());

// Assert that syscalls results will be processed.
let termination_reason = execute_wasm_with_custom_configs(
Expand Down Expand Up @@ -1036,7 +1040,7 @@ fn execute_wasm_with_custom_configs(
let incoming_message = IncomingMessage::new(
message_id.into(),
message_sender(),
Default::default(),
vec![1, 2, 3].try_into().unwrap(),
Default::default(),
Default::default(),
Default::default(),
Expand Down

0 comments on commit d444bda

Please sign in to comment.