Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Default to the blacklisting reserved IP ranges. #8870

Merged
merged 17 commits into from
Dec 9, 2020
Merged

Default to the blacklisting reserved IP ranges. #8870

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
bab2732
Default to the reserved IP ranges.
clokep Dec 3, 2020
9ccdd1d
Add a newsfragment.
clokep Dec 3, 2020
d3d54e5
Lint.
clokep Dec 3, 2020
6a7991f
Allow specifying a blank ip range blacklist.
clokep Dec 4, 2020
526dc0f
Fix private IP use in tests.
clokep Dec 4, 2020
2896de2
Merge remote-tracking branch 'origin/develop' into clokep/ip-blacklis…
clokep Dec 4, 2020
5d38dc0
Merge remote-tracking branch 'origin/develop' into clokep/ip-blacklis…
clokep Dec 8, 2020
c7013fd
Expand the default blacklist.
clokep Dec 8, 2020
eeb3ef3
Apply the same defaults to the media-repo blacklist.
clokep Dec 8, 2020
3cace62
Allow for an IP range whitelist.
clokep Dec 8, 2020
d391a0d
Fix a typo.
clokep Dec 8, 2020
107f2d2
Lint.
clokep Dec 8, 2020
1ad448b
Merge remote-tracking branch 'origin/develop' into clokep/ip-blacklis…
clokep Dec 9, 2020
ab996c9
Update the config.
clokep Dec 9, 2020
b046db7
Update install docs about netaddr.
clokep Dec 9, 2020
19b0964
Add upgrading notes.
clokep Dec 9, 2020
df7885c
Fix typo.
clokep Dec 9, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion changelog.d/8821.bugfix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
Apply the `federation_ip_range_blacklist` to push and key revocation requests.
Apply an IP range blacklist to push and key revocation requests.
1 change: 1 addition & 0 deletions changelog.d/8870.bugfix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Apply an IP range blacklist to push and key revocation requests.
22 changes: 11 additions & 11 deletions docs/sample_config.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -643,8 +643,8 @@ acme:
# - syd.example.com

# Prevent outgoing requests from being sent to the following blacklisted IP address
# CIDR ranges. If this option is not specified, or specified with an empty list,
# no IP range blacklist will be enforced.
# CIDR ranges. If this option is not specified or is empty then it defaults to
# private IP address ranges (see the example below).
#
# The blacklist applies to the outbound requests for federation, identity servers,
# push servers, and for checking key validitity for third-party invite events.
Expand All @@ -655,15 +655,15 @@ acme:
# This option replaces federation_ip_range_blacklist in Synapse v1.24.0.
clokep marked this conversation as resolved.
Show resolved Hide resolved
#
ip_range_blacklist:
- '127.0.0.0/8'
- '10.0.0.0/8'
- '172.16.0.0/12'
- '192.168.0.0/16'
- '100.64.0.0/10'
- '169.254.0.0/16'
- '::1/128'
- 'fe80::/64'
- 'fc00::/7'
#- '127.0.0.0/8'
#- '10.0.0.0/8'
#- '172.16.0.0/12'
#- '192.168.0.0/16'
#- '100.64.0.0/10'
#- '169.254.0.0/16'
#- '::1/128'
#- 'fe80::/64'
#- 'fc00::/7'

# Report prometheus metrics on the age of PDUs being sent to and received from
# the following domains. This can be used to give an idea of "delay" on inbound
Expand Down
37 changes: 25 additions & 12 deletions synapse/config/federation.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,19 @@
from synapse.config._util import validate_config


DEFAULT_IP_RANGE_BLACKLIST = [
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not 100% sure where these came from? They seem to partially block "private" IP spaces, but not all, see https://github.com/netaddr/netaddr/blob/master/netaddr/ip/__init__.py#L1918-L1972 / RFC6890.

Copy link
Member

Choose a reason for hiding this comment

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

I think it was copied from the url_preview blacklist, which itself has evolved rather than being put together carefully (see #1198 for example). It might be a good time to consider if there are others we should add.

Copy link
Member Author

Choose a reason for hiding this comment

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

In addition to what is there we should probably add:

  • 192.0.0.0/24: IANA IPv4 Special Purpose Address Registry (RFC 5736)
  • 198.18.0.0/15: Testing of inter-network communications between subnets (RFC 2544)
  • 0.0.0.0/8: Broadcast message (RFC 1700)
  • 192.0.2.0/24: TEST-NET examples and documentation (RFC 5737)
  • 198.51.100.0/24: TEST-NET-2 examples and documentation (RFC 5737)
  • 203.0.113.0/24: TEST-NET-3 examples and documentation (RFC 5737)

I don't see why we would ever talk to multicast addresses:

  • 239.0.0.0 - 239.255.255.255: Administrative Multicast
  • 224.0.0.0/4: Multicast
  • 240.0.0.0/4: Reserved for multicast assignments (RFC 5771)
  • 233.252.0.0/24: Multicast test network
  • 234.0.0.0 - 238.255.255.255: Reserved multicast
  • 225.0.0.0 - 231.255.255.255: Reserved multicast
  • ff00::/8: Multicast

Ones we should modify:

  • fe80::/64 -> fe80::/10: link local

Ones I'm unsure about:

  • 192.88.99.0/24: 6to4 anycast relays (RFC 3068)
  • fec0::/10: Site Local Addresses (deprecated - RFC 3879)
  • The other IPv6 reserved: ff00::/12, ::/8, 0100::/8, 0200::/7, 0400::/6, 0800::/5, 1000::/4, 4000::/3, 6000::/3, 8000::/3, A000::/3, C000::/3, E000::/4, F000::/5, F800::/6, FE00::/9

That's quite a big change though, what do you think?

Copy link
Member

Choose a reason for hiding this comment

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

generally I agree with adding all of those to the default value, but a few points:

  • Note that 0.0.0.0/32 is already special-cased (I can't quite remember the history there). Should we update that special-casing rather than add 0.0.0.0/8 to the default here?
  • Are the IPv6 reserved addresses just "not yet assigned"? My instinct is not to blacklist them (though I'm surprised to see ::/8 there, given ::1 is localhost)

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe you're correct that those IPv6 addresses have just not been assigned yet, which is quite different.

Copy link
Member Author

Choose a reason for hiding this comment

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

Seems like https://tools.ietf.org/html/rfc6890 is the proper reference to be using.

Copy link
Member

Choose a reason for hiding this comment

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

The list mentioned above looks sane to me.

IMO, we could and should add everything from the above lists, up to the "not yet assigned" addresses.

Regarding Site Local Addresses, RFC 3879 mentions that the block is not supposed to be reassigned except by future IETF action and that router implementations SHOULD prevent routing. Taking that into account, I don't see any downside in adding them to the default blacklist and it's nice from a defence in depth perspective, just in case there's a bad/old router implementation somewhere.

I think we can safely skip the "not yet assigned" addresses for now. On the topic of ::1, I suppose the intent is that the entire ::/8 block is unassigned, except for ::1 which is treated as an exception and a special address.

I can't think of anything else that should be added to the list at the moment.

'127.0.0.0/8',
'10.0.0.0/8',
'172.16.0.0/12',
'192.168.0.0/16',
'100.64.0.0/10',
'169.254.0.0/16',
'::1/128',
'fe80::/64',
'fc00::/7',
]


class FederationConfig(Config):
section = "federation"

Expand All @@ -36,7 +49,7 @@ def read_config(self, config, **kwargs):
for domain in federation_domain_whitelist:
self.federation_domain_whitelist[domain] = True

ip_range_blacklist = config.get("ip_range_blacklist", [])
ip_range_blacklist = config.get("ip_range_blacklist") or DEFAULT_IP_RANGE_BLACKLIST
clokep marked this conversation as resolved.
Show resolved Hide resolved

# Attempt to create an IPSet from the given ranges
try:
Expand Down Expand Up @@ -85,8 +98,8 @@ def generate_config_section(self, config_dir_path, server_name, **kwargs):
# - syd.example.com

# Prevent outgoing requests from being sent to the following blacklisted IP address
# CIDR ranges. If this option is not specified, or specified with an empty list,
# no IP range blacklist will be enforced.
# CIDR ranges. If this option is not specified or is empty then it defaults to
clokep marked this conversation as resolved.
Show resolved Hide resolved
# private IP address ranges (see the example below).
#
# The blacklist applies to the outbound requests for federation, identity servers,
# push servers, and for checking key validitity for third-party invite events.
Expand All @@ -97,15 +110,15 @@ def generate_config_section(self, config_dir_path, server_name, **kwargs):
# This option replaces federation_ip_range_blacklist in Synapse v1.24.0.
#
ip_range_blacklist:
- '127.0.0.0/8'
- '10.0.0.0/8'
- '172.16.0.0/12'
- '192.168.0.0/16'
- '100.64.0.0/10'
- '169.254.0.0/16'
- '::1/128'
- 'fe80::/64'
- 'fc00::/7'
#- '127.0.0.0/8'
#- '10.0.0.0/8'
#- '172.16.0.0/12'
#- '192.168.0.0/16'
#- '100.64.0.0/10'
#- '169.254.0.0/16'
#- '::1/128'
#- 'fe80::/64'
#- 'fc00::/7'

# Report prometheus metrics on the age of PDUs being sent to and received from
# the following domains. This can be used to give an idea of "delay" on inbound
Expand Down