search

interledger / interledger-rs

An easy-to-use, high-performance Interledger implementation written in Rust
http://interledger.rs
Other
201 stars 70 forks source link

Continued fuzzing and hardening #705

Open koivunej opened 3 years ago

koivunej commented 3 years ago

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:

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 e6f21c9b486b8397e0d0a961a2148646ff1f987b.~ 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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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.