From e876253276a4eca882f45e68b990f10a360404f2 Mon Sep 17 00:00:00 2001 From: Mario Rugiero Date: Thu, 15 Jun 2023 19:10:12 -0300 Subject: [PATCH] fix(security): DoS in parser (#1239) * 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 --- CHANGELOG.md | 2 ++ felt/src/bigint_felt.rs | 5 ++--- vm/src/serde/deserialize_program.rs | 27 +++++++++++++++++++++++++++ 3 files changed, 31 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index eb8c693b3f..42e9c6f38b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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 diff --git a/felt/src/bigint_felt.rs b/felt/src/bigint_felt.rs index 4ac543f963..f4fa998d6a 100644 --- a/felt/src/bigint_felt.rs +++ b/felt/src/bigint_felt.rs @@ -495,17 +495,16 @@ impl Pow for FeltBigInt { 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 for &'a FeltBigInt { type Output = FeltBigInt; - #[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), } } } diff --git a/vm/src/serde/deserialize_program.rs b/vm/src/serde/deserialize_program.rs index 585a297512..3c4155d9b5 100644 --- a/vm/src/serde/deserialize_program.rs +++ b/vm/src/serde/deserialize_program.rs @@ -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, + } + let malicious_input = &format!( + "{{ \"f\": {}e{} }}", + String::from_utf8(vec![b'9'; 1000]).unwrap(), + u32::MAX + ); + let f = serde_json::from_str::(malicious_input) + .unwrap() + .f + .unwrap(); + assert_eq!( + f, + Felt252::from_str_radix( + "2471602022505793130446032259107029522557827898253184929958153020344968292412", + 10 + ) + .unwrap() + ); + } }