Skip to content

Commit

Permalink
Authentication could be bypassed if URI had multiple slashes
Browse files Browse the repository at this point in the history
RequestPathRule now removes multiple slashes from the URI before
determining whether the path should be authenticated or not. For
client /foo and //foo are different URIs. On server side it
depends on implementation but they usually map to the same route
action.

Different PSR-7 implementations were behaving in different way.
Diactoros removes multiple leading slashes while Slim does not.

    * If you are authenticating a subfolder, for example /api,
      with Slim was possible to bypass authentication by doing
      a request to //api.

    * If you are using default setting of authenticating all
      routes you were not affected.

    * Diactoros was not affected.

See tuupola/slim-jwt-auth#50
  • Loading branch information
tuupola committed Feb 27, 2017
1 parent 69e2451 commit c615528
Show file tree
Hide file tree
Showing 3 changed files with 109 additions and 1 deletion.
51 changes: 51 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
# Changelog

All notable changes to this project will be documented in this file, in reverse chronological order by release.

## 2.2.2 - 2017-02-27

This is a security release.

`RequestPathRule` now removes multiple slashes from the URI before determining whether the path should be authenticated or not. For client `/foo` and `//foo` are different URIs. On server side it depends on implementation but they usually map to the same route action.

Different PSR-7 implementations were behaving in different way. Diactoros [removes multiple leading slashes](https://github.com/zendframework/zend-diactoros/blob/master/CHANGELOG.md#104---2015-06-23) while Slim does not.

This means if you are authenticating a subfolder, for example `/api`, with Slim it was possible to bypass authentication by doing a request to `GET //api`. Diactoros was not affected.

```php
$app->add(new \Slim\Middleware\HttpBasicAuthentication([
"path" => "/api",
"users" => [
"root" => "t00r",
"somebody" => "passw0rd"
]
]));
```

If you were using default setting of authenticating all routes you were not affected.

```php
$app->add(new \Slim\Middleware\HttpBasicAuthentication([
"users" => [
"root" => "t00r",
"somebody" => "passw0rd"
]
]));
```

### Added

- Nothing.

### Deprecated

- Nothing.

### Removed

- Nothing.

### Fixed

- Ported fix for bug [slim-jwt-auth/50](https://github.com/tuupola/slim-jwt-auth/issues/50) where in some cases it was possible to bypass authentication by adding multiple slashes to request URI.

19 changes: 18 additions & 1 deletion src/HttpBasicAuthentication/RequestPathRule.php
Original file line number Diff line number Diff line change
Expand Up @@ -17,22 +17,39 @@

use Psr\Http\Message\RequestInterface;

/**
* Rule to decide by request path whether the request should be authenticated or not.
*/

class RequestPathRule implements RuleInterface
{
/**
* Stores all the options passed to the rule
*/
protected $options = [
"path" => ["/"],
"passthrough" => []
];

/**
* Create a new rule instance
*
* @param string[] $options
* @return void
*/
public function __construct($options = [])
{
$this->options = array_merge($this->options, $options);
}

/**
* @param \Psr\Http\Message\RequestInterface $request
* @return boolean
*/
public function __invoke(RequestInterface $request)
{
$uri = "/" . $request->getUri()->getPath();
$uri = str_replace("//", "/", $uri);
$uri = preg_replace("#/+#", "/", $uri);

/* If request path is matches passthrough should not authenticate. */
foreach ((array)$this->options["passthrough"] as $passthrough) {
Expand Down
40 changes: 40 additions & 0 deletions tests/RequestPathRuleTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -118,4 +118,44 @@ public function testShouldPassthroughLogin()

$this->assertFalse($rule($request));
}

public function testBug50ShouldAuthenticateMultipleSlashes()
{
$request = (new Request)
->withUri(new Uri("https://example.com/"))
->withMethod("GET");

$rule = new RequestPathRule(["path" => "/v1/api"]);
$this->assertFalse($rule($request));

$request = (new Request)
->withUri(new Uri("https://example.com/v1/api"))
->withMethod("GET");

$this->assertTrue($rule($request));

$request = (new Request)
->withUri(new Uri("https://example.com/v1//api"))
->withMethod("GET");

$this->assertTrue($rule($request));

$request = (new Request)
->withUri(new Uri("https://example.com/v1//////api"))
->withMethod("GET");

$this->assertTrue($rule($request));

$request = (new Request)
->withUri(new Uri("https://example.com//v1/api"))
->withMethod("GET");

$this->assertTrue($rule($request));

$request = (new Request)
->withUri(new Uri("https://example.com//////v1/api"))
->withMethod("GET");

$this->assertTrue($rule($request));
}
}

0 comments on commit c615528

Please sign in to comment.