Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Parity does not filter 100.64.0.0/10 ip range when --allow-ips=public #5872

Closed
offlinehacker opened this issue Jun 19, 2017 · 9 comments
Closed
Assignees
Labels
F3-annoyance 💩 The client behaves within expectations, however this “expected behaviour” itself is at issue. M4-core ⛓ Core client code / Rust.

Comments

@offlinehacker
Copy link

All these private bogon ip addresses must be blocked https://www.team-cymru.org/Services/Bogons/bogon-bn-agg.txt

@offlinehacker
Copy link
Author

Our cloud provider opened abuse report and blocked our servers, as these ranges were not blocked.

@5chdn 5chdn added F3-annoyance 💩 The client behaves within expectations, however this “expected behaviour” itself is at issue. M4-core ⛓ Core client code / Rust. labels Jun 20, 2017
@5chdn
Copy link
Contributor

5chdn commented Jun 20, 2017

It seems Parity only blocks RFC1918 private network connections, but it should also block reserved IPs. This is not really a bug but an annoyance as 100.64.0.0/10 is not a private network.

We should consider blocking all reserved IP address spaces. https://en.wikipedia.org/wiki/IPv4#Special-use_addresses

I personally use iptables to circumvent this issue. https://ethereum.stackexchange.com/a/9964

@sjeohp-zz sjeohp-zz self-assigned this Jul 17, 2017
@sjeohp-zz
Copy link

Just confirming that the solution here is to filter all reserved ips from the public list?

@sjeohp-zz
Copy link

Also it looks like the only current criteria for the --allow-ips=private list is being not-global, so some reserved IPs (loopback, link-local, etc) can be included. Should probably filter those too?

sjeohp-zz pushed a commit that referenced this issue Jul 18, 2017
* Add checks for all ipv4 special use addresses
* Add comprehensive ipv4 test cases
@arkpar
Copy link
Collaborator

arkpar commented Jul 18, 2017

@sjeohp This should really be addressed by allowing custom ranges in addition to predefined "public" and "private" sets. E.g. --allow-ips="100.64.0.0/10 2.3.0.0/16"
Some VPS providers require certain ranges of public IPs to be filtered out as well.

@5chdn
Copy link
Contributor

5chdn commented Jul 18, 2017

On a practical note, it might be easier (from a user perspective) blacklisting rather than whitelisting sets of ip adresses, i.e., --block-ips="private,100.64.0.0/10,2.3.0.0/32"

@sjeohp-zz
Copy link

sjeohp-zz commented Jul 19, 2017

I've refactored to exclude more of the reserved/special-use IPs including 100.64.0.0/10, but there are still ambiguities in the official reserved lists, hence why Rust's std::net IP functions have been unstable for so long.

I can add both --allow-ips and --block-ips. Only issue I can see would be potential conflicts, e.g. passing --allow-ips="100.64.0.0/10 2.3.0.0/16" --block-ips="private,100.64.0.0/10,2.3.0.0/32", but I think an error in that case makes sense.

@sjeohp-zz
Copy link

sjeohp-zz commented Jul 19, 2017

On second thought maybe something like --allow-ips="private 100.64.0.0/10 -2.3.0.0/16"? Where that would allow all in the private list and the 100.64.0.0/10 range, and block 2.3.0.0/16, with order indicating precedence, either ascending or descending.

@arkpar
Copy link
Collaborator

arkpar commented Jul 19, 2017

I'd be happy with either solution. --allow-ips can be moved to legacy options and --block-ips made a priority if specified.

sjeohp-zz pushed a commit that referenced this issue Jul 22, 2017
sjeohp-zz pushed a commit that referenced this issue Jul 22, 2017
* Add IpFilter struct to wrap predefined filter (AllowIP) with custom
allow/block filters.
* Refactor parsing of --allow-ips to handle custom filters.
* Move AllowIP/IpFilter from ethsync to ethcore-network where they
are used.
sjeohp-zz pushed a commit that referenced this issue Jul 22, 2017
* Add "none" as a valid argument for --allow-ips to allow narrow
custom ranges, eg.: --allow-ips="none 10.0.0.0/8"
* Add tests for parsing filter arguments and node endpoints.
* Add ipnetwork crate to dev dependencies for testing.
sjeohp-zz pushed a commit that referenced this issue Jul 24, 2017
sjeohp-zz pushed a commit that referenced this issue Jul 25, 2017
sjeohp-zz pushed a commit that referenced this issue Jul 27, 2017
sjeohp-zz pushed a commit that referenced this issue Jul 27, 2017
This reverts commit 7a89064.
gavofyork pushed a commit that referenced this issue Jul 28, 2017
* Add checks for additional reserved ip addresses

100.64.0.0/10 and 240.0.0.0/4 are both reserved but not currently
filtered.

* Add check for special purpose addresses

192.0.0.0/24 - Used for the IANA IPv4 Special Purpose Address Registry

* Refactor ip_utils (#5872)

* Add checks for all ipv4 special use addresses
* Add comprehensive ipv4 test cases

* Refactor Ipv6 address checks (#5872)

* Refactor AllowIP (#5872)

* Add IpFilter struct to wrap predefined filter (AllowIP) with custom
allow/block filters.
* Refactor parsing of --allow-ips to handle custom filters.
* Move AllowIP/IpFilter from ethsync to ethcore-network where they
are used.

* Revert Cargo.lock

* Tests for custom ip filters (#5872)

* Add "none" as a valid argument for --allow-ips to allow narrow
custom ranges, eg.: --allow-ips="none 10.0.0.0/8"
* Add tests for parsing filter arguments and node endpoints.
* Add ipnetwork crate to dev dependencies for testing.

* Add ipv6 filter tests (#5872)

* Revert parity-ui-precompiled to master

* Fix minor detail in usage.txt (#5872)

* Spaces to tabs

* Rename IpFilter::new() to ::default()

* Small readability improvements

* Test (#5872)

* Revert "Test (#5872)"

This reverts commit 7a89064.
@5chdn 5chdn closed this as completed Aug 15, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
F3-annoyance 💩 The client behaves within expectations, however this “expected behaviour” itself is at issue. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

No branches or pull requests

4 participants