Security fixe reported by github

[paulocoutinhox]

Hi,

I received this warning message:

[paulocoutinhox]

I think that it was fixed when use 4.2b2.

Thanks.

[lfaraone]

Hi! This means you need to update your project to the newer, fixed version of pyyaml.

[joekohlsdorf]

So what is the security issue? I don't see anything in the changelog and there are only beta releases of 4.1.
Maybe the fix should be backported to the 3.x branch.

[RealOrangeOne]

There's a writeup in #193 about why the issue has come to light.

TL;DR:

Prior to #74, yaml.load could run arbitrary python when parsing yaml, by design. #74 changed this so that by default, load didnt run python, and instead was an alias to safe_load. In this release, this was reverted, which means it's now a known security issue, hence the release of a security notice (CVE-2017-18342).

How to fix:

If you don't want people running arbitrary python (you probably don't), then change uses of yaml.load to yaml.safe_load.

#193 discusses the idea that an API change should be made to make pyyaml safe by default, which will be a new major version.

[joekohlsdorf]

I think you should work with the Github security team to pull this security notice as long as there is no released version of a fix.

[RealOrangeOne]

@joekohlsdorf but there is a security issue? It's not so much that there's no fix, it's just you use the library in a different way. That's still a fix

[joekohlsdorf]

Except that I have no way of knowing which code path external libraries in my project take and no direct way of fixing it except monkey-patching.

[RealOrangeOne]

Then it's not your job to fix it, it's the job of your dependencies. Then when they release a new version, you update to that. I agree GitHub notifications are kinda annoying like this, but it's the right way to do it. Otherwise you end up not being told of issues in your software, which you may need to be aware of.

[umeboshi2]

I spent some time last year in correspondence with github explaining that reporting security issues that were actually false positives limits the effectiveness of this new service. It seems to be the original intent of the authors to provide the .safe_load when the developer knows they will be dealing with remote or arbitrary yaml files. In fact, projects like ansible have been using the .safe_load for a very long time, since the developers know that the yaml configs people use would be potentially coming from many sources. I still think that when a project developer knows that they will be handling potentially dubious input, they should be actively searching for input validation features in their solution. This issue is really just an informative advisory, unless the user of the library expects it to perform input validation without actually checking for this feature before they use it. The lxdock/lxdock#160 issue above really helps describe the usefulness of the alert, and a claim of the alert being spurious is made here marshmallow-code/apispec#278. I had also been notified that bootstrap had a vulnerability where the recommendation was to upgrade to the latest bootstrap4, yet the existence of the vulnerability was not verified in bootstrap-3.3.7, so those repos also received spurious alerts. It seems that github will recommend upgrading to a version that would break functionality before actually making a determination that a vulnerability even exists in the used version. It seems that github eventually removed the alerts for bootstrap3 only after the security provider narrowed the affected versions to > 4.0.

[alexpirine]

I feel it's pretty normal Github notifies with an alert about these kind of issues.

I don't see how developers are supposed to know that yaml.load is unsafe. Intuitively it should work in the same way as json.loads does, so I guess a lot of developers might completely miss the security issue.

[RealOrangeOne]

Simple, the manual!

Warning: It is not safe to call yaml.load with any data received from an untrusted source! yaml.load is as powerful as pickle.load and so may call any Python function. Check the yaml.safe_load function though.

https://pyyaml.org/wiki/PyYAMLDocumentation

[alexpirine]

Sure, if you go beyong data = load(stream, Loader=Loader) and I didn't…

And if you have time to read comments on stackoverflow…

Anyway, excellent news that this is safe by default now. This is how it should have been!

[stavmeir]

Is there an expected release date to a stable 4.2 version?
The latest pre-release version, 4.2b4, was released half a year ago and a stable version would stop the GitHub security notifications and save people from refactoring every yaml.load to yaml.safe_load without using a pre-release versions.

[mar10]

(sorry if I miss something here, since I did not read all linked resources).
If yaml.load() used to allow to execute arbitrary Python code, and we agree that this is unsafe, should we

• Leave yaml.safe_load() as is
• Make yaml.load() an alias for yaml.safe_load()
• have another method yaml.load_eval(), .load_unsave(), or similar that implements the previous behavior
• Implement this with a major version bump 5.x since this is a breaking change according to semver

?
Since upgrading a major version is expected to be breaking, this would be clearer, I guess - at least for folks that follow semantic versioning.

[RealOrangeOne]

@mar10 your suggestion is exactly what's being reversed. See #193. I believe the longer-term goal is to do something similar to that, making python evaluation opt-in for the simpler method rather than opt-out

[ssbarnea]

On this one I support GitHub decision. Compare a library with a car and imagine that the car does not have a seatbelt sensor enabled by default, or guns without safety locks.

This is what unsafe defaults mean. Anycone can bypass them but it matters them to be defaults and that bypassing informs the user about risks ("unsafe" in name being a very good example).

[kislyuk]

I appreciate the work by the maintainers of this package (@ingydotnet, @sigmavirus24, @nitzmahone, @xitology) but I think the handling of this issue and CVE leaves a lot of room for improvement.

The changelog and the PyPI releases are not consistent with the information in the CVE. The latest stable release on PyPI is version 3.13. The changelog and the CVE both reference version 4.1, which does not exist in PyPI. The GitHub notice mentions version 4.2b1, which is tagged as a pre-release version in PyPI. The latest pre-release on PyPI is 4.2b4. None of these versions are in line with SemVer.

There is no clear explanation of the issue or the remedy in the CVE, the GitHub notice, or the repo documentation. With the pre-releases, there is no stated release roadmap or versioning strategy. This situation is creating a bit of confusion regarding a package that is fairly popular and depended upon.

Is the project in need of maintainers? I volunteer to help maintain it if help is necessary.

[ingydotnet]

@kislyuk, PyYAML has a group of core maintainers. Work on the next release has begun by the maintainers.

If you want to help with the work, then you are welcome to join us in #pyyaml on irc.freenode.net.

[ingydotnet]

Closing this. See #193 (comment)

[IvanAnishchuk]

@kislyuk et al. > None of these versions are in line with SemVer.

Well, SemVer is not the only way to version stuff, people used version long before SemVer was even a thought and it's really not the one and only good thing and I don't think I would ever recommend enforcing it verbatim for anything published on pypi (no notion of post-releases or development releases, pre-releases are not well-defined). As you can see, pyyaml here uses two numbered version components, not three, so expectation for it to follow SemVer spec is a little bit assuming too much.

Actually, for this particular project I don't see much good in making the version numbers longer (there are not many new features added, releases happen not that often, api rarely change at all). Two numbers plus pre-release component seems perfectly fine to me. This change with load is backwards-incompatible if I understand correctly so the major version should be bumped from 3 to 4. All seems fine to me so far.

A different question is what would be the general recommendation for people who don't use pyyaml directly and only require it for some other libs? Most of the stuff uses safe_load or equivalents already. There might be some way to check whether it's actually so for the stuff I use. Simple grepping in virtualenv, perhaps. If there's no third-party lib that uses unsafe load() I'd probably just wait for the release. Otherwise, is it safe to upgrade to a beta in my production environment? Are the incompatible changes incompatible enough to break anything in unexpected places? A lot of unanswered questions that a final release of 4.2 would probably solve.