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 messages): add limits to rejection message and reason #4687

Merged
merged 6 commits into from
Jun 29, 2022

Conversation

oxarbitrage
Copy link
Contributor

@oxarbitrage oxarbitrage commented Jun 24, 2022

Motivation

Zcash limits the rejection message and the reason to specific constants. We want to do similar for Zebra.

Close #4632

Depends-On: #4714

Solution

Add constants and truncate Mesage::Reject::message to MAX_REJECT_MESSAGE_LENGTH and Mesage::Reject::reason to MAX_REJECT_REASON_LENGTH

Review

I think anyone can review, i left some comments with references to zcashd.

Reviewer Checklist

  • Code implements Specs and Designs
  • Tests for Expected Behaviour
  • Tests for Errors

@oxarbitrage oxarbitrage requested a review from a team as a code owner June 24, 2022 23:11
@oxarbitrage oxarbitrage requested review from teor2345 and removed request for a team June 24, 2022 23:11
@codecov
Copy link

codecov bot commented Jun 25, 2022

Codecov Report

Merging #4687 (cad038e) into main (d37d8aa) will decrease coverage by 0.11%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #4687      +/-   ##
==========================================
- Coverage   78.93%   78.82%   -0.12%     
==========================================
  Files         304      304              
  Lines       37506    37523      +17     
==========================================
- Hits        29604    29576      -28     
- Misses       7902     7947      +45     

Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

Looks great!

We could merge this like it is, but it increases the risk of memory denial of service, and there is an easy fix for that.

zebra-network/src/protocol/external/message.rs Outdated Show resolved Hide resolved
zebra-network/src/protocol/external/message.rs Outdated Show resolved Hide resolved
@teor2345 teor2345 added C-bug Category: This is a bug I-remote-node-overload Zebra can overload other nodes on the network A-network Area: Network protocol updates or fixes labels Jun 28, 2022
Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

Let's merge after the release

mergify bot added a commit that referenced this pull request Jun 29, 2022
@mergify mergify bot merged commit 4543a25 into main Jun 29, 2022
@mergify mergify bot deleted the issue4632 branch June 29, 2022 21:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-network Area: Network protocol updates or fixes C-bug Category: This is a bug I-remote-node-overload Zebra can overload other nodes on the network
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Limit the size of reject network message strings
3 participants