[libvirt-users] virDomainCoreDumpWithFormat files created as root

[noxdafox]

I've been spending a bit of time looking into libvirt's code and I 
believe this is not implemented as Daniel first said.

The issue is in the qemuOpenFileAs function in src/qemu/qemu_driver.c 
which ignores the dynamic ownership flag and does not set correct 
ownership on the file.

The qemuOpenFileAs function is used by other ones as well, so I wonder 
if this affects other QEMU features.

I tried to fix and test it in different use cases and I didn't notice 
any side effect, yet I'd like to provide tests as this function is quite 
a core one and it's implementation is a bit confusing (maybe a 
refactoring would simplify its logic).
Unfortunately the function is not exposed, therefore unittests are a bit 
challenging. Higher level tests seem to mock the driver 
(src/test/test_driver.c) as well.

Here's the patch fixing the issue. I set the correct uid and gid only if 
the file is being created and dynamic ownership is set.

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index a2cc002..1b47dc6 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -2932,6 +2932,11 @@ qemuOpenFileAs(uid_t fallback_uid, gid_t 
fallback_gid,
          if (path_shared <= 0 || dynamicOwnership)
              vfoflags |= VIR_FILE_OPEN_FORCE_OWNER;

+        if (dynamicOwnership) {
+            uid = fallback_uid;
+            gid = fallback_gid;
+        }
+
          if (stat(path, &sb) == 0) {
              /* It already exists, we don't want to delete it on error */
              need_unlink = false;

Please let me know what do you think about it and if we can somehow 
include it in the sources. I'd be glad to offer further help if necessary.

On 29/05/15 16:41, Mathew Moon wrote:
> Just a hunch but is the binary that creates the dump suid? You may 
> want to check the perms on it. Not sure if it is libvirtd or some 
> other binary.
>
> Sent from my iPhone
>
> On May 29, 2015, at 3:39 AM, NoxDaFox <noxdafox at gmail.com 
> <mailto:noxdafox at gmail.com>> wrote:
>
>> 2015-05-28 12:04 GMT+03:00 Michal Privoznik <mprivozn at redhat.com 
>> <mailto:mprivozn at redhat.com>>:
>>
>>     On 28.05.2015 09:29, NoxDaFox wrote:
>>     > Greetings,
>>     >
>>     > I am dumping a guest VM memory for inspection using the command
>>     > "virDomainCoreDumpWithFormat" and the created files appear to
>>     belong to
>>     > root (both user and group).
>>     >
>>     > I have searched around but didn't find any answer. Is there a
>>     way to
>>     > instruct QEMU to create those files under a different user?
>>     >
>>
>>     The coredump should belong to the user:group that the domain in
>>     question
>>     is running under. If (by default) your qemu processes run under
>>     root:root so will the coredump.
>>
>>
>> I changed default settings but it didn't work. I can see the qemu 
>> processes now belonging to my user but yet the core dump is 
>> root:root. I used to generate such file with plain QMP 'pmemsave' 
>> command and it was creating it under libvirt-qemu group which was a 
>> bit better. This new command is way more handy but if I can't sort 
>> this issue out I'll need to fallback to the previous one.
>>
>> I set in /etc/libvirt/qemu.conf the keys
>>
>> user = "myuser"
>> group = "mygroup"
>> dynamic_ownership = 1
>>
>> And restarted libvirtd.
>>
>>
>>     Although, I'm wondering if we should not disregard this and make
>>     coredump be always owned by root:root since a coredump may contain
>>     sensitive info, e.g. all kinds of cipher keys. We do that with disk
>>     images, so maybe we should reconsider our politics with coredumps.
>>
>>     Michal
>>
>>
>> This would limit the API quite much. There are use cases in which the 
>> need is to dump the memory for inspection and forcing it to be root 
>> would cripple the design quite a lot. A policy and an expert SysAdmin 
>> is all you need in these cases.
>>
>> In my specific case I'm building a test automation system and I was 
>> forced to run everything as admin or spawn a separate process, with 
>> admin rights, which would reset the correct user:group at every core 
>> dump taken. Quite clumsy!
>> _______________________________________________
>> libvirt-users mailing list
>> libvirt-users at redhat.com <mailto:libvirt-users at redhat.com>
>> https://www.redhat.com/mailman/listinfo/libvirt-users

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvirt-users/attachments/20151108/b22954e6/attachment.htm>