Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Add check for deserialising hex values over U256 limit #11309

Merged
merged 8 commits into from
Dec 6, 2019
Merged

Add check for deserialising hex values over U256 limit #11309

merged 8 commits into from
Dec 6, 2019

Conversation

lewisbelcher
Copy link
Contributor

Add a length check to return appropriate error when attempting to deserialise hex values over the U256 limit.

Fixes #11268

@parity-cla-bot
Copy link

It looks like @lewisbelcher hasn't signed our Contributor License Agreement, yet.

The purpose of a CLA is to ensure that the guardian of a project's outputs has the necessary ownership or grants of rights over all contributions to allow them to distribute under the chosen licence.
Wikipedia

You can read and sign our full Contributor License Agreement at the following URL: https://cla.parity.io

Once you've signed, please reply to this thread with [clabot:check] to prove it.

Many thanks,

Parity Technologies CLA Bot

@lewisbelcher lewisbelcher changed the title Add check for hex values over U256 limit Add check for deserialising hex values over U256 limit Dec 5, 2019
@lewisbelcher
Copy link
Contributor Author

[clabot:check]

@parity-cla-bot
Copy link

It looks like @lewisbelcher signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

@lewisbelcher
Copy link
Contributor Author

@niklasad1 I can't seem to add reviewers/labels, but could you take a look?

Given that slicing the string to ..66 might give unexpected results (e.g. "0x000000000000000000000000000000000000000000000000000000000000000001" -> 0) I figured the error might be more appropriate.

dvdplm
dvdplm suggested changes Dec 5, 2019
View reviewed changes
Copy link
Collaborator

@dvdplm dvdplm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, many thanks.

Could you add tests that exercise the various failure modes and check that the errors match what we expect?

json/src/uint.rs Outdated Show resolved Hide resolved
@dvdplm dvdplm added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. B0-patch-stable 🕷 Pull request should also be back-ported to the stable branch. B1-patch-beta 🕷🕷 labels Dec 5, 2019
@ordian ordian added this to the 2.7 milestone Dec 5, 2019
@ordian ordian added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Dec 5, 2019
niklasad1
niklasad1 reviewed Dec 5, 2019
View reviewed changes
json/src/uint.rs Outdated Show resolved Hide resolved
niklasad1
niklasad1 reviewed Dec 5, 2019
View reviewed changes
json/src/uint.rs Outdated Show resolved Hide resolved
niklasad1
niklasad1 reviewed Dec 5, 2019
View reviewed changes
json/src/spec/account.rs Outdated Show resolved Hide resolved
lewisbelcher and others added 2 commits December 6, 2019 09:50
@lewisbelcher @niklasad1
Update json/src/uint.rs
57505d2 
Co-Authored-By: Niklas Adolfsson <niklasadolfsson1@gmail.com>
@lewisbelcher @niklasad1
Update json/src/uint.rs
116e264 
Co-Authored-By: Niklas Adolfsson <niklasadolfsson1@gmail.com>
@lewisbelcher
Copy link
Contributor Author

The test account_balance_panics_over_u256_limit came into the picture before uint_deserialization_error_for_hex_too_large @niklasad1. I'll remove it if you think the latter is sufficient.

@lewisbelcher
Remove superfluous serialisation test
472cd9c 
The test `uint_deserialization_error_for_hex_too_large` sufficiently
covers the need for this test.
@lewisbelcher
Merge branch 'check-hex-length-before-deserialise' of github.com:lewi…
6bc93fc 
…sbelcher/parity-ethereum into check-hex-length-before-deserialise
niklasad1
niklasad1 approved these changes Dec 6, 2019
View reviewed changes
Copy link
Collaborator

@niklasad1 niklasad1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, thank you.

@ordian ordian requested a review from dvdplm December 6, 2019 12:12
ordian
ordian reviewed Dec 6, 2019
View reviewed changes
json/src/uint.rs
@@ -120,6 +125,7 @@ pub fn validate_optional_non_zero<'de, D>(d: D) -> Result<Option<Uint>, D::Error
mod test {
use super::Uint;
use ethereum_types::U256;
use serde_json::error::Category;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
use serde_json::error::Category;

@dvdplm dvdplm merged commit a6350c6 into openethereum:master Dec 6, 2019
@lewisbelcher lewisbelcher deleted the check-hex-length-before-deserialise branch December 6, 2019 13:12
dvdplm added a commit that referenced this pull request Dec 9, 2019
@dvdplm
Merge branch 'master' into dp/chore/kvdb-no-default-column
bf1bfea 
* master:
  Istanbul activation on xDai (#11299)
  Istanbul activation on POA Core (#11298)
  Adds support for ipc socket permissions (#11273)
  Add check for deserialising hex values over U256 limit (#11309)
  validate-chainspecs: check istanbul eips are in the foundation spec (#11305)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ordian ordian ordian approved these changes

@niklasad1 niklasad1 niklasad1 approved these changes

@dvdplm dvdplm dvdplm approved these changes

Assignees
No one assigned
Labels
A8-looksgood 🦄 Pull request is reviewed well. B0-patch-stable 🕷 Pull request should also be back-ported to the stable branch. M4-core ⛓ Core client code / Rust.
Projects
None yet
Milestone
2.7
Development

Successfully merging this pull request may close these issues.

Panic on assigning to an account an extraordinary big balance in the chain spec
5 participants
@lewisbelcher @parity-cla-bot @dvdplm @ordian @niklasad1