Skip to content

Conversation

@n-thumann
Copy link
Contributor

This PR fixes #19978.
Currently Site Local IPv6 addresses as treated as routable, even though as of RFC 3879 they should not be routed. I therefore implemented a check to detect and flag them as not routable.

@naumenkogs
Copy link
Contributor

I'd say our code doesn't have to implement the full spec, unless it's beneficial for Bitcoin.
Not sure we get any practical benefit from this change:

  • if someone wants to spam us with fake ipv6 addresses, there's a plenty other ways to do so :)
  • the probability of an old/buggy node relaying these specific unroutable addresses is very low, and the harm is very little

On the other hand:

  • it adds more lines
  • we might break some existing setting (yeah, based on the outdated addresses, who knows)

I'm -0 on this right now.
Perhaps you have another reasoning wrt this change on mind?

@laanwj
Copy link
Member

laanwj commented Sep 21, 2020

I think it makes sense to consistently reject all local networks here, even the deprecated one. It's a small straightforward code change too.
Code review ACK fbab5ed98fc936fe0904b05c1ebefcf8beb30ef6

@Saibato
Copy link
Contributor

Saibato commented Sep 21, 2020

Tyi. Not waving in here for now, if that makes sense or not.

But since rfc5000 is not longer the STD1 RFC and deprecated in rfc7100

I can only recommend to grep and use for standard advice in https://www.rfc-editor.org/standards

There are only few binding Standad's defined in regard to ipv6,

AFAICS rfc3879 is no Standard.

@practicalswift
Copy link
Contributor

practicalswift commented Sep 21, 2020

Concept ACK

Rationale:

  • We want to have the same definition of what constitutes a local network as Tor: connecting to fec0:: is disallowed via Tor due to being a local address
  • We don't want to run the risk of leaking users' local addresses to peers: could theoretically be used for fingerprinting or network reconnaissance?
  • We don't want our peers to be able to feed us addresses that are local and thus potentially make us connect to internal servers

Unfortunately fec0:: addresses still exist in the wild, so this is not a purely a theoretical issue :)

@Saibato
Copy link
Contributor

Saibato commented Sep 21, 2020

We definitely should not use Link-Local addresses

   |   10     |
   |  bits    |         54 bits         |          64 bits           |
   +----------+-------------------------+----------------------------+
   |1111111010|           0             |       interface ID         |
   +----------+-------------------------+----------------------------+

But digging further.
Not sure something was mixed and Tor is outdated since the newer Draft Standard rfc4291
clearly states about address of the Site-Local addresses format we discuss here

   |   10     |
   |  bits    |         54 bits         |         64 bits            |
   +----------+-------------------------+----------------------------+
   |1111111011|        subnet ID        |       interface ID         |
   +----------+-------------------------+----------------------------+
   The special behavior of this prefix defined in [RFC3513] must no
   longer be supported in new implementations (i.e., new implementations
   must treat this prefix as Global Unicast).

   Existing implementations and deployments may continue to use this
   prefix.

And thereby those address are to be treated as any global unicast like any routeable ipv6 address.

So rfc3879 is outdated as it refer to rfc3513 where those address are defined non routeable but now are routeable and then Tor needs some update.

Or I am simply wrong here? and we should anyway better follow Tor and risk no leaks and not follow the internet Draft Standard but use an outdated recommendation and an obsoleted proposed standard?

Its clearly true if we follow Tor we must change but since we had this practice all along and Tor follows outdated rfc, i am not sure if it might be better to file a Tor trac and earn for the bitcoin dev there some bug points?

@practicalswift
Copy link
Contributor

@Saibato

Oh, TIL! Thanks a lot!

I was wrong about the current routability status of fce0::/10 per the current standard.

