-
Notifications
You must be signed in to change notification settings - Fork 120
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Refactor MetaAddr to enable security fixes
Track multiple last used times for each peer: - Add separate untrusted_last_seen, attempt, success, and failed time fields (#1868, #1876, #1848) - Add the new fields to the peer states, so they only appear in states where they are valid - Insert initial DNS seeder peers in the AddressBook in the correct states Create a new MetaAddrChange type for AddressBook changes: - Ignore invalid state changes - Ignore updates to the untrusted last seen time (but update the services field) - If we get a gossiped or alternate change for a seed peer, use the last seen and services info - Once a peer has responded, don't go back to the NeverResponded... states - Update the address book metrics - Optimise getting the next connection address from the address book
- Loading branch information
Showing
10 changed files
with
1,434 additions
and
452 deletions.
There are no files selected for viewing
Large diffs are not rendered by default.
Oops, something went wrong.
Large diffs are not rendered by default.
Oops, something went wrong.
This file was deleted.
Oops, something went wrong.
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,45 +1,57 @@ | ||
| //! Shared test checks for MetaAddr | ||
| use super::super::MetaAddr; | ||
| use super::super::{MetaAddr, PeerAddrState::NeverAttemptedGossiped}; | ||
|
|
||
| use crate::constants::TIMESTAMP_TRUNCATION_SECONDS; | ||
|
|
||
| /// Make sure that the sanitize function reduces time and state metadata | ||
| /// leaks. | ||
| pub(crate) fn sanitize_avoids_leaks(entry: &MetaAddr) { | ||
| let sanitized = entry.sanitize(); | ||
| let sanitized = match entry.sanitize() { | ||
| Some(sanitized) => sanitized, | ||
| // Skip addresses that will never be sent to peers | ||
| None => { | ||
| return; | ||
| } | ||
| }; | ||
|
|
||
| // We want the sanitized timestamp to: | ||
| // - be a multiple of the truncation interval, | ||
| // - have a zero nanoseconds component, and | ||
| // - be within the truncation interval of the original timestamp. | ||
| assert_eq!( | ||
| sanitized.get_last_seen().timestamp() % TIMESTAMP_TRUNCATION_SECONDS, | ||
| 0 | ||
| ); | ||
| assert_eq!(sanitized.get_last_seen().timestamp_subsec_nanos(), 0); | ||
| // handle underflow and overflow by skipping the check | ||
| // the other check will ensure correctness | ||
| let lowest_time = entry | ||
| .get_last_seen() | ||
| .timestamp() | ||
| .checked_sub(TIMESTAMP_TRUNCATION_SECONDS); | ||
| let highest_time = entry | ||
| .get_last_seen() | ||
| .timestamp() | ||
| .checked_add(TIMESTAMP_TRUNCATION_SECONDS); | ||
| if let Some(lowest_time) = lowest_time { | ||
| assert!(sanitized.get_last_seen().timestamp() > lowest_time); | ||
| } | ||
| if let Some(highest_time) = highest_time { | ||
| assert!(sanitized.get_last_seen().timestamp() < highest_time); | ||
| if let Some(sanitized_last) = sanitized.get_last_success_or_untrusted() { | ||
| assert_eq!(sanitized_last.timestamp() % TIMESTAMP_TRUNCATION_SECONDS, 0); | ||
| assert_eq!(sanitized_last.timestamp_subsec_nanos(), 0); | ||
| // handle underflow and overflow by skipping the check | ||
| // the other check will ensure correctness | ||
| let lowest_time = entry | ||
| .get_last_success_or_untrusted() | ||
| .map(|t| t.timestamp().checked_sub(TIMESTAMP_TRUNCATION_SECONDS)) | ||
| .flatten(); | ||
| let highest_time = entry | ||
| .get_last_success_or_untrusted() | ||
| .map(|t| t.timestamp().checked_add(TIMESTAMP_TRUNCATION_SECONDS)) | ||
| .flatten(); | ||
| if let Some(lowest_time) = lowest_time { | ||
| assert!(sanitized_last.timestamp() > lowest_time); | ||
| } | ||
| if let Some(highest_time) = highest_time { | ||
| assert!(sanitized_last.timestamp() < highest_time); | ||
| } | ||
| } | ||
|
|
||
| // Sanitize to the the default state, even though it's not serialized | ||
| assert_eq!(sanitized.last_connection_state, Default::default()); | ||
| // Sanitize to the the gossiped state, even though it's not serialized | ||
| assert!(matches!( | ||
| sanitized.last_connection_state, | ||
| NeverAttemptedGossiped { .. } | ||
| )); | ||
|
|
||
| // We want the other fields to be unmodified | ||
| assert_eq!(sanitized.addr, entry.addr); | ||
| // Services are sanitized during parsing, so we don't need to make | ||
| // any changes in sanitize() | ||
| assert_eq!(sanitized.services, entry.services); | ||
| assert_eq!( | ||
| sanitized.get_untrusted_services(), | ||
| entry.get_untrusted_services() | ||
| ); | ||
| } |
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,28 +1,59 @@ | ||
| //! Test vectors for MetaAddr. | ||
| use super::{super::MetaAddr, check}; | ||
| use super::{ | ||
| super::{MetaAddr, PeerAddrState}, | ||
| check, | ||
| }; | ||
|
|
||
| use chrono::{MAX_DATETIME, MIN_DATETIME}; | ||
|
|
||
| /// Make sure that the sanitize function handles minimum and maximum times. | ||
| #[test] | ||
| fn sanitize_extremes() { | ||
| use PeerAddrState::*; | ||
|
|
||
| zebra_test::init(); | ||
|
|
||
| let min_time_entry = MetaAddr { | ||
| let min_time_untrusted = MetaAddr { | ||
| addr: "127.0.0.1:8233".parse().unwrap(), | ||
| last_connection_state: NeverAttemptedGossiped { | ||
| untrusted_last_seen: MIN_DATETIME, | ||
| untrusted_services: Default::default(), | ||
| }, | ||
| }; | ||
|
|
||
| let min_time_local = MetaAddr { | ||
| addr: "127.0.0.1:8233".parse().unwrap(), | ||
| last_connection_state: Responded { | ||
| last_attempt: MIN_DATETIME, | ||
| last_success: MIN_DATETIME, | ||
| last_failed: Some(MIN_DATETIME), | ||
| untrusted_last_seen: Some(MIN_DATETIME), | ||
| services: Default::default(), | ||
| }, | ||
| }; | ||
|
|
||
| let max_time_untrusted = MetaAddr { | ||
| addr: "127.0.0.1:8233".parse().unwrap(), | ||
| services: Default::default(), | ||
| last_seen: MIN_DATETIME, | ||
| last_connection_state: Default::default(), | ||
| last_connection_state: NeverAttemptedGossiped { | ||
| untrusted_last_seen: MAX_DATETIME, | ||
| untrusted_services: Default::default(), | ||
| }, | ||
| }; | ||
|
|
||
| let max_time_entry = MetaAddr { | ||
| let max_time_local = MetaAddr { | ||
| addr: "127.0.0.1:8233".parse().unwrap(), | ||
| services: Default::default(), | ||
| last_seen: MAX_DATETIME, | ||
| last_connection_state: Default::default(), | ||
| last_connection_state: Responded { | ||
| last_attempt: MAX_DATETIME, | ||
| last_success: MAX_DATETIME, | ||
| last_failed: Some(MAX_DATETIME), | ||
| untrusted_last_seen: Some(MAX_DATETIME), | ||
| services: Default::default(), | ||
| }, | ||
| }; | ||
|
|
||
| check::sanitize_avoids_leaks(&min_time_entry); | ||
| check::sanitize_avoids_leaks(&max_time_entry); | ||
| check::sanitize_avoids_leaks(&min_time_untrusted); | ||
| check::sanitize_avoids_leaks(&min_time_local); | ||
| check::sanitize_avoids_leaks(&max_time_untrusted); | ||
| check::sanitize_avoids_leaks(&max_time_local); | ||
| } |
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
Oops, something went wrong.