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

Update pyyaml to prevent arbitrary code execution #3675

Merged
merged 2 commits into from
Dec 16, 2019

Conversation

sphuber
Copy link
Contributor

@sphuber sphuber commented Dec 16, 2019

Fixes #3310

Before pyyaml==5.1 the yaml.load function was vulnerable to
arbitrary code execution, because it loaded the full set of YAML. There
was an alternative safe_load but this was not the default and could
only load a sub set of the markup language. The new version of pyyaml
deprecates the old vulnerable code and provides the FullLoader that
can load the full set without being vulnerable.

@sphuber sphuber requested a review from ltalirz December 16, 2019 07:37
@sphuber
Copy link
Contributor Author

sphuber commented Dec 16, 2019

@ltalirz I have marked it as blocked because it does quite a bit the same as your PR. Once yours is merged I will rebase to include the few remaining warning fixes.

We discussed a long time ago to use ruamel.yaml instead of sticking with pyyaml but the latter seems to be developed again and the vulnerability is fixed. In contrast, the former started really changing the interface where it was a big job to start adapting it all.

Also, it seems like I will have to start updating the whole topika, kiwipy plumpy stack ☹️

@ltalirz
Copy link
Member

ltalirz commented Dec 16, 2019

Once yours is merged I will rebase to include the few remaining warning fixes.

Thanks, please go ahead!

We discussed a long time ago to use ruamel.yaml instead of sticking with pyyaml but the latter seems to be developed again and the vulnerability is fixed. In contrast, the former started really changing the interface where it was a big job to start adapting it all.

Ok, no strong feelings about this. Feel free to close the corresponding issue.

 * Do not use `Test` as prefix for dummy classes
 * Replace `imp` for `importlib` in REST API
 * Use `identifier` instead of deprecated `node_class`
@sphuber sphuber changed the title [BLOCKED] Update pyyaml to prevent arbitrary code execution Update pyyaml to prevent arbitrary code execution Dec 16, 2019
@sphuber
Copy link
Contributor Author

sphuber commented Dec 16, 2019

This depends on a new version of kiwipy that I just released but that isn't on conda-forge yet.

Before `pyyaml==5.1` the `yaml.load` function was vulnerable to
arbitrary code execution, because it loaded the full set of YAML. There
was an alternative `safe_load` but this was not the default and could
only load a sub set of the markup language. The new version of pyyaml
deprecates the old vulnerable code and provides the `FullLoader` that
can load the full set without being vulnerable.
@sphuber
Copy link
Contributor Author

sphuber commented Dec 16, 2019

@ltalirz this is unblocked and good to go

Copy link
Member

@ltalirz ltalirz left a comment

Choose a reason for hiding this comment

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

Thanks @sphuber !

@ltalirz ltalirz merged commit 999ae3a into aiidateam:develop Dec 16, 2019
@sphuber sphuber deleted the fix_3310_update_pyyaml branch December 16, 2019 17:52
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

Successfully merging this pull request may close these issues.

consider switch form pyyaml to ruamel
2 participants