Add instructions on how to fuzz

[hrxi]

Fixes #2990.

[hrxi]

@personnumber3377 Does this work for you?

[personnumber3377]

Hi @hrxi !

I don't think that works, because you must enable the "fuzz" feature. See the source code in fuzz/src/bin/bitset.rs:

fn main() {
    #[cfg(feature = "fuzz")]
    afl::fuzz!(|data: &[u8]| {
        use nimiq_collections::BitSet;
        use nimiq_serde::Deserialize as _;
        let _ = BitSet::deserialize_from_vec(data);
    })
}

This line here: #[cfg(feature = "fuzz")] signifies that you must also specify the features "fuzz" . I compiled my fuzzers with cargo afl build --features "fuzz" and it works. I am not sure if the --release flag works. I haven't tested that yet. Those original commands in the new README.md file compile the binaries succesfully, but you don't get any coverage, because the afl::fuzz block is actually never executed if you do not compile with --features "fuzz". So instead of having just cargo afl build --release in the documentation, there should be cargo afl build --release --features "fuzz".

I also added some more fuzzers for example I added a fuzzer which fuzzes keypairs and contracts, but I am yet to find any bugs. I attached the source code to these fuzzers in fuzzer_sources.zip.

fuzzer_sources.zip

Edit: Yeah, I just confirmed that the --release flag should work when fuzzing.

[hrxi]

Thanks for noticing the missing --feature fuzz. I added that in the original PR to fix the CI, as the normal build does not like the AFL macros. I subsequently forgot that again when trying to document it now.

Thanks for writing additional fuzzers. Do you want to create a PR for them? Or would you like me to do that for you?

[personnumber3377]

Hi @hrxi !

Sorry for the late reply. I created a pull request here: #3003 . I cleaned up some of my code. It should add a couple of more fuzzers for the source code. Also I have this idea of doing a "round trip" fuzzers, which first deserializes a vector and then serializes the object back into a vector and then checks if the contents of those two vectors are the same. If they aren't then there is a bug, because serializing and deserializing should be inverse operations. I haven't gotten around to implementing these yet.

Edit: Maybe something like this?

fn main() { 
    #[cfg(feature = "fuzz")]
    afl::fuzz!(|data: &[u8]| {
        use nimiq_serde::Deserialize as _;
        use nimiq_account::StakingContract;
        let (contr, err) = StakingContract::deserialize_from_vec(data); // err I think is of type DeserializeError

        // Now check if contr exists. If it does (aka. the original data was a valid staking contract) then try to serialize it back to a vector, then check if the original vector and the new vector are the same, if they aren't then there is a bug in the parsing logic.

        // The existance of error implies that contr does not exist.
        if (err == None) {
            // Contr exists
            assert!(contr, "contr didn't exist, even though err == None!!!!"); // Debug.
            // Now try to serialize to vec
            let serialized = StakingContract::serialize_to_vec();
            // Now check if they are equal:
            assert_eq!(data == serialized, "Round trip failed!");
        }

    })
}

/*
    let contract_2a =
        StakingContract::deserialize_from_vec(&contract_2.serialize_to_vec()).unwrap();

    assert_eq!(contract_2, contract_2a);
*/

[personnumber3377]

This seems to work:

fn main() { 
    #[cfg(feature = "fuzz")]
    afl::fuzz!(|data: &[u8]| {
        use nimiq_serde::{Deserialize, Serialize};
        use nimiq_account::StakingContract;
        let res = StakingContract::deserialize_from_vec(data); // err I think is of type DeserializeError

        // Now check if contr exists. If it does (aka. the original data was a valid staking contract) then try to serialize it back to a vector, then check if the original vector and the new vector are the same, if they aren't then there is a bug in the parsing logic.

        // The existance of error implies that contr does not exist.

        match res {
            Ok(v) => assert_eq!(data, StakingContract::serialize_to_vec(&v)),
            Err(e) => return,
        }

        /*
        if (err == None) {
            // Contr exists
            assert!(contr, "contr didn't exist, even though err == None!!!!"); // Debug.
            // Now try to serialize to vec
            let serialized = StakingContract::serialize_to_vec(contr);
            // Now check if they are equal:
            assert_eq!(data, serialized);
        }
        */


    })
}

/*
    let contract_2a =
        StakingContract::deserialize_from_vec(&contract_2.serialize_to_vec()).unwrap();

    assert_eq!(contract_2, contract_2a);
*/

That compiles, but doesn't fuzz correctly, since there is this comment:

pub trait Deserialize: serde::de::DeserializeOwned {
    /// Deserialize from bytes. Extra data may remain at the end.
    fn deserialize_from_vec(bytes: &[u8]) -> Result<Self, DeserializeError> {
        postcard::from_bytes(bytes).map_err(DeserializeError::from)
    }

in serde/src/lib.rs. This means that these kinds of inputs:

oof@oof-h8-1440eo:~/core-rs-albatross$ ./target/release/staking_contract < findings/default/crashes/id\:000000\,sig\:06\,src\:000007\,time\:690013\,execs\:12723168\,op\:havoc\,rep\:6 
thread 'main' panicked at fuzz/src/bin/staking_contract.rs:13:22:
assertion `left == right` failed
  left: [0, 0, 0, 0, 18, 0, 0, 0, 0, 0, 0, 0, 2, 226, 40, 0, 2, 0, 63]
 right: [0, 0, 0, 0, 18, 0, 0, 0, 0, 0, 0]
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
Aborted

Are interpreted as crashes. I think this can be solved by "clipping" the longer matrix of the two to the same length as the shorter one. This makes the comparison valid.

This seems to work:

fn main() { 
    #[cfg(feature = "fuzz")]
    afl::fuzz!(|data: &[u8]| {
        use nimiq_serde::{Deserialize, Serialize};
        use nimiq_account::StakingContract;
        let res = StakingContract::deserialize_from_vec(data); // err I think is of type DeserializeError

        // Now check if contr exists. If it does (aka. the original data was a valid staking contract) then try to serialize it back to a vector, then check if the original vector and the new vector are the same, if they aren't then there is a bug in the parsing logic.

        // The existance of error implies that contr does not exist.

        match res {
            Ok(v) => {
                // First calculate the minimum of the two.
                let otherdata = StakingContract::serialize_to_vec(&v);

                /*
                let buf = (0..64).collect::<Vec<u8>>();
                let z: &[u8; 64] = &(&buf[..]).try_into().unwrap();
                */

                assert!((otherdata.len() <= data.len()), "The size of the serialized version was bigger than the original vector! This shouldn't happen!");
                let original_data: &[u8] = data[..(otherdata.len())].try_into().unwrap(); // Yuck!!!

                assert_eq!(original_data, otherdata);
            },
            Err(e) => {
                // println!("Error thing!!!\n");
                return;
            },
        }

        /*
        if (err == None) {
            // Contr exists
            assert!(contr, "contr didn't exist, even though err == None!!!!"); // Debug.
            // Now try to serialize to vec
            let serialized = StakingContract::serialize_to_vec(contr);
            // Now check if they are equal:
            assert_eq!(data, serialized);
        }
        */


    })
}

/*
    let contract_2a =
        StakingContract::deserialize_from_vec(&contract_2.serialize_to_vec()).unwrap();

    assert_eq!(contract_2, contract_2a);
*/

I added these changes in this commit: 8ab70ba on my own fork.

Of course the code needs a bit of cleanup but that is the basic premise. Then that just needs to be implemented for the other types. Also if I find such a "round trip" vulnerability, is that vulnerability applicable to your bug bounty program?

[personnumber3377]

I actually think that it is just easier to compare the objects instead of the bytearrays like so:

fn main() { 
    #[cfg(feature = "fuzz")]
    afl::fuzz!(|data: &[u8]| {
        use nimiq_serde::{Deserialize, Serialize};
        use nimiq_account::StakingContract;
        let res = StakingContract::deserialize_from_vec(data);
        // The existance of error implies that contr does not exist.
        match res {
            Ok(v) => {
                let otherdata = StakingContract::serialize_to_vec(&v); // Serialize to a vector
                let round_trip = StakingContract::deserialize_from_vec(&otherdata); // Deserialize back
                assert_eq!(v, round_trip.unwrap()); // The contracts should be the same and if not, then there is a "round-trip" bug.
            },
            Err(e) => {
                return;
            },
        }
    })
}

That seems like a lot smarter solution to the "round-trip" fuzzer than what I was doing before.

[jsdanielh]

Rebasing branch to merge it