Commit 8ad79ceb authored by catch's avatar catch

Issue #2224777 by pwolanin, dawehner: Unlimited allowed length of path in menu...

Issue #2224777 by pwolanin, dawehner: Unlimited allowed length of path in menu router allows DoS attack on Drupal 8.
parent 6a4ba9df
......@@ -284,7 +284,7 @@ services:
- [setRequest, ['@?request=']]
router.route_provider:
class: Drupal\Core\Routing\RouteProvider
arguments: ['@database', '@router.builder']
arguments: ['@database', '@router.builder', '@state']
tags:
- { name: event_subscriber }
router.route_preloader:
......@@ -334,7 +334,7 @@ services:
arguments: ['@database']
router.dumper:
class: Drupal\Core\Routing\MatcherDumper
arguments: ['@database']
arguments: ['@database', '@state']
router.builder:
class: Drupal\Core\Routing\RouteBuilder
arguments: ['@router.dumper', '@lock', '@event_dispatcher', '@module_handler', '@controller_resolver', '@state']
......
......@@ -7,6 +7,7 @@
namespace Drupal\Core\Routing;
use Drupal\Core\KeyValueStore\StateInterface;
use Symfony\Component\Routing\RouteCollection;
use Drupal\Core\Database\Connection;
......@@ -30,6 +31,13 @@ class MatcherDumper implements MatcherDumperInterface {
*/
protected $routes;
/**
* The state.
*
* @var \Drupal\Core\KeyValueStore\StateInterface
*/
protected $state;
/**
* The name of the SQL table to which to dump the routes.
*
......@@ -43,11 +51,14 @@ class MatcherDumper implements MatcherDumperInterface {
* @param \Drupal\Core\Database\Connection $connection
* The database connection which will be used to store the route
* information.
* @param \Drupal\Core\KeyValueStore\StateInterface $state
* The state.
* @param string $table
* (optional) The table to store the route info in. Defaults to 'router'.
*/
public function __construct(Connection $connection, $table = 'router') {
public function __construct(Connection $connection, StateInterface $state, $table = 'router') {
$this->connection = $connection;
$this->state = $state;
$this->tableName = $table;
}
......@@ -79,6 +90,7 @@ public function dump(array $options = array()) {
$options += array(
'provider' => '',
);
// If there are no new routes, just delete any previously existing of this
// provider.
if (empty($this->routes) || !count($this->routes)) {
......@@ -88,6 +100,8 @@ public function dump(array $options = array()) {
}
// Convert all of the routes into database records.
else {
// Accumulate the menu masks on top of any we found before.
$masks = array_flip($this->state->get('routing.menu_masks.' . $this->tableName, array()));
$insert = $this->connection->insert($this->tableName)->fields(array(
'name',
'provider',
......@@ -101,6 +115,11 @@ public function dump(array $options = array()) {
foreach ($this->routes as $name => $route) {
$route->setOption('compiler_class', '\Drupal\Core\Routing\RouteCompiler');
$compiled = $route->compile();
// The fit value is a binary number which has 1 at every fixed path
// position and 0 where there is a wildcard. We keep track of all such
// patterns that exist so that we can minimize the the number of path
// patterns we need to check in the RouteProvider.
$masks[$compiled->getFit()] = 1;
$names[] = $name;
$values = array(
'name' => $name,
......@@ -136,6 +155,10 @@ public function dump(array $options = array()) {
watchdog_exception('Routing', $e);
throw $e;
}
// Sort the masks so they are in order of descending fit.
$masks = array_keys($masks);
rsort($masks);
$this->state->set('routing.menu_masks.' . $this->tableName, $masks);
}
// The dumper is reused for multiple providers, so reset the queued routes.
$this->routes = NULL;
......
......@@ -93,7 +93,10 @@ public static function getFit($path) {
// We store the highest index of parts here to save some work in the fit
// calculation loop.
$slashes = $number_parts - 1;
// The fit value is a binary number which has 1 at every fixed path
// position and 0 where there is a wildcard. We keep track of all such
// patterns that exist so that we can minimize the the number of path
// patterns we need to check in the RouteProvider.
$fit = 0;
foreach ($parts as $k => $part) {
if (strpos($part, '{') === FALSE) {
......
......@@ -8,6 +8,7 @@
namespace Drupal\Core\Routing;
use Drupal\Component\Utility\String;
use Drupal\Core\KeyValueStore\StateInterface;
use Symfony\Component\EventDispatcher\EventSubscriberInterface;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\Routing\Exception\RouteNotFoundException;
......@@ -42,6 +43,13 @@ class RouteProvider implements RouteProviderInterface, EventSubscriberInterface
*/
protected $routeBuilder;
/**
* The state.
*
* @var \Drupal\Core\KeyValueStore\StateInterface
*/
protected $state;
/**
* A cache of already-loaded routes, keyed by route name.
*
......@@ -56,12 +64,15 @@ class RouteProvider implements RouteProviderInterface, EventSubscriberInterface
* A database connection object.
* @param \Drupal\Core\Routing\RouteBuilderInterface $route_builder
* The route builder.
* @param \Drupal\Core\KeyValueStore\StateInterface $state
* The state.
* @param string $table
* The table in the database to use for matching.
*/
public function __construct(Connection $connection, RouteBuilderInterface $route_builder, $table = 'router') {
public function __construct(Connection $connection, RouteBuilderInterface $route_builder, StateInterface $state, $table = 'router') {
$this->connection = $connection;
$this->routeBuilder = $route_builder;
$this->state = $state;
$this->tableName = $table;
}
......@@ -115,10 +126,6 @@ public function getRouteCollectionForRequest(Request $request) {
$collection = $this->getRoutesByPath($path);
}
if (!$collection->count()) {
throw new ResourceNotFoundException(String::format("The route for '@path' could not be found", array('@path' => $path)));
}
return $collection;
}
......@@ -202,7 +209,24 @@ public function getCandidateOutlines(array $parts) {
// The highest possible mask is a 1 bit for every part of the path. We will
// check every value down from there to generate a possible outline.
$masks = range($end, 0);
if ($number_parts == 1) {
$masks = array(1);
}
elseif ($number_parts <= 3) {
// Optimization - don't query the state system for short paths. This also
// insulates against the state entry for masks going missing for common
// user-facing paths since we generate all values without checking state.
$masks = range($end, 1);
}
elseif ($number_parts <= 0) {
// No path can match, short-circuit the process.
$masks = array();
}
else {
// Get the actual patterns that exist out of state.
$masks = (array) $this->state->get('routing.menu_masks.' . $this->tableName, array());
}
// Only examine patterns that actually exist as router items (the masks).
foreach ($masks as $i) {
......@@ -260,14 +284,18 @@ protected function getRoutesByPath($path) {
return $value !== NULL && $value !== '';
}));
$collection = new RouteCollection();
$ancestors = $this->getCandidateOutlines($parts);
if (empty($ancestors)) {
return $collection;
}
$routes = $this->connection->query("SELECT name, route FROM {" . $this->connection->escapeTable($this->tableName) . "} WHERE pattern_outline IN (:patterns) ORDER BY fit DESC, name ASC", array(
':patterns' => $ancestors,
))
->fetchAllKeyed();
$collection = new RouteCollection();
foreach ($routes as $name => $route) {
$route = unserialize($route);
if (preg_match($route->compile()->getRegex(), $path, $matches)) {
......
......@@ -2,11 +2,13 @@
/**
* @file
* Definition of Drupal\system\Tests\Routing\UrlMatcherDumperTest.
* Contains \Drupal\system\Tests\Routing\MatcherDumperTest.
*/
namespace Drupal\system\Tests\Routing;
use Drupal\Core\KeyValueStore\KeyValueMemoryFactory;
use Drupal\Core\KeyValueStore\State;
use Symfony\Component\Routing\Route;
use Symfony\Component\Routing\RouteCollection;
......@@ -27,6 +29,13 @@ class MatcherDumperTest extends UnitTestBase {
*/
protected $fixtures;
/**
* The state.
*
* @var \Drupal\Core\KeyValueStore\StateInterface
*/
protected $state;
public static function getInfo() {
return array(
'name' => 'Dumper tests',
......@@ -39,6 +48,7 @@ function __construct($test_id = NULL) {
parent::__construct($test_id);
$this->fixtures = new RoutingFixtures();
$this->state = new State(new KeyValueMemoryFactory());
}
function setUp() {
......@@ -50,7 +60,7 @@ function setUp() {
*/
function testCreate() {
$connection = Database::getConnection();
$dumper= new MatcherDumper($connection);
$dumper= new MatcherDumper($connection, $this->state);
$class_name = 'Drupal\Core\Routing\MatcherDumper';
$this->assertTrue($dumper instanceof $class_name, 'Dumper created successfully');
......@@ -61,7 +71,7 @@ function testCreate() {
*/
function testAddRoutes() {
$connection = Database::getConnection();
$dumper= new MatcherDumper($connection);
$dumper= new MatcherDumper($connection, $this->state);
$route = new Route('test');
$collection = new RouteCollection();
......@@ -82,7 +92,7 @@ function testAddRoutes() {
*/
function testAddAdditionalRoutes() {
$connection = Database::getConnection();
$dumper= new MatcherDumper($connection);
$dumper= new MatcherDumper($connection, $this->state);
$route = new Route('test');
$collection = new RouteCollection();
......@@ -118,7 +128,7 @@ function testAddAdditionalRoutes() {
*/
public function testDump() {
$connection = Database::getConnection();
$dumper= new MatcherDumper($connection, 'test_routes');
$dumper = new MatcherDumper($connection, $this->state, 'test_routes');
$route = new Route('/test/{my}/path');
$route->setOption('compiler_class', 'Drupal\Core\Routing\RouteCompiler');
......@@ -142,12 +152,41 @@ public function testDump() {
$this->assertTrue($loaded_route instanceof Route, 'Route object retrieved successfully.');
}
/**
* Tests the determination of the masks generation.
*/
public function testMenuMasksGeneration() {
$connection = Database::getConnection();
$dumper = new MatcherDumper($connection, $this->state, 'test_routes');
$collection = new RouteCollection();
$collection->add('test_route_1', new Route('/test-length-3/{my}/path'));
$collection->add('test_route_2', new Route('/test-length-3/hello/path'));
$collection->add('test_route_3', new Route('/test-length-5/{my}/path/marvin/magrathea'));
$collection->add('test_route_4', new Route('/test-length-7/{my}/path/marvin/magrathea/earth/ursa-minor'));
$dumper->addRoutes($collection);
$this->fixtures->createTables($connection);
$dumper->dump(array('provider' => 'test'));
// Using binary for readability, we expect a 0 at any wildcard slug. They
// should be ordered from longest to shortest.
$expected = array(
bindec('1011111'),
bindec('10111'),
bindec('111'),
bindec('101'),
);
$this->assertEqual($this->state->get('routing.menu_masks.test_routes'), $expected);
}
/**
* Tests that changing the provider of a route updates the dumped value.
*/
public function testDumpRouteProviderRename() {
$connection = Database::getConnection();
$dumper = new MatcherDumper($connection, 'test_routes');
$dumper = new MatcherDumper($connection, $this->state, 'test_routes');
$this->fixtures->createTables($connection);
$route = new Route('/test');
......
......@@ -812,7 +812,8 @@ function system_schema() {
),
'route' => array(
'description' => 'A serialized Route object',
'type' => 'text',
'type' => 'blob',
'size' => 'big',
),
'number_parts' => array(
'description' => 'Number of parts in this router path.',
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment