Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix --anonymous-inbound data leak #9632

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

vtnerd
Copy link
Contributor

@vtnerd vtnerd commented Dec 19, 2024

As the title says. Hopefully get some testers to make sure nothing for broken (it is fairly basic).

src/p2p/net_node.inl Outdated Show resolved Hide resolved
@vtnerd vtnerd force-pushed the fix/anonymous_inbound branch from 20dd81b to dc7e3ed Compare December 19, 2024 23:52
@selsta selsta added the daemon label Dec 19, 2024
@vtnerd
Copy link
Contributor Author

vtnerd commented Dec 19, 2024

After "rubber ducking" things, I decided on the following changes from the initial PR:

  • Switch to std::shuffle which is not deprecated, unlike the other shuffle variants
  • Set all last_seen timestamps to 0 when not in public zone.

@vtnerd vtnerd changed the title Fix --anonymouns-inbound data leak Fix --anonymous-inbound data leak Dec 19, 2024
Copy link
Collaborator

@0xFFFC0000 0xFFFC0000 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@j-berman j-berman left a comment

Choose a reason for hiding this comment

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

To make sure I understand the comment and its implications (which are relevant to this PR):

Let's say my node has anonymous inbound setup. I make an outgoing connection to another tor node. That outgoing node will then initiate the timed sync request to my node over the connection, and my node will respond with my node's onion address and peer ID shuffled somewhere in the peerlist. If the outgoing node then initiates connections to all nodes in the peerlist that my node sent, it will be able to map my node's peer ID to my node's onion address.

Is the above accurate?

src/p2p/net_node.inl Outdated Show resolved Hide resolved
@vtnerd
Copy link
Contributor Author

vtnerd commented Jan 1, 2025

it will be able to map my node's peer ID to my node's onion address.

The peer ID <-> onion address link happens as soon as the peerlist is sent, there's no need for another connection. Did you mean that the peer ID could be used as an identifier in other connections?

Every Tor peer id defaults to 1. Only the public zone randomizes the value.

@vtnerd vtnerd force-pushed the fix/anonymous_inbound branch from dc7e3ed to 2c1c673 Compare January 1, 2025 17:07
@j-berman
Copy link
Collaborator

j-berman commented Jan 1, 2025

Did you mean that the peer ID could be used as an identifier in other connections?

This is what I meant. This part of the comment is what made me think it might be possible:

[The connecting peer's address] is relayed to them, iff the node has setup an inbound hidden service. The other peer will have to use the random peer_id value to link the two

Seems like it wouldn't be possible to link onion address to random peer_id thanks to the fact that every Tor peer id defaults to 1. Can you clarify the comment?

Copy link
Collaborator

@j-berman j-berman left a comment

Choose a reason for hiding this comment

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

Code looks good. I have a question on the comment, but code LGTM.

@jeffro256
Copy link
Contributor

Is there any benefit to shuffling the entire list versus a single random insertion?

@vtnerd
Copy link
Contributor Author

vtnerd commented Jan 2, 2025

The list is already randomized (as part of anonymize = true), so the random insertion would achieve the same thing with a small speed bump. I'll update this tomorrow.

@vtnerd vtnerd force-pushed the fix/anonymous_inbound branch from 2c1c673 to 8e14699 Compare January 3, 2025 22:46
@vtnerd
Copy link
Contributor Author

vtnerd commented Jan 3, 2025

@vtnerd vtnerd force-pushed the fix/anonymous_inbound branch from 8e14699 to e2eea7b Compare January 4, 2025 00:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants