Unlimited allowed length of path in menu router allows DoS attack on Drupal 8

reduce the length of the IN query. It seems that the current code is a carry-over when we allowed matching on a path that had unmatched trailing path parts. We can reduce this from a exponential number of parts to a linear number.

Not really. Let's have the following examples of routes:

• '/some/path/{value}' with {value} being optional
• '/some/path/{foo}' with {foo} not being optional
• '/some/path/here'

In order to find the first one we don't store the {value} as placeholder, so '/some/path' also matches. One strategy certainly could be to
store both the optional and not optional elements in the DB and limit the lenght of the ancestors, though this seems to be worse how we do it at the moment.

.

1. +++ b/core/modules/system/lib/Drupal/system/Tests/Routing/MatcherDumperTest.php
   @@ -7,6 +7,8 @@
   @@ -27,6 +29,13 @@ class MatcherDumperTest extends UnitTestBase {
   

   This test case should also check the state variable routing.menu_masks after the dump() invocation to make sure it exists and is populated as expected.

2. +++ b/core/modules/system/lib/Drupal/system/Tests/Routing/MatcherDumperTest.php
   @@ -143,11 +153,31 @@ public function testDump() {
      /**
   +   * Tests the determination of the max parts length.
   +   */
   +  public function testMaxPathPartsLength() {
   

   So that does not really belong into this patch, because we are not doing the max parts thing yet.

Looks very good!

+++ b/core/lib/Drupal/Core/Routing/RouteProvider.php
@@ -202,7 +209,27 @@ public function getCandidateOutlines(array $parts) {

The only thing missing is now a security test for this function: pass it a higher number of parts (15?) and check that it does not return a gazillion candidate outlines. That way we protect ourselves from future DoS regressions.

15: routing-2224777-15.patch queued for re-testing.

15: routing-2224777-15.patch queued for re-testing.

Discussed this extensively at the dev days sprint, and this looks good now. It covers the DoS security issue and brings back a nice performance improvement from D7.

+1 for the current way. Further optimizations can come in.

+++ b/core/lib/Drupal/Core/Routing/RouteProvider.php
@@ -202,7 +209,25 @@ public function getCandidateOutlines(array $parts) {
+    elseif ($number_parts <= 3) {
+      // Optimization - don't query the state system for short paths. This also
+      // protects against the masks going missing for common user-facing paths.
+      $masks = range($end, 1);
+    }

This is nice, I was concerned about the extra state get on every page, then saw this :)

Giving this a bit more time before commit, but no complaints from me.

1. +++ b/core/lib/Drupal/Core/Routing/RouteProvider.php
   @@ -202,7 +209,25 @@ public function getCandidateOutlines(array $parts) {
   +      // Optimization - don't query the state system for short paths. This also
   +      // protects against the masks going missing for common user-facing paths.
   

   How does it protect against that?

2. +++ b/core/lib/Drupal/Core/Routing/RouteProvider.php
   @@ -202,7 +209,25 @@ public function getCandidateOutlines(array $parts) {
   +    else {
   +      if ($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());
   +      }
   +    }
   

   This shouldn't be an embedded if. Just another elseif.

3. +++ b/core/modules/system/lib/Drupal/system/Tests/Routing/MatcherDumperTest.php
   @@ -143,11 +153,40 @@ public function testDump() {
   +      bindec('1011111'),
   +      bindec('10111'),
   +      bindec('111'),
   +      bindec('101'),
   

   These can be integer literals in 5.4 thanks to the new syntax: http://php.net/manual/en/migration54.new-features.php

   0b10111 (etc.)

Going just by the patch, I have NFI what this code is doing or why. Without the issue summary or the explanation I just got in IRC, this is all just so much extra nonsense. That's why I removed it from the routing system when porting from the old one to the new one; I couldn't figure out what it was actually doing for functionality, I didn't need it, so I dropped it.

Before this goes in it MUST have better docs to explain what a mask is and WHY we're caching them and doing all of this stuff. As is, from the code I couldn't tell you which means it's going to get optimized out again by some well-meaning developer who doesn't know that it's there for DDOS reasons. There is absolutely nothing in the code or comments right now that even hints at DDOS as a reason.

No matter what we do here, it can't get worse than it is now.
@crell
Just in case you want to reproduce that DDOS attack goto /node/1/foo/bar/baz/foo/bar/baz/foo/bar/baz/foo/bar/baz/foo/bar/baz/foo/bar/baz/foo/bar/baz/

Committed/pushed to 8.x, thanks!