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

Continued fuzzing and hardening #705

Open
koivunej opened this issue Apr 6, 2021 · 3 comments
Open

Continued fuzzing and hardening #705

koivunej opened this issue Apr 6, 2021 · 3 comments

Comments

@koivunej
Copy link
Collaborator

koivunej commented Apr 6, 2021

#693 introduced fuzzing targets to some crates and in the interest of avoiding forever lasting PRs, I didn't fix all of the found issues. Current status at the end of the PR is:

  • packet: address is good, packet roundtrip added late, immediate roundtrip failure(s), #[ignore] tests
  • btp: most fuzzed, looking quite good
  • stream: roundtrip added late, immediate roundtrip failure(s), #[ignore] tests
  • ccp: roundtripping, might be good

Other crates have not been fuzzed.

One obvious fix which isn't implemented yet is matching the interledger-packet datetime string against a regex before asking chrono to parse it in crates/interledger-packet/src/packet.rs. This fix would be similar to what was done for the variable length btp timestamp in e6f21c9. Done in #707.

#[ignore]'d test cases:

Marking this as good-first-issue as with any hand-written parsing code, there are most likely issues still to be discovered even after fixing the #[ignore] test cases. Mentoring is available by pinging me or anyone who recently contributed to the repository. As there are many things to do, it might be best to avoid huge PRs like I ended up doing and tackling one issue at a time.

@koivunej
Copy link
Collaborator Author

koivunej commented Apr 7, 2021

interledger_packet::packet::fuzzed::fuzzed_0_trailing_bytes

Just realized that there are existing test cases in crates/interledger-packet/src/packet.rs which would suggest that supporting additional trailer bytes is a feature. The tests are inside multiple aspect testing test_try_from named #[test] fns. If this is per the RFCs then the fuzzing target should be fixed and the test removed. I can't really see why this should be supported however and there are no comments on the topic either.

@koivunej
Copy link
Collaborator Author

koivunej commented May 6, 2021

Removed the security label because the above linked PRs haven't contained any security fixes, or even large mallocs, only strictness, or roundtrippability changes.

Strictness: #707, #710, #715, #716
Roundtrippability: #719, #720

The #720 highlights a possible correctness issue with the parsing of leap seconds on every date which should be further investigated.

@koivunej
Copy link
Collaborator Author

The corpus files after minimization in interledger-packet:

$ du -h fuzz/corpus/
492K    fuzz/corpus/packet
436K    fuzz/corpus/address
932K    fuzz/corpus/

btp is around 820K, stream is 724K, ccp is 828K in total. Would probably be great to push them into the repo, have the CI run 100_000 runs of each for good measure or -max_total_time=1s which must be the cputime of the fuzz_target.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant