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

safely deserialize k8 yaml #4071

Merged
merged 4 commits into from
Aug 8, 2023
Merged

safely deserialize k8 yaml #4071

merged 4 commits into from
Aug 8, 2023

Conversation

anthonywoo
Copy link
Contributor

Note: Samson is a public repo, do not include Zendesk-internal information, urls, etc.

Using YAML.load_stream is unsafe and allows instantiating arbitrary classes which may lead to remote code execution (RCE)

I had to use multiple streams because Samson needs to support multiple documents in a single yaml file

References

  • Jira link:

Risks

  • Medium? Some k8 deploys may fail if it is using a disallowed class

@anthonywoo anthonywoo requested review from a team and grosser as code owners August 4, 2023 16:22
@anthonywoo anthonywoo requested a review from a team August 4, 2023 16:22
Copy link
Contributor

@grosser grosser left a comment

Choose a reason for hiding this comment

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

there are no other usages of this method ?

@@ -6,7 +6,14 @@ def self.parse_file(contents, filepath)

if filename.ends_with?('.yml', '.yaml')
# NOTE: this will always return an array of entries
YAML.load_stream(contents, filepath)
Copy link
Contributor

Choose a reason for hiding this comment

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

wrapping this into a yaml_safe_load_stream method would be nice + a test that makes sure it actually does not load unsafe

@anthonywoo
Copy link
Contributor Author

there are no other usages of this method ?

I couldn't find any other usages where the stream content can come from an untrusted source

Copy link
Contributor

@grosser grosser left a comment

Choose a reason for hiding this comment

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

nice test case 🎉

@grosser
Copy link
Contributor

grosser commented Aug 7, 2023

don't worry about bundle_audit that can be another PR

Co-authored-by: Michael Grosser <michael@grosser.it>
@anthonywoo anthonywoo merged commit 22c15a9 into master Aug 8, 2023
9 of 10 checks passed
@anthonywoo anthonywoo deleted the awoo/k8-parse-safe-load branch August 8, 2023 21:29
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.

2 participants