[NCC-E005955-7DU] zebra-network: Fragile State Transition During Address Book Update

[mpguerra]

Impact

Failure to reject out of order address change requests corrupts the Address Book’s state and opens the Zebra node to state manipulation attacks.

Description

The zebra_network’s AddressBook update implementation uses MetaAddrChange’s apply_to_meta_addr() to update the entry’s previous state to the received updated state.
The apply_to_meta_addr() function validates the change against the previous state and optionally returns the new MetaAddr. If the received state is not the never-attempted state (the else condition on line 831) the current state is one of { AttemptPending, Responded, Failed}. In order to tolerate an address change request that is received out of order, the implementation picks the maximum of { last_response, last_attempt, last_failure} timestamps. Thus these timestamps will never revert to their previous values. However, independent of what the previous state was, on line 853, the new address state is returned. The last_connection_state records the outcome of local node’s most recent
communication attempt with this peer:

zebra/zebra-network/src/meta_addr.rs

Lines 785 to 860 in 5a88fe7

	/// Apply this change to a previous `MetaAddr` from the address book,
	/// producing a new or updated `MetaAddr`.
	///
	/// If the change isn't valid for the `previous` address, returns `None`.
	pub fn apply_to_meta_addr(&self, previous: impl Into<Option<MetaAddr>>) -> Option<MetaAddr> {
	if let Some(previous) = previous.into() {
	assert_eq!(previous.addr, self.addr(), "unexpected addr mismatch");
	let previous_has_been_attempted = !previous.last_connection_state.is_never_attempted();
	let change_to_never_attempted = self
	.into_new_meta_addr()
	.map(|meta_addr| meta_addr.last_connection_state.is_never_attempted())
	.unwrap_or(false);
	if change_to_never_attempted {
	if previous_has_been_attempted {
	// Existing entry has been attempted, change is NeverAttempted
	// - ignore the change
	//
	// # Security
	//
	// Ignore NeverAttempted changes once we have made an attempt,
	// so malicious peers can't keep changing our peer connection order.
	None
	} else {
	// Existing entry and change are both NeverAttempted
	// - preserve original values of all fields
	// - but replace None with Some
	//
	// # Security
	//
	// Preserve the original field values for NeverAttempted peers,
	// so malicious peers can't keep changing our peer connection order.
	Some(MetaAddr {
	addr: self.addr(),
	services: previous.services.or_else(|| self.untrusted_services()),
	untrusted_last_seen: previous
	.untrusted_last_seen
	.or_else(|| self.untrusted_last_seen()),
	// The peer has not been attempted, so these fields must be None
	last_response: None,
	last_attempt: None,
	last_failure: None,
	last_connection_state: self.peer_addr_state(),
	})
	}
	} else {
	// Existing entry and change are both Attempt, Responded, Failed
	// - ignore changes to earlier times
	// - update the services from the change
	//
	// # Security
	//
	// Ignore changes to earlier times. This enforces the peer
	// connection timeout, even if changes are applied out of order.
	Some(MetaAddr {
	addr: self.addr(),
	// We want up-to-date services, even if they have fewer bits,
	// or they are applied out of order.
	services: self.untrusted_services().or(previous.services),
	// Only NeverAttempted changes can modify the last seen field
	untrusted_last_seen: previous.untrusted_last_seen,
	// Since Some(time) is always greater than None, `max` prefers:
	// - the latest time if both are Some
	// - Some(time) if the other is None
	last_response: self.last_response().max(previous.last_response),
	last_attempt: self.last_attempt().max(previous.last_attempt),
	last_failure: self.last_failure().max(previous.last_failure),
	last_connection_state: self.peer_addr_state(),
	})
	}
	} else {
	// no previous: create a new entry
	self.into_new_meta_addr()
	}
	}

Recommendation

Update apply_to_meta_addr() to return None when the state transition is invalid, e.g., the request is received out-of-order.

Location

zebra-network/src/meta_addr.rs, line 853

[mpguerra]

Hey team! Please add your planning poker estimate with Zenhub @arya2 @conradoplg @dconnolly @oxarbitrage @teor2345 @upbqdn

[teor2345]

This suggested solution won't quite work, because incoming network messages from the same peer can be processed in parallel by different tasks. So we need to:

• add times to each message
• compare the times within some tolerance
• prioritise the "worst/latest" peer state if the times are close (Failed, Responded, AttemptPending)

[teor2345]

There's also a separate fix needed to avoid one peer overwhelming the shared address book update channel.