Skip to content

Commit

Permalink
fix(security)!: avoid DoS on malicious insert to memory (#1099)
Browse files Browse the repository at this point in the history
Co-authored-by: Pedro Fontana <fontana.pedro93@gmail.com>
  • Loading branch information
Oppen and pefontana committed May 4, 2023
1 parent 72065eb commit 913aa49
Show file tree
Hide file tree
Showing 3 changed files with 31 additions and 2 deletions.
6 changes: 6 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,12 @@
%}
```

* fix(security)!: avoid DoS on malicious insertion to memory [#1099](https://github.com/lambdaclass/cairo-rs/pull/1099)
* A program could crash the library by attempting to insert a value at an address with a big offset; fixed by trying to reserve to check for allocation failure
* A program could crash the program by exploiting an integer overflow when attempting to insert a value at an address with offset `usize::MAX`

BREAKING: added a new error variant `MemoryError::VecCapacityExceeded`

* fix(starknet-crypto): bump version to `0.5.0` [#1088](https://github.com/lambdaclass/cairo-rs/pull/1088)
* This includes the fix for a `panic!` in `ecdsa::verify`.
See: [#365](https://github.com/xJonathanLEI/starknet-rs/issues/365) and [#366](https://github.com/xJonathanLEI/starknet-rs/pulls/366)
Expand Down
2 changes: 2 additions & 0 deletions src/vm/errors/memory_errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,8 @@ pub enum MemoryError {
// SegmentArenaBuiltin
#[error("segment_arena_builtin: assert used >= INITIAL_SEGMENT_SIZE")]
InvalidUsedSizeSegmentArena,
#[error("Vector capacity exceeded")]
VecCapacityExceeded,
}

#[derive(Debug, PartialEq, Eq, Error)]
Expand Down
25 changes: 23 additions & 2 deletions src/vm/vm_memory/memory.rs
Original file line number Diff line number Diff line change
Expand Up @@ -136,8 +136,15 @@ impl Memory {

//Check if the element is inserted next to the last one on the segment
//Forgoing this check would allow data to be inserted in a different index
if segment.len() <= value_offset {
segment.resize(value_offset + 1, None);
let (len, capacity) = (segment.len(), segment.capacity());
if len <= value_offset {
let new_len = value_offset
.checked_add(1)
.ok_or(MemoryError::VecCapacityExceeded)?;
segment
.try_reserve(new_len.saturating_sub(capacity))
.map_err(|_| MemoryError::VecCapacityExceeded)?;
segment.resize(new_len, None);
}
// At this point there's *something* in there

Expand Down Expand Up @@ -1523,6 +1530,20 @@ mod memory_tests {
assert_eq!((ord, pos), mem.memcmp(lhs.into(), rhs.into(), len));
}

#[test]
fn insert_alloc_fails_gracefully() {
let mut mem = memory![((0, 0), 1)];
let err = mem.insert((0, usize::MAX >> 1).into(), Felt252::one());
assert_eq!(err, Err(MemoryError::VecCapacityExceeded));
}

#[test]
fn insert_overflow_fails_gracefully() {
let mut mem = memory![((0, 0), 1)];
let err = mem.insert((0, usize::MAX).into(), Felt252::one());
assert_eq!(err, Err(MemoryError::VecCapacityExceeded));
}

#[test]
fn memcmp() {
check_memcmp((0, 0), (0, 0), 3, Equal, 3);
Expand Down

0 comments on commit 913aa49

Please sign in to comment.