Skip to content

Update PyYAML (CVE-2017-18342) #718

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

Closed
wants to merge 1 commit into from

Conversation

ramnes
Copy link
Contributor

@ramnes ramnes commented Jan 8, 2019

No description provided.

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ramnes
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: lavalamp

If they are not already assigned, you can assign the PR to them by writing /assign @lavalamp in a comment when ready.

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jan 8, 2019
@micw523
Copy link
Contributor

micw523 commented Jan 8, 2019

We should not request a beta version for a production repo like this. Also, version 4.1 already fixed the problem.

Copy link
Contributor

@micw523 micw523 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment

@@ -3,7 +3,7 @@ six>=1.9.0 # MIT
python-dateutil>=2.5.3 # BSD
setuptools>=21.0.0 # PSF/ZPL
urllib3>=1.23 # MIT
pyyaml>=3.12 # MIT
pyyaml>=4.2b1 # MIT
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
pyyaml>=4.2b1 # MIT
pyyaml>=4.1 # MIT

@ramnes
Copy link
Contributor Author

ramnes commented Jan 8, 2019

@micw523 PyYAML 4.1 doesn't exist on PyPI, they published it and then removed it. No version under 4.2b1 fixes this CVE. If we don't want to take the risk to upgrade to one of the beta versions, another solution is to replace all the yaml.load with yaml.safe_load. But this will have to be reverted when upgrading to 4.2.

Also, I'm realizing that since it's a beta, the version number must be pinned if we want the dependency to be pulled, and not marked as >=. I'm also realizing that a 4.2b4 exists. So I'd be in favor of doing pyyaml==4.2b4. But you tell me!

@micw523
Copy link
Contributor

micw523 commented Jan 8, 2019

@ramnes
Thanks! I just realized that PyYaml 4.1 is indeed pulled from PyPI. However, kubernetes-client/python-base#111 just got merged. The yaml load has been replaced by safe_load in the python-base repo. Now the old versions of PyYAML should be fine as well.

Also, in new versions of PyYAML, calling safe_load has not been depreciated; instead it has been set as an equivalent fucntion of load. See https://github.com/yaml/pyyaml/blob/master/lib/yaml/__init__.py#L77. Therefore we don't need to revert the code if someone is using PyYAML 4.2 or higher when it is released.

Therefore, I think we can safely close the PR - what do you think?

@ramnes
Copy link
Contributor Author

ramnes commented Jan 8, 2019

Oh, I didn't see that other PR on python-base, nice!

There are still some yaml.load lying around here though, including one in utils which I guess might be dangerous. I'll close this PR and open a new one to replace the unsafe calls by precaution, if you don't mind.

@ramnes ramnes closed this Jan 8, 2019
ramnes added a commit to ramnes/python that referenced this pull request Jan 8, 2019
@ramnes ramnes mentioned this pull request Jan 8, 2019
ramnes added a commit to ramnes/python that referenced this pull request Jan 9, 2019
roycaihw pushed a commit to roycaihw/client-python that referenced this pull request Feb 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants