Skip to content

Use yaml.safe_load instead of yaml.load #16

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

Merged
merged 1 commit into from
Jan 14, 2020

Conversation

disconnect3d
Copy link
Contributor

TLDR: yaml.load can deserialize Python objects and so is insecure. Though, this is rather a cosmetic change here as risky_roles.yaml is static anyway.

TLDR: `yaml.load` can deserialize Python objects and so is insecure. Though, this is rather a cosmetic change here as `risky_roles.yaml` is static anyway.
@g3rzi
Copy link
Collaborator

g3rzi commented Jan 13, 2020

I think I already fixed this problem in the past here:
191edf4

But it affected the version on docker hub because the python version is not updated there.
I will check how your solution affect the container and update.

@disconnect3d
Copy link
Contributor Author

disconnect3d commented Jan 13, 2020

By the way, the yaml.load was unsafe for a very long time, even when they stated in pyyaml 5.1 that it is supposed to be safe. E.g.:
image

This has been known to pyyaml for some time as reported in this issue, and it seems they finally fixed it in 5.2, e.g. the payload from the screenshot above will throw:

ConstructorError: could not determine a constructor for the tag 'tag:yaml.org,2002:python/object/apply:subprocess.Popen'
  in "<unicode string>", line 1, column 1:
    !!python/object/apply:subprocess ...
    ^

Anyway, in theory we should be fine assuming that we use pyyaml 5.2 but...

  • KubiScan does not have pyyaml in its requirements.txt so it depends on its dependencies to grab proper version of pyyaml - this may end bandly e.g. if someone controls its dependencies
  • From the last two comments in FullLoader is not much safer than UnsafeLoader yaml/pyyaml#321 (comment) it seems that there still might be some possibility of code execution through the default loader in 5.2, though it might be very hard to find proper gadgets

Also, this change works for me, but that's always good to do more testing :).

@g3rzi
Copy link
Collaborator

g3rzi commented Jan 14, 2020

OK I see,
Thanks for notice.
I will try your fix

Copy link
Collaborator

@g3rzi g3rzi left a comment

Choose a reason for hiding this comment

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

I tested it on the container and it seems to work fine.

@g3rzi g3rzi merged commit 83b82fd into cyberark:master Jan 14, 2020
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