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

Problem: limit administrator functionality by IP address – add IPv6 support #1821

Closed
Jimadine opened this issue May 22, 2024 · 0 comments · Fixed by #1830
Closed

Problem: limit administrator functionality by IP address – add IPv6 support #1821

Jimadine opened this issue May 22, 2024 · 0 comments · Fixed by #1830
Labels
Type: bug A flaw in the code that causes the software to produce an incorrect or unexpected result.

Comments

@Jimadine
Copy link
Contributor

Jimadine commented May 22, 2024

Current Behavior

Steps to reproduce the behavior

Setup

  1. Browse to the AtoM site
  2. Click the Login button and enter the email address and password of a user with the administrator privilege
  3. Browse to the Security Settings page (/index.php/settings/security) and enter an IPv6 IP range into the box (separate the start and end range with a or , character)

Reproduce

  1. From an IPv6 machine outside of the IP range just added, launch a browser and browse to the AtoM site. Click the Login button and enter the email address and password of a user with the administrator privilege.
  2. Click the Admin cog, and attempt to browse to any of the menu links

Current behaviour is that when attempting to access pages from an IPv6 address outside of the set ranges, AtoM allows access when the expectation is that it shouldn’t. This apparently happens when the client IP address is an IPv6 address. This unexpected behaviour does appear to be dependant on having entered an IPv6 range as a range restriction.

Expected Behavior

When attempting to access pages from an IP address outside of the set ranges, AtoM should consistently return a Sorry you do not have permission to access this page message, irrespective of whether the remote address is IPv6 or IPv4.

Possible Solution

Well, I'm not a PHP programmer, but I've managed to re-work lib/filter/QubitLimitIp.class.php to deliver the expected behaviour:

<?php

/*
 * This file is part of the Access to Memory (AtoM) software.
 *
 * Access to Memory (AtoM) 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.
 *
 * Access to Memory (AtoM) 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 General Public License for more details.
 *
 * You should have received a copy of the GNU General Public License
 * along with Access to Memory (AtoM).  If not, see <http://www.gnu.org/licenses/>.
 */

class QubitLimitIpFilter extends sfFilter
{
    public function execute($filterChain)
    {
        $this->context = $this->getContext();
        $this->request = $this->context->getRequest();

        $this->limit = explode(';', sfConfig::get('app_limit_admin_ip'));

        // Pass if:
        // - Debug mode is on
        // - Setting "limit_admin_ip" is not set
        // - The filter is forwarding to admin/secure (isFirstCall)
        // - Route is user/logout
        if (
            $this->context->getConfiguration()->isDebug()
            || !$this->limit
            || !$this->isFirstCall()
            || ('user' == $this->request->getParameter('module') && 'logout' == $this->request->getParameter('action'))
        ) {
            $filterChain->execute();

            return;
        }

        // Forward to admin/secure if not allowed (only applies if user is authenticated)
        if ($this->context->user->isAuthenticated() && !$this->isAllowed()) {
            $this->context->getController()->forward(sfConfig::get('sf_secure_module'), sfConfig::get('sf_secure_action'));

            throw new sfStopException();
        }

        $filterChain->execute();
    }

    protected function getRemoteAddress()
    {
        $pathInfo = $this->request->getPathInfoArray();

        return $pathInfo['REMOTE_ADDR'];
    }

    protected function isAllowed()
    {
        $address = $this->getRemoteAddress();
        $addressBinary = inet_pton($address);

        // Check if empty
        if (1 == count($this->limit) && empty($this->limit[0])) {
            return true;
        }

        foreach ($this->limit as $item) {
            // Ranges are supported, using a comma or a dash
            $limit = preg_split('/[,-]/', $item);
            $limitBinary = inet_pton(trim($limit[0]));

            // Single IP
            if (1 == count($limit) && $addressBinary == $limitBinary && strlen($addressBinary) == strlen($limitBinary)) {
                return true;
            }

            // Range
            if (2 == count($limit)) {
                $limit[0] = trim($limit[0]);
                $limit[1] = trim($limit[1]);
                $firstInRangeBinary = inet_pton($limit[0]);
                $lastInRangeBinary = inet_pton($limit[1]);

                if (
                    (strlen($addressBinary) == strlen($firstInRangeBinary))
                     && ($addressBinary >= $firstInRangeBinary && $addressBinary <= $lastInRangeBinary)
                   ) {
                    return true;
                }
            }
        }
        return false;
    }

}

My changes replace ip2long with inet_pton as the latter can handle both IPv4 and IPv6 addresses. I've only done a very limited amount of testing, and would appreciate a second opinion. I have also added strlen checks, as according to this SO answer "when comparing them the length of the string must also be checked in order to prevent an IPv4 address to match the first part of an IPv6 address.".

Context and Notes

A bit more local information and background:

  • We are using the LIMIT ADMINISTRATOR FUNCTIONALITY BY IP ADDRESS feature documented here
  • We have three “allowed” IP ranges set – two IPv4 ranges and one IPv6 range — on the assumption that both v4 and v6 are supported by the feature.
  • I am using Opera browser's built-in VPN to test this. Changing the continent in the browser changes the IP address and sometimes the IP address type (v4/v6).
  • From the Nginx access logs (/var/log/nginx/access.log), it appears that when the client IP address is IPv4, AtoM behaves as expected – access is blocked (403 response code). However, when the client IP address is IPv6, access is wrongly allowed (200 response code)
  • Sample log lines below:
77.111.245.12 - - [20/May/2024:14:57:14 +0100] "GET /index.php/settings/global HTTP/2.0" 403 4331 "https://borthcat.york.ac.uk/index.php/accession/browse" "Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/123.0.0.0 Safari/537.36 OPR/109.0.0.0"
2001:67c:2628:647:f::223 - - [20/May/2024:15:03:46 +0100] "GET /index.php/settings/global HTTP/2.0" 200 7210 "https://borthcat.york.ac.uk/index.php/aclGroup/list" "Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/123.0.0.0 Safari/537.36 OPR/109.0.0.0"

It looks like lib/filter/QubitLimitIp.class.php is handling this behind the scenes. This file uses the ip2long function, which is apparently only designed to cope with IPv4 IP addresses (the ip2long function is described: “Converts a string containing an (IPv4) Internet Protocol dotted address into a long integer”).

Version used

AtoM 2.7.3

Operating System and version

Ubuntu 20.04

Default installation culture

en

PHP version

PHP 7.4

Contact details

No response

@Jimadine Jimadine added the Type: bug A flaw in the code that causes the software to produce an incorrect or unexpected result. label May 22, 2024
@anvit anvit linked a pull request Jun 21, 2024 that will close this issue
anvit pushed a commit that referenced this issue Jun 26, 2024
Updates QubitLimitIp.class.php to add support for IPv6 ranges
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: bug A flaw in the code that causes the software to produce an incorrect or unexpected result.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant