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

Add sanity check to ensure that all data decoded by parity_scale_codec are consumed #2127

Closed
appetrosyan opened this issue Apr 20, 2022 · 2 comments
Assignees
Labels
Bug Something isn't working iroha2-dev The re-implementation of a BFT hyperledger in RUST Security This issue asks for improved security

Comments

@appetrosyan
Copy link
Contributor

I don't think this is an issue at all

Marin, [20.04.22 12:54]
the byte representations match for VersionedQueryResult and paginated version

Marin, [20.04.22 12:54]
to a point

Marin, [20.04.22 12:54]
if it was the other way around, it would fail. If you were encoding QueryResult and decoding PaginatedQueryResult

Marin, [20.04.22 12:55]
if there was something after if in the encoded form, it would fail as well

Aleksandr Petrosyan, [20.04.22 12:55]
[Forwarded from Aleksandr Petrosyan]
Thankfully it doesn't decode QueryResult, but the fact that it happily works with types containing VersionedQueryResult is a little worrying

Marin, [20.04.22 12:55]
but seems that scale happily disregards trailing bytes if it finished decoding an object

Aleksandr Petrosyan, [20.04.22 12:56]
[In reply to Marin]
It works more like a parser than a binary serialisation format.

Marin, [20.04.22 12:56]
we can inject a check I guess

Marin, [20.04.22 12:56]
to see if all bytes were consumed

Marin, [20.04.22 12:57]
check that parity_scale_codec::Input is depleted

Aleksandr Petrosyan, [20.04.22 12:58]
Letting unrecognised data into a blockchain is a ticking time-bomb.
@appetrosyan appetrosyan added iroha2-dev The re-implementation of a BFT hyperledger in RUST Security This issue asks for improved security labels Apr 20, 2022
@mversic
Copy link
Contributor

mversic commented Apr 20, 2022

I'll give an example of what's happening here. Let's say we have two structures:

struct Struct1 {
    a: u32,
    b: u16,
}
struct Struct2 {
    a: u32,
}

where Struct2 is a subtype of the Struct1 in the encoded format meaning that all valid encodings of Struct1 will successfully decode into Struct2 and there will always be 2 trailing bytes left at the end of the encoded array. I'm not sure this is a security issue or how could this be exploited, or if it's a feature or a bug.

If this is not a feature that we need/want then we have to check that the parity_scale_codec::Input is depleted after decoding. This can be done by wrapping the Decode::decode<I: Input>(input: &mut I) method with a wrapper and make sure that all the decode calls go through the wrapper

@appetrosyan appetrosyan added the Bug Something isn't working label May 4, 2022
@Arjentix
Copy link
Contributor

Arjentix commented Jun 14, 2022

I think we can do it by introducing our own macro for (de-)coding and use it in derives

@Arjentix Arjentix self-assigned this Jun 14, 2022
Arjentix added a commit that referenced this issue Jun 27, 2022
…`parity_scale_codec` is consumed (#2378)

Signed-off-by: Daniil Polyakov <arjentix@gmail.com>
BAStos525 pushed a commit to BAStos525/soramitsu-iroha that referenced this issue Jul 8, 2022
…decoded by `parity_scale_codec` is consumed (hyperledger#2378)

Signed-off-by: Daniil Polyakov <arjentix@gmail.com>
Signed-off-by: BAStos525 <jungle.vas@yandex.ru>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working iroha2-dev The re-implementation of a BFT hyperledger in RUST Security This issue asks for improved security
Projects
None yet
Development

No branches or pull requests

3 participants