Bug #90047: BUG with IP6 in IpLocker.php after protokoll switch ipv4 to ipv6 - TYPO3 Core - TYPO3 Forge

Actions

Copy link

Bug #90047

closed

BUG with IP6 in IpLocker.php after protokoll switch ipv4 to ipv6

Added by Timo Poppinga over 4 years ago. Updated about 4 years ago.

Status:

Closed

Priority:

Should have

Assignee:

Category:

Target version:

Start date:

2020-01-02

Due date:

% Done:

100%

Estimated time:

TYPO3 Version:

PHP Version:

Tags:

Complexity:

no-brainer

Is Regression:

Sprint Focus:

Description

IpLocker.php dose not work with

TYPO3\CMS\Core\Utility\MathUtility\IpLocker

wrong code

public function validateRemoteAddressAgainstSessionIpLock(string $ipAddress, string $sessionIpLock): bool
{
        if ($sessionIpLock === static::DISABLED_LOCK_VALUE) {
            return true;
        }

$ipToCompare = $this->isIpv6Address($sessionIpLock)
            ? $this->getIpLockPartForIpv6Address($ipAddress)
            : $this->getIpLockPartForIpv4Address($ipAddress);
        return $ipToCompare === $sessionIpLock;
    }

Can only be commperd if same protokoll is used

to fix:

$ipToCompare = $this->isIpv6Address($ipAddress) !!!!!
            ? $this->getIpLockPartForIpv6Address($ipAddress)
            : $this->getIpLockPartForIpv4Address($ipAddress);

But then user will be logged out at protocol switch

Related issues 1 (0 open — 1 closed)

Actions

Copy link

Updated by Markus Klein over 4 years ago

Status changed from New to Needs Feedback
Target version deleted (~~next-patchlevel~~)

With a protocol switch you mean switching from v4 to v6, right?
If so, we suggest disabling the IPlocking completely, since it has no benefit.

Updated by Markus Klein over 4 years ago

Related to Bug #89869: IP Lock feature broken by modern IPv6 - Should be disabled by default or refactored added

Actions

Copy link

Updated by Timo Poppinga about 4 years ago

Hi Markus, yes that is what I mean "With a protocol switch you mean switching from v4 to v6". I do alredy disable IP-Lock witch is working fine.

My Issue is an hint if some one doese not disable IP-Lock an warning is thrown witch is in an good server setup an error. By changing the code no warning appiers but you are right user will still be locked out. Thx

Actions

Copy link

Updated by Markus Klein about 4 years ago

If I understand you correctly, I can close this ticket, right?

Actions

Copy link

Updated by Timo Poppinga about 4 years ago

yes and no

point is this code doese not work well on default setting iplock = on:

$ipToCompare = $this->isIpv6Address($sessionIpLock)
? $this->getIpLockPartForIpv6Address($ipAddress)
: $this->getIpLockPartForIpv4Address($ipAddress);
return $ipToCompare === $sessionIpLock;
}

if ip swichtes the condition checking choosing the normalizer checks on $sessionIpLock but then the $ipAddress is send to the methoed. If for example session is locked witch ip4 and then connection is swicht to ipv6 $this->getIpLockPartForIpv4Address tryes to prosess an ipv6 witch triggerd an warning in most context this throws an php error.

I see three options:

1. accept it and update documentation to do not use iplock in mixed ip stack
2. make some more checks in getIpLockPartFor Ipv4Address getIpLockPartFor Ipv6 Address so that no wornings are thrown if ip stack is swicht
3. Use my code above (my is has security impact)
4.

My favorite one is the throw a special exception if ipLook is on and and ip stack change is detected

like:

if($this->isIpv6Address($sessionIpLock) !== $this->isIpv6Address($ipAddress)) {
            throw new \Exception('You are using both ip stacks ipv6 and ipv4 ip lock does not work please disable it in the install tool');
        }

Actions

Copy link

Updated by Markus Klein about 4 years ago

Status changed from Needs Feedback to Accepted

Alright.

I don't think it is good to introduce a new exception that might break one's Frontend.
I would rather stick to logging a critical-message.

I would appreciate implementing your fix to prevent PHP errors as a quick and easy patch.
The logging part should be an improvement for integrators and I would put that into a dedicated change.

Can you provide the change you proposed to our review system?

Actions

Copy link

Updated by Gerrit Code Review about 4 years ago

Status changed from Accepted to Under Review

Patch set 1 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/63001

Actions

Copy link

Updated by Gerrit Code Review about 4 years ago

Patch set 2 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/63001

Actions

Copy link

Updated by Gerrit Code Review about 4 years ago

Patch set 1 for branch 9.5 of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/62975

Actions

Copy link

#10