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

UPNP is not disabled when tor-only settings are in effect. #2927

Closed
ghost opened this issue Aug 23, 2013 · 14 comments · Fixed by #6153
Closed

UPNP is not disabled when tor-only settings are in effect. #2927

ghost opened this issue Aug 23, 2013 · 14 comments · Fixed by #6153
Labels

Comments

@ghost
Copy link

ghost commented Aug 23, 2013

With the following enabled, UPNP should not attempt to open a port at the router for 8333, however in the latest stable the port is uselessly opened. At best it's doing nothing productive, at worst it's just advertising the existence of a bitcoin client.

onlynet=tor
tor=127.0.0.1:9050
discover=0
listen=0

Compiling with USE_UPNP=- works as a temporary solution, but is by no means ideal.

@Diapolo
Copy link

Diapolo commented Aug 25, 2013

If using Bitcoin-Qt make sure to unset the UPnP option. For using bitcoind, I'm looking into a little patch currently.

@gmaxwell
Copy link
Contributor

@63 Thanks for the report! A lot of the developers build with UPNP much of the time (or, well, at least I do), unfortunate that this slipped through.

@Diapolo we should not UPNP if listen=0, and separately, we should not UPNP if onlynet=tor.

@sipa
Copy link
Member

sipa commented Aug 25, 2013

As far as I can see, -listen=0 should already disable UPnP by default. Only if you enable it explicitly through -upnp (or upnp=1), or the GUI should it be enabled. If that's not the case, it's a bug.

@ghost
Copy link
Author

ghost commented Aug 28, 2013

@sipa Reproduced in the latest master.

bitcoin.conf:

active UPNP mappings on the router:

@laanwj
Copy link
Member

laanwj commented Nov 7, 2013

My reasoning would be that the bind=XXX re-enables listen, and hence upnp is not disabled.

Edit: does it work if you explicitly add upnp=0 to the config?

@sipa
Copy link
Member

sipa commented Nov 7, 2013

If you explicitly set listen to false, bind shouldn't re-enable it.

@ghost
Copy link
Author

ghost commented Nov 29, 2013

Yes, @laanwj, running with the line

upnp=0 

or compiling without UPNP does work, but logically if we're not listening it shouldn't be enabled to begin with, right?

@laanwj
Copy link
Member

laanwj commented Jan 17, 2014

Guess this needs to be fixed before 0.9 as well.

@Diapolo
Copy link

Diapolo commented Jan 17, 2014

Would this suffice (change in net.cpp)?

    // only map ports when not using Tor
    if (!IsLimited(NET_TOR))
        // Map ports with UPnP
        MapPort(GetBoolArg("-upnp", fDefaultUpnp));

We already have in init.cpp the check for listen:

    if (!GetBoolArg("-listen", true)) {
        // do not map ports or try to retrieve public IP when not listening (pointless)
        SoftSetBoolArg("-upnp", false);
        SoftSetBoolArg("-discover", false);
    }

@laanwj
Copy link
Member

laanwj commented Jan 17, 2014

Your change would make it impossible to map ports when using Tor.

The goal is to make the default (if not overridden using -upnp) to not map ports when using Tor, not to make it completely impossible.

@Diapolo
Copy link

Diapolo commented Jan 17, 2014

I'm out... too many cases to consider for my ill brain ^^.

@laanwj laanwj removed this from the 0.9.0 milestone Feb 24, 2014
@gmaxwell gmaxwell added this to the 0.9.2 milestone May 11, 2014
@gmaxwell
Copy link
Contributor

This appears to have been already solved:

if (!GetBoolArg("-listen", true)) {
    // do not map ports or try to retrieve public IP when not listening (pointless)
    if (SoftSetBoolArg("-upnp", false))
...

@laanwj
Copy link
Member

laanwj commented May 12, 2014

@gmaxwell well, the problem seems to be that he has a bind:XXX statement (for a hidden service), which re-enables listen so it never goes into that.

@laanwj laanwj removed this from the 0.9.2 milestone Nov 27, 2014
Repository owner removed this from the 0.9.2 milestone Nov 27, 2014
laanwj added a commit to laanwj/bitcoin that referenced this issue May 18, 2015
To protect privacy, do not use UPNP when a proxy is set. The user may
still specify -listen=1 to listen locally (for a hidden service), so
don't rely on this happening through -listen.

Fixes bitcoin#2927.
@laanwj
Copy link
Member

laanwj commented May 18, 2015

Foremost, if your goal is to hide completely behind a proxy you should use -proxy, not -tor. -tor is only meant for hyrid Tor/clearnet nodes.

Given that, see #6153 for fix.

laanwj added a commit that referenced this issue Jun 2, 2015
To protect privacy, do not use UPNP when a proxy is set. The user may
still specify -listen=1 to listen locally (for a hidden service), so
don't rely on this happening through -listen.

Fixes #2927.

Conflicts:
	src/init.cpp

Rebased-From: 8c35b6f
Github-Pull: #6153
IntegralTeam pushed a commit to IntegralTeam/bitcoin that referenced this issue Jun 4, 2019
reddink pushed a commit to reddcoin-project/reddcoin-3.10 that referenced this issue May 27, 2020
To protect privacy, do not use UPNP when a proxy is set. The user may
still specify -listen=1 to listen locally (for a hidden service), so
don't rely on this happening through -listen.

Fixes bitcoin#2927.

Conflicts:
	src/init.cpp

Rebased-From: 8c35b6f
Github-Pull: bitcoin#6153
(cherry picked from commit ebd7d8d)
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants