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

[Fuzzing] Crash on nfuzz_attestation #659

Open
mratsim opened this issue Dec 23, 2019 · 6 comments
Open

[Fuzzing] Crash on nfuzz_attestation #659

mratsim opened this issue Dec 23, 2019 · 6 comments
Labels
bug

Comments

@mratsim
Copy link
Member

@mratsim mratsim commented Dec 23, 2019

See the fuzzing input pinned in the fuzzing discord channel.

@mratsim mratsim added the bug label Dec 23, 2019
@arnetheduck

This comment has been minimized.

Copy link
Member

@arnetheduck arnetheduck commented Dec 23, 2019

attach files here, note version?

@gnattishness

This comment has been minimized.

Copy link
Contributor

@gnattishness gnattishness commented Dec 27, 2019

A bit annoying I can't attach the ssz directly:
nim_attestation_crash.ssz

This is a BeaconState + Attestation ssz container that, when passed to nfuzz_attestation, triggers the following assertion:
nim_attestation_trace.txt

@djrtwo

This comment has been minimized.

Copy link
Contributor

@djrtwo djrtwo commented Dec 31, 2019

I haven't investigated this scenario, but I assume this highlights a gap in our consensus tests.
Am I correct in that assumption?

Also, can someone add me to the fuzzing discord?

@gnattishness

This comment has been minimized.

Copy link
Contributor

@gnattishness gnattishness commented Jan 2, 2020

I think I've narrowed it down:
I can't see any equivalent in nim-beacon-chain for the following
assert data.index < get_committee_count_at_slot(state, data.slot) (https://github.com/ethereum/eth2.0-specs/blob/dev/specs/core/0_beacon-chain.md#attestations).

Because a larger index is allowed through, index > count is possible in compute_committee, and thus an endIdx > index_count.

@gnattishness

This comment has been minimized.

Copy link
Contributor

@gnattishness gnattishness commented Jan 2, 2020

@djrtwo I'm not certain re concensus tests, as this is effectively a "crash" of nim-beacon-chain, when it should return false (allowing nimbus to continue running).
If it's possible for the consensus tests to differentiate between an error return and a crash, then this could be a gap in the tests.

In pyspec, this would be triggering an AssertionError anyway. If the above understanding of the bug is correct, an equivalent bug in the pyspec would be for assert data.index < get_committee_count_at_slot(state, data.slot) to be missing from process_attestation, and the input triggers a different Assert in compute_shuffled_index: assert index < index_count

If the tests are only checking whether an assert is triggered, they would not be able to differentiate between these.

Can someone confirm whether my understanding is correct?

@djrtwo

This comment has been minimized.

Copy link
Contributor

@djrtwo djrtwo commented Jan 2, 2020

Interesting, I see.

That said, this codepath is clearly not triggered in Nimbus during the consensus tests. It seems likely that we can and should craft a consensus test that would trigger this code path.

I'll monitor this thread and make a call when y'all post a fix

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.