Skip to content

Commit ee24bd4

Browse files
authored
Fix an open redirect weakness in getLoginRedirect() (#795)
Because of how browsers handle the `Location` header, values beginning with `\` can be leveraged to create redirect targets on other domains. Thanks to Volker Dusch and the PHP Ecosystem security team for reporting this.
1 parent 6d6cc71 commit ee24bd4

2 files changed

Lines changed: 49 additions & 2 deletions

File tree

src/AuthenticationService.php

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -398,15 +398,29 @@ public function getLoginRedirect(ServerRequestInterface $request): ?string
398398
) {
399399
return null;
400400
}
401+
$value = (string)$params[$redirectParam];
401402

402-
$parsed = parse_url((string)$params[$redirectParam]);
403+
// In the `Location` header, Browsers normalize \ to /
404+
// (see WHATWG URL Standard).
405+
// We do the same to prevent injection via \ sequences.
406+
$normalized = str_replace('\\', '/', $value);
407+
408+
// A leading run of `//` or `\\` are rejected
409+
if (str_starts_with($normalized, '//')) {
410+
return null;
411+
}
412+
413+
$parsed = parse_url($normalized);
403414
if ($parsed === false) {
404415
return null;
405416
}
406417
if (!empty($parsed['host']) || !empty($parsed['scheme'])) {
407418
return null;
408419
}
409420
$parsed += ['path' => '/', 'query' => ''];
421+
if (str_contains($parsed['path'], '\\')) {
422+
return null;
423+
}
410424
if (strlen($parsed['path']) && $parsed['path'][0] !== '/') {
411425
$parsed['path'] = '/' . $parsed['path'];
412426
}

tests/TestCase/AuthenticationServiceTest.php

Lines changed: 34 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -822,7 +822,7 @@ public function testGetUnauthenticatedRedirectUrlWithBasePath(): void
822822
);
823823
}
824824

825-
public function testGetLoginRedirect(): void
825+
public function testGetLoginRedirectInvalid(): void
826826
{
827827
$service = new AuthenticationService([
828828
'unauthenticatedRedirect' => '/users/login',
@@ -863,6 +863,39 @@ public function testGetLoginRedirect(): void
863863
'/path/with?query=string',
864864
$service->getLoginRedirect($request),
865865
);
866+
867+
$request = ServerRequestFactory::fromGlobals(
868+
['REQUEST_URI' => '/login'],
869+
['redirect' => '/\\evil.com'],
870+
);
871+
$this->assertNull(
872+
$service->getLoginRedirect($request),
873+
);
874+
875+
$request = ServerRequestFactory::fromGlobals(
876+
['REQUEST_URI' => '/login'],
877+
['redirect' => '\\/\\evil.com'],
878+
);
879+
$this->assertNull(
880+
$service->getLoginRedirect($request),
881+
);
882+
883+
$request = ServerRequestFactory::fromGlobals(
884+
['REQUEST_URI' => '/login'],
885+
['redirect' => '\\evil.com/path'],
886+
);
887+
$this->assertSame(
888+
'/evil.com/path',
889+
$service->getLoginRedirect($request),
890+
);
891+
892+
$request = ServerRequestFactory::fromGlobals(
893+
['REQUEST_URI' => '/login'],
894+
['redirect' => '\\\\evil.com/path'],
895+
);
896+
$this->assertNull(
897+
$service->getLoginRedirect($request),
898+
);
866899
}
867900

868901
/**

0 commit comments

Comments
 (0)