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

common: do not use DNS to determine if address is local #8854

Merged
merged 1 commit into from
Jul 7, 2023

Conversation

tobtoht
Copy link
Collaborator

@tobtoht tobtoht commented May 12, 2023

Alternative to #8849.

I'm not convinced we need to support DNS lookup at all here. This PR considers a node "local" if the host is "localhost" or a IPv{4,6} address in the loopback range. The is_privacy_preserving_network check is no longer needed.

@plowsof
Copy link
Contributor

plowsof commented May 12, 2023

May i confirm my method of seeing the bug (after looking at 8849) - simply using wireshark to show that dns A records at the onion daemon address are being looked up at wallet open time?

@tobtoht
Copy link
Collaborator Author

tobtoht commented May 12, 2023

@plowsof, yes, A or AAAA records. It should no longer happen with 8849 or this PR.

@pokkst
Copy link

pokkst commented May 12, 2023

@plowsof You can test it in just about every wallet too. The only wallet I've tested so far that didn't seem to experience the issue is Feather. For other wallets, it happens for onion addresses, i2p addresses, and clearnet addresses (clearnet is not really an issue if proxy is disabled, of course).

If you need an app for Android to observe network traffic then PCAPdroid on F-Droid is good.

@plowsof
Copy link
Contributor

plowsof commented May 14, 2023

with wireshark open and a display filter of dns.qry.name contains ".onion"
with release and this PR i still see the same number of dns calls when i e.g. unlock the wallet. Am i testing with the wrong parameters? (no --proxy or torsocks, so the inputs wont work)
./monero-wallet-cli --daemon-address plowsof3t5hogddwabaeiyrno25efmzfxyro2vligremt7sxpsclfaid.onion:18089

@tobtoht
Copy link
Collaborator Author

tobtoht commented May 14, 2023

@plowsof --proxy is needed for testing. A DNS leak happens elsewhere if --proxy is omitted, because it assumes it's connecting a clearnet host. It might be a good idea to have simplewallet warn* if a user tries to connect to a .onion/i2p daemon address without proxy, but this is a separate issue.

* Or refuse to connect, though this would frustrate users with transparent Tor proxying (e.g. Whonix).

src/common/util.cpp Outdated Show resolved Hide resolved
@plowsof
Copy link
Contributor

plowsof commented May 18, 2023

confirming that this PR fixes the issue (no dns leak with an .onion daemon and --proxy where release would otherwise do so). *using a clearnet daemon with proxy no leak

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.

Small comment on the warning log, otherwise looks good to me.

I wrote some unit tests:

#include "common/util.h"

TEST(LocalAddress, localhost) { ASSERT_TRUE(tools::is_local_address("localhost")); }
TEST(LocalAddress, localhost_port) { ASSERT_TRUE(tools::is_local_address("localhost:18081")); }
TEST(LocalAddress, localhost_suffix) { ASSERT_TRUE(tools::is_local_address("test.localhost")); }
TEST(LocalAddress, loopback) { ASSERT_TRUE(tools::is_local_address("127.0.0.1")); }
TEST(LocalAddress, loopback_port) { ASSERT_TRUE(tools::is_local_address("127.0.0.1:18081")); }
TEST(LocalAddress, loopback_protocol) { ASSERT_TRUE(tools::is_local_address("http://127.0.0.1")); }
TEST(LocalAddress, loopback_hi) { ASSERT_TRUE(tools::is_local_address("127.255.255.255")); }
TEST(LocalAddress, loopback_lo) { ASSERT_TRUE(tools::is_local_address("127.0.0.0")); }
TEST(LocalAddress, loopback_ipv6) { ASSERT_TRUE(tools::is_local_address("[0:0:0:0:0:0:0:1]")); }

TEST(LocalAddress, onion) { ASSERT_FALSE(tools::is_local_address("vww6ybal4bd7szmgncyruucpgfkqahzddi37ktceo3ah7ngmcopnpyyd.onion")); }
TEST(LocalAddress, i2p) { ASSERT_FALSE(tools::is_local_address("xmrto2bturnore26xmrto2bturnore26xmrto2bturnore26xmr2.b32.i2p")); }
TEST(LocalAddress, valid_ip) { ASSERT_FALSE(tools::is_local_address("1.2.3.4")); }
TEST(LocalAddress, valid_ipv6) { ASSERT_FALSE(tools::is_local_address("[0:0:0:0:0:0:0:2]")); }
TEST(LocalAddress, valid_domain) { ASSERT_FALSE(tools::is_local_address("getmonero.org")); }
TEST(LocalAddress, local_prefix) { ASSERT_FALSE(tools::is_local_address("localhost.com")); }
TEST(LocalAddress, invalid) { ASSERT_FALSE(tools::is_local_address("test")); }
TEST(LocalAddress, empty) { ASSERT_FALSE(tools::is_local_address("")); }

src/common/util.cpp Outdated Show resolved Hide resolved
@tobtoht
Copy link
Collaborator Author

tobtoht commented May 25, 2023

Added your unit tests and suggested change, rebased so Windows build passes. Thanks for the review. :)

tests/unit_tests/util.cpp Outdated Show resolved Hide resolved
@luigi1111 luigi1111 merged commit c34dc5b into monero-project:master Jul 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants