Skip to content

Commit

Permalink
fix(security): DoS in parser (#1239)
Browse files Browse the repository at this point in the history
* fix(security): DoS in parser

Malicious input can cause a denial of service by exponentiating a number
to a huge power and taking the CPU for galactic amounts of time, as well
as exponentially increasing memory usage.
The added test can trigger it before this commit even in release mode.
At the time of writing this it's still running after 50' and more than
2700MB.
This was found by the fuzzer using the externally reachable parser, even
though the test only attacks the culprit here.

The cause of this issue is the following:
- Recently we added support for scientific notation numbers in the
  `data` section of an input program;
- `NumBigUint` only supports `modpow` when all arguments are `BigUint`;
- `FeltBigInt` for primitive-type exponents tried to avoid creating a
  new `BigUint` as an attempt at optimization, so it used
  `pow`+`mod_floor` instead;
- This combination with a huge exponent means an ever increasing number
  of allocations.

The fix consists in building the `BigUint` and using `modpow` instead.
This uses constant space because internally it uses the Montgomery
exponentiation algorithm. With this the test finishes in 11ms on my
machine.

* Leftover `dbg!`

* Remove clippy allow
  • Loading branch information
Oppen authored Jun 15, 2023
1 parent 80dd475 commit e876253
Show file tree
Hide file tree
Showing 3 changed files with 31 additions and 3 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

#### Upcoming Changes

* fix(security): avoid denial of service on malicious input exploiting the scientific notation parser [#1239](https://github.com/lambdaclass/cairo-rs/pull/1239)

* perf: accumulate `min` and `max` instruction offsets during run to speed up range check [#1080](https://github.com/lambdaclass/cairo-rs/pull/)
BREAKING: `Cairo_runner::get_perm_range_check_limits` no longer returns an error when called without trace enabled, as it no longer depends on it

Expand Down
5 changes: 2 additions & 3 deletions felt/src/bigint_felt.rs
Original file line number Diff line number Diff line change
Expand Up @@ -495,17 +495,16 @@ impl<const PH: u128, const PL: u128> Pow<u32> for FeltBigInt<PH, PL> {
type Output = Self;
fn pow(self, rhs: u32) -> Self {
FeltBigInt {
val: self.val.pow(rhs).mod_floor(&CAIRO_PRIME_BIGUINT),
val: self.val.modpow(&BigUint::from(rhs), &CAIRO_PRIME_BIGUINT),
}
}
}

impl<'a, const PH: u128, const PL: u128> Pow<u32> for &'a FeltBigInt<PH, PL> {
type Output = FeltBigInt<PH, PL>;
#[allow(clippy::needless_borrow)] // the borrow of self.val is necessary becase it's of the type BigUInt, which doesn't implement the Copy trait
fn pow(self, rhs: u32) -> Self::Output {
FeltBigInt {
val: (&self.val).pow(rhs).mod_floor(&CAIRO_PRIME_BIGUINT),
val: self.val.modpow(&BigUint::from(rhs), &CAIRO_PRIME_BIGUINT),
}
}
}
Expand Down
27 changes: 27 additions & 0 deletions vm/src/serde/deserialize_program.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1437,4 +1437,31 @@ mod tests {
)
);
}

#[test]
#[cfg_attr(target_arch = "wasm32", wasm_bindgen_test)]
fn test_felt_from_number_with_scientific_notation_big_exponent() {
#[derive(Deserialize, Debug, PartialEq)]
struct Test {
#[serde(deserialize_with = "felt_from_number")]
f: Option<Felt252>,
}
let malicious_input = &format!(
"{{ \"f\": {}e{} }}",
String::from_utf8(vec![b'9'; 1000]).unwrap(),
u32::MAX
);
let f = serde_json::from_str::<Test>(malicious_input)
.unwrap()
.f
.unwrap();
assert_eq!(
f,
Felt252::from_str_radix(
"2471602022505793130446032259107029522557827898253184929958153020344968292412",
10
)
.unwrap()
);
}
}

1 comment on commit e876253

@github-actions
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.30.

Benchmark suite Current: e876253 Previous: 80dd475 Ratio
add_u64_with_felt/1 3 ns/iter (± 0) 2 ns/iter (± 0) 1.50
add_u64_with_felt/2 3 ns/iter (± 0) 2 ns/iter (± 0) 1.50
add_u64_with_felt/5 2 ns/iter (± 0) 1 ns/iter (± 0) 2

This comment was automatically generated by workflow using github-action-benchmark.

CC: @unbalancedparentheses

Please sign in to comment.