Bug 782474

Summary: Consider using PrivateTmp=true in service file for libvirt
Product: [Community] Virtualization Tools Reporter: Daniel Walsh <dwalsh>
Component: libvirtAssignee: Libvirt Maintainers <libvirt-maint>
Status: CLOSED WONTFIX QA Contact:
Severity: unspecified Docs Contact:
Priority: unspecified    
Version: unspecifiedCC: berrange, clalancette, crobinso, dougsland, eblake, itamar, jforbes, laine, libvirt-maint, rbalakri, veillard, virt-maint
Target Milestone: ---   
Target Release: ---   
Hardware: Unspecified   
OS: Unspecified   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2016-04-14 20:18:10 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:
Bug Depends On:    
Bug Blocks: 782466    

Description Daniel Walsh 2012-01-17 15:12:11 UTC
I would like to propose using PrivateTmp for Libvirt, to make it more secure
and avoid users from being able to potentially effect it.

http://fedoraproject.org/wiki/Features/ServicesPrivateTmp

Comment 1 Daniel Walsh 2012-02-06 20:46:00 UTC
Any change on this bug.  We are coming up to Feature Freeze, and would like some comment on this bug.

If you do not believe this application uses /tmp than please comment on this and close the bug.  

If you believe this application needs to use /tmp to communicate with other applications or users then you can close this bug with that comment.

If your app does not use systemd, then close this bug with that comment.

If you have no idea, then please add a comment, and change the bug to assigned.

I need to update the status on this feature.


Thanks for your help.

Comment 2 Eric Blake 2012-02-06 21:02:16 UTC
Libvirt should not be explicitly using /tmp for anything needed for communication with the user.  The last known instance of using /tmp was patched in libvirt 0.9.8 by:

commit bd6083c9baf1a8248b3b9fcde0a2c3f44e8e61ad
Author: Eric Blake <eblake>
Date:   Wed Nov 9 10:23:49 2011 -0700

    nwfilter: avoid failure with noexec /tmp
    
    If /tmp is mounted with the noexec flag (common on security-conscious
    systems), then nwfilter will fail to initialize, because we cannot
    run any temporary script via virRun("/tmp/script"); but we _can_
    use "/bin/sh /tmp/script".  For that matter, using /tmp risks collisions
    with other unrelated programs; we already have /var/run/libvirt as a
    dedicated temporary directory for use by libvirt.
    
    * src/nwfilter/nwfilter_ebiptables_driver.c
    (ebiptablesWriteToTempFile): Use internal directory, not /tmp;
    drop attempts to make script executable; and detect close error.
    (ebiptablesExecCLI): Switch to virCommand, and invoke the shell to
    read the script, rather than requiring an executable script.

Supposing we want private temp enabled, what would the patch to the systemd service file look like?

Comment 3 Cole Robinson 2012-02-06 21:11:55 UTC
Eric, I think all the patch would be is adding PrivateTmp=true to the unit file.

That said, I think enabling this feature would cause us pain, because I guarantee there are users out there storing disk images or serial logs or whatever in /tmp (for better or worse), and I'm pretty sure this feature would cause their guests to now fail to start.

If we are gonna enable it, finding some way to indicate we have private tmp in an error message, or at least documenting the crap out of it would be important.

Comment 4 Eric Blake 2012-02-06 21:17:07 UTC
Patch proposed: https://www.redhat.com/archives/libvir-list/2012-February/msg00314.html

Comment 5 Eric Blake 2012-02-06 21:20:37 UTC
(In reply to comment #3)
> Eric, I think all the patch would be is adding PrivateTmp=true to the unit
> file.
> 
> That said, I think enabling this feature would cause us pain, because I
> guarantee there are users out there storing disk images or serial logs or
> whatever in /tmp (for better or worse), and I'm pretty sure this feature would
> cause their guests to now fail to start.

Indeed, commands such as 'virsh screenshot domain /tmp/file.bmp' would fail if the /tmp/file.bmp seen by libvirtd is different than the /tmp/file.bmp seen by the user issuing the virsh command.

> 
> If we are gonna enable it, finding some way to indicate we have private tmp in
> an error message, or at least documenting the crap out of it would be
> important.

Then let's make sure we discuss the ramifications on the upstream patch; if it gets rejected for reasons like you have mentioned, then we can reject this bug for libvirtd.

Comment 6 Eric Blake 2012-02-06 21:28:38 UTC
(In reply to comment #5)
> Indeed, commands such as 'virsh screenshot domain /tmp/file.bmp' would fail if
> the /tmp/file.bmp seen by libvirtd is different than the /tmp/file.bmp seen by
> the user issuing the virsh command.

Actually, 'virsh screenshot' is a bad example - that sets up a stream, where the end file is created on the user side (virsh), not the server side (libvirtd).  But we _do_ have libvirt API that pass filenames over the RPC call for libvirtd to open.  Any API that can also pass streams instead of filenames is safe, but we don't have all of our APIs converted to streams yet (for example, virDomainCoreDump has no stream counterpart).

Comment 7 Daniel Walsh 2012-02-06 21:32:58 UTC
I would say if this seems at all dangerous then put it off until you  guys are ready.

Comment 8 Cole Robinson 2012-02-06 23:30:03 UTC
Yeah I thought that this would affect a qemu guest trying use a disk in /tmp
but I guess I was wrong. In which case private tmp probably won't have too many
realistic effects.

Comment 9 Cole Robinson 2012-06-07 14:14:15 UTC
While this wasn't outright rejected, I think there's work to be done in libvirt before we could enable this. Moving this to the upstream tracker

Comment 10 Cole Robinson 2016-04-14 20:18:10 UTC
I just tried this. This basically breaks any usage of disk images in /tmp, since libvirt wants to inspect them to determine their disk formats. While putting disk images in tmp might not be a recommended setup, I'm sure someone is doing it somewhere, and PrivateTmp would make it fail in a very non-obvious way. So this isn't something we could unconditionally enable IMO