Problem/Motivation

The Symfony Yaml class has static methods to parse and dump data. However, the parse method tries to guess if the string is a file name and load it if there is no "\n" character. Thus, very simple valid YAML content be used as a file name. Further, since Symfony can't even get a YAML parser right even if you skip this insecure wrapper and call the parser directly it still can't parse the simplest files, those without any line breaks.

Note earlier problem of supporting object by default is now resolved upstream by a change to the defaults.

Proposed resolution

Skip the broken parts. Just used the underlying classes that do the work instead of the static wrapper methods (besically we're just reimplementing those and skipping the check to guess if it's a file name). And add a line break.

Remaining tasks

In a followup, write a standard compliant YAML parser. This issue merely skips the most broken parts of the Symfony YAML parser which does not implement the standard.

User interface changes

none

API changes

none

Original report by @Heine

Atm, there are no warning cones surrounding the YAML parser, so I have to assume the following weaknesses will lead to a bloodbath during D8's lifetime, unless corrected.

Problems:

  1. Yaml::parse() accepts a string that either contains a filename, or YAML. A filename may end up being parsed as YAML, or a YAML string may end up being used as a filename.
  2. Yaml::enablePhpParsing() enables PHP parsing for all subsequent uses of Yaml::parse().
  3. Yaml::parse() will instantiate objects, there's no way to stop this behaviour.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Heine’s picture

Symfony appears to have a newer YAML parser that at least stops object support from being the default.

https://github.com/symfony/Yaml/blob/master/Yaml.php

pwolanin’s picture

Priority: Normal » Critical

So this is really a security hole in the upstream Symfony component?

chx’s picture

*facepalm* in light of the recent Rails fiasco I did read the Parser class hunting for secholes but I have not checked Yaml.php. We do not use Yaml::Parse AFAIK. If we exclude tests, then git grep only finds:

core/vendor/symfony/dependency-injection/Symfony/Component/DependencyInjection/Loader/YamlFileLoader.php:        return $this->validate(Yaml::parse($file), $file);
core/vendor/symfony/routing/Symfony/Component/Routing/Loader/YamlFileLoader.php:        $config = Yaml::parse($path);
core/vendor/symfony/validator/Symfony/Component/Validator/Mapping/Loader/YamlFileLoader.php:            $this->classes = Yaml::parse($this->file);

I can see two solutions:

  1. Symfony 2.3 is scheduled for this year which will remove this capacity. We wait for that.
  2. We submit an upstream patch to remove these calls and remove Yaml::parse altogether. I have no idea whether that'd be accepted etc.

All my worst nightmares about how Drupal and Symfony security will work are seemingly coming true here. I fervently hope we can fix this mess :/

gdd’s picture

There were two issues patched in Symfony a couple of weeks ago related to the YAML parser

http://symfony.com/blog/security-release-symfony-2-0-22-and-2-1-7-released

Assumedly this came on the heels of the recent issues in the Rails community.

If there are security issues in Symfony, we should not be patching these components locally, or even making issues about them in the core queue, since this amounts to public disclosure. Instead we should be working with the Symfony security team as outlined at

http://symfony.com/doc/master/contributing/code/security.html

I feel this issue should be unpublished.

EDIT: If it turns out we are in fact discussing upstream Symfony security issues that is (since it seems I may be somewhat misunderstanding the above)

Heine’s picture

Yep we are. And it turns out a Yaml Parser update was released Jan 17th, so this points at a gap in our "taking updates" from upstream process.

Anonymous’s picture

FYI, There was some discussion of using YAML::parse() in #1897612: Add YAML support to serialization module. I had already raised concerns about the security and have now closed the issue based on the current discussions.

klausi’s picture

Issue tags: +Security improvements

@Heine: we do not synchronize our Symfony code base regularly and we are still in the development cycle of Drupal 8, which does not guarantee any security updates until a stable release has been made. See http://drupal.org/security-advisory-policy

Of course we will follow-up much more closely with Symfony once 8.0 is released, i.e. the Drupal security team will be responsible to communicate with Symfony and will make sure that security fixes land in Drupal in a timely fashion. I think there are already efforts on the way to establish a private group of representatives from all the projects that depend on Symfony to coordinate security releases.

chx’s picture

