[PATH v2 0/2] qemu: don't change ownership of cache directory
See path 2 for detail. v1 -> v2: - Add commit message for path1 [Peter] Peng Liang (2): qemu: move temp file of screenshot and memorypeek to autoDumpPath qemu: don't change ownership of cache directory src/qemu/qemu_driver.c | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) -- 2.31.1
The temp files of screenshot and memory peek, which are created by QEMU, are put in the cache directory. However, the caches of domain capabilities, which are created and used by libvirtd, are also put in the cache directory. In order to make the cache directory more secure, move the temp files of screenshot and memory peek to autoDumpPath. Since the temp files are just temporary files and are only used by libvirtd (libvirtd will delete them after use), the use of screenshot and memory peek will be affected. Signed-off-by: Peng Liang <liangpeng10@huawei.com> --- src/qemu/qemu_driver.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index dfc27572c461..e929e950e848 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3431,7 +3431,7 @@ qemuDomainScreenshot(virDomainPtr dom, } } - tmp = g_strdup_printf("%s/qemu.screendump.XXXXXX", cfg->cacheDir); + tmp = g_strdup_printf("%s/qemu.screendump.XXXXXX", cfg->autoDumpPath); if ((tmp_fd = g_mkstemp_full(tmp, O_RDWR | O_CLOEXEC, S_IRUSR | S_IWUSR)) == -1) { virReportSystemError(errno, _("g_mkstemp(\"%s\") failed"), tmp); @@ -10692,7 +10692,7 @@ qemuDomainMemoryPeek(virDomainPtr dom, if (virDomainObjCheckActive(vm) < 0) goto endjob; - tmp = g_strdup_printf("%s/qemu.mem.XXXXXX", cfg->cacheDir); + tmp = g_strdup_printf("%s/qemu.mem.XXXXXX", cfg->autoDumpPath); /* Create a temporary filename. */ if ((fd = g_mkstemp_full(tmp, O_RDWR | O_CLOEXEC, S_IRUSR | S_IWUSR)) == -1) { -- 2.31.1
On Mon, Sep 13, 2021 at 07:11:04PM +0800, Peng Liang wrote:
The temp files of screenshot and memory peek, which are created by QEMU, are put in the cache directory. However, the caches of domain capabilities, which are created and used by libvirtd, are also put in the cache directory. In order to make the cache directory more secure, move the temp files of screenshot and memory peek to autoDumpPath.
Since the temp files are just temporary files and are only used by libvirtd (libvirtd will delete them after use), the use of screenshot and memory peek will be affected.
autoDumpPath does nt look like the right thing to be using here. Why don't we just put these files in a subdirectory of the cache dir to avoid the problem with capabilities ? Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 9/13/2021 7:23 PM, Daniel P. Berrangé wrote:
On Mon, Sep 13, 2021 at 07:11:04PM +0800, Peng Liang wrote:
The temp files of screenshot and memory peek, which are created by QEMU, are put in the cache directory. However, the caches of domain capabilities, which are created and used by libvirtd, are also put in the cache directory. In order to make the cache directory more secure, move the temp files of screenshot and memory peek to autoDumpPath.
Since the temp files are just temporary files and are only used by libvirtd (libvirtd will delete them after use), the use of screenshot and memory peek will be affected.
autoDumpPath does nt look like the right thing to be using here. Why don't we just put these files in a subdirectory of the cache dir to avoid the problem with capabilities ?
Ah, I just find that autoDumpPath is for watchdog event to auto-dump a guest. But I think the files libvirtd put in the cache directory (except capabilites) are just temporary files instead of cache files. So IMHO, a subdir in the cache directory is also not the perfect path for these files. How about putting these files in the pre-domain dir (e.g. /var/lib/libvirt/qemu/domain-1-test)?
Regards, Daniel
Thanks, Peng
Commit 6bcf25017bc6 ("virDomainMemoryPeek API") introduced memory peek and commit 9936aecfd1b4 ("qemu: Implement the driver methods") introduced screenshot. Both of them will put temporary files in /var/cache/libvirt/qemu, and the temporary files are created by QEMU. Therefore, the ownership of /var/cache/libvirt/qemu should be changed to user and group configured in qemu.conf to make sure that QEMU process can create and write files in the cache directory. Libvirt will only put the temporary files in /var/cache/libvirt/qemu until commit cbde35899b90 ("Cache result of QEMU capabilities extraction"), which will put the cache of QEMU capabilities in 'capabilities' subdir of the cache directory. Because the capabilities is used by libvirt, the ownership of both 'capabilities' subdir and capabilitie files are root. However, when QEMU process runs as a regular user (e.g. qemu user), the ownership of /var/cache/libvirt/qemu will be changed to qemu:qemu while that of /var/cache/libvirt/qemu/capabilities will be still root:root. Then the regular user could spoof different capabilities, which maybe lead to denial of service. Since the previous patch has move the temp files of screenshot and memory peek to autoDumpPath, no one except domain capabilities uses cacheDir currently. And since domain capabilities are used by libvirtd instead of QEMU, no need to change the ownership of cacheDir to qemu:qemu explicitly. Signed-off-by: Peng Liang <liangpeng10@huawei.com> --- src/qemu/qemu_driver.c | 7 ------- 1 file changed, 7 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index e929e950e848..8a6fd5767893 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -748,13 +748,6 @@ qemuStateInitialize(bool privileged, (int)cfg->group); goto error; } - if (chown(cfg->cacheDir, cfg->user, cfg->group) < 0) { - virReportSystemError(errno, - _("unable to set ownership of '%s' to %d:%d"), - cfg->cacheDir, (int)cfg->user, - (int)cfg->group); - goto error; - } if (chown(cfg->saveDir, cfg->user, cfg->group) < 0) { virReportSystemError(errno, _("unable to set ownership of '%s' to %d:%d"), -- 2.31.1
participants (2)
-
Daniel P. Berrangé -
Peng Liang