PREG_BACKTRACK_LIMIT_ERROR in Yaml parser #20411

Closed
RichardBradley opened this Issue Nov 4, 2016 · 5 comments

Projects

None yet

6 participants

@RichardBradley
Contributor
RichardBradley commented Nov 4, 2016 edited

A YAML string with too many backslash escapes can trigger a PREG_BACKTRACK_LIMIT_ERROR error in the Yaml parser.

Steps to reproduce:

Run the following unit test:

use Symfony\Bundle\FrameworkBundle\Test\WebTestCase;
use Symfony\Component\Yaml\Yaml;

class YamlParserBacktrackLimitTest extends WebTestCase
{
    public function testYamlQuotedString() {

        $foo = str_repeat("x\r\n\\\"x\"x", 1000);
        $yamlString = Yaml::dump(["foo" => $foo]);

        $arrayFromYaml = Yaml::parse($yamlString);

        $this->assertEquals($foo, $arrayFromYaml['foo']);
    }
}

Observed behaviour

This test will pass if you reduce the "1000" constant to "100", and fail as-is.

The reported exception is:

Malformed inline YAML string ("x\r\n\\\ ...
at n/a
        in ...\vendor\symfony\symfony\src\Symfony\Component\Yaml\Inline.php line 344

The relevant source is here:

       const REGEX_QUOTED_STRING = '(?:"([^"\\\\]*(?:\\\\.[^"\\\\]*)*)"|\'([^\']*(?:\'\'[^\']*)*)\')';
...
        if (!preg_match('/'.self::REGEX_QUOTED_STRING.'/Au', substr($scalar, $i), $match)) {
            throw new ParseException(sprintf('Malformed inline YAML string: %s.', substr($scalar, $i)));
        }

Expected behaviour

  1. The test should pass, i.e. the YAML should be parseable, since it fits in memory and is well formed.

  2. The exception thrown by Symfony if the preg_match fails due to PREG_BACKTRACK_LIMIT_ERROR should indicate that the op failed due to resource exhaustion and not incorrectly blame invalid YAML.

Suggested fix

I suspect that changing the REGEX_QUOTED_STRING regex to use possessive qualifiers will probably fix bug 1 above without otherwise changing the YAML parser's behaviour.

Fixing bug 2 above needs a call to preg_last_error() after each "preg_match" call in Symfony code, which will be tedious.

@stof
Member
stof commented Nov 4, 2016

using possessive quantifiers should indeed be done.

Regarding your point 2, this would involve distinguishing 0 and false as return value for preg_match (false would indicate a regex error, which should trigger a different exception saying that something was broken in the parser).
But this is quite tedious indeed, so it would be better to fix the root error (avoid needless backtracking)

@javiereguiluz javiereguiluz added the Yaml label Nov 5, 2016
@RichardBradley RichardBradley added a commit to RichardBradley/symfony that referenced this issue Jan 13, 2017
@RichardBradley RichardBradley #20411 fix Yaml parsing for very long quoted strings e464160
@RichardBradley RichardBradley added a commit to RichardBradley/symfony that referenced this issue Jan 13, 2017
@RichardBradley RichardBradley #20411 fix Yaml parsing for very long quoted strings 100a0ce
@RichardBradley RichardBradley added a commit to RichardBradley/symfony that referenced this issue Jan 13, 2017
@RichardBradley RichardBradley #20411 fix Yaml parsing for very long quoted strings 51bca66
@xabbuh xabbuh added a commit that referenced this issue Jan 14, 2017
@xabbuh xabbuh bug #21279 #20411 fix Yaml parsing for very long quoted strings (Rich…
…ardBradley)

This PR was merged into the 2.7 branch.

Discussion
----------

#20411 fix Yaml parsing for very long quoted strings

| Q             | A
| ------------- | ---
| Branch?       | 2.7
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #20411
| License       | MIT
| Doc PR        | no

This fixes #20411, a YAML string with too many backslash escapes can trigger a `PREG_BACKTRACK_LIMIT_ERROR` error in the Yaml parser.

There should be no behavioural change other than the bug fix

I have included a test which fails before this fix and passes after this fix.

Commits
-------

