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

fix(network): validate addr length before reading #6320

Merged
merged 1 commit into from
Mar 15, 2023
Merged

fix(network): validate addr length before reading #6320

merged 1 commit into from
Mar 15, 2023

Conversation

oxarbitrage
Copy link
Contributor

@oxarbitrage oxarbitrage commented Mar 14, 2023

Motivation

We want to validate addr length before reading and deserializing the addr itself.

Close #6280

Solution

There is a sizeAddr field in the spec that we can use first to validate. Then we use zcash_deserialize_bytes_external_count to read the address with the length we already have.

Review

Not sure if there is a better alternative. Anyone can review.

We already have tests for this, for example parses_msg_addr_v2_ip.

Reviewer Checklist

  • Will the PR name make sense to users?
    • Does it need extra CHANGELOG info? (new features, breaking changes, large changes)
  • Are the PR labels correct?
  • Does the code do what the ticket and PR says?
    • Does it change concurrent code, unsafe code, or consensus rules?
  • How do you know it works? Does it have tests?

Follow Up Work

@oxarbitrage oxarbitrage requested a review from a team as a code owner March 14, 2023 19:28
@oxarbitrage oxarbitrage requested review from upbqdn and removed request for a team March 14, 2023 19:28
@github-actions github-actions bot added C-bug Category: This is a bug C-feature Category: New features labels Mar 14, 2023
@oxarbitrage oxarbitrage self-assigned this Mar 14, 2023
@oxarbitrage oxarbitrage added C-security Category: Security issues C-audit Category: Issues arising from audit findings labels Mar 14, 2023
@codecov
Copy link

codecov bot commented Mar 14, 2023

Codecov Report

Merging #6320 (da2b75c) into main (93c702a) will increase coverage by 0.03%.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6320      +/-   ##
==========================================
+ Coverage   77.80%   77.84%   +0.03%     
==========================================
  Files         304      304              
  Lines       39525    39528       +3     
==========================================
+ Hits        30754    30772      +18     
+ Misses       8771     8756      -15     

Copy link
Member

@upbqdn upbqdn left a comment

Choose a reason for hiding this comment

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

All looks great to me.

mergify bot added a commit that referenced this pull request Mar 14, 2023
@mergify mergify bot merged commit ccb7722 into main Mar 15, 2023
@mergify mergify bot deleted the issue6280 branch March 15, 2023 01:49
@mpguerra mpguerra mentioned this pull request Mar 16, 2023
36 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-audit Category: Issues arising from audit findings C-bug Category: This is a bug C-feature Category: New features C-security Category: Security issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[NCC-E005955-HV6] zebra-network: Buffer length validation after memory allocation
2 participants