Off-by-One Access in engine_state.rs?

[tokatoka]

Describe the bug

I found two crash cases with fuzzing

crash1.txt
crash2.txt

I guess both of them have the same cause.

These crashes were found by libafl_libfuzzer fuzzer.
I can submit a fuzzer later, if it is ok.

How to reproduce

For both crash1, crash2

toka@toka:/tmp/nushell/crates/nu-parser/fuzz/crashes$ nu crash1.txt 
thread 'main' panicked at 'slice index starts at 7 but ends at 6', /home/toka/.cargo/registry/src/index.crates.io-6f17d22bba15001f/nu-protocol-0.81.0/src/engine/engine_state.rs:1424:29
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

and

toka@toka:/tmp/nushell/crates/nu-parser/fuzz/crashes$ nu crash2.txt 
thread 'main' panicked at 'slice index starts at 118 but ends at 117', /home/toka/.cargo/registry/src/index.crates.io-6f17d22bba15001f/nu-protocol-0.81.0/src/engine/engine_state.rs:1424:29
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Expected behavior

it shouldn't panic

Screenshots

No response

Configuration

/tmp/nushell/crates/nu-parser/fuzz/crashes> version | transpose key value | to md --pretty 06/12/2023 07:21:44 PM

key	value
version	0.81.0
branch
commit_hash
build_os	linux-x86_64
build_target	x86_64-unknown-linux-gnu
rust_version	rustc 1.72.0-nightly (a97c36dd2 2023-06-07)
rust_channel	nightly-x86_64-unknown-linux-gnu
cargo_version	cargo 1.72.0-nightly (b0fa79679 2023-06-03)
build_time	2023-06-12 17:35:19 +02:00
build_rust_channel	release
features	default, sqlite, trash, which, zip
installed_plugins
/tmp/nushell/crates/nu-parser/fuzz/crashes>

Additional context

No response

[amtoine]

these are quite strange files 😅

> open crash1.txt | into binary
Length: 6 (0x6) bytes | printable whitespace ascii_other non_ascii
00000000:   2e 2e 3d 7f  2e 2e                                   ..=•..

> open crash2.txt | into binary
Length: 117 (0x75) bytes | printable whitespace ascii_other non_ascii
00000000:   4a 4a 4a 4a  4a 2e f4 00  10 7c 2e 2e  2e 7f 4a 4a   JJJJJ.×0•|...•JJ
00000010:   4a 4a 4a 4a  4a 4e b7 4a  4a 4a 4a 4a  4a 4e 4a 4a   JJJJJN×JJJJJJNJJ
00000020:   4a 4a 4a 4a  4a 4a 4a 4a  4a 4a 4a 4a  4a 4a 4a 4a   JJJJJJJJJJJJJJJJ
00000030:   4a 4a 4a 2e  f4 7c 7b 7c  2e 2e 2e 7f  4a 4a 7c 7b   JJJ.×|{|...•JJ|{
00000040:   86 2e 2e 4a  4a 4a 4a d2  7c 6f 00 10  00 00 22 22   ×..JJJJ×|o0•00""
00000050:   7c 7b 7c cd  3d 24 65 2e  7b 5b 7b 7c  32 00 10 5b   |{|×=$e.{[{|20•[
00000060:   7b 7c 32 00  10 28 7b 7c  7b 7c 2e 2e  3d 69 4a 4a   {|20•({|{|..=iJJ
00000070:   7c 7b 7c 2e  2e                                      |{|..

but yeah it shouldn't crash, for sure 👍

[tokatoka]

these are quite strange files 😅

yeah it's generated by the fuzzer.

I think the shortest crashing case is "..=.."

[sholderbach]

Thank you very much for those failure cases and your fuzzing effort. This looks very much like a parser bug that transfers invalid spans that later cause a failure.

Would you mind sharing how you set up your fuzzing, if other folks want to dedicate some compute to target nushell?

[tokatoka]

Would you mind sharing how you set up your fuzzing, if other folks want to dedicate some compute to target nushell?

Yes ofc.
I mean yes of course I can share the fuzzer

[tokatoka]

I will commit back the fuzzer I used later once I have set it up

[tokatoka]

crash-ba5b0bd650e29124.txt

I have another crash that seems to crash at a different location, and is not an off-by-one bug.
i don't know if this has the same root-cause as the first two crash files.

toka@toka:~$ nu crash-ba5b0bd650e29124.txt
thread 'main' panicked at 'index out of bounds: the len is 2 but the index is 5', /home/toka/.cargo/registry/src/index.crates.io-6f17d22bba15001f/nu-parser-0.81.0/src/parser.rs:740:28
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

[sholderbach]

crash-ba5b0bd650e29124.txt

I have another crash that seems to crash at a different location, and is not an off-by-one bug. i don't know if this has the same root-cause as the first two crash files.

toka@toka:~$ nu crash-ba5b0bd650e29124.txt
thread 'main' panicked at 'index out of bounds: the len is 2 but the index is 5', /home/toka/.cargo/registry/src/index.crates.io-6f17d22bba15001f/nu-parser-0.81.0/src/parser.rs:740:28
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

This is probably a different beast as it is crashing in the parser already. I suspect we have more than one parser bug that chokes on arbitrary input.

Feels a bit scary to execute whatever code the fuzzer can cook up :D ^rm -rf --no-preserve-root / and send it :P

[addisoncrump]

The fuzzer is thankfully quite unlikely to issue such a command -- and we can merely hook the execve function if this is a major concern 😉

[tokatoka]

Feels a bit scary to execute whatever code the fuzzer can cook up :D ^rm -rf --no-preserve-root / and send it :P

Yes I know 😄
We don't fuzz by sending random bytes to nu either.

For you to reproduce, Here's the rust code to reproduce the same bug.

use std::io::Read;
use nu_parser::*;
use nu_protocol::ast::{Argument, Call, PathMember};
use nu_protocol::Span;
use nu_protocol::{
    ast::{Expr, Expression, PipelineElement},
    engine::{Command, EngineState, Stack, StateWorkingSet},
    ParseError, PipelineData, ShellError, Signature, SyntaxShape,
};

fn main() {
    let args: Vec<String> = std::env::args().collect();
    let inp = std::fs::File::open(args[1].clone()).unwrap();

    let mut reader = std::io::BufReader::new(inp);
    let mut data = Vec::new();
    
    reader.read_to_end(&mut data).unwrap();

    let engine_state = EngineState::new();
    let mut working_set = StateWorkingSet::new(&engine_state);

    let block = parse(&mut working_set, None, &data, true);
}

you can compile it into poc and then

toka@toka:/tmp/nushell/crates/nu-parser/fuzz$ ./poc ~/crash-ba5b0bd650e29124.txt 
thread 'main' panicked at 'index out of bounds: the len is 2 but the index is 5', /tmp/nushell/crates/nu-parser/src/parser.rs:740:28
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

[sholderbach]

Phew, that looks much safer :D shouldn't be able to do any intentional or direct damage.

(I was initially unsure why the panics were popping up in the engine part but yeah that is just the engine state holding any created definitions)

@addisoncrump as nushell does a bunch of direct syscalls to file system etc. without shelling out to another program just locking down execve wouldn't suffice to properly sandbox it.

[addisoncrump]

Interesting; I didn't see those syscalls, just wrappers for e.g. std's Command and such. I will investigate further 🙂