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

Limit input length for decode command #982

Merged
merged 8 commits into from Mar 2, 2023
Merged

Conversation

SkymanOne
Copy link
Contributor

@SkymanOne SkymanOne commented Feb 24, 2023

Closes #980

The hex bytes are consumed for decoding values of arguments on demand. If there is not enough data to fill the buffer, the error is already thrown. Previously, spare bytes were just ignored. Since this PR, an error will be thrown if input buffer contains spare bytes.

Here is the small demo demonstrating that extra bytes in buffer do not affect decoding of value.

//boolean is 1 byte long
let value = true;
let encoded: Vec<u8> = vec![1, 12, 3];
let mut encoded: &[u8] = encoded.as_ref();
let decoded: bool = bool::decode::<&[u8]>(&mut encoded).unwrap();
// is true
assert_eq!(decoded, value);
//prints [12, 3]
println!("{encoded:?}")

Copy link
Collaborator

@ascjones ascjones left a comment

Choose a reason for hiding this comment

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

The alternative approach would have been to do the bytes length check just once directly in decode.rs after the decoding is complete. This way is also okay, with adding it to each of the called methods, although as mentioned decode_contract_event is used in one other place so we need to check whether that still works.

crates/transcode/src/lib.rs Outdated Show resolved Hide resolved
@@ -347,6 +353,24 @@ impl ContractMessageTranscoder {
Ok(Value::Unit)
}
}

/// Checks if buffer empty, otherwise returns am error
fn validate_length(data: &[u8], label: &str, args: &[(Value, Value)]) -> Result<()> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Extracted check to a separate function instead of moving it to a higher layer because each decode function returns a generic anyhow error. We would need to return a custom error and map all anyhow errors inside each decode function to a specific error. We will also need to handle the error on the higher decode function. Additionally, events and messages/constructors have different structs that are used to extract label. That will also unnecessary overcomplicate error handling.

Therefore, it is better that all validation happens on the lower level at each decode function independently.

Copy link
Collaborator

@ascjones ascjones left a comment

Choose a reason for hiding this comment

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

LGTM

crates/transcode/src/util.rs Outdated Show resolved Hide resolved
@SkymanOne SkymanOne merged commit d0613d5 into master Mar 2, 2023
@SkymanOne SkymanOne deleted the gn/limit-decode-input branch March 2, 2023 12:38
@ascjones
Copy link
Collaborator

ascjones commented Mar 2, 2023

Since @HCastano had also reviewed this, would also have been nice to ask for his approval too.

This was referenced Mar 10, 2023
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.

Lack of input validation in the decode command
3 participants