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.

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

fix(security): DoS in parser #1239

Merged
merged 3 commits into from
Jun 15, 2023
Merged

fix(security): DoS in parser #1239

merged 3 commits into from
Jun 15, 2023

Conversation

Oppen
Copy link
Contributor

@Oppen Oppen commented Jun 15, 2023

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. I
aborted the run after 40 minutes and over 2GB of memory.
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.

Checklist

  • Linked to Github Issue
  • Unit tests added
  • Integration tests added.
  • This change requires new documentation.
    • Documentation has been added/updated.
    • CHANGELOG has been updated.

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.
@Oppen Oppen force-pushed the fix/dos_int_parsing branch from fa69875 to f5157b9 Compare June 15, 2023 14:01
felt/src/bigint_felt.rs Show resolved Hide resolved
vm/src/serde/deserialize_program.rs Outdated Show resolved Hide resolved
@github-actions
Copy link

github-actions bot commented Jun 15, 2023

Benchmark Results for unmodified programs 🚀

Command Mean [s] Min [s] Max [s] Relative
base blake2s_integration_benchmark 10.163 ± 0.105 9.990 10.285 1.01 ± 0.02
head blake2s_integration_benchmark 10.082 ± 0.196 9.965 10.613 1.00
Command Mean [s] Min [s] Max [s] Relative
base compare_arrays_200000 3.428 ± 0.037 3.386 3.483 1.00
head compare_arrays_200000 3.478 ± 0.033 3.445 3.546 1.01 ± 0.01
Command Mean [s] Min [s] Max [s] Relative
base dict_integration_benchmark 2.338 ± 0.065 2.301 2.511 1.00
head dict_integration_benchmark 2.344 ± 0.019 2.314 2.374 1.00 ± 0.03
Command Mean [s] Min [s] Max [s] Relative
base factorial_multirun 3.599 ± 0.037 3.551 3.680 1.00
head factorial_multirun 3.652 ± 0.020 3.621 3.680 1.01 ± 0.01
Command Mean [s] Min [s] Max [s] Relative
base fibonacci_1000_multirun 3.066 ± 0.027 3.044 3.123 1.00
head fibonacci_1000_multirun 3.079 ± 0.014 3.058 3.103 1.00 ± 0.01
Command Mean [ms] Min [ms] Max [ms] Relative
base field_arithmetic_get_square_benchmark 153.0 ± 1.3 151.1 154.9 1.00
head field_arithmetic_get_square_benchmark 153.5 ± 2.1 151.1 157.1 1.00 ± 0.02
Command Mean [s] Min [s] Max [s] Relative
base integration_builtins 8.759 ± 0.142 8.639 9.118 1.00
head integration_builtins 8.996 ± 0.128 8.791 9.268 1.03 ± 0.02
Command Mean [s] Min [s] Max [s] Relative
base keccak_integration_benchmark 10.363 ± 0.125 10.088 10.543 1.01 ± 0.02
head keccak_integration_benchmark 10.257 ± 0.153 10.113 10.528 1.00
Command Mean [s] Min [s] Max [s] Relative
base linear_search 3.462 ± 0.061 3.416 3.614 1.00
head linear_search 3.490 ± 0.051 3.456 3.612 1.01 ± 0.02
Command Mean [s] Min [s] Max [s] Relative
base math_cmp_and_pow_integration_benchmark 2.446 ± 0.039 2.419 2.551 1.01 ± 0.02
head math_cmp_and_pow_integration_benchmark 2.427 ± 0.011 2.417 2.453 1.00
Command Mean [s] Min [s] Max [s] Relative
base math_integration_benchmark 2.282 ± 0.015 2.264 2.313 1.00
head math_integration_benchmark 2.286 ± 0.009 2.276 2.305 1.00 ± 0.01
Command Mean [s] Min [s] Max [s] Relative
base memory_integration_benchmark 1.994 ± 0.055 1.961 2.148 1.00
head memory_integration_benchmark 2.019 ± 0.051 1.989 2.163 1.01 ± 0.04
Command Mean [s] Min [s] Max [s] Relative
base operations_with_data_structures_benchmarks 2.320 ± 0.073 2.271 2.520 1.00 ± 0.03
head operations_with_data_structures_benchmarks 2.313 ± 0.018 2.295 2.357 1.00
Command Mean [ms] Min [ms] Max [ms] Relative
base pedersen 824.8 ± 2.0 822.2 829.5 1.00
head pedersen 825.8 ± 2.8 822.3 828.9 1.00 ± 0.00
Command Mean [s] Min [s] Max [s] Relative
base poseidon_integration_benchmark 1.571 ± 0.006 1.565 1.585 1.00
head poseidon_integration_benchmark 1.574 ± 0.015 1.560 1.611 1.00 ± 0.01
Command Mean [s] Min [s] Max [s] Relative
base secp_integration_benchmark 2.642 ± 0.015 2.625 2.667 1.00
head secp_integration_benchmark 2.651 ± 0.005 2.645 2.660 1.00 ± 0.01
Command Mean [s] Min [s] Max [s] Relative
base set_integration_benchmark 1.318 ± 0.011 1.310 1.347 1.00
head set_integration_benchmark 1.331 ± 0.018 1.314 1.376 1.01 ± 0.02
Command Mean [s] Min [s] Max [s] Relative
base uint256_integration_benchmark 6.548 ± 0.064 6.465 6.678 1.00 ± 0.03
head uint256_integration_benchmark 6.536 ± 0.203 6.450 7.111 1.00

@codecov
Copy link

codecov bot commented Jun 15, 2023

Codecov Report

Merging #1239 (2102217) into main (4221723) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1239      +/-   ##
==========================================
+ Coverage   97.61%   97.63%   +0.01%     
==========================================
  Files          88       88              
  Lines       36112    36137      +25     
==========================================
+ Hits        35251    35281      +30     
+ Misses        861      856       -5     
Impacted Files Coverage Δ
felt/src/bigint_felt.rs 95.33% <100.00%> (ø)
vm/src/serde/deserialize_program.rs 97.25% <100.00%> (+0.49%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@pefontana pefontana added this pull request to the merge queue Jun 15, 2023
Merged via the queue into main with commit e876253 Jun 15, 2023
@pefontana pefontana deleted the fix/dos_int_parsing branch June 15, 2023 22:32
kariy pushed a commit to dojoengine/cairo-rs that referenced this pull request Jun 23, 2023
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants