Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

trusted proxies validation is invalid and allows nonsense values #544

Open
isdnfan opened this issue Nov 19, 2024 · 1 comment
Open

trusted proxies validation is invalid and allows nonsense values #544

isdnfan opened this issue Nov 19, 2024 · 1 comment
Labels
bug Something isn't working

Comments

@isdnfan
Copy link

isdnfan commented Nov 19, 2024

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 isdnfan added the bug Something isn't working label Nov 19, 2024
@icewind1991
Copy link
Member

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants