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

Fuzz or property test deserialization with random bytes #2282

Closed
3 tasks done
Tracked by #3247
teor2345 opened this issue Jun 11, 2021 · 2 comments
Closed
3 tasks done
Tracked by #3247

Fuzz or property test deserialization with random bytes #2282

teor2345 opened this issue Jun 11, 2021 · 2 comments
Labels
A-consensus Area: Consensus rule updates A-network Area: Network protocol updates or fixes A-rust Area: Updates to Rust code C-security Category: Security issues C-testing Category: These are tests I-panic Zebra panics with an internal error message

Comments

@teor2345
Copy link
Contributor

teor2345 commented Jun 11, 2021

Motivation

In Zebra, we do some random struct serialization round-trip property tests (#27).

But we don't take random bytes and try to deserialize them. This means that it's harder to catch security issues like #2263.

API Reference

proptest has Arbitrary random data implementations for fixed-size byte arrays and variable-size byte vectors:
https://docs.rs/proptest/1.0.0/proptest/arbitrary/trait.Arbitrary.html

Solution

For each deserialized type:

  • deserialize a random fixed-size byte array or variable-sized byte vector
    • if deserialization panics, fail the test
  • if deserialization is successful, serialize the type back into bytes, and check that they match

Try to prioritise smaller types over larger types, because a lot of larger types are just aggregates of smaller types.

Issues

Alternatives

If we don't do this, we risk panic security vulnerabilities.

Related Work

Use property testing for message struct serialization round-trips #27 - goes from random types to bytes and back

@teor2345 teor2345 added A-consensus Area: Consensus rule updates A-rust Area: Updates to Rust code C-enhancement Category: This is an improvement S-needs-triage Status: A bug report needs triage NU-5 Network Upgrade: NU5 specific tasks P-Medium C-security Category: Security issues I-panic Zebra panics with an internal error message A-network Area: Network protocol updates or fixes Epic Zenhub Label. Denotes a theme of work under which related issues will be grouped labels Jun 11, 2021
@mpguerra mpguerra removed the S-needs-triage Status: A bug report needs triage label Jun 14, 2021
@teor2345 teor2345 added C-testing Category: These are tests and removed NU-5 Network Upgrade: NU5 specific tasks C-enhancement Category: This is an improvement labels Dec 16, 2021
@mpguerra mpguerra removed the Epic Zenhub Label. Denotes a theme of work under which related issues will be grouped label Jan 27, 2022
@mpguerra
Copy link
Contributor

Converted to an issue

@teor2345
Copy link
Contributor Author

teor2345 commented Mar 1, 2022

This is a useful test, but it's not urgent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-consensus Area: Consensus rule updates A-network Area: Network protocol updates or fixes A-rust Area: Updates to Rust code C-security Category: Security issues C-testing Category: These are tests I-panic Zebra panics with an internal error message
Projects
None yet
Development

No branches or pull requests

2 participants