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

security(agent): Replace unsafe pyyaml loader with SafeLoader #7035

Merged
merged 3 commits into from
Mar 22, 2024

Conversation

matheudev
Copy link
Contributor

@matheudev matheudev commented Mar 21, 2024

Background

Changes 🏗️

The default loaders in PyYAML are not safe to use with untrusted data. They potentially make your application vulnerable to arbitrary code execution attacks. If you open a YAML file from an untrusted source, and the file is loaded with the default loader, an attacker could execute arbitrary code on your machine.

This codemod hardens all yaml.load() calls against such attacks by replacing the default loader with yaml.SafeLoader. This is the recommended loader for loading untrusted data. For most use cases it functions as a drop-in replacement for the default loader.

Calling yaml.load() without an explicit loader argument is equivalent to calling it with Loader=yaml.Loader, which is unsafe. This usage [has been deprecated](https://github.com/yaml/pyyaml/wiki/PyYAML-yaml.load(input\)-Deprecation) since PyYAML 5.1. This codemod will add an explicit SafeLoader argument to all yaml.load() calls that don't use an explicit loader.

More reading

PR Quality Scorecard ✨

The changes from this codemod look like the following:

  import yaml
  data = b'!!python/object/apply:subprocess.Popen \\n- ls'
- deserialized_data = yaml.load(data, yaml.Loader)
+ deserialized_data = yaml.load(data, Loader=yaml.SafeLoader)
  • Have you used the PR description template?   +2 pts
  • Is your pull request atomic, focusing on a single change?   +5 pts
  • Have you linked the GitHub issue(s) that this PR addresses?   +5 pts
  • Have you documented your changes clearly and comprehensively?   +5 pts
  • Have you changed or added a feature?   -4 pts
    • [] Have you added/updated corresponding documentation?   +4 pts
    • Have you added/updated corresponding integration tests?   +5 pts
  • Have you changed the behavior of AutoGPT?   -5 pts
    • Have you also run agbenchmark to verify that these changes do not regress performance?   +10 pts

pixeebot bot and others added 2 commits March 21, 2024 15:43
Copy link

netlify bot commented Mar 21, 2024

Deploy Preview for auto-gpt-docs canceled.

Name Link
🔨 Latest commit 7c28b3b
🔍 Latest deploy log https://app.netlify.com/sites/auto-gpt-docs/deploys/65fd8963af24fc0008a7dc9b

Copy link

codecov bot commented Mar 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 45.51%. Comparing base (30bc761) to head (7c28b3b).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #7035   +/-   ##
=======================================
  Coverage   45.51%   45.51%           
=======================================
  Files         139      139           
  Lines        6530     6530           
  Branches      917      917           
=======================================
  Hits         2972     2972           
  Misses       3408     3408           
  Partials      150      150           
Flag Coverage Δ
Linux 45.40% <100.00%> (ø)
Windows 43.64% <100.00%> (ø)
autogpt-agent 45.48% <100.00%> (ø)
macOS 44.85% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@Pwuts Pwuts left a comment

Choose a reason for hiding this comment

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

Good idea, I don't think we use the extra/dangerous YAML features anyways.

@Pwuts Pwuts changed the title Replace unsafe pyyaml loader with SafeLoader security(agent): Replace unsafe pyyaml loader with SafeLoader Mar 22, 2024
@Pwuts Pwuts merged commit a1ffe15 into Significant-Gravitas:master Mar 22, 2024
18 of 19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants