Skip to content
This repository has been archived by the owner. It is now read-only.

do not use a predictable filenames in the LXC plugin #1941

Merged
merged 1 commit into from Apr 2, 2016

Conversation

Projects
None yet
3 participants
@evgeni
Copy link
Contributor

commented Apr 1, 2016

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

lxc_container

ANSIBLE VERSION

Git

SUMMARY

The attach script of the LXC module currently uses predictable names, allowing symlink attacks.
Using mkstemp and not NamedTempFile to keep Py 2.4 compat :(

CVE-2016-3096

PoC:

root@container:/# cd /opt/
root@container:/opt# echo foo > foo
root@container:/opt# ln -s foo .lxc-attach-script
root@container:/opt# ls -alh
total 12K
drwxr-xr-x  2 root root 4.0K Apr  1 13:46 .
drwxr-xr-x 22 root root 4.0K Apr  1 13:39 ..
lrwxrwxrwx  1 root root    3 Apr  1 13:46 .lxc-attach-script -> foo
-rw-r--r--  1 root root    4 Apr  1 13:46 foo
root@container:/opt# cat foo 
foo

# run ansible with "container_command: echo bar"

root@container:/opt# cat foo 
#!/usr/bin/env bash
pushd "$(grep $(whoami) /etc/passwd | awk -F':' '{print $6}')"
    if [[ -f ".bashrc" ]];then
        source .bashrc
    fi
popd

# User defined command
echo bar

@evgeni evgeni force-pushed the evgeni:lxc-predictable-filenames branch from 114f313 to b906715 Apr 1, 2016

@evgeni

This comment has been minimized.

Copy link
Contributor Author

commented Apr 1, 2016

@retropc could you also please have a look? you mentioned you had seen more issues in #1936

@gregdek

This comment has been minimized.

Copy link
Contributor

commented Apr 1, 2016

Thanks @evgeni. To the current maintainers, @cloudnull please review according to guidelines (http://docs.ansible.com/ansible/developing_modules.html#module-checklist) and comment with text 'shipit' or 'needs_revision' as appropriate.

[This message brought to you by your friendly Ansibull-bot.]

@abadger

This comment has been minimized.

Copy link
Member

commented Apr 1, 2016

@evgeni another path that could be fixed is archive_path. It currently defaults to /tmp/ so an attacker could make symlinks there. https://github.com/ansible/ansible-modules-extras/pull/1941/files#diff-cf760a9c318e06abfcf03558a58b2bc4R143 https://github.com/ansible/ansible-modules-extras/blob/devel/cloud/lxc/lxc_container.py#L1750

Since the purpose of archive is to create a backup of the container that the user can find later we probably can't make it into an unpredictable tempfile. I think the best thing for that is to remove the default for archive_path. then add a required_if to arg spec so that if archive is True, archive_path must be set by the user.

@abadger

This comment has been minimized.

Copy link
Member

commented Apr 1, 2016

@evgeni Code here looks good. If you're around go ahead and squash your commits for easier cherry-picking and let me know.

I'm going to start working on the archive_path issue unless you tell me that you're already working on it (want to get these cherry-picked to stable-2.0 so that if we decide to do a 2.0.2rc3, it gets included) (Not sure if we'll do an rc3 yet... but I want to make sure the code is ready if we do.)

@evgeni

This comment has been minimized.

Copy link
Contributor Author

commented Apr 1, 2016

@abadger I have the code ready on my laptop. Just needs a round if testing. Will push later tonight.

@abadger

This comment has been minimized.

Copy link
Member

commented Apr 1, 2016

@evgeni Excellent thanks. I'll merge and cherry-pick once you're ready.

CVE-2016-3096: do not use predictable paths in lxc_container
* do not use a predictable filename for the LXC attach script
* don't use predictable filenames for LXC attach script logging
* don't set a predictable archive_path

this should prevent symlink attacks which could result in
* data corruption
* data leakage
* privilege escalation

@evgeni evgeni force-pushed the evgeni:lxc-predictable-filenames branch from b906715 to 8c6fe64 Apr 2, 2016

@evgeni

This comment has been minimized.

Copy link
Contributor Author

commented Apr 2, 2016

@abadger there, pushed.

sorry it took longer, but I just fell asleep yesterday after arriving home from the customer

@abadger

This comment has been minimized.

Copy link
Member

commented Apr 2, 2016

@evgeni No worries. Thanks for taking the time to create this PR and test!

@abadger abadger merged commit 7c3999a into ansible:devel Apr 2, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@abadger

This comment has been minimized.

Copy link
Member

commented Apr 2, 2016

Merged to devel, stable-2.0 and stable-1.9 branches. CHANGELOG updated on stable-2.0 and stable-1.9. Thanks for the fix! it will be in our next release on all three branches.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.