Skip to content

Conversation

sashaodessa
Copy link

Replaces unsafe port-finding functions with secure bound socket APIs to eliminate race conditions.

Problem

The existing unused_tcp*_port() and unused_udp*_port() functions had a classic Time-of-Check-Time-of-Use (TOCTOU) vulnerability:

  • Functions would bind to port 0, get the assigned port number, then immediately close the socket
  • Between getting the port number and later binding to it, another process could claim the same port
  • The LRU cache provided false security and didn't prevent the race condition

Solution

Added new safe APIs that return already-bound sockets:

  • bind_tcp4_any() / bind_tcp6_any() → returns TcpListener
  • bind_udp4_any() / bind_udp6_any() → returns UdpSocket

* Add co-author to mergify commits.

* Remove unnecessary pull request rules from mergify config.

* Revert automation removals
Copy link

cla-assistant bot commented Sep 9, 2025

CLA assistant check
All committers have signed the CLA.

Copy link

cla-assistant bot commented Sep 9, 2025

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@michaelsproul
Copy link
Member

Thanks!

We are aware of this issue and had been phasing these functions out in favour of using 0 ports explicitly. The hard part is replacing the uses of the old functions in code that assumes it can determine an unused port without binding a socket, e.g. here:

let port = unused_tcp4_port().expect("Unable to find unused port.");
CommandLineTest::new()
.flag("port", Some(port.to_string().as_str()))
.flag("allow-insecure-genesis-sync", None)
.run()
.with_config(|config| {
assert_eq!(
config.network.listen_addrs().v4().map(|listen_addr| (
listen_addr.disc_port,
listen_addr.quic_port,
listen_addr.tcp_port
)),
// quic_port should be (tcp_port + 1) if tcp_port is given as non-zero.
Some((port, port + 1, port))
);
});

If you would be interested in coming up with clever fixes for these cases, that would be extremely helpful. That would then allow us to delete rather than deprecate the unsafe functions.

@michaelsproul
Copy link
Member

Most of the uses are in tests, but it's still important for us to get rid of them (if possible), as they sometimes contribute to CI flakiness.

@michaelsproul michaelsproul added waiting-on-author The reviewer has suggested changes and awaits thier implementation. Networking labels Sep 9, 2025
@sashaodessa sashaodessa requested a review from jxs as a code owner September 10, 2025 08:26
daveth3t3chg33k

This comment was marked as spam.

@sashaodessa
Copy link
Author

@daveth3t3chg33k

@michaelsproul
Copy link
Member

@sashaodessa Can you also please rebase on unstable? All PRs need to be merged to unstable

@sashaodessa
Copy link
Author

@michaelsproul

@michaelsproul
Copy link
Member

Looks good at a glance, I've requested a review from some our networking experts on the changes that impact prod

@michaelsproul michaelsproul changed the base branch from stable to unstable September 14, 2025 12:34
Copy link
Member

@AgeManning AgeManning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes look good to me.

Agree a future PR to remove deprecation would be nice.

Comment on lines +108 to +111
{% for commit in commits | unique(attribute='email_author') %}
Co-Authored-By: {{ commit.author }} <{{ commit.email_author }}>
{% endfor %}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these changes relevant?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No this is some leftover stuff from stable that hasn't been backmerged to unstable yet (see https://github.com/sigp/lighthouse/commits/stable/), it should be deleted from this PR.

@michaelsproul
Copy link
Member

I think we can probably just delete the deprecated functions in this PR, AFAICT all (?) usages have been removed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Networking waiting-on-author The reviewer has suggested changes and awaits thier implementation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants