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

Initial fuzzing support and fixes #11

Merged
merged 11 commits into from
Dec 1, 2022
Merged

Initial fuzzing support and fixes #11

merged 11 commits into from
Dec 1, 2022

Conversation

szszszsz
Copy link
Member

@szszszsz szszszsz commented Nov 19, 2022

Add initial fuzzing support and fixes found during its executions.

Corpus generated with the pynitrokey API tests (not included here). Each input file contains single command only at this point. No new errors found after 20 minutes, with 11 jobs set. A fork of Flexiber was used, which had the crashing todo() replaced with returning Error code.

Future work / to discuss:

  • might be worth to run it longer, or in CI
  • extend the corpus to have more commands - Select, Validate, CalculateAll, SendRemaining
  • disable not tested commands (as mentioned) skipped
  • add coverage report generation
  • extend the corpus to run multiple commands at a time
  • dockerize/reuse fuzz setup
  • something better than assertfn ?

Inspired by the opcard-rs fuzzing implementation.

Fixes #8

@szszszsz szszszsz added the enhancement New feature or request label Nov 19, 2022
@szszszsz
Copy link
Member Author

@sosthene-nitrokey Any ideas how to fix that?

$ cd fuzz && make fuzz-cov
# ...
Merging raw coverage data...
Coverage data merged and saved in "/home/sz/work/OTP/oath-authenticator/fuzz/coverage/fuzz_target_1/coverage.profdata".
llvm-cov show --format=html \
        --instr-profile=coverage/fuzz_target_1/coverage.profdata \
         /tmp/cargo/x86_64-unknown-linux-gnu/release/fuzz_target_1 \
        > fuzz_coverage.html
error: /tmp/cargo/x86_64-unknown-linux-gnu/release/fuzz_target_1: Failed to load coverage: unsupported instrumentation profile format version

@sosthene-nitrokey
Copy link
Collaborator

@sosthene-nitrokey Any ideas how to fix that?

$ cd fuzz && make fuzz-cov
# ...
Merging raw coverage data...
Coverage data merged and saved in "/home/sz/work/OTP/oath-authenticator/fuzz/coverage/fuzz_target_1/coverage.profdata".
llvm-cov show --format=html \
        --instr-profile=coverage/fuzz_target_1/coverage.profdata \
         /tmp/cargo/x86_64-unknown-linux-gnu/release/fuzz_target_1 \
        > fuzz_coverage.html
error: /tmp/cargo/x86_64-unknown-linux-gnu/release/fuzz_target_1: Failed to load coverage: unsupported instrumentation profile format version

I appear to have the same issue on my side with opcard.

Looking at it it's probably an issue of incompatible lllvm version. Current nightly rust uses LLVM 15, but most distros ship an older version (even Arch is on 14). The fix seems to be using the llvm-cov binary shipped with rustup's llvm-tools-preview component. For me that's ~/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/bin/llvm-cov

@sosthene-nitrokey
Copy link
Collaborator

The proper way to invoke it would probably be cargo-binutils, but I'm not sure if it's worth it adding it to the pipeline image

@szszszsz
Copy link
Member Author

Yes, this worked for me, thanks!

Copy link
Collaborator

@sosthene-nitrokey sosthene-nitrokey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the empty lib.rs required?

src/lib.rs Outdated Show resolved Hide resolved

use libfuzzer_sys::fuzz_target;

fuzz_target!(|data: &[u8]| {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we want to be chain multiple commands within one round of fuzzing. Otherwise many code paths can't be explored, for example for authentication, and modification/deletion. You can use Arbitrary to get a vec of commands.

Also does Oath need authentication for some command? I think it would be very hard for the fuzzer find the correct password so it should probably be in a seed corpus.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Multiple commands - yes, that's already in the list. Will check your implementation with Arbitrary.
  2. No need for authentication by default.
  3. Removed the empty lib.rs.

While fuzzing is currently shallow due to causes you have mentioned, it already found problems within the binary parser, which I expect will be the only one found here.

@szszszsz
Copy link
Member Author

szszszsz commented Dec 1, 2022

Merging the current state, as it is required for the daily use at the moment. To be continued in the next PRs, as described earlier.

@szszszsz szszszsz merged commit 9428bdc into main Dec 1, 2022
@szszszsz szszszsz deleted the 8-fuzzing branch December 1, 2022 12:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Stability improvements
2 participants