rlp: input limit and other corrections #711
Conversation
|
Not sure what you mean by optional struct fields exactly, but we MUST have the opporunity to encode and decode nils for pointer types from empty RLP values. |
|
Do you have a use for optional (possibly nil) fields somewhere? I'm fine either way, but after thinking about it for two weeks I can no longer see any justification for being so unsafe by default. |
|
but how is this unsafe. It is exactly like saying go is unsafe cos it initialises pointer types as nil no? :) |
|
We have seen multiple cases where being lenient when decoding RLP can lead to surprising bugs such as transaction malleability. I consider the current semantics unsafe by default because all callers need to expend significant effort to be safe (not exhibit these kinds of bugs). |
fjl
force-pushed
the
rlp-size-validation
branch
from
a2b04f4
to
07ad6a4
Compare
fjl
force-pushed
the
rlp-size-validation
branch
from
43bd49e
to
66aa8ba
Compare
|
@obscuren This can now be looked at. Our tests pass. Syncing the chain doesn't show errors. I have run some of the block tests and they seem to work as well. But please see the linked Yellow Paper issue. The ambiguities in RLP can lead to consensus issues and it would be good to resolve them before launch. |
fjl
force-pushed
the
rlp-size-validation
branch
from
66aa8ba
to
cf2cc3a
Compare
|
rebased on develop. |
This is a preliminary fix for ethereum#420 (SEC-18 RLP decoder unsafe allocation). If a sane input limit is set on the rlp.Stream, it should no longer be possible to cause huge []byte allocations.
It is not safe to add anything to s.size.
A single zero byte carries information and should not set the pointer to nil. This is arguably a corner case. While here, fix the comment to explain pointer reuse.
All integers (including size information in type tags) need to be encoded using the smallest possible encoding. This commit expands the stricter validation introduced for *big.Int in commit 59597d2 to all integer types and size tags.
Input strings of length 1 containing a byte < 56 are non-minimal and should be encoded as a single byte instead. Reject such strings.
The rules have changed as follows: * When decoding into pointers, empty values no longer produce a nil pointer. This can be overriden for struct fields using the struct tag "nil". * When decoding into structs, the input list must contain an element for each field.
We decode into [1]DiscReason in a few places. That doesn't work anymore because package rlp no longer accepts RLP lists for byte arrays.
fjl
force-pushed
the
rlp-size-validation
branch
from
cf2cc3a
to
7180699
Compare
…tedAt-to-UTC-time-string-format Update createdAt, updatedAt to UTC time string format
This is supposed to address #420 and fixes an as-of-yet unreported integer overflow vulnerability in package rlp.
Further changes in this PR will remove the optional struct fields feature, which I now believe is a better fix for #506 than checking for nil pointers all over the codebase. Please do not merge until the
in progresslabel is removed.