Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OOB memory access on from-consensus-buff? #591

Merged
merged 10 commits into from
Jan 7, 2025
Merged

OOB memory access on from-consensus-buff? #591

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
5b33e6f
fix: allocate memory for deserialization values
BowTiedWoo Dec 18, 2024
c77ce91
fix: write deserialization value at `offset_result`
BowTiedWoo Dec 18, 2024
343d552
Merge branch 'main' into fix/from-consensus-buff-memory-oob
BowTiedWoo Dec 18, 2024
7903bf7
chore: remove oob unit test
BowTiedWoo Dec 18, 2024
30eadb9
chore: rename `result_offset` accordingly
BowTiedWoo Dec 19, 2024
92ef299
chore: remove struct and modify TODO
BowTiedWoo Jan 3, 2025
e00d337
fix: format code
BowTiedWoo Jan 3, 2025
a91d2df
chore: revert `Cargo.lock`
BowTiedWoo Jan 6, 2025
bec1895
Merge branch 'main' into fix/from-consensus-buff-memory-oob
BowTiedWoo Jan 6, 2025
2cef72a
Merge branch 'main' into fix/from-consensus-buff-memory-oob
BowTiedWoo Jan 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
121 changes: 87 additions & 34 deletions clar2wasm/src/deserialize.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,7 @@ impl WasmGenerator {
builder: &mut InstrSeqBuilder,
memory: MemoryId,
offset_local: LocalId,
offset_result: LocalId,
end_local: LocalId,
) -> Result<(), GeneratorError> {
// Create a block for the body of this operation, so that we can
Expand Down Expand Up @@ -195,22 +196,22 @@ impl WasmGenerator {
then.i32_const(1);

// Allocate space for the principal on the call stack
let result_offset = self.module.locals.add(ValType::I32);
then.global_get(self.stack_pointer).local_tee(result_offset);
let principal_offset = self.module.locals.add(ValType::I32);
then.local_get(offset_result).local_tee(principal_offset);
then.i32_const(STANDARD_PRINCIPAL_BYTES as i32)
.binop(BinaryOp::I32Add)
.global_set(self.stack_pointer);
.local_set(offset_result);

// Copy the principal to the destination
then.local_get(result_offset)
then.local_get(principal_offset)
.local_get(offset_local)
.i32_const(1)
.binop(BinaryOp::I32Add)
.i32_const(PRINCIPAL_BYTES as i32)
.memory_copy(memory, memory);

// Write the contract name length (0)
then.local_get(result_offset).i32_const(0).store(
then.local_get(principal_offset).i32_const(0).store(
memory,
StoreKind::I32_8 { atomic: false },
MemArg {
Expand All @@ -227,7 +228,7 @@ impl WasmGenerator {
.local_set(offset_local);

// Push the offset and length onto the stack
then.local_get(result_offset)
then.local_get(principal_offset)
.i32_const(STANDARD_PRINCIPAL_BYTES as i32);

// Break out of the block
Expand Down Expand Up @@ -431,6 +432,7 @@ impl WasmGenerator {
memory: MemoryId,
offset_local: LocalId,
end_local: LocalId,
offset_result: LocalId,
value_ty: &TypeSignature,
) -> Result<(), GeneratorError> {
// Create a block for the body of this operation, so that we can
Expand Down Expand Up @@ -513,11 +515,17 @@ impl WasmGenerator {
.local_set(offset_local);

// Deserialize the inner value
self.deserialize_from_memory(&mut some_block, offset_local, end_local, value_ty)?;
self.deserialize_from_memory(
&mut some_block,
offset_local,
end_local,
offset_result,
value_ty,
)?;

// Check if the deserialization failed:
// - Store the value in locals
// - Check the inidicator now on top of the stack
// - Check the indicator now on top of the stack
let inner_locals = self.save_to_locals(&mut some_block, value_ty, true);
some_block.unop(UnaryOp::I32Eqz).if_else(
None,
Expand Down Expand Up @@ -573,12 +581,14 @@ impl WasmGenerator {
/// | 0x07 | serialized ok value |
/// Err:
/// | 0x08 | serialized err value |
#[allow(clippy::too_many_arguments)]
fn deserialize_response(
&mut self,
builder: &mut InstrSeqBuilder,
memory: MemoryId,
offset_local: LocalId,
end_local: LocalId,
offset_result: LocalId,
ok_ty: &TypeSignature,
err_ty: &TypeSignature,
) -> Result<(), GeneratorError> {
Expand Down Expand Up @@ -638,7 +648,7 @@ impl WasmGenerator {
.local_set(offset_local);

// Deserialize the inner value
self.deserialize_from_memory(&mut ok_block, offset_local, end_local, ok_ty)?;
self.deserialize_from_memory(&mut ok_block, offset_local, end_local, offset_result, ok_ty)?;

// Check if the deserialization failed:
// - Store the value in locals
Expand Down Expand Up @@ -698,7 +708,13 @@ impl WasmGenerator {
.local_set(offset_local);

// Deserialize the inner value
self.deserialize_from_memory(&mut err_block, offset_local, end_local, err_ty)?;
self.deserialize_from_memory(
&mut err_block,
offset_local,
end_local,
offset_result,
err_ty,
)?;

// Check if the deserialization failed:
// - Store the value in locals
Expand Down Expand Up @@ -759,6 +775,7 @@ impl WasmGenerator {
memory: MemoryId,
offset_local: LocalId,
end_local: LocalId,
offset_result: LocalId,
list_ty: &ListTypeData,
) -> Result<(), GeneratorError> {
// Create a block for the body of this operation, so that we can
Expand Down Expand Up @@ -840,18 +857,18 @@ impl WasmGenerator {
// Allocate space for the list on the call stack
let element_ty = list_ty.get_list_item_type();
let result = self.module.locals.add(ValType::I32);
let result_offset = self.module.locals.add(ValType::I32);
let element_offset = self.module.locals.add(ValType::I32);
let element_size = get_type_size(element_ty);
block
.global_get(self.stack_pointer)
.local_get(offset_result)
Acaccia marked this conversation as resolved.
Show resolved Hide resolved
.local_tee(result)
.local_tee(result_offset);
.local_tee(element_offset);
block
.local_get(length)
.i32_const(element_size)
.binop(BinaryOp::I32Mul)
.binop(BinaryOp::I32Add)
.global_set(self.stack_pointer);
.local_set(offset_result);

// Update the offset to point to the first element
block
Expand Down Expand Up @@ -893,7 +910,13 @@ impl WasmGenerator {

// Deserialize the element. Note, this will update the offset to point
// to the next element.
self.deserialize_from_memory(&mut loop_block, offset_local, end_local, element_ty)?;
self.deserialize_from_memory(
&mut loop_block,
offset_local,
end_local,
offset_result,
element_ty,
)?;

// Check if the deserialization failed:
// - Store the value in locals
Expand All @@ -917,14 +940,14 @@ impl WasmGenerator {
for local in inner_locals {
loop_block.local_get(local);
}
let bytes_written = self.write_to_memory(&mut loop_block, result_offset, 0, element_ty)?;
let bytes_written = self.write_to_memory(&mut loop_block, element_offset, 0, element_ty)?;

// Increment the result offset by the number of bytes written
loop_block
.local_get(result_offset)
.local_get(element_offset)
.i32_const(bytes_written as i32)
.binop(BinaryOp::I32Add)
.local_set(result_offset);
.local_set(element_offset);

// Increment the index by 1
loop_block
Expand Down Expand Up @@ -960,6 +983,7 @@ impl WasmGenerator {
memory: MemoryId,
offset_local: LocalId,
end_local: LocalId,
offset_result: LocalId,
tuple_ty: &TupleTypeSignature,
) -> Result<(), GeneratorError> {
// We need to be able to parse the keys coming in a random order, only one occurence of each key.
Expand Down Expand Up @@ -1214,6 +1238,7 @@ impl WasmGenerator {
&mut case_block,
offset_local,
end_local,
offset_result,
field_ty,
)?;
for &l in field_locals.iter().rev() {
Expand Down Expand Up @@ -1629,6 +1654,7 @@ impl WasmGenerator {
memory: MemoryId,
offset_local: LocalId,
end_local: LocalId,
offset_result: LocalId,
string_utf8_ty: &TypeSignature,
) -> Result<(), GeneratorError> {
let max_len: u32 = match string_utf8_ty {
Expand Down Expand Up @@ -1694,9 +1720,7 @@ impl WasmGenerator {
then.if_else(
return_type,
|then| {
let (offset_result, _len) =
self.create_call_stack_local(then, string_utf8_ty, false, true);

// convert utf8 to string-utf8
then.local_get(offset_local)
.local_get(string_length)
.local_get(offset_result)
Expand All @@ -1708,6 +1732,12 @@ impl WasmGenerator {
.local_get(string_length)
.binop(BinaryOp::I32Add)
.local_set(offset_local);

// move offset-result to the end of the deserialized utf8 string
then.local_get(offset_result)
.i32_const(max_len as i32 * 4)
.binop(BinaryOp::I32Add)
.local_set(offset_result);
},
|else_| {
else_.i32_const(0).i32_const(0).i32_const(0);
Expand Down Expand Up @@ -1735,6 +1765,7 @@ impl WasmGenerator {
builder: &mut InstrSeqBuilder,
offset_local: LocalId,
end_local: LocalId,
offset_result: LocalId,
ty: &TypeSignature,
) -> Result<(), GeneratorError> {
let memory = self.get_memory()?;
Expand All @@ -1745,23 +1776,34 @@ impl WasmGenerator {
self.deserialize_integer(builder, memory, offset_local, end_local, ty == &IntType)
}
PrincipalType | CallableType(_) | TraitReferenceType(_) => {
self.deserialize_principal(builder, memory, offset_local, end_local)
self.deserialize_principal(builder, memory, offset_local, offset_result, end_local)
}
ResponseType(types) => self.deserialize_response(
builder,
memory,
offset_local,
end_local,
offset_result,
&types.0,
&types.1,
),
BoolType => self.deserialize_bool(builder, memory, offset_local, end_local),
OptionalType(value_ty) => {
self.deserialize_optional(builder, memory, offset_local, end_local, value_ty)
}
SequenceType(SequenceSubtype::ListType(list_ty)) => {
self.deserialize_list(builder, memory, offset_local, end_local, list_ty)
}
OptionalType(value_ty) => self.deserialize_optional(
builder,
memory,
offset_local,
end_local,
offset_result,
value_ty,
),
SequenceType(SequenceSubtype::ListType(list_ty)) => self.deserialize_list(
builder,
memory,
offset_local,
end_local,
offset_result,
list_ty,
),
SequenceType(SequenceSubtype::BufferType(type_length)) => self.deserialize_buffer(
builder,
memory,
Expand All @@ -1777,12 +1819,23 @@ impl WasmGenerator {
end_local,
type_length.into(),
),
utf8 @ SequenceType(SequenceSubtype::StringType(StringSubtype::UTF8(_))) => {
self.deserialize_string_utf8(builder, memory, offset_local, end_local, utf8)
}
TupleType(tuple_ty) => {
self.deserialize_tuple(builder, memory, offset_local, end_local, tuple_ty)
}
utf8 @ SequenceType(SequenceSubtype::StringType(StringSubtype::UTF8(_))) => self
.deserialize_string_utf8(
builder,
memory,
offset_local,
end_local,
offset_result,
utf8,
),
TupleType(tuple_ty) => self.deserialize_tuple(
builder,
memory,
offset_local,
end_local,
offset_result,
tuple_ty,
),
NoType => unreachable!("NoType should not be deserialized"),
ListUnionType(_) => unreachable!("ListUnionType should not be deserialized"),
}
Expand Down
5 changes: 4 additions & 1 deletion clar2wasm/src/words/consensus_buff.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,8 @@ impl ComplexWord for FromConsensusBuff {
// Traverse the input buffer, leaving the offset and length on the stack.
generator.traverse_expr(builder, args.get_expr(1)?)?;

// TODO: true, true is too big; see issue: #593
let (offset_result, _len) = generator.create_call_stack_local(builder, &ty, true, true);
let offset = generator.module.locals.add(walrus::ValType::I32);
let end = generator.module.locals.add(walrus::ValType::I32);
builder
Expand All @@ -134,7 +136,8 @@ impl ComplexWord for FromConsensusBuff {
.binop(BinaryOp::I32Add)
.local_set(end);

generator.deserialize_from_memory(builder, offset, end, &value_ty)?;
// Write the deserialized value on the stack at offset_result
generator.deserialize_from_memory(builder, offset, end, offset_result, &value_ty)?;

// If the entire buffer was not consumed, return none.
builder
Expand Down
Loading