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.

Issue 3 is a problem when usersupplied YAML is parsed; The instantiated object contains user-supplied values and the class destructor will be called once the object goes out of scope. See http://heine.familiedeelstra.com/security/unserialize

Proposal:

  • Split file parsing from parse() into eg parseFile()
  • Make enablePHPparsing a flag, or create an unsafe parser class.
  • Do not support "!!php/object" syntax, unless specifically asked for (flag or unsafe parser class).

Comments

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

Priority:Normal» Critical

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

*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 :/

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)

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.

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

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.

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.

@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.

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.

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.

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.

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.

Status:Postponed» Closed (won't fix)

Closing per #12.

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.

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.

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?

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

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!