To clarify: my worst nightmare is that instead of filing a patch here which nukes Yaml::parse and converts those three usages we need to haggle with upstream to accept some solution. I am not interested in that and I am unfollowing this issue.

plach’s picture

@chx proposed two solutions in #3: is there anything else on the table?

If not it seems both of them involve as sole action on the Drupal side including an updated version of Symfony (or just the Yaml component). Given that, I'd say this should become a critical task. Perhaps a critical metaissue to track security updates from external projects and ensure that when we ship all of them are included in our codebase.

Or we could demote this to a normal task (bug?) and link it from a new critical metaissue.

Damien Tournoud’s picture

Priority: Critical » Normal
Status: Active » Postponed

So, nothing in Drupal uses Yaml::parse(), so there is no security issue for point (1) and (2).

Point (3) is valid, but only a problem if we parse untrusted user input, which we do not (yet). This issue will solve itself when we update the Symfony component, so just postponing this until we do.

Anonymous’s picture

One of the security issues discussed yesterday which did not make it in to this issue is that Symfony will not remove the unsafe Yaml::parse() until version 3. lsmith from Symfony felt this wasn't a problem since the developer of the client code should know what they are doing... which probably works better as an assumption in their context than it does in ours.

As EclipseGC points out, if we want to ensure that this vulnerability isn't part of the Drupal ecosystem, we'd have to watch contrib to ensure people aren't using Yaml::parse() in an unsafe way.

klausi suggested that we might create a fork until version 3, and remove the unsafe function in the fork so that contrib developers can't use it unknowingly. lsmith seemed to think this was a reasonable approach and suggested a way to do it in composer.

Damien Tournoud’s picture

Nothing in Drupal uses Yaml::parse(). There is no security issue and there is no point in looking at this further. There are *many* functions accesible in a Drupal installation that you can use if you want to shoot yourself in the foot. No need to focus on this one in particular.

Also, just to clarify, Yaml::parse() is just a convenience procedural wrapper, that you do not need to and shouldn't use anyway.

dcrocks’s picture

I found 1 occurrence of Yaml::parse() in core/modules/config/lib/Drupal/config/Tests/Storage/FileStorageTest.php . There are 4 occurrences in Symphony's YamlFileLoader.php, 1 in Yaml.php, and 2 in Symphony's Yaml docs. YamlFileLoader is referenced in numerous Symphony components. But for all I know, that doesn't mean that it is used(wrongly?).
This is the 8.x-dev dated 2/5/13, searched with TextWrangler. Really neat.

Heine’s picture

