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

TOB-FUEL-31: Sway parser is crashing on certain inputs #5049

Open
xgreenx opened this issue Aug 28, 2023 · 3 comments
Open

TOB-FUEL-31: Sway parser is crashing on certain inputs #5049

xgreenx opened this issue Aug 28, 2023 · 3 comments
Assignees
Labels
audit-report Related to the audit report bug Something isn't working compiler: parser Everything to do with the parser

Comments

@xgreenx
Copy link
Contributor

xgreenx commented Aug 28, 2023

Description

A fuzzing campaign for the sway parser revealed several crashes. This finding serves as an umbrella finding to list all the different crashes we observed. The harness for the fuzzer is shown in the following figure.

Figure 31.1: Fuzz harness

#[test_fuzz::test_fuzz]
pub fn fuzz_parse(buf: &[u8]) {
    if let Ok(str) = String::from_utf8(Vec::from(buf)) {
        let type_engine = TypeEngine::default();
        let decl_engine = DeclEngine::default();
        let query_engine = QueryEngine::default();
        let engines = Engines::new(&type_engine, &decl_engine, &query_engine);
        let _result = parse(Arc::from(str), engines, None);
    }
}

The findings are relevant for use cases where untrusted Sway programs are parsed, for example as part of a Sway playground application, similar to the Rust playground.

Stack overflow

The Rust program invoking the parser with the following inputs might panic with the message thread 'fuzz_parse_fuzz::entry' has overflowed its stack.

Figure 31.2: Inputs separated by ----- that cause a stack overflow

script;
fn main() {
    let a: b256 =
0x000000([[[[[[[[[[[[[[[[[[[[[[[[[[[[[000000000000000000000009000(([[[[[[[[[[[[[[[[[
[[[[[[[[[[[[[[
-----
script;
fn main() {
    let a: b256 =
0x000000000000000000000000000000000(([[[[[[[[[[[[[[[[[[[[[[[[[[[[[[{{{{{{is_empty({{
{{{{{{{{{{{{{{
-----
script;
fn main() {
    let a: b256 =
0x0000000000000[[[[[[[[[[[[[[[00000(([[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[y;[[00000(([[[[[
[[[[[[[[[[[[[
-----
script;
fn main() {
    let a: b256 =  00000000@(((((((((((((((((C(((((((((((((((((
((((((alle(((((C(((((((((((((t {
  d, wriet {
#n -----
script;
fn main() {
    let a: b256 =
0x00000E000000(((((((((((((((((((((((((((((((000000000000000000000(((((((((((((((((C
((((((((((((((
-----
script;
fn main() {
(([[[[[[[[[[[[[[[[[[@[[[[[[000000000p000000000000000(([[[[[[[[[[[[[[[[[[[[[[[[[yable
, storage(readG write)]
qn -----
script;
fn main() {
  b256 =
0x00000E000000000000000000000000000(((((((((((((((((C((((((((((((((((((((((((allet
((((((((((((((((((((((((
-----
script;
fn main() {
    let a: b256 =
0x000000000000000000000000000000000(([[[[[[[[[[[r[[[[[[[[[[[[[[[[[[[y[[[[[[[[[[[[[[[
[[[[[[[[[[[[[[

Unwrap on None value

The parser panics in emit_error in line 50.

Figure 31.3: Input that causes a panic.

script;

fn main(256߄

Panic while slicing

The parser panics when calling as_str.

Figure 31.4: Input that causes a panic.

script;
fn karr() {
    let c: f828 =  0x00000000000000000000000vncifxp;
abi Zezybt {
    #[mfzbezc, storage(r#
true }
}
cug

Panic while creating span

The parser panics when calling span.

Figure 31.4: Hex encoded inputs separated by ----- that cause a panic.

7363726970743b0a0a636f7265666e206d61696e2829207b0a202020206c657420613a2062323536203d
202030783030303e30303030303030736372692073203d2022666c696272617279204932343b0a0a7573
6520636f72653a3a7072696d6974697665733a3a2a3b0a757365207374643a3a6173736572743a3a6173
736572743b0a0a2f2f2fdfab
-----
6c6962726172793b0a0a757365203a3a646174615f737472756374757265613a3a7b536f6d65456e756d
2c20536f6d655374727563747d3b0a75736520636f72653a3a6f70733a3a45713b0a0a696d706c204571
28666f7220536f6d65456e756d3c7533323e207b2f2f20414e43484f523a206c69627a6172790a2f2a20
414e43484f523a206d6f64756c650a6c6962726172793b0a2f2f20414e43594f525f454e443a206d6f64
0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a756c650a0a2f2f2043616e6e6f74df96

Recommendations

Short term, avoid panicking by handling the errors appropriately. For the stack overflows, make sure to avoid recursive calls and instead implement parsing iteratively.
Long term, deploy a fuzzer for the Sway parser. The above fuzz harness can be used together with Trail of Bits’ test-fuzz fuzzer. The instructions for setting it up can be found in the documentation of the project.

@andresmayorca
Copy link

I'm interested in this issue

@anton-trunov anton-trunov added compiler: parser Everything to do with the parser bug Something isn't working labels Sep 6, 2023
@IGI-111 IGI-111 self-assigned this Sep 28, 2023
IGI-111 added a commit that referenced this issue Sep 29, 2023
Will partially address #5049
IGI-111 added a commit that referenced this issue Oct 2, 2023
Will partially address #5049
IGI-111 added a commit that referenced this issue Oct 2, 2023
## Description

Will partially address #5049

## Checklist

- [x] I have linked to any relevant issues.
- [x] I have commented my code, particularly in hard-to-understand
areas.
- [ ] I have updated the documentation where relevant (API docs, the
reference, and the Sway book).
- [x] I have added tests that prove my fix is effective or that my
feature works.
- [x] I have added (or requested a maintainer to add) the necessary
`Breaking*` or `New Feature` labels where relevant.
- [x] I have done my best to ensure that my PR adheres to [the Fuel Labs
Code Review
Standards](https://github.com/FuelLabs/rfcs/blob/master/text/code-standards/external-contributors.md).
- [x] I have requested a review from the relevant team or maintainers.
@IGI-111
Copy link
Contributor

IGI-111 commented Oct 27, 2023

The parser still contains the stack overflow panics, but fixing those is going to require a significant rewrite.

@xgreenx
Copy link
Contributor Author

xgreenx commented Oct 27, 2023

We marked the issue as acknowledged. We plan to fix it in the future, but as stated above, it requires rewriting of the parser. It is not a critical issue and doesn't bring vulnerabilities into users' code.

We keep this issue open to track the progress in our backlog.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
audit-report Related to the audit report bug Something isn't working compiler: parser Everything to do with the parser
Projects
None yet
Development

No branches or pull requests

4 participants