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

Use yaml.safe_load instead of yaml.load to prevent avoid code execution #1105

Closed
wants to merge 2 commits into from

Conversation

ypid
Copy link

@ypid ypid commented Nov 21, 2017

Ref: yaml/pyyaml#5

To keep us on the safe side before yaml/pyyaml#74 makes it’s way downstream.

Edit: CI build issues seem to be unrelated to my PR. I will sign the CLA later.

@untergeek
Copy link
Member

untergeek commented Nov 21, 2017

Thank you for checking out against 5.x.

Aside from signing the CLA (would you please be so kind), there are a few tests which are still not working:

======================================================================
FAIL: test_do_something_with_int_value (test.integration.test_envvars.TestEnvVars)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/build/elastic/curator/test/integration/test_envvars.py", line 75, in test_do_something_with_int_value
    os.environ.get(evar)
AssertionError: '${JMILLQK9}' != '1234'
======================================================================
FAIL: test_not_present (test.integration.test_envvars.TestEnvVars)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/build/elastic/curator/test/integration/test_envvars.py", line 49, in test_not_present
    self.assertIsNone(cfg['client']['hosts'])
AssertionError: '${CFRR6PTB}' is not None
======================================================================
FAIL: test_not_present_with_default (test.integration.test_envvars.TestEnvVars)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/build/elastic/curator/test/integration/test_envvars.py", line 61, in test_not_present_with_default
    default
AssertionError: '${ODS5TJRH:CRDYI2RJ}' != 'CRDYI2RJ'
======================================================================
FAIL: test_present (test.integration.test_envvars.TestEnvVars)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/build/elastic/curator/test/integration/test_envvars.py", line 38, in test_present
    os.environ.get(evar)
AssertionError: '${LCE4Q46B}' != '1234'

Each of the 4 tests calls:

cfg = curator.get_yaml(self.args['configfile'])

So I imagine that there must be something related to the change.

@untergeek
Copy link
Member

Could it be that yaml.safe_load doesn't respect environment variables?

@ypid
Copy link
Author

ypid commented Nov 21, 2017

@untergeek Thanks for you help. I have not checked this in detail, but the !single syntax looks like it depends on unsafe yaml.load. The risk of local code execution is not as bad as remote code execution which the use of yaml.load had in other projects. Feel free to use other ways of environment variable substitution.

@ypid
Copy link
Author

ypid commented Dec 18, 2021

This would require rework and I switched to ILM. Sorry for not continuing this!

@ypid ypid closed this Dec 18, 2021
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