Status: Postponed » Closed (won't fix)

Closing per #12.

chx’s picture

Priority: Normal » Critical
Status: Closed (won't fix) » Active

We do not ship with landmines lying around, nope. I do not care how but this must be fixed.

gdd’s picture

Priority: Critical » Normal
Status: Active » Closed (won't fix)

I agree with Damz that this is a non-issue. Once we update to current Symfony this will be no worse than a dozen other ways you can screw yourself with core code. In IRC this evening msonnabaum also suggested that we should default to using the PECL Yaml extension if it is available, which I fully agree with, so I've filed #1920902: Add a Drupal Yaml wrapper so we can default to PECL Yaml component if it is available to address that.

pwolanin’s picture

Status: Closed (won't fix) » Active

re-opening - I think serialization is a significant potential security in our use of Symfony. Why shouldn't we at least override the class and make that method just throw an exception or something like, or even fork it if that's the ony bullet-proof approach?

cweagans’s picture

Which method? #1920902 would be a good place to do that.

bzitzow’s picture

Hi. I just came here looking for issues to contribute to.

I searched the tag: "D8 stable release blocker" and this came up as active.

After reading through the comments, I'm not clear if this is active or not. If it is active, I can't discern the actionable task.

As far as I can tell, the proposed solutions are:

  1. Update the Symfony component
  2. Wrap the Symfony component in a Drupal service and override the unsafe method, throwing an exception.
  3. Write a drupal interface and service that implements either the Symfony YAML component or the PECL YAML component.

I'm new to Drupal contributing so if you could just help me understand the process a little better I would appreciate that greatly! Thanks and cheers!

YesCT’s picture

Issue summary: View changes
Issue tags: -D8 stable release blocker +Security

if this is blocking d8 release, it should be critical.

chx’s picture

Title: Security issue: YAML Parser Design » Very simple config files can't be read
Issue tags: -Security improvements, -Security

The security issue is resolved since now the object support defaults to false. The file-as-input as marked as deprecated and enablePhpParsing is just gone.

But if you have a config file without a line break in it, like foo: bar then that'll be interpreted as a filename...

Suggested fix: add a \n in Drupal\Component\Serialization before calling parse (yes, seriously, that's a fix).

pwolanin’s picture

Status: Active » Needs review
FileSize
708 bytes

ok, simple fix (avoids the is_file call).

Status: Needs review » Needs work

The last submitted patch, 22: 1907170-22.patch, failed testing.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
715 bytes
673 bytes

Nice - the parser fails on:

"{}\n\n"
chx’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests
jhedstrom’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
1.43 KB
1.43 KB
2.13 KB

Here's a start at unit tests.

Damien Tournoud’s picture

That sounds like an upstream bug, and upstream bugs ought to be fixed upstream...

return Symfony::parse(rtrim($raw) . "\n", TRUE);

You probably want rtrim($raw, "\n"), or else you are at the minimum potentially "fixing" broken files.

The last submitted patch, 26: yaml-simple-1907170-TEST-ONLY.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 26: yaml-simple-1907170-26.patch, failed testing.

The last submitted patch, 26: yaml-simple-1907170-TEST-ONLY.patch, failed testing.

chx’s picture

if (strpos($input, "\n") === false && is_file($input)) {

writing a test for this bug is untrivial -- you need to have an input file that exists. Maybe chdir(sys_get_temp_dir()) write out a file called foo: bar test and remove it.

Damien Tournoud’s picture

So this is the same thing identified by Heine a long time ago?

Can't we start using a function that doesn't do this magic "the parameter is either a document or filename" dance?

pwolanin’s picture

Status: Needs work » Needs review
FileSize
1.32 KB

Yes, really an easier solution is to not use the Symfony static wrapper at all but just the underlying classes it instantiates.

chx’s picture

Status: Needs review » Reviewed & tested by the community

That's great. (And I must note that, again and again, that we should fork Symfony so that we can fix it as we want, because setIndentation should return $this but it takes such a huge effort of upstream PR; downstream upgrade of Symfony that single line simply can't be added.)

jibran’s picture

I don't think patch and IS is in sync.

chx’s picture

Issue summary: View changes
pwolanin’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update
chx’s picture

Issue summary: View changes
webchick’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

Let's add some tests.

pwolanin’s picture

@webchick - what tests could we add. we are basically just unwrapping 2 lines of code

chx’s picture

Issue tags: -Needs tests

Indeed. Symfony code is tested or if not it's the role of this issue to write those.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
2.92 KB
769 bytes

Ok, webchick wins - since I forgot to add the "\n" and hence the underlying Symfony code still failed to parse it (even though the possible security hole was removed).

Adding the test from #26 also. We already have that, so I should have included it and it verifies we can parse a very simple YAML string with no trailing newline.

chx’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

Niiice. Also, webchick always wins :)

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
Related issues: +#1920902: Add a Drupal Yaml wrapper so we can default to PECL Yaml component if it is available
FileSize
1.57 KB
3.16 KB

I don't think that fixing the double new line thing is worth it for an empty array. We should file an upstream PR to fix. What is relevant here is that additional YAML parsers should conform to the same behaviour - see #1920902: Add a Drupal Yaml wrapper so we can default to PECL Yaml component if it is available

alexpott’s picture

I've had a look at making this change in Symfony's YAML parser and it breaks some of their test expectations - so it's definitely not ok to make to Drupal. For example, search their YAML tests for 'Literal block chomping clip without trailing newline' - if you put this fix into Parser::cleanup() then that breaks.

alexpott’s picture

pwolanin’s picture

Status: Needs review » Reviewed & tested by the community

ok, I don't feel strongly about it. Avoiding trying to read a file is the main bug to fix I think.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Okie doke, committed and pushed to 8.0.x. Thanks for the upstream fix, Alex!

  • webchick committed d2412ba on 8.0.x
    Issue #1907170 by pwolanin, jhedstrom, alexpott, Heine: Very simple...
YesCT’s picture

Issue tags: +Upstream bugfix

adding tag so we can find #47

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

apaderno’s picture

Issue tags: -Upstream bugfix +Needs upstream bugfix