Turns out this prefix has had a bumpy road:

  • It was non-routable as of RFC 3513 (2003): "Routers must not forward any packets with site-local source or destination addresses outside of the site."
  • It was non-routable as of RFC 3879 (2004): "However, router implementations SHOULD be configured to prevent routing of this prefix by default."
  • It became routable again as of RFC 4291 (2006, current standard): "The special behavior of this prefix defined in [RFC3513] must no longer be supported in new implementations (i.e., new implementations must treat this prefix as Global Unicast)."

I guess Tor is erring on the side of caution by disallowing connections to a prefix that has previously been for site local use :)

@practicalswift
Copy link
Contributor

practicalswift commented Sep 22, 2020

It should be noted that fec0::/10 has always been reserved by the IETF and has thus never been in active Internet use. Any use of fec0:: in a non-site local context (such as in ADDR messages) is thus likely accidental. I think it makes sense to have the same definition as Tor on what constitutes a local address to rule out potential issues such as IP address leaks and node fingerprinting.

From https://www.iana.org/assignments/ipv6-address-space/ipv6-address-space.xhtml:

fec0::/10 | Reserved by IETF | [RFC3879] | Deprecated by [RFC3879] in September 2004. Formerly a Site-Local scoped address prefix.

@Saibato
Copy link
Contributor

Saibato commented Sep 22, 2020

@practicalswift
Time to make some internet history, I find the IETF in contrast to IANA approach always a bit more in favor of an open internet, those address blocking's where more like proposal rfc spam than ever be a standard or close to that
In my view the ... attempt to get blocking list into routers and to fragment the internet.

In roundabout 2007 those source code lines to follow blocking rules by rfc3879 came into Tor core. from utils.c to address.c they where since then never updated to follow the newest standard.

I tend to file a bug trac issue on Tor to decide whom they want to follow, but since u noticed first that we did never blocked those ip ranges. please outpace me in that.

as in ADDR messages) is thus likely accidental.

Though i hope u see the potential here to dig some privacy tunnels, IETF had granted us to get the internet back again.

Although if u can outline one real world scenario where those addresses could even remotely leak or fingerprint in any way like what the client does since eons with seeding and -onlynet=onion, i consent instantly.

@practicalswift
Copy link
Contributor

practicalswift commented Sep 22, 2020

I tend to file a bug trac issue on Tor to decide whom they want to follow, but since u noticed first that we did never blocked those ip ranges. please outpace me in that.

Feel free to open such an issue if you want their position clarified.

Personally I think Tor is intentionally erring on the side of caution here, and I think we should do that too :)

FWIW you'll occasionally see fec0::/10 addresses in ADDR messages on mainnet. In other words this is not purely theoretical :)

@n-thumann n-thumann force-pushed the net-deprecate-site-local-ipv6 branch from fbab5ed to c4ba79b Compare September 25, 2020 15:42
@practicalswift
Copy link
Contributor

An example from mainnet where Bitcoin Core tries to connect to a fec0::/10 address learned from a mainnet peer.

The connections is blocked by Tor:

Oct 10 12:34:56.000 [warn] Rejecting SOCKS request for anonymous connection to private address fec0::5b4f:fa94:a34b:1731.

@fanquake
Copy link
Member

@gmaxwell do you have any thoughts/insight here?

@laanwj
Copy link
Member

laanwj commented Nov 20, 2020

We should either merge this or close #19978 as "not an issue". I'd personally err on the side of merging this, though I also get the point that it makes very little difference and we can't and shouldn't maintain a global IPv6 blacklist as part of bitcoin core and make a dayjob out of rule-lawyering IETF specs.

@practicalswift
Copy link
Contributor

Agree with @laanwj. FWIW I think this is the only divergence between what we consider to be the public Internet and what Tor considers to be the public Internet :)

@Saibato
Copy link
Contributor

Saibato commented Nov 20, 2020

shouldn't maintain a global IPv6 blacklist as part of bitcoin core and make a dayjob out of rule-lawyering IETF specs.

Agree, I also think we can now close #19978, since Tor will move from 0.4.6.x there codebase in favor of also not exempt those addresses any more and follow the advance of the Internet standardization process.

Tyi: https://gitlab.torproject.org/tpo/core/tor/-/milestones/65

So thx, to all who participated in this,

Once again, Bitcoin has proved that we stand for no censorship at all and specially thx to @practicalswift to notice that issue and we together prevented shit happened before ipv6 is wildly used,

Also special thx to @n-thumann for done this now void work but be sure u where part of something really important.

Copy link
Member

@jonatack jonatack left a comment

Choose a reason for hiding this comment

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

I've reviewed and tested this but it's not clear to me if it should be done.

ACK c4ba79b if we want to do this

@Saibato
Copy link
Contributor

Saibato commented Nov 20, 2020

@jonatack tyi.

if it should be done. ???

Concept NACK Bcs. Tor made to *not block those any longer* a milestone for next releases and probably even backport's, since to block them even in the first place is technical debt on Tor side to not keep it since it was never a standard but just on the wish list of early drafts of censors that was reverted by the standard process.

@sipa
Copy link
Member

sipa commented Nov 20, 2020

Should we just implement https://www.iana.org/assignments/ipv6-address-space/ipv6-address-space.xhtml? Only 2000::/3 is global unicast. Other ranges may be allocated in the future of course, but given that only 1.39% of 2000::/3 is even assigned to RIRs, I don't expect that to change any time soon. And the current blacklisted ranges can change too... so perhaps it's easier to just have 1 whitelisted range rather than dozens of individually blacklisted ranges.

@Saibato
Copy link
Contributor

Saibato commented Nov 21, 2020

Only 2000::/3 is global unicast

@sipa That is what IANA defines, suppose they assign port 8333 solely for router internal use after 2020? Will we then follow that Whitelist too, when RFC's track say;s otherwise?
What IANA assigned or assigns is imo irrelevant to the Internet Standard process and Bitcoin the Internet Money is good advised to only follow the RFC's.

There is and will probably ever be only one exemption in ipv6 from the unicast range to be not global unicast assignable (what in effect mean they will be not routeable like ::0 and ::1) and that is the whole ff00::/8 range defined by Standard Track RFC4291

If we would do a Whitelist, where do you start and when do u stop and what is the update interval to that Whitelist?, u become a censor and slave of IANA, when in fact there is only one Internet Standard and that very app that clams to be Internet compatible has to follow strict, its the RFC standard track process,
When Tor followed RFC 3879 what was not even in the standards track but just a proposed standard wish list document like any other opinion one can have or not, that was in hindsight a mistake in Tor .
We should not do the same.

The internet as such is the RFC's track there is no second Internet, same as we would never allow miners to censor tx that we and the user define as valid.,

@laanwj
Copy link
Member

laanwj commented Dec 21, 2020

so perhaps it's easier to just have 1 whitelisted range rather than dozens of individually blacklisted ranges.

I really like this idea from a simplicity point of view. I sometimes wonder what other P2P software does with regard to whitelisting/blacklisting subnet ranges for relayed addresses.
By the definition it is only relevant to relay globally routable unicast addresses, not local ones, not special-purpose ones, not multi-cast ones.

@DrahtBot
Copy link
Contributor

DrahtBot commented Jun 7, 2021

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@DrahtBot
Copy link
Contributor

DrahtBot commented Jul 8, 2021

🐙 This pull request conflicts with the target branch and needs rebase.

Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".

@DrahtBot
Copy link
Contributor

There hasn't been much activity lately and the patch still needs rebase. What is the status here?
  • Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
  • Is it no longer relevant? ➡️ Please close.
  • Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

@maflcko
Copy link
Member

maflcko commented Apr 29, 2022

Marked "up for grabs"

@maflcko
Copy link
Member

maflcko commented Apr 29, 2022

Closing for now. Let us know when it should be reopened.

@maflcko maflcko closed this Apr 29, 2022
@bitcoin bitcoin locked and limited conversation to collaborators Apr 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

"Site-local" addresses in the fec0::/10 range treated as routable

10 participants