Jump to:

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.

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

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

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.

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

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.

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?

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

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.