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

[NCC-E005955-VM7] zebra-network: Off-by-One Error in zebra-network Retry Parameter #6393

Closed
Tracked by #6277
mpguerra opened this issue Mar 23, 2023 · 0 comments · Fixed by #6460
Closed
Tracked by #6277

[NCC-E005955-VM7] zebra-network: Off-by-One Error in zebra-network Retry Parameter #6393

mpguerra opened this issue Mar 23, 2023 · 0 comments · Fixed by #6460
Assignees
Labels
A-network Area: Network protocol updates or fixes C-audit Category: Issues arising from audit findings C-cleanup Category: This is a cleanup

Comments

@mpguerra
Copy link
Contributor

mpguerra commented Mar 23, 2023

Impact

The parameter MAX_SINGLE_PEER_RETRIES may not behave as expected due to an off-by-one error.

Description

Finding Off-by-One Errors and Inconsistent Usage of PARAMETER_DOWNLOAD_MAX_RETRIES documented an off-by-one issue with retry behavior in zebra-consensus. This finding documents a similar issue in zebra-network. Note that this does not represent a security issue or vulnerability, outside of potential user error during configuration.

The following code is part of peer DNS resolution, where each peer is resolved individually, and the complete process is repeated if no resolution is successful. The parameter MAX_SINGLE_PEER_RETRIES implies that it dictates the number of retry attempts for a given peer.

/// The number of times Zebra will retry each initial peer's DNS resolution,
/// before checking if any other initial peers have returned addresses.
const MAX_SINGLE_PEER_RETRIES: usize = 1;

// We retry each peer individually, as well as retrying if there are
// no peers in the combined list. DNS failures are correlated, so all
// peers can fail DNS, leaving Zebra with a small list of custom IP
// address peers. Individual retries avoid this issue.
let peer_addresses = peers
.iter()
.map(|s| Config::resolve_host(s, MAX_SINGLE_PEER_RETRIES))
.collect::<futures::stream::FuturesUnordered<_>>()
.concat()
.await;

The retry function loops using for retry_count in 1..=max_retries, meaning that the value MAX_SINGLE_PEER_RETRIES will result in 1 single attempt and not 1 retry. Therefore, this parameter may be better named as MAX_SINGLE_PEER_ATTEMPTS or the behavior could be updated to loop one additional time.

Recommendation

Ensure the naming of the parameter accurately reflects its usage. Either rename the parameter to reflect the maximum number of connection attempts or revise the behavior to reflect its current name.

Location

zebra-network/src/config.rs

@mpguerra mpguerra added this to Zebra Mar 23, 2023
@github-project-automation github-project-automation bot moved this to 🆕 New in Zebra Mar 23, 2023
@mpguerra mpguerra added C-cleanup Category: This is a cleanup S-needs-triage Status: A bug report needs triage P-Medium ⚡ A-network Area: Network protocol updates or fixes C-audit Category: Issues arising from audit findings labels Mar 23, 2023
@teor2345 teor2345 self-assigned this Apr 3, 2023
@mergify mergify bot closed this as completed in #6460 Apr 4, 2023
@github-project-automation github-project-automation bot moved this from 🆕 New to ✅ Done in Zebra Apr 4, 2023
@mpguerra mpguerra removed the S-needs-triage Status: A bug report needs triage label Apr 19, 2023
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-audit Category: Issues arising from audit findings C-cleanup Category: This is a cleanup
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants