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

Securize ConsensusContext Deserialization #618

Merged
merged 5 commits into from
Mar 5, 2019

Conversation

shargon
Copy link
Member

@shargon shargon commented Mar 4, 2019

No description provided.

Copy link
Contributor

@jsolman jsolman left a comment

Choose a reason for hiding this comment

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

I originally had some safeguards like this; however Erik made the point that it wasn’t necessary for the consensus context, since the consensus context itself is never sent over the network it is only serialized and deserialized locally from the same node. So there should not be any vulnerability by not checking these maximums on deserialization.

@shargon
Copy link
Member Author

shargon commented Mar 4, 2019

This kind of checks is necessary, there is no reason for don't establish a safe maximum, although the process is trustable

@jsolman
Copy link
Contributor

jsolman commented Mar 4, 2019

While, I don’t think this fixes any actual security vulnerability in this case, I agree with being defensive on the deserialization and having the safeguards, so I approved. It is a general good practice to always validate input.

Copy link
Contributor

@igormcoelho igormcoelho left a comment

Choose a reason for hiding this comment

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

I agree that this should become the standard practice. Although not necessary in this situation, it also helps semantically understanding the maximum limit on each field.

@jsolman jsolman merged commit eb26278 into neo-project:master Mar 5, 2019
@shargon shargon deleted the small-fix branch March 5, 2019 08:32
Thacryba pushed a commit to simplitech/neo that referenced this pull request Feb 17, 2020
* 2.9.0

* updates for 2.9.0

* Update v2.9.0.md (neo-project#610)

Adjustment for instruction of getting nep-5 applicationlog.

* Update invokescript.md (neo-project#613)

Add tx

* Update invokefunction.md (neo-project#612)

Add tx for response.

* Create getwalletheight (neo-project#611)

Add getwalletheight api.

* updates for 2.9.0

* minor updates

* Update setup.md (neo-project#617)

Add introduction of Plugins.

* Update v2.9.0.md (neo-project#615)

Add introduction for setting config.json

* final updates

* Update v2.9.0.md (neo-project#618)

Add notes for install plugins.

* plugin related
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.

3 participants