Molecule: PyYAML update CVE-2017-18342

[gundalow]

Issue Type

• Bug report

summary

Currently we have
https://github.com/ansible/molecule/blob/9d1dec85ed2f995e2ebd24e234fd8fe4334ecb62/requirements.txt#L15

CVE-2017-18342 defines

Vulnerable versions: < 4.2b1
Patched version: 4.2b1
In PyYAML before 4.1, the yaml.load() API could execute arbitrary code. In other words, yaml.safe_load is not used.

Do we need to update?

[decentral1se]

Following the note from jborean93 that we should make sure we're using safe_load, I grepped around and can't find an instance of the plain 'ol load call. Someone might want to check that again more solidly but I think we're OK for now

[seandst]

greppage: find files with 'yaml' in them (to get import yaml, from yaml.module import foo, etc...), then print lines with 'load' in them:

$ for file in `grep -rl yaml molecule/`
> do if grep -q load $file
> then echo --
> echo $file:
> grep load $file
> fi
> done
--
molecule/config.py:
                    defaults, util.safe_load(interpolated_config))
                                        util.safe_load(interpolated_config))
        d = util.safe_load_file(env_file)
--
molecule/driver/ec2.py:
        instance_config_dict = util.safe_load_file(
--
molecule/driver/vagrant.py:
        instance_config_dict = util.safe_load_file(
--
molecule/driver/gce.py:
        instance_config_dict = util.safe_load_file(
--
molecule/driver/openstack.py:
        instance_config_dict = util.safe_load_file(
--
molecule/driver/delegated.py:
        instance_config_dict = util.safe_load_file(
--
molecule/driver/linode.py:
        instance_config_dict = util.safe_load_file(
--
molecule/driver/azure.py:
        instance_config_dict = util.safe_load_file(
--
molecule/provisioner/ansible.py:
    The playbook loading order is:
--
molecule/provisioner/ansible/plugins/filters/molecule_core.py:
    return util.safe_load(interpolated_data)
--
molecule/provisioner/ansible/plugins/libraries/molecule_vagrant.py:
  vagrant_config_hash = YAML::load_file('{{ vagrantfile_config }}')
--
molecule/util.py:
def safe_load(string):
        return yaml.safe_load(string) or {}
def safe_load_file(filename):
        return safe_load(stream)
--
molecule/model/schema_v2.py:
    data = util.safe_load(stream)
--
molecule/migrate.py:
        return json.loads(json.dumps(od))
        d = util.safe_load(open(self._molecule_file))

I see no instances of yaml.load. Everything seems to go through util.safe_load, including util.safe_load_file, which uses util.safe_load, which itself uses yaml.safe_load.

[decentral1se]

Thanks @seandst, solid :+1:

[ssbarnea]

This asks for a PR to bump the minimal requirements. This is what I did on another project https://github.com/pycontribs/jira/blob/master/requirements-dev.txt#L17 even if pyyaml is not yet released we can use the pre-release version in this case.

I do not want to speculate on how long it would take for pyyaml to make an official release due to its "recent" history and we should not make this a hard-dependency for us.

[webknjaz]

I'd argue that it's not a security issue. I even dare to say that that "CVE" is nonsense. The real issue is users not reading docs, not the version of a library which explicitly states in docs that there's safe_load(). Anyway, there's no stable version with the API swapped and we mustn't depend on unstable pre-releases so let's not praise formalities over common sense.

Other than that, it's fine to bump the dependency requirement as soon as there's a stable version to bump to.

[gundalow]

Yup, agree that as is this isn't a security problem. This issue was raised so we ensured we had multiple set of eyes to check the code, as well as raising awareness

PyYAML 5.1 has just been released https://pypi.org/project/PyYAML/#history
I don't suggest we upgrade to this during v2.20 as it's so new, though maybe something we look at as apart of general Python Dependency pinning (and usability) during v2.21

[webknjaz]

/me was about to post about pyyaml release first

[webknjaz]

@gundalow agree about looking :eyes: at in as a more general dependency management problem.
Maybe even raise the idea of switching to semver to better indicate breaking changes

[ssbarnea]

I am not sure abotu all places where pyyaml is used but I do fancy the idea of switching from pyyaml to ruamel.yaml because that one has published binaries for all plaforms, as opposed to pyyaml.
It seems that pre-commit also considering the same move.

[seandst]

I've run into issues with indentation in yaml dumping that could be fixed by using ruamel.yml. As far as all place where pyyaml is used in molecule, my comment from last week gives me high confidence that all yaml loading is filtered through the util module.

A more thorough search would be required, with a pretty easy method being to change the requirements to pull in ruamel.yml, adjust the utils module to use it, and see what explodes in the test suite when pyyaml can no longer be imported.

[seandst]

Woops. ansible itself still depends on pyyaml, so this change would likely need to affect the entire stack.

[webknjaz]

@seandst I think we'll end up with two yaml libs then, just like ansible-lint.

[seandst]

Yeah, I was thinking that finding references to pyyaml by uninstalling it would be impossible because molecule depends on ansible, and ansible depends on pyyaml. Unless the entire dependency stack was altered to no longer install pyyaml (presumably by replacing pyyaml with ruamel.yaml), my "pretty easy method" to find references to pyyaml in molecule is actually not easy at all. Pardon my ambiguity. :)

That all said, I'm still reasonably confident that most, if not all, yaml loads are done through those util module functions. I'm hoping to find some time to do a more thorough search if nobody beats me to it.

[triplepoint]

In any case it'd be nice to bump molecule's dependency to the 5.1 release of pyyaml, or at least test on 5.1 and loosen up the version pinning. Every project I have that imports molecule has Github emailing me about "critical" security vulnerabilities.

[decentral1se]

Regarding pinning and the entire project, there was some work in https://github.com/ansible/molecule/pull/1832.

[webknjaz]

@decentral1se @ssbarnea I think it's safe now to turn exact version pins into ranges in setup.cfg's install_requires and extras_require now.

[decentral1se]

Done.