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

Implementing trusted_proxies CIDR notation capability for IPv6 #12535

Closed
wants to merge 31 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
4dd4379
Adding IpAddress handling utility classes in OC\Net namespace
olivermg Oct 31, 2018
5efc12f
Working on IpAddress classes for CIDR
olivermg Oct 31, 2018
54c3694
Merge branch 'master' of github.com:nextcloud/server into feature/655…
olivermg Nov 4, 2018
9c98e43
Adding test for IpAddressFactory
olivermg Nov 4, 2018
fc7b744
adding tests to IpAddressV4Test.php
olivermg Nov 5, 2018
9674d3b
Merge branch 'master' of github.com:nextcloud/server into feature/655…
olivermg Nov 12, 2018
7b9f5bc
adding tests for IpAddressV4
olivermg Nov 12, 2018
587025a
renaming tests
olivermg Nov 12, 2018
8d35b0e
adding tests for IpAddressV6
olivermg Nov 12, 2018
5fa3396
adding tests for localhost
olivermg Nov 12, 2018
af610cd
moving common methods from IPAddressV[46] to AbstractIpAddress
olivermg Nov 12, 2018
a5503ad
adding author to Request.php
olivermg Nov 12, 2018
cf554e0
removing empty setUp & tearDown methods from tests
olivermg Nov 12, 2018
0920691
shrinking IIpAddress interface to sensible minimum
olivermg Nov 12, 2018
b26e862
Merge branch 'master' of github.com:nextcloud/server into feature/655…
olivermg Nov 19, 2018
b4a5d8f
remove matchesTrustedProxy function from Request.php
olivermg Nov 19, 2018
02c6ed1
adding license headers to new source files
olivermg Nov 19, 2018
ad76224
updating config.sample.php to reflect IPv6 CIDR notation
olivermg Nov 19, 2018
361871d
adding function header comments
olivermg Nov 19, 2018
efc02c6
adding license file headers to test files
olivermg Nov 19, 2018
29488ec
Merge branch 'master' of github.com:nextcloud/server into feature/655…
olivermg Nov 19, 2018
caf3df2
updating license statements in files of OC\Net
olivermg Nov 19, 2018
4ad17fc
pushing @since in \OC\Net\IIpAddress to 16.0.0
olivermg Nov 19, 2018
9b66b4f
moving IIpAddress and IpAddressFactory to OCP namespace
olivermg Nov 20, 2018
36aec3f
adding @author lines
olivermg Nov 20, 2018
811befa
removing obsolete members from concrete IpAddressV[46] classes
olivermg Nov 25, 2018
f82f4fb
introduce IIpAddressFactory and move IpAddressFactory to OC namespace
olivermg Nov 25, 2018
733b416
Merge branch 'master' of github.com:olivermg/server into feature/6550…
olivermg Nov 27, 2018
638d876
Merge branch 'master' of github.com:nextcloud/server into feature/655…
olivermg Nov 27, 2018
bc413ea
inject IIpAddressFactory into Request
olivermg Nov 27, 2018
a8f7ea4
check if an IIpAddressFactory exists in Request
olivermg Nov 27, 2018
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
4 changes: 1 addition & 3 deletions config/config.sample.php
Original file line number Diff line number Diff line change
Expand Up @@ -1523,9 +1523,7 @@
* - IPv4 addresses, e.g. `192.168.2.123`
* - IPv4 ranges in CIDR notation, e.g. `192.168.2.0/24`
* - IPv6 addresses, e.g. `fd9e:21a7:a92c:2323::1`
*
* _(CIDR notation for IPv6 is currently work in progress and thus not
* available as of yet)_
* - IPv6 ranges in CIDR notation, e.g. `fd9e:21a7:a92c:2323::/64`
*
* When an incoming request's `REMOTE_ADDR` matches any of the IP addresses
* specified here, it is assumed to be a proxy instead of a client. Thus, the
Expand Down
52 changes: 22 additions & 30 deletions lib/private/AppFramework/Http/Request.php
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
* @author Lukas Reschke <lukas@statuscode.ch>
* @author Mitar <mitar.git@tnode.com>
* @author Morris Jobke <hey@morrisjobke.de>
* @author Oliver Wegner <void1976@gmail.com>
* @author Robin Appelman <robin@icewind.nl>
* @author Robin McCorkell <robin@mccorkell.me.uk>
* @author Roeland Jago Douma <roeland@famdouma.nl>
Expand Down Expand Up @@ -43,6 +44,7 @@
use OC\Security\TrustedDomainHelper;
use OCP\IConfig;
use OCP\IRequest;
use OCP\Net\IIpAddressFactory;
use OCP\Security\ICrypto;
use OCP\Security\ISecureRandom;

Expand Down Expand Up @@ -111,6 +113,8 @@ class Request implements \ArrayAccess, \Countable, IRequest {
protected $crypto;
/** @var CsrfTokenManager|null */
protected $csrfTokenManager;
/** @var IIpAddressFactory */
protected $ipAddressFactory;

/** @var bool */
protected $contentDecoded = false;
Expand All @@ -136,12 +140,14 @@ public function __construct(array $vars= [],
ISecureRandom $secureRandom = null,
IConfig $config,
CsrfTokenManager $csrfTokenManager = null,
string $stream = 'php://input') {
string $stream = 'php://input',
IIpAddressFactory $ipAddressFactory = null) {
Copy link
Member

Choose a reason for hiding this comment

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

Any particular reason why this is nullable?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not from an application logic point of view and I really don't like it this way. However, making it a mandatory argument will force me to touch quite a few lines of code elsewhere (most in tests though).

$ egrep -rn 'new ([\\[:alnum:]]+\\)?Request[^[:alnum:]]' apps tests lib contribute ocs* ocm-provider settings resources themes config core | grep -v Sabre | wc -l
96

I had initially been happy that there were only two locations in the code that really instantiate Request, but I had not found all of them in production code (e.g. in base.php) and had also not taken the tests into account.

It'll take some time for me to put that additional effort into this.

Copy link
Member

Choose a reason for hiding this comment

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

It'll take some time for me to put that additional effort into this.

Unfortunately this is the correct way to do this. Otherwise this is bound to fail someone in the future that assumes the argument is optional.

$this->inputStream = $stream;
$this->items['params'] = [];
$this->secureRandom = $secureRandom;
$this->config = $config;
$this->csrfTokenManager = $csrfTokenManager;
$this->ipAddressFactory = $ipAddressFactory;

if(!array_key_exists('method', $vars)) {
$vars['method'] = 'GET';
Expand Down Expand Up @@ -598,42 +604,28 @@ public function getId(): string {
return $this->requestId;
}

/**
* Checks if given $remoteAddress matches given $trustedProxy.
* If $trustedProxy is an IPv4 IP range given in CIDR notation, true will be returned if
* $remoteAddress is an IPv4 address within that IP range.
* Otherwise $remoteAddress will be compared to $trustedProxy literally and the result
* will be returned.
* @return boolean true if $remoteAddress matches $trustedProxy, false otherwise
*/
protected function matchesTrustedProxy($trustedProxy, $remoteAddress) {
$cidrre = '/^([0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3})\/([0-9]{1,2})$/';

if (preg_match($cidrre, $trustedProxy, $match)) {
$net = $match[1];
$shiftbits = min(32, max(0, 32 - intval($match[2])));
$netnum = ip2long($net) >> $shiftbits;
$ipnum = ip2long($remoteAddress) >> $shiftbits;

return $ipnum === $netnum;
}

return $trustedProxy === $remoteAddress;
}

/**
* Checks if given $remoteAddress matches any entry in the given array $trustedProxies.
* For details regarding what "match" means, refer to `matchesTrustedProxy`.
* 'Matching' here means
* - $remoteAddress is either equal to an entry in $trustedProxies or
* - $remoteAddress is an IP address in the range of any IP ranges specified in $trustedProxies
* @return boolean true if $remoteAddress matches any entry in $trustedProxies, false otherwise
*/
protected function isTrustedProxy($trustedProxies, $remoteAddress) {
foreach ($trustedProxies as $tp) {
if ($this->matchesTrustedProxy($tp, $remoteAddress)) {
return true;
if ($this->ipAddressFactory !== null) {
$ipAddressRemote = $this->ipAddressFactory->getInstance($remoteAddress);

foreach ($trustedProxies as $tp) {
$ipAddressProxy = $this->ipAddressFactory->getInstance($tp);
if ($ipAddressProxy->containsAddress($ipAddressRemote)) {
return true;
}
}
}

return false;
return false;
} else {
return \in_array($remoteAddress, $trustedProxies);
}
}

/**
Expand Down
149 changes: 149 additions & 0 deletions lib/private/Net/AbstractIpAddress.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,149 @@
<?php
declare(strict_types=1);
/**
* @copyright Copyright (c) 2018, Oliver Wegner (void1976@gmail.com)
*
* @author Oliver Wegner <void1976@gmail.com>
*
* @license GNU AGPL version 3 or any later version
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU Affero General Public License as
* published by the Free Software Foundation, either version 3 of the
* License, or (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU Affero General Public License for more details.
*
* You should have received a copy of the GNU Affero General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*
*/

namespace OC\Net;

use OCP\Net\IIpAddress;

abstract class AbstractIpAddress implements IIpAddress {
abstract public function getMaxBitlength(): int;
abstract protected function getCidrRegex(): string;
abstract protected function matchCidr(IIpAddress $other): bool;

protected $original = '';
protected $netPart = '';
protected $netmaskBits = 0;

/**
* Constructor that takes an IP address in string form and
* initializes this instance to represent that address
*
* @param string $address
*/
public function __construct(string $address) {
$this->setOriginal($address);

if (preg_match($this->getCidrRegex(), $address, $match)) {
$this->setNetPart($match[1]);
$this->setNetmaskBits(max(0, min($this->getMaxBitlength(), intval($match[2]))));
} else {
$this->setNetPart($address);
$this->setNetmaskbits($this->getMaxBitlength());
}
}

/**
* Sets the literal address string that this instance
* represents
*
* @param string $original
*/
protected function setOriginal(string $original) {
$this->original = $original;
}

/**
* Returns the literal address string that this instance
* represents
*
* @return string
*/
protected function getOriginal(): string {
return $this->original;
}

/**
* Sets the network part of the
* address/range represented by this instance
*
* @param string $netPart
*/
protected function setNetPart(string $netPart) {
$this->netPart = $netPart;
}

/**
* Returns the network part of the
* address/range represented by this instance
*
* @return string
*/
protected function getNetPart(): string {
return $this->netPart;
}

/**
* Sets the number of bits of the net part of the IP
* address/range represented by this instance
*
* @param int $bits
*/
protected function setNetmaskBits(int $bits) {
$this->netmaskBits = $bits;
}

/**
* Returns the number of bits of the net part of the IP
* address/range represented by this instance
*
* @return int
*/
protected function getNetmaskBits(): int {
return $this->netmaskBits;
}

/**
* Returns whether $other is literally equivalent to this instance
*
* @return bool
*/
protected function matchOriginal(IIpAddress $other): bool {
return $other->getOriginal() === $this->getOriginal();
}

/**
* Returns whether this instance represents an IP range (vs.
* a single IP address)
*
* @return bool
*/
public function isRange(): bool {
return $this->getNetmaskBits() < $this->getMaxBitlength();
}

/**
* Returns whether given $other address is either
* - equal to this instance regarding its IP address or
* - is contained in the IP address range represented by this instance
*
* @param IIpAddress $other
* @return bool
*/
public function containsAddress(IIpAddress $other): bool {
return $this->isRange()
? $this->matchCidr($other)
: $this->matchOriginal($other);
}
}

63 changes: 63 additions & 0 deletions lib/private/Net/IpAddressFactory.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
<?php
declare(strict_types=1);
/**
* @copyright Copyright (c) 2018, Oliver Wegner (void1976@gmail.com)
*
* @author Oliver Wegner <void1976@gmail.com>
*
* @license GNU AGPL version 3 or any later version
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU Affero General Public License as
* published by the Free Software Foundation, either version 3 of the
* License, or (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU Affero General Public License for more details.
*
* You should have received a copy of the GNU Affero General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*
*/

namespace OC\Net;

use OCP\Net\IIpAddressFactory;
use OCP\Net\IIpAddress;
use OC\Net\IpAddressV4;
use OC\Net\IpAddressV6;

/**
* This factory creates instances of IIpAddress, given an IP address
* string (e.g. "192.168.1.2" or subnet string in CIDR format (e.g.
* "192.168.1.0/24").
*/
class IpAddressFactory implements IIpAddressFactory {
/**
* Returns whether $address represents an IPv6 address
*
* @param string $address
* @return bool
*/
protected static function isIpv6(string $address): bool {
return strpos($address, ':') !== false;
}

/**
* Creates a new instance conforming to IIpAddress and
* representing the given $address.
*
* @param string $address
* @return IIpAddress
*/
public function getInstance(string $address): IIpAddress {
if (self::isIpv6($address)) {
return new IpAddressV6($address);
} else {
return new IpAddressV4($address);
}
}
}

65 changes: 65 additions & 0 deletions lib/private/Net/IpAddressV4.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
<?php
declare(strict_types=1);
/**
* @copyright Copyright (c) 2018, Oliver Wegner (void1976@gmail.com)
*
* @author Oliver Wegner <void1976@gmail.com>
*
* @license GNU AGPL version 3 or any later version
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU Affero General Public License as
* published by the Free Software Foundation, either version 3 of the
* License, or (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU Affero General Public License for more details.
*
* You should have received a copy of the GNU Affero General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*
*/

namespace OC\Net;

use OCP\Net\IIpAddress;
use OC\Net\AbstractIpAddress;

class IpAddressV4 extends AbstractIpAddress {
/**
* Returns the length of the represented IP address format in bits.
*
* @return int
*/
public function getMaxBitlength(): int {
return 32;
}

/**
* Returns the regular expression for recognizing CIDR notation.
*
* @return string
*/
protected function getCidrRegex(): string {
return '/^([0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3})\/([0-9]{1,2})$/';

Choose a reason for hiding this comment

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

This regex matches invalid IPv4 subnets: https://regex101.com/r/tjtE95/1 (same for IPv6 in the IpAddressV6 class).

As IP address are numbers, I think you could use numerical comparison and avoid relying on regexes (that will most certainly be incorrect in a way or another). I'm not really into PHP so I'm not sure what's the best way to do that (to be honest it bugs me that this is not part of the standard library).

Also, I think it would be useful to warn the administrator when they put a wrong value in the configuration (thus using another "parsing" method).

}

/**
* Returns whether given $other address is either
* - equal to this instance regarding its IP address or
* - is contained in the IP address range represented by this instance
*
* @param IIpAddress $other
* @return bool
*/
protected function matchCidr(IIpAddress $other): bool {
$shiftbits = $this->getMaxBitlength() - $this->getNetmaskBits();
$thisnum = ip2long($this->getNetPart()) >> $shiftbits;
$othernum = ip2long($other->getNetPart()) >> $shiftbits;

return $othernum === $thisnum;
}
}

Loading