Stop using private IP addresses by default

teor2345 commented 2 years ago

Motivation

Zebra currently connects to private IP addresses, and advertises them to its peers.

But this is a security issue, because Zebra can be used to probe internal network addresses, and disclose if they're running a Zcash node. Zebra might also overload other internal services with connections. (But we have a rate-limit for this.)

Zebra also discloses the internal IP address of the machine it is on.

Tasks

[ ] Reject private IP addresses in address book updates
- [ ] Reject private IP addresses in the local listener address in the address book
[ ] Reject private IP addresses when querying configured DNS seeders
- [ ] What should we do about seed peers configured with private IP address literals?
[ ] Stop putting private IP addresses in the inbound or outbound handshake fields
[ ] Add a debug_allow_private_ip_addressesconfig that allows private IP addresses for testing

Related Work

We might want to merge this PR as part of this fix:

2035

mpguerra commented 2 years ago

Hey team! Please add your planning poker estimate with ZenHub @conradoplg @dconnolly @jvff @oxarbitrage @teor2345 @upbqdn

teor2345 commented 1 year ago

I'm not sure if this is a priority at the moment?

mpguerra commented 1 year ago

it's not but I think it's ok to keep open for now

teor2345 commented 1 year ago

@mpguerra is this something we want to do before the stable release? It seems like a privacy issue that some users might be concerned about. (And they might assume that we'd never leak private addresses.)

mpguerra commented 1 year ago

@mpguerra is this something we want to do before the stable release? It seems like a privacy issue that some users might be concerned about. (And they might assume that we'd never leak private addresses.)

Yup, I think so. I thought it was in the epic already.

teor2345 commented 1 year ago

@mpguerra I just noticed this again, is it something we should do before the stable release, or right after it?

mpguerra commented 1 year ago

I think since it's been a low priority issue it can wait until after. If we can get it in before, great, but I wouldn't block on it.

teor2345 commented 1 year ago

Note from engineering sync: this seems like a risky change to make between the final release candidate and the first stable release. But we could do it in stages, or do it with extra tests.

ZcashFoundation / zebra