Bug #90047

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

Added by Timo Poppinga 23 days ago. Updated 3 days ago.

Status:
Resolved
Priority:
Should have
Assignee:
-
Category:
-
Target version:
-
Start date:
2020-01-02
Due date:
% Done:

100%

TYPO3 Version:
10
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

Related to TYPO3 Core - Bug #89869: IP Lock feature broken by modern IPv6 - Should be disabled by default or refactored Closed 2019-12-06

Associated revisions

Revision 7fa0dcb2 (diff)
Added by Timo Poppinga 3 days ago

[BUGFIX] IP locking: Handle IP stack switch without PHP error

Adjust the IP validation to avoid a warning in case the IP stack
used by the current user has changed.

Resolves: #90047
Releases: master, 9.5
Change-Id: I814e72648c7b404acc5eaf0b2270685dbab16abe
Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/63001
Tested-by: TYPO3com <>
Tested-by: Markus Klein <>
Tested-by: Georg Ringer <>
Reviewed-by: Markus Klein <>
Reviewed-by: Richard Haeser <>
Reviewed-by: Georg Ringer <>

History

#1 Updated by Markus Klein 9 days 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.

See also #89869

#2 Updated by Markus Klein 9 days ago

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

#3 Updated by Timo Poppinga 6 days 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

#4 Updated by Markus Klein 6 days ago

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

#5 Updated by Timo Poppinga 6 days 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');
}

#6 Updated by Markus Klein 6 days 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?

#7 Updated by Gerrit Code Review 5 days 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

#8 Updated by Gerrit Code Review 5 days 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

#9 Updated by Gerrit Code Review 3 days 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

#10 Updated by Timo Poppinga 3 days ago

  • Status changed from Under Review to Resolved
  • % Done changed from 0 to 100

Also available in: Atom PDF