51bca66 #20411 fix Yaml parsing for very long quoted strings
c18c93b
@xabbuh xabbuh closed this Jan 14, 2017
@xabbuh xabbuh added the Bug label Jan 14, 2017
@mahagr
mahagr commented Jan 14, 2017

👍 Thanks for fixing this one, its a big one. It should also improve yaml parsing performance (on affected input) by a great deal.

@RichardBradley
Contributor

You're welcome.

Do you know when I can expect a 3.2 based release which will include this fix?

@mahagr
mahagr commented Jan 14, 2017

I'm waiting for the release as much as you do :)

@xabbuh
Member
xabbuh commented Jan 14, 2017

Since the last patch releases have just been released two days ago you will have to wait for about four weeks for the next releases.

@fabpot fabpot added a commit that referenced this issue Jan 21, 2017
@fabpot fabpot Merge branch '2.7' into 2.8
* 2.7:
  fixed typo
  Revert "fixed typo"
  fixed typo
  fixed CS
  Avoid setting request attributes from signature arguments in AnnotationClassLoader
  [DependencyInjection] Add some missing typehints in YamlFileLoader
  [DependencyInjection] minor: Fix a DocBlock
  [HttpKernel] Give higher priority to adding request formats
  [FrameworkBundle] Fix third level headers for MarkdownDescriptor
  [TwigBundle] do not lose already set method calls
  #20411 fix Yaml parsing for very long quoted strings
  CS: apply is_null
  DX: remove invalid inheritdoc
  bumped Symfony version to 2.7.24
  updated VERSION for 2.7.23
  update CONTRIBUTORS for 2.7.23
  updated CHANGELOG for 2.7.23
  [FrameworkBundle] Skip test if xdebug.file_link_format is defined.
a784d5c
@fabpot fabpot added a commit that referenced this issue Jan 21, 2017
@fabpot fabpot Merge branch '2.8' into 3.1
* 2.8: (26 commits)
  fixed CS
  fixed CS
  fixed CS fixer config
  fixed typo
  Revert "fixed typo"
  fixed typo
  fixed CS
  Avoid setting request attributes from signature arguments in AnnotationClassLoader
  [DependencyInjection] Add some missing typehints in YamlFileLoader
  [DependencyInjection] minor: Fix a DocBlock
  [HttpKernel] Give higher priority to adding request formats
  [PropertyInfo] Don't try to access a property thru a static method
  [PropertyInfo] Exclude static methods form properties guessing
  [FrameworkBundle] Fix third level headers for MarkdownDescriptor
  [TwigBundle] do not lose already set method calls
  #20411 fix Yaml parsing for very long quoted strings
  CS: apply is_null
  DX: remove invalid inheritdoc
  bumped Symfony version to 2.8.17
  updated VERSION for 2.8.16
  ...
20bdaa6
@fabpot fabpot added a commit that referenced this issue Jan 21, 2017
@fabpot fabpot Merge branch '3.1' into 3.2
* 3.1: (31 commits)
  fixed CS
  fixed CS
  fixed CS fixer config
  fixed typo
  Revert "fixed typo"
  fixed typo
  fixed CS
  Avoid setting request attributes from signature arguments in AnnotationClassLoader
  [DependencyInjection] Add some missing typehints in YamlFileLoader
  [DependencyInjection] minor: Fix a DocBlock
  [HttpKernel] Give higher priority to adding request formats
  [PropertyInfo] Don't try to access a property thru a static method
  [PropertyInfo] Exclude static methods form properties guessing
  [FrameworkBundle] Fix third level headers for MarkdownDescriptor
  [Ldap] Using Ldap stored username instead of form submitted one
  [Ldap] load users with the good username case
  [DoctrineBridge] Fixed invalid unique value as composite key
  [Doctrine Bridge] fix UniqueEntityValidator for composite object primary keys
  [TwigBundle] do not lose already set method calls
  #20411 fix Yaml parsing for very long quoted strings
  ...
ebdbd96
@RichardBradley RichardBradley added a commit to RichardBradley/symfony that referenced this issue Feb 3, 2017
@RichardBradley RichardBradley #20411 fix Yaml parsing for very long quoted strings 6e001c8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment