Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CVE-2017-18342 status #207

Closed
Jwomers opened this issue Jul 17, 2018 · 10 comments
Closed

CVE-2017-18342 status #207

Jwomers opened this issue Jul 17, 2018 · 10 comments

Comments

@Jwomers
Copy link

Jwomers commented Jul 17, 2018

Hi, I am maintaining the safety database at PyUp, and I want to get clarity on which version of PyYAML fixes CVE-2017-18342? I know 4.1 was released and then retracted from PyPi.

Does 3.13 fix CVE-2017-18342?

Thanks

@Kobold
Copy link

Kobold commented Jul 18, 2018

I haven't read the code, but it seems like 3.13 does not fix the CVE. The CVE was regarding the default-unsafe load method, and the change that fixed it (PR #74) was only merged in 4.1 onward, and has since been reverted from the main branch that 4.2 will be cut from.

Issue #193 has the most clarity on the release plan, but it's unclear what the timeline for having #74 (that is, a fix for the CVE) re-merged is. It might be a while before there's a fix for the CVE.

Thanks again for the work you do with PyUp @Jwomers — I would say this is a tricky situation for a safety database. The CVE is not a new thing, it merely got unsafe defaults that have been present since 2006 flagged. Users of PyYAML need to know that they're using a potentially dangerous package, but at the same time, this isn't shocking to anyone who has some familiarity with PyYAML and its sharp edges.

@ingydotnet
Copy link
Member

@Kobold's assessment is spot on.

3.13 only addresses PyYAML working with Python 3.7.

The next release (almost certainly versioned as 4.2) has high hopes of including a fix for this issue.

@ljvmiranda921
Copy link

Hi, thanks for the pyyaml project. It has been very helpful!
Any updates on this bug? Thank you very much for your time and effort!

devenney added a commit to oneirism/dicebot that referenced this issue Aug 1, 2018
Works around build failures caused by yaml/pyyaml#207 until a safe, stable release lands.
devenney added a commit to oneirism/dicebot that referenced this issue Aug 1, 2018
Works around build failures caused by yaml/pyyaml#207 until a safe, stable release lands.

Signed-off-by: Brendan Devenney <[email protected]>
devenney added a commit to oneirism/dicebot that referenced this issue Aug 1, 2018
Works around build failures caused by yaml/pyyaml#207 until a safe, stable release lands.

Signed-off-by: Brendan Devenney <[email protected]>
@hashar
Copy link

hashar commented Jan 24, 2019

The summary from my analysis on our downstream bug https://phabricator.wikimedia.org/T214560#4905113

pyyaml.load allows execution of code, one would always want to use pyyaml.safe_load instead. The proposal in #74 is to change the semantic of load to make it safe by default and to introduce a new danger_load when one knows code should be executed.The table is:

Method Current PR #74
load unsafe safe
safe_load safe safe
danger_load N/A unsafe

That has been included in 4.1 (which got unreleased), 4.2b1 and 4.2b2. It has been reverted in branches 4.2 and 4.3.

There is a safey-api branch which might be related with commit 3dc3f5f . And I guess reimplement the feature in a different way. The only release that has this change is 4.2b1, it is not in 4.2b1 nor in 4.2/4.3 branches.

My conclusion is the CVE complains that load() is unsafe (since people should almost always use safe_load(). The idea behind #74 is to change load() to be safe and add a new method danger_load() for the case when ones wants to allow code execution.

I think the CVE can be dismissed, but at least it raises awareness that code relying on pyyaml should be audited for usage of load() / dump() and those should be converted to safe_load() / safe_dump.

To make it nicer to users that do not pay attention to the possible code execution when using load(), a major release of pyyaml should probably introduce the breaking change of making load() to be safe which would solve the CVE.

@hashar
Copy link

hashar commented Jan 24, 2019

Also from Debian https://security-tracker.debian.org/tracker/CVE-2017-18342

This is a well-known design deficiency in pyyaml, various CVE IDs have been assigned
to applications misusing the API over the years. The CVE ID was assigned to raise
awareness (and 4.1 now fixes the default behaviour as well)
#74

thinkAmi added a commit to thinkAmi/dj_ringo_tabetter that referenced this issue Feb 14, 2019
GitHubで CVE-2017-18342 のアラートが出ていた。
対応策として以下のissueがあった。

- yaml/pyyaml#193 (comment)
- yaml/pyyaml#207 (comment)

そのため、yaml.safe_load()へと差し替えた
@ingydotnet
Copy link
Member

This issue is being addressed in the upcoming 5.1 release. 5.1b1 is out, and 5.1b3 went out tonight.

The PR that resolves this issue is #257

@FichteFoll
Copy link

I'm not sure if issueing a "warning" is enough to solve this issue immediately. While it's of course the correct step, this is only half of the way to fix the issue eventually.

@The-Compiler
Copy link
Contributor

I'm not sure if issueing a "warning" is enough to solve this issue immediately. While it's of course the correct step, this is only half of the way to fix the issue eventually.

Did you take a look at #257? From there:

We added a new loader class called FullLoader, and we made it the default for
load(). This class is almost as complete for serialization as
UnsafeLoader/Loader, but it avoids arbitrary code execution. We don't expect it
will break any code in the wild.

We still recommend that people choose SafeLoader for untrusted data, but
aribitrary code execution will no longer be possible using yaml.load() with
the default loader (FullLoader). FullLoader will instantiate objects of classes
that you have imported. Since object instantiation runs the class's constructor
code, that may be exploitable.

@FichteFoll
Copy link

My bad, I must have missed that FullLoader is the new default. This is perfectly fine, then. 👍

sverch added a commit to getcloudless/cloudless that referenced this issue Mar 2, 2019
Using the beta version may include the security fix, but it causes
installation issues.  Using safe_yaml is safe across versions according
to yaml/pyyaml#207.
sverch added a commit to getcloudless/cloudless that referenced this issue Mar 2, 2019
Using the beta version may include the security fix, but it causes
installation issues.  Using safe_load is safe across versions according
to yaml/pyyaml#207.
sverch added a commit to getcloudless/cloudless that referenced this issue Mar 2, 2019
Using the beta version may include the security fix, but it causes
installation issues.  Using safe_load is safe across versions according
to yaml/pyyaml#207.
sverch added a commit to getcloudless/cloudless that referenced this issue Mar 2, 2019
Using the beta version may include the security fix, but it causes
installation issues.  Using safe_load is safe across versions according
to yaml/pyyaml#207.
@ingydotnet
Copy link
Member

PyYAML 5.1 has been released.

https://pypi.org/project/PyYAML/5.1/

This release resolves CVE-2017-18342.

For latest details, see: https://github.com/yaml/pyyaml/wiki/PyYAML-yaml.load(input)-Deprecation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants