rhaiscript / rhai

Rhai - An embedded scripting language for Rust.
https://crates.io/crates/rhai
Apache License 2.0
3.66k stars 175 forks source link

Integration with google/oss-fuzz for continuous fuzz testing #775

Open silvergasp opened 9 months ago

silvergasp commented 9 months ago

Hey Rhai Team,

I hope this message finds you well. I've been following along with Rhai for some time now, and I've really enjoyed using it in one of my side projects. I'd like to suggest and champion an effort to set up some basic fuzz-testing and combine it with google/oss-fuzz for continuous fuzzing. I'm fully aware that you are very busy and I don't want to overload your review/maintenance capacity by introducing too many new ideas. Is this a bad time to discuss potential security/reliability improvements?

If your not familiar with fuzzing or oss-fuzz I've included a few brief notes below.

Benefits of Fuzz-Testing

Google/oss-fuzz for Continuous Fuzzing

I’d be more than happy to lead the effort in integrating fuzz testing with Rhai and assist in any way required.

As a proof of concept I created a super simple minimal fuzz harness for the Rhai in #774

schungx commented 9 months ago

Fuzzing is definitely welcome! I personally don't know much about setting one up and it is great that you offer to help.

Let me know what you need and I can set things up your way.

silvergasp commented 9 months ago

Cool, there is an application process for google/oss-fuzz. I'm pretty confident that this project will get accepted as it is quite popular.

A few notes that I should have included in my original message;

I can complete the application on your behalf. To do this I'll need the following from you;

I'll put together a draft PR shortly so that you can see what all this would look like.

silvergasp commented 9 months ago

Opened a draft PR with oss-fuzz here https://github.com/google/oss-fuzz/pull/11189 once marked as not a draft this will act as both;

schungx commented 9 months ago

I have registered rhaiscript@gmail.com for things that need an email. Does this work?

silvergasp commented 9 months ago

Yeah that should work :)

schungx commented 8 months ago

OK, the PR is merged.

silvergasp commented 8 months ago

Perfect I've marked the oss-fuzz application/integration as ready for review https://github.com/google/oss-fuzz/pull/11189

schungx commented 8 months ago

What should I do now?

silvergasp commented 8 months ago

Nothing for the moment, the oss-fuzz team will either reject/accept https://github.com/google/oss-fuzz/pull/11189. Usually they respond within a couple of weeks but if not I'll ping them. Once that all goes through there won't be much more to do except improve the fuzzers and fix bugs.

After it's merged you'll start receiving bug reports on the gmail email address you created and you'll get access to the dashboard for rhai here https://oss-fuzz.com/login

Thanks for being so responsive with all this it definitely helps when things have some momentum.

silvergasp commented 8 months ago

Looks like everything is up and running. I got a couple of emails this morning with bug reports, so you should be able to login in to the the dashboard and take a look at those. Let me know if you have any further questions, or need help interpreting the bug reports:)

schungx commented 8 months ago

Help will be great, as I don't even know how to login to the dashboard...

silvergasp commented 8 months ago

No worries, so head on over to https://oss-fuzz.com/login you'll be able to login with the rhaiscript@gmail.com email address that you created. You'll see something like this; Screenshot_select-area_20231119101801

Clicking on the "open crashes" will give you a rundown of the bugs that the fuzzer has found. The crash statistics can also be useful to get general trends as to how the fuzzer is handling things.

schungx commented 8 months ago

I clicked on the links from the emails and I successfully logged on to the dashboard.

I can now see the crash reports. They have great!

They can really catch edge cases.

schungx commented 8 months ago

Fuzzing has been great to catch subtle bugs that sneaked through unit testing!

Thanks for setting this up!

silvergasp commented 8 months ago

No worries. I'm going to take a look at this one tomorrow.

Seems like it's either.

  1. Hitting a quadratic code path or
  2. It's a bug in my fuzz harness rather than rhai.

Let me know if you need help reproducing any of these bugs these locally. Its not always entirely obvious.

schungx commented 8 months ago

Well, yes, this one is timed out, but I think probably due to complexity, as by default Rhai does not put a limit on operations count, so very complex expressions can run the parser for a long time...

Others, such as the stack overflows, are bugs, which I already fixed. Thanks to fuzzing!

silvergasp commented 8 months ago

Well, yes, this one is timed out, but I think probably due to complexity, as by default Rhai does not put a limit on operations count, so very complex expressions can run the parser for a long time...

Yeah I attempted to put a hard limit on the time here. So there is either a bug in my timeout code, or there a single operations is stuck taking more than 60s.

Others, such as the stack overflows, are bugs, which I already fixed. Thanks to fuzzing!

Are you certain that you fixed these bugs? Usually oss-fuzz will automatically detect if a bug is fixed and close each issue. This hasn't happened. So either there is a bug in oss-fuzz's bugfix detection system or the bugs haven't been fixed correctly. Just as a note, there have been a few issues with oss-fuzz's bugfix detection for rust in the last few months, so it's not entirely outlandish to think we might have run into one now. But if you are certain they are fixed but it's not being picked up we should file an issue with oss-fuzz.

schungx commented 8 months ago

Are you certain that you fixed these bugs

Oh, I fixed the bugs in my own dev repo. I haven't pushed it to master yet. I'll do that.

schungx commented 8 months ago

@silvergasp I have found that, even though I pushed new commits to the trunk, oss-fuzz continues to use an older commit for new tasks or progression tasks. Do you know how to force it to use the latest commit?

silvergasp commented 8 months ago

Hmm it should be using the latest commit. I'll look into it. It has definitely updated recently as I saw that those bugs that you fixed where marked as fixed automatically. But I'll look into it and make sure.

silvergasp commented 8 months ago

So I can confirm that it is indeed downloading the latest commit each night. If you go to the build-status page here and ctrl-F search for 'GIT_REV' you'll see the latest commit that was used.

$ git checkout 269302c7c8e6af028390c4e280012b5f2c2e409e
$ git log
commit 269302c7c8e6af028390c4e280012b5f2c2e409e (HEAD, upstream/main)
Merge: ee6261bf 33505173
Author: Stephen Chung <schungx@live.com>
Date:   Tue Nov 28 13:31:49 2023 +0800

    Merge pull request #787 from schungx/master

    Fix dynamic parameters bug.
silvergasp commented 8 months ago

Oh you'll need to click on the latest build tag as well e.g.

image

schungx commented 8 months ago

Ah OK. I realize that I just have to wait until it builds the latest. Thanks!

schungx commented 8 months ago

@silvergasp As far as I can tell, the time-out bugs were triggered based on a very very long and very very complicated script that causes the parser to run for a long time. Although Rhai controls run time, it does not control parsing time, because it is easy to reject long scripts before it gets to the parser.

I suggest we limit the input script length? Not sure how we can set this in the config...

schungx commented 8 months ago

Also, I wonder whatever happens to the OptimizationLevel input... I never see it in test case data, and there is no indication which value was used in each run...

silvergasp commented 8 months ago

I suggest we limit the input script length? Not sure how we can set this in the config

Seems reasonable to me. We can probably just truncate the input string in the fuzzer itself. The fuzzer will learn that passing in larger scripts won't actually improve vicarage.

silvergasp commented 8 months ago

Also, I wonder whatever happens to the OptimizationLevel input... I never see it in test case data, and there is no indication which value was used in each run...

You should be able to see which value is used in the debug outputs. I'll post some instructions when I'm in front of a computer.

silvergasp commented 8 months ago

Ah apologies this slipped my, mind and now that I'm testing it all, I'm not sure why it's not printing the debug output when in repro mode... It's a little odd. It's easy enough to hack around e.g. just print the ctx object and then run the reproduction again.

println!("{ctx:?}");

You just probably wouldn't want to leave print statement in while fuzzing as it'll slow things down unnecesarily. So the steps would look something like this;

  1. Download the minimized test case from ossfuzz.com e.g. image

  2. Add the println statement as above;

  3. Run the fuzzer against the minimized test case e.g. cargo fuzz scripting <path_to_downloaded_testcase>

For whatever reason, when you find a bug for the first time while running cargo fuzz it'll print the debug representation for the crashing input. However when reproducing the bug as described above it doesn't... I'm going to open up an issue with rust-fuzz, as I feel like this is a bug. I don't see any reason to omit the debug print in reproduce mode.

silvergasp commented 8 months ago

Also as mentioned earlier, I've opened up a PR that will add fuzzing to the CI here #790

silvergasp commented 8 months ago

Regarding not being able to see the OptimizationLevel I've filed an issue with cargo-fuzz here https://github.com/rust-fuzz/cargo-fuzz/issues/348. I don't feel like we should have to hack around this.

schungx commented 8 months ago

@silvergasp There seems to be a new bug for oss-fuzz.

Data that contains invalid UTF8, when converted to String input, will crash at input preparation stage.

silvergasp commented 6 months ago

@silvergasp There seems to be a new bug for oss-fuzz.

Data that contains invalid UTF8, when converted to String input, will crash at input preparation stage.

Sorry I missed this one. I'm not sure that I understand the problem you've mentioned do you mind expanding on this a little?

Also re your query about printing the Ctx inputs I got a response from oss-fuzz here. Looks like it's pretty simple just cargo fuzz fmt fuzz_target_1 artifacts/fuzz_target_1/crash-adc83b19e793491b1c6ea0fd8b46cd9f32e592fc

