fix: disallow leading zeros in RLP decoding #335
Conversation
|
This would be a breaking behavior change. Is there a motivating reason? |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #335 +/- ##
==========================================
- Coverage 80.48% 80.46% -0.03%
==========================================
Files 54 54
Lines 6140 6179 +39
==========================================
+ Hits 4942 4972 +30
- Misses 1198 1207 +9
☔ View full report in Codecov by Sentry. |
Sorry, should have been more specific in the PR description. The main motivation is that leading zeros on RLP uints is defined to be invalid, and encoding would also never output uints encoded this way. This also makes writing certain kinds of fuzz tests possible, specifically fuzz tests that:
If we accept invalid RLP input, and are more permissive in decoding than encoding as a result, these fuzz tests quickly fail and do not act as meaningful fuzz tests. |
|
@prestwich just bumping, any thoughts on this given the context? |
|
The ethereum.org spec is clear as mud, but the yellowpaper appears to be unambiguous about this (see (197)) . Accepting the non-minimal uint is off-spec, so we should fix it. However, because this silently changes behavior in a way that breaks downstream users, we should release it as a major version bump |
| // leading zeros | ||
| if !bytes.is_empty() && bytes[0] == 0 { | ||
| return Err(Error::LeadingZero); | ||
| } |
There was a problem hiding this comment.
RLP spec explicitly says
integers must be represented in big-endian binary form with no leading zeroes (thus making the integer value zero equivalent to the empty byte array).
meaning that the old accepting behaviour was not according to spec and a bug. Fixing bugs is not a breaking change.
There's 'robustness principle' argument that could be made to make this error optionally a warning (i.e. introduce a non-strict mode). But that seems more effort than it's worth. Unless someone volunteers the strict mode should be default.
This proposed test assumes that encoding is canonical. This assumption is only true when leading zeros are rejected.
Both yellow paper and ethereum.org spec clearly state leading zeros are an error. While neither mention it explicitly, the implicit goal is to make it such that there is only one canonical RLP encoding. This is important since it RLP encoding is often used to hash data structures. Having multiple valid hashes for the same data structure leads to all forms of madness. |
The accepting behavior was a bug, any downstream users depending on this bug are in error (as is clear by the RLP spec). IMO this a bug fix and not a breaking change. |
|
I've also submitted a PR to the RLP spec doc on the Ethereum website to disambiguate this the worst part of this is how it breaks the layer boundary between RLP and "higher-order" protocols. You cannot accurately decode RLP without knowing whether the higher-order protocol contains positive integers |
|
Updated the changelog and made other requested changes, @prestwich mind reviewing again? |
good catch, will do |
Motivation
Currently
UintRLP decoding ignores leading zeros.Solution
Disallow leading zeros.
PR Checklist