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

Make bool's deserialization non-malleable #112

Merged
merged 4 commits into from
Dec 11, 2020
Merged

Conversation

ValarDragon
Copy link
Member

Description

Makes the deserialization of bool non-malleable. Now the only strings that can be deserialized as a boolean are the encodings of 0u8, 1u8.

closes: #95

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (main)
  • Linked to Github issue with discussion and accepted design OR have an explanation in the PR that describes this work.
  • Wrote unit tests
  • Updated relevant documentation in the code
  • Added a relevant changelog entry to the Pending section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer

@Pratyush
Copy link
Member

Pratyush commented Dec 6, 2020

Small comment: instead of arbitrarily mutating the data, why not try mutating it to every possible byte value? That’ll remove the dependence on ark-ff

@ValarDragon
Copy link
Member Author

ValarDragon commented Dec 7, 2020

That works for testing bool, but wouldn't generalize if we wanted to test this for any data type with a larger serialization.

We have no other data type implemented in serialize (AFAIK) which has a non-1-to-1 encoding to test for atm, so maybe that point is moot. (Also for the current API to generalize well, we'd probably need to change valid_mutation to return type T instead of a valid/invalid boolean)

Maybe it'd make sense to refactor this method out of tests, and do the above change, so other types (e.g. Fields) can check this if they wanted to?

serialize/Cargo.toml Outdated Show resolved Hide resolved
@Pratyush Pratyush changed the title Make bool's deserialization non-malleable Make bool's deserialization non-malleable Dec 11, 2020
@Pratyush Pratyush merged commit 54c1627 into master Dec 11, 2020
@Pratyush Pratyush deleted the bool_canonical_encoding branch December 11, 2020 17:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Too liberal bool deserialize?
2 participants