Skip to content

Commit 9fa14ca

Browse files
sleinersleiner
committed
Support specifying IPv6 proxies in CIDR notation
Previously, it was not possible to use CIDR notation for IPv6 proxies in the trusted_proxies parameter of config.php [1]. This patch adds support for that, mainly by relying on inet_pton and a custom bitwise string comparison function instead of ip2long (which does not work for IPv6 because of its 128 bit address space). It is worth noting that the bitwise comparison could be implemented a bit more straightforward using GMP [2], but since Nextcloud does not currently require GMP to be available, this alternative implementation was chosen instead. [1]: https://docs.nextcloud.com/server/24/admin_manual/configuration_server/reverse_proxy_configuration.html#defining-trusted-proxies [2]: https://www.php.net/manual/en/class.gmp.php Signed-off-by: Simon Leiner <simon@leiner.me>
1 parent 00f9ad3 commit 9fa14ca

File tree

2 files changed

+126
-10
lines changed

2 files changed

+126
-10
lines changed

‎lib/private/AppFramework/Http/Request.php‎

Lines changed: 73 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@
2525
* @author Thomas Müller <thomas.mueller@tmit.eu>
2626
* @author Thomas Tanghus <thomas@tanghus.net>
2727
* @author Vincent Petry <vincent@nextcloud.com>
28+
* @author Simon Leiner <simon@leiner.me>
2829
*
2930
* @license AGPL-3.0
3031
*
@@ -574,25 +575,87 @@ public function getId(): string {
574575

575576
/**
576577
* Checks if given $remoteAddress matches given $trustedProxy.
577-
* If $trustedProxy is an IPv4 IP range given in CIDR notation, true will be returned if
578-
* $remoteAddress is an IPv4 address within that IP range.
578+
* If $trustedProxy is an IP range given in CIDR notation, true will be returned if
579+
* $remoteAddress is an IP address within that IP range.
579580
* Otherwise $remoteAddress will be compared to $trustedProxy literally and the result
580581
* will be returned.
581582
* @return boolean true if $remoteAddress matches $trustedProxy, false otherwise
582583
*/
583584
protected function matchesTrustedProxy($trustedProxy, $remoteAddress) {
584-
$cidrre = '/^([0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3})\/([0-9]{1,2})$/';
585+
[$proxyAddress, $prefixLength] = array_pad(explode('/', $trustedProxy, 2), 2, null);
586+
return $this->ipAddressInNetwork(inet_pton($remoteAddress),
587+
inet_pton($proxyAddress),
588+
intval($prefixLength));
589+
}
590+
591+
/**
592+
* Checks if a given $ipAddr is part of a given subnet, as defined by $netAddr and
593+
* $netPrefixLength. Both $ipAddr and $netAddr must be given as packed in_addr
594+
* representation (for more details, see the documentation of inet_pton()).
595+
* @return boolean true if $ipAddr is part of the given subnet, false otherwise
596+
*/
597+
private function ipAddressInNetwork(string $ipAddr, string $netAddr, int $netPrefixLength) : bool {
598+
$ipLengthBytes = strlen($ipAddr);
599+
$netLengthBytes = strlen($netAddr);
600+
601+
if ($ipLengthBytes != $netLengthBytes) {
602+
// $netAddr and $ipAddr have different length.
603+
// => They cannot match.
604+
return false;
605+
}
606+
607+
// A prefix must have positive length.
608+
// Also, it cannot be larger than the net address.
609+
$hasValidPrefix = (0 < $netPrefixLength) && ($netPrefixLength <= $ipLengthBytes * 8);
610+
611+
if ($hasValidPrefix) {
612+
return $this->bitstringsShareCommonPrefix($ipAddr, $netAddr, $netPrefixLength);
613+
614+
} else {
615+
// No prefix length was given.
616+
// => The net consists of only a single address.
617+
return $ipAddr === $netAddr;
618+
}
619+
}
620+
621+
/**
622+
* Equivalent of strncmp with the length parameter referring to bits instead of bytes.
623+
* The function performs a binary-safe comparison of $str1 and $str2 for at most $numBits bits.
624+
* @return true if $str1 and $str2 are either equal or, if at least one of them is longer than
625+
* $numBits,their first $numBits are equal, false otherwise
626+
*/
627+
private function bitstringsShareCommonPrefix(string $str1, string $str2, int $numBits) : bool {
628+
$numBitsInByte = 8;
585629

586-
if (preg_match($cidrre, $trustedProxy, $match)) {
587-
$net = $match[1];
588-
$shiftbits = min(32, max(0, 32 - intval($match[2])));
589-
$netnum = ip2long($net) >> $shiftbits;
590-
$ipnum = ip2long($remoteAddress) >> $shiftbits;
630+
// $numBits may not be a multiple of 8. Thus, the prefix may span a
631+
// number of "whole" bytes ($cmpBytes) and possibly one "fractional" byte at
632+
// the end where only $cmpBits are relevant to the comparison.
633+
// This implementation first checks the whole bytes, and the fractional one
634+
// afterwards (if there is one and the whole bytes were all equal).
635+
$cmpBytes = intdiv($numBits, $numBitsInByte);
591636

592-
return $ipnum === $netnum;
637+
// "whole" byte comparison
638+
if (strncmp($str1, $str2, $cmpBytes) != 0) {
639+
return false;
593640
}
594641

595-
return $trustedProxy === $remoteAddress;
642+
// "fractional" byte comparison
643+
$maxBytes = min(strlen($str1), strlen($str2));
644+
if ($cmpBytes >= $maxBytes) {
645+
// The whole string was already compared.
646+
// => No need for further comparison.
647+
return true;
648+
}
649+
650+
// Because we are only interested in the leftmost $cmpBits bits, we can
651+
// discard the bits on the right of that.
652+
$cmpBits = $numBits % $numBitsInByte;
653+
$irrelevantBits = $numBitsInByte - $cmpBits;
654+
655+
$relevant1 = ord($str1[$cmpBytes]) >> $irrelevantBits;
656+
$relevant2 = ord($str2[$cmpBytes]) >> $irrelevantBits;
657+
658+
return $relevant1 == $relevant2;
596659
}
597660

598661
/**

‎tests/lib/AppFramework/Http/RequestTest.php‎

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -585,6 +585,59 @@ public function testGetRemoteAddressWithNotMatchingCidrTrustedRemote() {
585585
$this->assertSame('192.168.3.99', $request->getRemoteAddress());
586586
}
587587

588+
public function testGetRemoteIpv6AddressWithMatchingIpv6CidrTrustedRemote() {
589+
$this->config
590+
->expects($this->at(0))
591+
->method('getSystemValue')
592+
->with('trusted_proxies')
593+
->willReturn(['fd::/8']);
594+
$this->config
595+
->expects($this->at(1))
596+
->method('getSystemValue')
597+
->with('forwarded_for_headers')
598+
->willReturn(['HTTP_X_FORWARDED_FOR']);
599+
600+
$request = new Request(
601+
[
602+
'server' => [
603+
'REMOTE_ADDR' => 'fd:1234::5678',
604+
'HTTP_X_FORWARDED' => '10.4.0.5, 10.4.0.4',
605+
'HTTP_X_FORWARDED_FOR' => '192.168.0.233'
606+
],
607+
],
608+
$this->requestId,
609+
$this->config,
610+
$this->csrfTokenManager,
611+
$this->stream
612+
);
613+
614+
$this->assertSame('192.168.0.233', $request->getRemoteAddress());
615+
}
616+
617+
public function testGetRemoteAddressIpv6WithNotMatchingCidrTrustedRemote() {
618+
$this->config
619+
->expects($this->once())
620+
->method('getSystemValue')
621+
->with('trusted_proxies')
622+
->willReturn(['fd::/8']);
623+
624+
$request = new Request(
625+
[
626+
'server' => [
627+
'REMOTE_ADDR' => '2001:db8:85a3:8d3:1319:8a2e:370:7348',
628+
'HTTP_X_FORWARDED' => '10.4.0.5, 10.4.0.4',
629+
'HTTP_X_FORWARDED_FOR' => '192.168.0.233'
630+
],
631+
],
632+
$this->requestId,
633+
$this->config,
634+
$this->csrfTokenManager,
635+
$this->stream
636+
);
637+
638+
$this->assertSame('2001:db8:85a3:8d3:1319:8a2e:370:7348', $request->getRemoteAddress());
639+
}
640+
588641
public function testGetRemoteAddressWithXForwardedForIPv6() {
589642
$this->config
590643
->expects($this->at(0))

0 commit comments

Comments
 (0)