[libvirt] [PATCH] move ebiptables script out of /tmp
Hi, I noticed today that ebiptablesWriteToTempFile() creates a temporary file in /tmp that is later executed. It uses mkstemp() and therefore is safe from symlinks attacks, however, there is not really any reason that I can see why it is using /tmp instead of somewhere like /var/lib/libvirt. If libvirtd is confined under a MAC which allows execution of /tmp/virtd* and a vulnerability is found in libvirtd, the /tmp path leaves an opportunity for a local non-root attacker to write a script in /tmp and then subvert libvirt to execute that script. Putting it in /var/lib/libvirt (or somewhere without world-write permissions) would prevent this. I do not consider this a security vulnerability, but rather defensive programming. Attached is a patch that uses LOCAL_STATE_DIR "/lib/libvirt/virtdXXXXXX". Feel free to move it somewhere else if desired. Patch is against head. Thanks -- Jamie Strandboge | http://www.canonical.com
On 06/16/2010 02:54 PM, Jamie Strandboge wrote:
Hi,
I noticed today that ebiptablesWriteToTempFile() creates a temporary file in /tmp that is later executed. It uses mkstemp() and therefore is safe from symlinks attacks, however, there is not really any reason that I can see why it is using /tmp instead of somewhere like /var/lib/libvirt. If libvirtd is confined under a MAC which allows execution of /tmp/virtd* and a vulnerability is found in libvirtd, the /tmp path leaves an opportunity for a local non-root attacker to write a script in /tmp and then subvert libvirt to execute that script.
I don't mind the move of the temporary file, but I'd like to understand how would someone subvert libvirt to run an arbitrary script? Stefan
On Wed, 2010-06-16 at 15:11 -0400, Stefan Berger wrote:
On 06/16/2010 02:54 PM, Jamie Strandboge wrote:
Hi,
I noticed today that ebiptablesWriteToTempFile() creates a temporary file in /tmp that is later executed. It uses mkstemp() and therefore is safe from symlinks attacks, however, there is not really any reason that I can see why it is using /tmp instead of somewhere like /var/lib/libvirt. If libvirtd is confined under a MAC which allows execution of /tmp/virtd* and a vulnerability is found in libvirtd, the /tmp path leaves an opportunity for a local non-root attacker to write a script in /tmp and then subvert libvirt to execute that script.
I don't mind the move of the temporary file, but I'd like to understand how would someone subvert libvirt to run an arbitrary script?
The main reason is to not use /tmp when you don't have to. I consider this good defensive programming. Regarding your question on subverting libvirt: Assume that someone has strictly confined libvirtd with a Mandatory Access Control (MAC) system. libvirtd would presumably then be allowed to execute only those commands allowed by the MAC (a very small subset of all commands on the system). If the MAC also allows execution of /tmp/virtd* (which it needs to for proper ebiptables functionality in 0.8.0 and higher without my suggested change), then this weakens the MAC protection because if there is a vulnerability discovered in libvirt that allows a non-root local attacker to execute arbitrary code, the attacker can write a script called /tmp/virtd.gotcha and have libvirt execute that as root. If the file is moved to /var/lib/libvirt (or wherever) as I suggested, then the MAC would allow execute permissions on the files /var/lib/libvirt/virtd*, not files in /tmp. The non-root user cannot circumvent the MAC in this manner even if the user has an exploit against libvirt because the MAC doesn't allow execute of files in an area that the user has write access to. Obviously, some of this is dependent on the MAC policy in place. Like I said, I don't consider this a vulnerability in and of itself because of the current proper use of mkstemp(), but since the file doesn't seem to need to be shared, why have it in a directory that is world-writable? -- Jamie Strandboge | http://www.canonical.com
On 06/16/2010 01:11 PM, Stefan Berger wrote:
On 06/16/2010 02:54 PM, Jamie Strandboge wrote:
Hi,
I noticed today that ebiptablesWriteToTempFile() creates a temporary file in /tmp that is later executed. It uses mkstemp() and therefore is safe from symlinks attacks, however, there is not really any reason that I can see why it is using /tmp instead of somewhere like /var/lib/libvirt. If libvirtd is confined under a MAC which allows execution of /tmp/virtd* and a vulnerability is found in libvirtd, the /tmp path leaves an opportunity for a local non-root attacker to write a script in /tmp and then subvert libvirt to execute that script.
I don't mind the move of the temporary file, but I'd like to understand how would someone subvert libvirt to run an arbitrary script?
I'm with Stefan - the whole point of mkstemp is that the created file has 0600 permissions, and /tmp is restricted-deletion, so no other user can either overwrite the file contents or unlink it and replace it with an alternate file. Then again, gnulib documents that glibc 2.0.7 or older would create a file with group/other permissions if the umask wasn't set prior to the mkstemp() call, and gnulib's mkstemp() does not work around this issue; but that's a rather old version of glibc to be worrying about. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Wed, 2010-06-16 at 14:04 -0600, Eric Blake wrote:
I'm with Stefan - the whole point of mkstemp is that the created file has 0600 permissions, and /tmp is restricted-deletion, so no other user can either overwrite the file contents or unlink it and replace it with an alternate file. Then again, gnulib documents that glibc 2.0.7 or older would create a file with group/other permissions if the umask wasn't set prior to the mkstemp() call, and gnulib's mkstemp() does not work around this issue; but that's a rather old version of glibc to be worrying about.
This has nothing to do with mkstemp(). As I said, libvirt's use of it is fine and there is no symlink race or security vulnerability by itself. The issue is that use of /tmp is not required *and* it becomes difficult to properly confine libvirtd via a MAC if you must allow execution of files in /tmp. See my answer to Stefan's question for an example scenario. -- Jamie Strandboge | http://www.canonical.com
On Wed, Jun 16, 2010 at 3:57 PM, Jamie Strandboge <jamie@canonical.com> wrote:
On Wed, 2010-06-16 at 14:04 -0600, Eric Blake wrote:
I'm with Stefan - the whole point of mkstemp is that the created file has 0600 permissions, and /tmp is restricted-deletion, so no other user can either overwrite the file contents or unlink it and replace it with an alternate file. Then again, gnulib documents that glibc 2.0.7 or older would create a file with group/other permissions if the umask wasn't set prior to the mkstemp() call, and gnulib's mkstemp() does not work around this issue; but that's a rather old version of glibc to be worrying about.
This has nothing to do with mkstemp(). As I said, libvirt's use of it is fine and there is no symlink race or security vulnerability by itself.
The issue is that use of /tmp is not required *and* it becomes difficult to properly confine libvirtd via a MAC if you must allow execution of files in /tmp. See my answer to Stefan's question for an example scenario.
-- Jamie Strandboge | http://www.canonical.com
For what it's worth, I've seen many times when people secure system that /tmp is mounted with noexec. If any of you have a shared web host and you can ssh into it and confirm. $ mount ...snip... /dev/mapper/vg-tmp on /tmp type ext3 (rw,noexec,nosuid,noatime) ...snip... -- Doug Goldstein
On Wed, Jun 16, 2010 at 03:57:50PM -0500, Jamie Strandboge wrote:
On Wed, 2010-06-16 at 14:04 -0600, Eric Blake wrote:
I'm with Stefan - the whole point of mkstemp is that the created file has 0600 permissions, and /tmp is restricted-deletion, so no other user can either overwrite the file contents or unlink it and replace it with an alternate file. Then again, gnulib documents that glibc 2.0.7 or older would create a file with group/other permissions if the umask wasn't set prior to the mkstemp() call, and gnulib's mkstemp() does not work around this issue; but that's a rather old version of glibc to be worrying about.
This has nothing to do with mkstemp(). As I said, libvirt's use of it is fine and there is no symlink race or security vulnerability by itself.
The issue is that use of /tmp is not required *and* it becomes difficult to properly confine libvirtd via a MAC if you must allow execution of files in /tmp. See my answer to Stefan's question for an example scenario.
ACK to your patch but really this whole set of code needs to be shot. We simply should not be writing shell scripts and then executing them, when we have perfectly capable APIs for directly executing commands. Using the shell adds no value here IMHO. I will be rewriting this code when I finish the virCommandPtr code that I proposed a few weeks back Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On 06/17/2010 06:20 AM, Daniel P. Berrange wrote:
On Wed, Jun 16, 2010 at 03:57:50PM -0500, Jamie Strandboge wrote:
On Wed, 2010-06-16 at 14:04 -0600, Eric Blake wrote:
I'm with Stefan - the whole point of mkstemp is that the created file has 0600 permissions, and /tmp is restricted-deletion, so no other user can either overwrite the file contents or unlink it and replace it with an alternate file. Then again, gnulib documents that glibc 2.0.7 or older would create a file with group/other permissions if the umask wasn't set prior to the mkstemp() call, and gnulib's mkstemp() does not work around this issue; but that's a rather old version of glibc to be worrying about.
This has nothing to do with mkstemp(). As I said, libvirt's use of it is fine and there is no symlink race or security vulnerability by itself.
The issue is that use of /tmp is not required *and* it becomes difficult to properly confine libvirtd via a MAC if you must allow execution of files in /tmp. See my answer to Stefan's question for an example scenario.
ACK to your patch but really this whole set of code needs to be shot.
I had tried to get rid of the script creation a while ago, but couldn't get rid of all of them, particularly those that create the 'anchors' for the iptables are a bit more complicated where I run tests whether the necessary rules already exists and only if they don't exist run a certain set of iptables commands to create user-defined tables into which libvirt then builds the rules pertaining to individual VMs. Also, the whole XML filter tree is first translated into iptables rules to detect errors there (references to filters that don't exist) and then run, rather than executing the rules individually while translating a single XML rule. Further, some of the rules may fail, particularly those that do the tear-down, but don't cause the script to abort. Others may not fail and would cause the abort and the cleanup of the rules afterwards. So, what previously was a construct like this here that checked the return code cmd='iptables ...' ${cmd} if [ $? -ne 0 ]; then echo "Error: command ${cmd} failed." exit 1; fi became something like this here: iptables ... STOPONERROR where STOPONERROR was basically emulating the check of the iptables status code above. So it was processing a command like iptables, split its parameters into an array for passing to the libvirt api, then checked whether the next command was a STOPONERROR and took appropriate action if the status code was != 0. The patch from a while ago is attached. Stefan
On Thu, Jun 17, 2010 at 09:37:33AM -0400, Stefan Berger wrote:
On 06/17/2010 06:20 AM, Daniel P. Berrange wrote:
On Wed, Jun 16, 2010 at 03:57:50PM -0500, Jamie Strandboge wrote:
On Wed, 2010-06-16 at 14:04 -0600, Eric Blake wrote:
I'm with Stefan - the whole point of mkstemp is that the created file has 0600 permissions, and /tmp is restricted-deletion, so no other user can either overwrite the file contents or unlink it and replace it with an alternate file. Then again, gnulib documents that glibc 2.0.7 or older would create a file with group/other permissions if the umask wasn't set prior to the mkstemp() call, and gnulib's mkstemp() does not work around this issue; but that's a rather old version of glibc to be worrying about.
This has nothing to do with mkstemp(). As I said, libvirt's use of it is fine and there is no symlink race or security vulnerability by itself.
The issue is that use of /tmp is not required *and* it becomes difficult to properly confine libvirtd via a MAC if you must allow execution of files in /tmp. See my answer to Stefan's question for an example scenario.
ACK to your patch but really this whole set of code needs to be shot.
I had tried to get rid of the script creation a while ago, but couldn't get rid of all of them, particularly those that create the 'anchors' for the iptables are a bit more complicated where I run tests whether the necessary rules already exists and only if they don't exist run a certain set of iptables commands to create user-defined tables into which libvirt then builds the rules pertaining to individual VMs.
Also, the whole XML filter tree is first translated into iptables rules to detect errors there (references to filters that don't exist) and then run, rather than executing the rules individually while translating a single XML rule. Further, some of the rules may fail, particularly those that do the tear-down, but don't cause the script to abort.
Yep, any replacement solution would have to allow for commands to fail. The virCommand APIs[1] provide a convenient way to store a command + set of args in memory. I'm thinking we'd introduce a second object virCommandSet which stored struct virCommandEntry { virCommand cmd; bool stopOnError; }; struct virCommandSet { size_t ncmds; virCommandEntry cmds; } So the filter code would populate a virCommandSet object completely, allowing for validation of the full XML filter tree. Then it would invoke an execute operation against the virCommandSet to run the whole thing. If we wanted to be really clever, then we could make the virCommandEntry contain 2 virCommand objects, one normal command to run, and one 'rollback' command to undo its effects. If a virCommandSet failed part way through, then we can just iterate backwards over the entries executing any rollback commands. This will have to wait till I finish the basic virCommand bits though. Regards, Daniel [1] https://www.redhat.com/archives/libvir-list/2010-May/msg00991.html -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
participants (5)
-
Daniel P. Berrange -
Doug Goldstein -
Eric Blake -
Jamie Strandboge -
Stefan Berger