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

[Med] Snyk - Arbitrary Code Execution (due 9/10) #3280

Closed
2 of 3 tasks
lbeaufort opened this issue Jul 12, 2018 · 13 comments · Fixed by #3362
Closed
2 of 3 tasks

[Med] Snyk - Arbitrary Code Execution (due 9/10) #3280

lbeaufort opened this issue Jul 12, 2018 · 13 comments · Fixed by #3362
Assignees
Labels
Security: moderate Remediate within 60 days
Milestone

Comments

@lbeaufort
Copy link
Member

lbeaufort commented Jul 12, 2018

Vulnerable module: PyYAML

Introduced through: project@0.0.0 › bandit@1.4.0 › PyYAML@3.13 Removed
Introduced through: project@0.0.0 › flask-apispec@0.6.0.post0 › apispec@0.19.0 › PyYAML@3.13
Introduced through: project@0.0.0 › apispec@0.19.0 › PyYAML@3.13

No current remediation path. Best choice is to see what we can swap out for other packages or remove.

After discussing with @vrajmohan and putting in an issue to apispec to address the PyYAML vulnerability, our best approach is to:

@lbeaufort lbeaufort added this to the Sprint 6.4 milestone Jul 12, 2018
@lbeaufort lbeaufort self-assigned this Jul 12, 2018
@JonellaCulmer JonellaCulmer assigned vrajmohan and unassigned lbeaufort Jul 13, 2018
vrajmohan pushed a commit that referenced this issue Jul 16, 2018
Partially addresses #3280 as it removes one of the
packages that requires PyYAML.
@vrajmohan
Copy link
Contributor

There appears to be no release of PyYAML _on PyPI _ beyond 3.13 at this time - https://pypi.org/project/PyYAML/#history. There is a 4.1 release on GitHub but the fixes in it appear to be reversed in the 4.2 tree.

Also, note that there is only a single dependency on PyYAML that remains - from apispec.

The dependency from bandit has been eliminated and the dependency from flask_apispec is indirect, through apispec.

@vrajmohan
Copy link
Contributor

Given that the next PyYAML release is not out yet but is due soon (there are some beta releases), my recommendation is to wait on this for another sprint. Tagging @lbeaufort and @patphongs to inform Jay?

@vrajmohan
Copy link
Contributor

Detailed discussion around the PyYAML release fiasco: yaml/pyyaml#193

@lbeaufort lbeaufort changed the title [HIGH] Snyk - Arbitrary Code Execution [Medium] Snyk - Arbitrary Code Execution Jul 18, 2018
@lbeaufort lbeaufort removed their assignment Jul 18, 2018
@lbeaufort lbeaufort modified the milestones: Sprint 6.4, Sprint 6.5 Jul 18, 2018
@lbeaufort
Copy link
Member Author

Per Jay, risk assessment is moderate and we can follow-up in 6.5

@PaulClark2 PaulClark2 modified the milestones: Sprint 6.5, Sprint 6.6 Jul 24, 2018
@pkfec pkfec changed the title [Medium] Snyk - Arbitrary Code Execution [High Risk] Snyk - Arbitrary Code Execution Jul 25, 2018
@pkfec
Copy link
Contributor

pkfec commented Jul 25, 2018

changing the severity to HIGH as reported in snyk:
https://snyk.io/org/fecgov/project/a95ea997-b012-4b3b-a026-2fdbe6ac0398

@PaulClark2
Copy link
Contributor

PaulClark2 commented Jul 26, 2018

@pkfec has Jay’s assessment of this issue changed? It looks like this is slated to be worked on this sprint, correct?

@lbeaufort lbeaufort changed the title [High Risk] Snyk - Arbitrary Code Execution [Med] Snyk - Arbitrary Code Execution Aug 9, 2018
@lbeaufort
Copy link
Member Author

@PaulClark2 per Jay, this issue is moderate.

@lbeaufort lbeaufort changed the title [Med] Snyk - Arbitrary Code Execution [Med] Snyk - Arbitrary Code Execution (due 9/10) Aug 9, 2018
@lbeaufort lbeaufort removed this from the Sprint 6.6 milestone Aug 9, 2018
@lbeaufort
Copy link
Member Author

We should try ‘safe_load’: Screenly/Anthias#878 (comment)

@lbeaufort
Copy link
Member Author

After discussing with @vrajmohan and putting in an issue to apispec to address the PyYAML vulnerability, our best approach is to:

@lbeaufort
Copy link
Member Author

Update:

I put in a PR that was merged to the apispec library to use yaml.safe_load(), but we still need to:

  • address a couple issues with updating to the latest version (see https://github.com/fecgov/apispec/pull/1) - might need to put in another issue
  • wait until a new version of apispec is released
  • upgrade to that version

@lbeaufort
Copy link
Member Author

Addressed by marshmallow-code/apispec#278 and #3362

@lbeaufort
Copy link
Member Author

Note - snyk is still flagging prance and apispec for using PyYAML but I confirmed that both packages use yaml.safe_load() and not yaml.load() so I silenced the warning in snyk.

@lbeaufort
Copy link
Member Author

lbeaufort commented Sep 19, 2018

@justin5p here's a summary of this issue:

The package that we use (apispec) was using PyYaml's yaml.load() function, which was flagged as a security vulnerability. I put in a change to apispec to use yaml.safe_load() which is the safe version of the load() function. Then, when upgrading to the latest version, we ran into a code problem that was preventing us from using the latest version of apispec. We put in an issue to their repo but haven't heard back, so we forked the latest version and made the code change on our end, then switched to using our forked copy.

At this time, both flagged packages (prance and apispec) use yaml.safe_load() and no longer present a security vulnerability.

Going forward, it would be best to keep an eye on the latest PyYaml release to see if they fix the load issue, and apispec to see if they fix the issue that caused us to need to fork their repo.

Please let me know if you have questions, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Security: moderate Remediate within 60 days
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants