notify_push icon indicating copy to clipboard operation
notify_push copied to clipboard

trusted proxies validation is invalid and allows nonsense values

Open isdnfan opened this issue 1 year ago • 1 comments

How to use GitHub

  • Please use the 👍 reaction to show that you are affected by the same issue.
  • Please don't comment if you have no relevant information to add. It's just extra noise for everyone subscribed to this issue.
  • Subscribe to receive notifications on status change and new comments.

Steps to reproduce

  1. add trusted_proxies with IPv6 syntax
  2. create invalid notify_push config e.g. wrong trusted_proxies or missing headers
  3. perform either occ notify_push:setup ... or occ notify_push:self-test
  4. a warning about as invalid IPv6 proxy is shown

Expected behaviour

according to docs https://docs.nextcloud.com/server/stable/admin_manual/configuration_server/reverse_proxy_configuration.html reverse proxy could be IPv4 addresses and ranges in CIDR notation, IPv6 addresses and ranges in CIDR notation. the check should reflect this fact.

  • IPv6 syntax should be valid
  • IPv4 CIDR syntax should be verified properly (current regex allows nonsense values like 333.444.555.666/77)

Actual behaviour

🗴 push server is not a trusted proxy by Nextcloud or another proxy in the chain.
  Nextcloud resolved the following client address for the test request: "Invalid response when testing if the push server is a trusted proxy: invalid IP address syntax" instead of the expected "1.2.3.4" test value.
  The following trusted proxies are currently configured: "172.16.0.0/12", "192.168.0.0/16", "10.0.0.0/8", "fc00::/7", "fe80::/10", "2001:db8::/32"
    of which the following seem to be invalid: "fc00::/7", "fe80::/10", "2001:db8::/32"
  The following x-forwarded-for header was received by Nextcloud: ""
    from the following remote:

the problem results from $cidrre = '/^([0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3})\/([0-9]{1,2})$/'; regex used in SelfTest.php which doesn't match IPv6 (and is not strict enough to match only valid IPv4 syntax)

https://github.com/nextcloud/notify_push/blob/f71fcbc1a0b348d7b4417bb65cb624bd79eb22bc/lib/SelfTest.php#L240C1-L248C3

private function isValidProxyConfig(string $proxyConfig): bool {
	$cidrre = '/^([0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3})\/([0-9]{1,2})$/';

	if (filter_var($proxyConfig, FILTER_VALIDATE_IP) !== false) {
		return true;
	} else {
		return (bool)preg_match($cidrre, $proxyConfig);
	}
}

there are better regex like https://uibakery.io/regex-library/ip-address-regex-php or better check functions like https://gist.github.com/pavinjosdev/cb1d636ea9dc2bd201d54107d10650c5

isdnfan avatar Nov 19 '24 21:11 isdnfan

Note that isValidProxyConfig is only used for generating the diagnostics when a trusted proxy issue is detected. It has no impact on whether or not the trusted setup check succeeds or fails.

(it should still be fixed ofc)

icewind1991 avatar Jan 20 '25 19:01 icewind1991