schungx commented 6 months ago

Sorry I missed this one. I'm not sure that I understand the problem you've mentioned do you mind expanding on this a little?

I believe my problem is not related to oss-fuzz and is already resolved. Sorry for the false alarm!

schungx commented 6 months ago

@silvergasp crash https://oss-fuzz.com/testcase-detail/6378147608068096 is a time-out.

However, since it is mocking an object, is there any way to actually print out the object before running the fuzz?

What if we put in a println!("{:?}", ctx.all_types)?

Otherwise there won't be any way to tell what is causing the runaway behavior -- although I believe it is probably due to printing large structures over and over again and concatnating into strings...

silvergasp commented 6 months ago

I don't think the println! is neccesary. You should be able to print it using the fmt subcommand as described here.

cargo +nightly fuzz fmt fuzz_serde clusterfuzz-testcase-minimized-fuzz_serde-6378147608068096

schungx commented 6 months ago

Well, it says like this:

Error: failed to run `cargo fuzz fmt` on input: clusterfuzz-testcase-fuzz_serde-4745120863813632

Caused by:
    Fuzz target 'fuzz_serde' exited with failure when attempting to debug formatting an interesting input that we discovered!

    Artifact: clusterfuzz-testcase-fuzz_serde-4745120863813632

    Command: "cargo" "run" "--manifest-path" "C:\\Git\\rhai\\fuzz\\Cargo.toml" "--target" "x86_64-pc-windows-msvc" "--release" "--bin" "fuzz_serde" "--" "-artifact_prefix=C:\\Git\\rhai\\fuzz\\artifacts\\fuzz_serde\\" "clusterfuzz-testcase-fuzz_serde-4745120863813632"

    Status: exit code: 1

    === stdout ===

    === stderr ===
    error: failed to run `rustc` to learn about target-specific information

    Caused by:
      process didn't exit successfully: `rustc - --crate-name ___ --print=file-names -Cpasses=sancov-module -Cllvm-args=-sanitizer-coverage-level=4 -Cllvm-args=-sanitizer-coverage-inline-8bit-counters -Cllvm-args=-sanitizer-coverage-pc-table -Cllvm-args=-sanitizer-coverage-trace-compares --cfg fuzzing -Clink-dead-code -Zsanitizer=address -Cdebug-assertions -Clink-arg=/include:main -C codegen-units=1 --target x86_64-pc-windows-msvc --crate-type bin --crate-type rlib --crate-type dylib --crate-type cdylib --crate-type staticlib --crate-type proc-macro --print=sysroot --print=split-debuginfo --print=crate-name --print=cfg` (exit code: 1)
      --- stdout
      ___.exe
      lib___.rlib
      ___.dll
      ___.dll
      ___.lib
      ___.dll
      C:\Users\Stephen\.rustup\toolchains\nightly-x86_64-pc-windows-msvc
      packed
      ___
      debug_assertions
      fuzzing
      overflow_checks
      panic="unwind"
      proc_macro
      relocation_model="pic"
      sanitize="address"
      target_abi=""
      target_arch="x86_64"
      target_endian="little"
      target_env="msvc"
      target_family="windows"
      target_feature="fxsr"
      target_feature="sse"
      target_feature="sse2"
      target_has_atomic
      target_has_atomic="16"
      target_has_atomic="32"
      target_has_atomic="64"
      target_has_atomic="8"
      target_has_atomic="ptr"
      target_has_atomic_equal_alignment="16"
      target_has_atomic_equal_alignment="32"
      target_has_atomic_equal_alignment="64"
      target_has_atomic_equal_alignment="8"
      target_has_atomic_equal_alignment="ptr"
      target_has_atomic_load_store
      target_has_atomic_load_store="16"
      target_has_atomic_load_store="32"
      target_has_atomic_load_store="64"
      target_has_atomic_load_store="8"
      target_has_atomic_load_store="ptr"
      target_os="windows"
      target_pointer_width="64"
      target_thread_local
      target_vendor="pc"
      windows

      --- stderr
      error: address sanitizer is not supported for this target

      error: aborting due to 1 previous error
silvergasp commented 6 months ago

Are you sure you ran it with +nightly address sanitizer isn't supported on stable (not that you should need it in this specific case).

silvergasp commented 6 months ago

Also this looks like it was run on windows, which doesn't support some sanitizer's maybe that's the problem. Do you have WSL installed?

schungx commented 6 months ago

Also this looks like it was run on windows, which doesn't support some sanitizer's maybe that's the problem. Do you have WSL installed?

No...

Maybe I can use it on code space...

Or can you run it on your computer? I've attached the two test cases.

schungx commented 6 months ago

testcases.zip