[NCC-E005955-MMC] zebra-network: Uncaught Nonce Reuse and Fragile Nonce Cache Eviction

[mpguerra]

Impact

Failure to check if the nonce cache already contains the new nonce may prevent the self-connection detection from working as intended. Incorrectly evicting an entry from the nonce cache will have the same effect.

Description

The function negotiate_version() in zebra-network/src/peer/handshake.rs is used to negotiate the network version used when connecting to a new peer. In order to match outgoing messages with incoming responses, a nonce is included, and cached locally to help identify self-connection attempts:

zebra/zebra-network/src/peer/handshake.rs

Lines 584 to 591 in 5a88fe7

	// Create a random nonce for this connection
	let local_nonce = Nonce::default();
	// # Correctness
	//
	// It is ok to wait for the lock here, because handshakes have a short
	// timeout, and the async mutex will be released when the task times
	// out.
	nonces.lock().await.insert(local_nonce);

It was observed that the return value of the insert function is not checked. This function returns true if the value was successfully inserted, and false if the value was already contained in the set. Therefore, nonce reuse could be detected at this step by checking the return value of this function. The above freshly generates each nonce via Nonce::default(), so the probability of a collision is negligible, but it is nevertheless recommended to check the result of the insert operation as a precaution.

Later, in the same function, the following code handles received nonces:

zebra/zebra-network/src/peer/handshake.rs

Lines 677 to 693 in 5a88fe7

	// Check for nonce reuse, indicating self-connection
	//
	// # Correctness
	//
	// We must wait for the lock before we continue with the connection, to avoid
	// self-connection. If the connection times out, the async lock will be
	// released.
	let nonce_reuse = {
	let mut locked_nonces = nonces.lock().await;
	let nonce_reuse = locked_nonces.contains(&remote.nonce);
	// Regardless of whether we observed nonce reuse, clean up the nonce set.
	locked_nonces.remove(&local_nonce);
	nonce_reuse
	};
	if nonce_reuse {
	Err(HandshakeError::NonceReuse)?;
	}

If the incoming message contains a nonce currently in the cache, that means it corresponds to a negotiation version message originating from itself, indicating a self-connection attempt. When this occurs, an error is returned and the nonce associated with the request is evicted from the cache.

Earlier, this finding highlighted a potential case where nonces could be re-used without correct detection. If such a nonce is evicted after the first connection attempt then a second connection attempt might succeed. Building on this observation, a malicious party may attempt to replay messages or craft messages containing an observed nonce, thereby
causing the nonce to be evicted incorrectly. Subsequently, a self-connection attempt would not be correctly identified.

Based on the above, it is recommended that the approach to nonce handling in this use case be re-evaluated to ensure the intended security goals are met. It may be safer to cache all nonces for a set duration to ensure that malicious or incorrect behavior cannot force nonce eviction from the cache.

Recommendation

• Consider adding an explicit check for nonce reuse when adding values to the cache and returning HandshakeError::NonceReuse as necessary.
• Consider caching nonce entries for a longer period of time to ensure that nonces are not prematurely evicted from the cache.

Location

zebra-network/src/peer/handshake.rs

[mpguerra]

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

[teor2345]

The above freshly generates each nonce via Nonce::default(), so the probability of a collision is negligible, but it is nevertheless recommended to check the result of the insert operation as a precaution.

In particular, nonces use 64 bits of randomness, so it is unlikely we will make 2^32 connections on the same node and get a collision:
https://en.wikipedia.org/wiki/Birthday_problem#Square_approximation