Make MetaAddr.last_seen into a private field and fix sanitize security risk #1942
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
`sanitize` could be misused in two ways: * accidentally modifying the addresses in the address book itself * forgetting to sanitize new fields added to `MetaAddr` This change prevents accidental modification by taking `&self`, and explicitly creates a new sanitized `MetaAddr` with all fields listed.
teor2345
added
C-bug
teor2345
commented
Mar 25, 2021
Comment on lines
-127
to
+177
| /// Sanitize this `MetaAddr` before sending it to a remote peer. | ||
| pub fn sanitize(mut self) -> MetaAddr { | ||
| /// Return a sanitized version of this `MetaAddr`, for sending to a remote peer. | ||
| pub fn sanitize(&self) -> MetaAddr { | ||
| let interval = crate::constants::TIMESTAMP_TRUNCATION_SECONDS; | ||
| let ts = self.last_seen.timestamp(); | ||
| self.last_seen = Utc.timestamp(ts - ts.rem_euclid(interval), 0); | ||
| self.last_connection_state = Default::default(); | ||
| self | ||
| let ts = self.get_last_seen().timestamp(); | ||
| let last_seen = Utc.timestamp(ts - ts.rem_euclid(interval), 0); | ||
| MetaAddr { | ||
| addr: self.addr, | ||
| services: self.services, | ||
| last_seen, | ||
| // the state isn't sent to the remote peer, but sanitize it anyway | ||
| last_connection_state: Default::default(), | ||
| } |
There was a problem hiding this comment.
Here is the security risk fix:
- peers in the address book can't be modified because
sanitizetakes&self - new fields must be sanitized because
sanitizeconstructs a newMetaAddr, listing every field
teor2345
commented
Mar 25, 2021
| services: *services, | ||
| last_seen: *last_seen, | ||
| // the state is Zebra-specific, it isn't part of the Zcash network protocol | ||
| last_connection_state: NeverAttempted, |
There was a problem hiding this comment.
Use explicit NeverAttempted rather than PeerAddrState::default() for readability.
This refactor lets us remove `MetaAddr::update_last_seen()`.
|
Re-running jobs due to a cargo download failure. |
dconnolly
approved these changes
Mar 25, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
In #1849, we want to replace the
MetaAddr.last_seenfield with 3 more specific time fields.This PR prepares for this change by making
last_seeninto a private field.It also modifies the
sanitizefunction so it's harder to misuse.Solution
The code in this pull request has:
Security
In 9bcfb44, this PR fixes a security risk when using
sanitize, where new code could:See my comment for details https://github.com/ZcashFoundation/zebra/pull/1942/files#discussion_r601212080
Review
@oxarbitrage can review this PR. It's urgent because the rest of #1849 depends on it.
This PR is mostly interactive renames and code movement. I've added review comments in the two places that actually change Zebra's behaviour.
Related Issues
First part of #1849
Follow Up Work
Rest of #1849