Code injection via preg_replace()

So what we should do as part of that issue is to write a test. The bufix will be done automatically as part of #2508231: Raise minimum required version of PHP to 5.5.9 if I understand it correctly?

Well yeah I totally think we should validate our regular expression here ... well ideally we would not pas it along, given what happens if you can execute a really slow regular
expression. Maybe an idea, could we hash them and also sent a token along, so we know, its always coming from Drupal itself?

So would it be safe enough to just validate on of the following characters: [a-zA-Z0-9\[\]\(\)\.\+\*\\]?

Here's a simple start on a patch.

Seems like the idea is to replace multi-lingual patterns? I don't know - this whole methods smells bad.

@pwolanin
I think the most important part of this issue is not to fix it but to rather write the tests given that it will be fixed in PHP 5.5 anyway.

@dawhener - yes, agreed, especially since it's not at all obvious what the point of this method is.

Wait a minute, if this is fixed in PHP 5.5 and we raising to PHP 5.5 what do we want to test?? It's not our duty to test the PHP engine.

If we get PHP warnings from user input, we should try to prevent those too, but that's just a major bug and not release blocking at all. The test could definitely check for those.

Once we require PHP 5.5, I think I'd probably downgrade this to major, but we should leave it critical until that's actually happened.

Wait a minute, if this is fixed in PHP 5.5 and we raising to PHP 5.5 what do we want to test?? It's not our duty to test the PHP engine.

Well, tests are also an explicit documentation of what is going on or should. Whether under the hood the problem still exists or not, is kinda an implementation detail :)

@catch, @alexpott, @effulgentsia, and I discussed this issue today. This issue becomes major once #2508231: Raise minimum required version of PHP to 5.5.9 is committed, but currently that issue needs to be postponed pending testbot support. Since this is a nasty security vulnerability for current D8 sites, we propose committing the current patch as a hotfix pending the necessary review, without waiting for test coverage. Then we'll downgrade the issue to major for test coverage and any needed improvement.

I've reviewed the issue and proposed fix carefully again and using preg_quote solves the problem.

HOWEVER:

I can't RTBC it, because of:

modules/image/src/PathProcessor/PathProcessorImageStyles.php: $rest = preg_replace('|^' . $path_prefix . '|', '', $path);
modules/search/search.module: $text = trim(preg_replace('/' . $boundary . '(?:' . implode('|', $keys) . ')' . $boundary . '/iu', '\0', ' ' . $text . ' '));
modules/system/src/MachineNameController.php: $transliterated = preg_replace('@' . $replace_pattern . '@', $replace, $transliterated);
lib/Drupal/Core/Block/BlockBase.php: $transliterated = preg_replace('@' . $replace_pattern . '@', '', $transliterated);

using the exact same pattern. (audited via: grep preg_replace -r modules/ lib/ | grep --color '\. \$')

At least BlockBase looks really really suspicious.

I suggest to do a more extensive search - but there are a lot of preg_replace in core.

In BlockBase, on the previous line it's just $replace_pattern = '[^a-z0-9_.]+';, not sure why it even uses a temporary variable.

Though it does have this comment:

    // @todo This is basically the same as what is done in
    //   \Drupal\system\MachineNameController::transliterate(), so it might make
    //   sense to provide a common service for the two.

modules/image/src/PathProcessor/PathProcessorImageStyles.php: $rest = preg_replace('|^' . $path_prefix . '|', '', $path);

Well to be clear, this requires access to "administer site configuration" which is registered ...

$directory_path = file_stream_wrapper_get_instance_by_scheme('public')->getDirectoryPath();
    if (strpos($path, $directory_path . '/styles/') === 0) {
      $path_prefix = $directory_path . '/styles/';
    }
    elseif (strpos($path, 'system/files/styles/') === 0) {
      $path_prefix = 'system/files/styles/';
    }

lib/Drupal/Core/Block/BlockBase.php: $transliterated = preg_replace('@' . $replace_pattern . '@', '', $transliterated);
Agreed, we control the replace_pattern entirely.

modules/search/search.module: $text = trim(preg_replace('/' . $boundary . '(?:' . implode('|', $keys) . ')' . $boundary . '/iu', '\0', ' ' . $text . ' '));

Note: $keys are preg_quoted(), see

      // Quote slashes so they can be used in regular expressions.
      $temp_keys[] = preg_quote($key, '/');
    }
  }
  // Several keywords could have simplified down to the same thing, so pick
  // out the unique ones.
  $keys = array_unique($temp_keys);

I think we still should set a good example in Core.

Someone might just grep for code in core or see it accidentally while looking at the API docs and then "learn" from it ...

Well fair ...

#19 is missing #7 unfortunately ...

@Fabianx,

I have combine #7 and #19.

#21 Uhm, no we wanted to change MachineNameController.php, too.

oops... I did not notice that it was a different file. Here is an updated version of the patch base on #23 suggestion.

per xjm in #13, let's create a follow-up for test coverage and resolve the bug now.

Patch looks like a reasonable combination of the 2 preg_quote calls and cleanup in BlockBase

I think this is far given that its a non issue in 5.5 AND the testbot probably can't test this either because they are running in a higher PHP version.

RTBC + 1, Looks good to me.

Doesn't the page on php.net say that the /e modifier is deprecated, not removed?
http://php.net/manual/en/function.preg-replace.php

I don't understand how bumping PHP version solves anything?

@Chi
Thanks for the clarification.

Committed d433aec and pushed to 8.0.x. Thanks!

As per #13, let's open a followup for adding the test coverage.

Opened #2515736: Add test coverage for "Code injection via preg_replace()".

removing tag since the follow up was opened.

This change seems to create some trouble with machine name inputs. See #2525870: Regression: machine name inputs no longer work properly after #2508735