Tests load python modules from hazardous locations (~, ...)

[cans]

Issue Type

• Bug report

Ansible and Ansible Lint details

ansible --version
ansible 2.9.7
  config file = ~/.ansible.cfg
  configured module search path = ['~/.ansible/plugins/modules', '/usr/share/ansible/plugins/modules']
  ansible python module location = ~/.virtualenvs/ansible-lint/lib/python3.7/site-packages/ansible
  executable location = ~/.virtualenvs/ansible-lint/bin/ansible
  python version = 3.7.3 (default, Apr  3 2019, 05:39:12) [GCC 8.3.0]
ansible-lint --version
ansible-lint 4.3.0a1.dev18+g4171317

• ansible installation method: pip
• ansible-lint installation method: source

Desired Behaviour

Ansible-lint test suite should not load Python modules from arbitrary locations.

Actual Behaviour (Bug report only)

When running the tests, on my machine, the following test failed.

test/TestRulesCollection.py::test_rulesdir_user_expansion FAILED

The reason is that I have a .py file in my home directory that imports a module that is not available in my ansible-lint development virtual env. This is a security hazard: I could have had a python module that did far worst than fail to load and fail the test.

In previous PR (#572) we laid some ground work to make sure passed reading the configuration, and merging it with command-line options, ansible-lint only works with absolute path. Additional work was made in #561 (not yet merged, may need additional work) to ensure ansible-lint does not work with relative path internally.

There is one missing bit which is variable expansion, for rulesdir values. Hence I believe the ansiblelint.cli.get_config() function should be either modified to use ansiblelint.utils.expand_paths_vars() or current ansibleline.cli.abspath() function be adapted to allow variable expansion.

Then the call to the former within the RulesCollection.__init__() should no longer be necessary. The test above should become useless, thus removed along with its hazardous behavior.

[webknjaz]

Not sure what we should do with this (I'll probably be taking a closer look at #741 after #740 is merged) but it looks like this should've been reported via https://github.com/ansible/ansible-lint/security/policy.

P.S. The most recent changes to the expansion utils were done by @evilhamsterman in #672, it'd be useful to invite him into the review.

[evilhamsterman]

I added the path expansion because I and many others like having a set of rules and excluded paths (galaxy installed roles) that are shared among multiple projects. If there's a cleaner way to accomplish that goal I'm fine.

I do think the path expansion is necessary for the excluded paths because that may need to point to an environment variable such as one that points to the Galaxy Roles install path. That shouldn't be an issue because its not loading code just paths to specifically not load.

I can see the potential security issue with loading the rules in a test, but I don't see one with running as you should be the one defining where the rules are expanding. I'm not sure how to rectify that with the testing. Maybe drop the expansion on rulesdirs and recommend people resolve using a shared rules folder by using links?

[ssbarnea]

I find this bug confusing as it lacks instructions to reproduce the a failure and unless this is sorted I will close it.

Based on what I read, I tried to create a ~/foo.py that contains an invalid python import. After this I tried to run the linter tests directly and also using tox and I did not see any error related to it.

I want to see what goes wrong and that does not go wrong when just ansible is used.

And if the concern is that running the lint would run some python files from the linted repository, yes that is expected and not a security issue. It gives the same level of security as running bash, it is up to the user to know what he is running.

[webknjaz]

@ssbarnea I think this is about the module search path:

configured module search path = ['~/.ansible/plugins/modules', '/usr/share/ansible/plugins/modules']

Try creating a module under ~/.ansible/plugins/modules and use it in a playbook.

[ssbarnea]

AFAIK, that is the same path as used by Ansible, so why to bother?

[webknjaz]

so why to bother?

Because linters are not supposed to do something dangerous and as a result, the common perception is that those test envs shouldn't be as secure. While users expect some code to be run by Ansible, they'd be surprised if a linter started producing some weird side-effects.

[ssbarnea]

Life is dangerous, it always ends bad.

I am ok to close it, it does not add extra security concerns more that running ansible itself.

[webknjaz]

It adds more security concerns than a linter should have. It should at least be documented loudly in the security considerations section.

[ssbarnea]

Closing as I do not see anything actionale on it and because it does not introduce problems that do not already affect ansible itself, which will also import modules from various locations.