On Fri, Mar 25, 2016 at 11:02:35AM +0100, Guido Günther wrote:
> Hi Philipp,
>
> thanks for looking into this!
>
> On Thu, Mar 24, 2016 at 05:51:21PM +0100, Philipp Hahn wrote:
> > diff -Nru libvirt-1.2.9/debian/changelog libvirt-1.2.9/debian/changelog
> > --- libvirt-1.2.9/debian/changelog 2015-08-26 08:34:22.000000000 +0200
> > +++ libvirt-1.2.9/debian/changelog 2016-03-05 08:18:07.000000000 +0100
> > @@ -1,3 +1,13 @@
> > +libvirt (1.2.9-9+deb8u2) jessie; urgency=medium
> > +
> > + * Non-maintainer upload.
> > + * Fix CVE-2015-5313 (Closes: #808273)
> > + * libvirt-daemon: Expects qemu-bridge-helper in /usr/libexec/
> > + (Closes: #816602)
> > + * Fix several FTBFS errors
>
> Could you elaborate on the FTBFS errors? I just rebuilt the current
> jessie version without any changes so "Fix several FTBFS errors" sounds
> very vague and exaggeratet. We already had a point release in Jessie so
> I wonder how these would get introduced?
>
> > +
> > + -- Philipp Matthias Hahn <pmh...@debian.org> Fri, 04 Mar 2016 12:01:36
> > +0100
> > +
> > libvirt (1.2.9-9+deb8u1) jessie; urgency=medium
> >
> > [ Guido Günther ]
> > diff -Nru libvirt-1.2.9/debian/control libvirt-1.2.9/debian/control
> > --- libvirt-1.2.9/debian/control 2015-08-24 16:20:54.000000000 +0200
> > +++ libvirt-1.2.9/debian/control 2016-03-04 13:42:30.000000000 +0100
> > @@ -5,6 +5,7 @@
> > Uploaders: Guido Günther <a...@sigxcpu.org>, Laurent Léonard
> > <laur...@open-minds.org>
> > Build-Depends:
> > debhelper (>= 7),
> > + dh-autoreconf,
> > dh-systemd (>= 1.18~),
> > libxml2-dev,
> > libncurses5-dev,
> > diff -Nru
> > libvirt-1.2.9/debian/patches/debian/Debianize-bridge-helper-path.patch
> > libvirt-1.2.9/debian/patches/debian/Debianize-bridge-helper-path.patch
> > --- libvirt-1.2.9/debian/patches/debian/Debianize-bridge-helper-path.patch
> > 1970-01-01 01:00:00.000000000 +0100
> > +++ libvirt-1.2.9/debian/patches/debian/Debianize-bridge-helper-path.patch
> > 2016-03-05 08:18:07.000000000 +0100
> > @@ -0,0 +1,42 @@
> > +libvirt-daemon: Expects qemu-bridge-helper in /usr/libexec/
> > +
> > +$ strings /usr/lib/libvirt/connection-driver/libvirt_driver_qemu.so | grep
> > bridge-helper
> > +/usr/libexec/qemu-bridge-helper
> > +
> > +$ dpkg -S bridge-helper
> > +qemu-system-common: /usr/lib/qemu/qemu-bridge-helper
> > +
> > +Closes #816602
> > +--- a/src/qemu/qemu.conf
> > ++++ b/src/qemu/qemu.conf
> > +@@ -357,7 +357,7 @@
> > + # is used to create <source type='bridge'> interfaces when libvirtd is
> > + # running unprivileged. libvirt invokes the helper directly, instead
> > + # of using "-netdev bridge", for security reasons.
> > +-#bridge_helper = "/usr/libexec/qemu-bridge-helper"
> > ++#bridge_helper = "/usr/lib/qemu/qemu-bridge-helper"
> > +
> > +
> > +
> > +--- a/src/qemu/qemu_conf.c
> > ++++ b/src/qemu/qemu_conf.c
> > +@@ -244,7 +244,7 @@ virQEMUDriverConfigPtr virQEMUDriverConf
> > + goto error;
> > + }
> > +
> > +- if (VIR_STRDUP(cfg->bridgeHelperName,
> > "/usr/libexec/qemu-bridge-helper") < 0)
> > ++ if (VIR_STRDUP(cfg->bridgeHelperName,
> > "/usr/lib/qemu/qemu-bridge-helper") < 0)
> > + goto error;
> > +
> > + cfg->clearEmulatorCapabilities = true;
> > +--- a/src/qemu/test_libvirtd_qemu.aug.in
> > ++++ b/src/qemu/test_libvirtd_qemu.aug.in
> > +@@ -56,7 +56,7 @@ module Test_libvirtd_qemu =
> > + { "auto_dump_bypass_cache" = "0" }
> > + { "auto_start_bypass_cache" = "0" }
> > + { "hugetlbfs_mount" = "/dev/hugepages" }
> > +-{ "bridge_helper" = "/usr/libexec/qemu-bridge-helper" }
> > ++{ "bridge_helper" = "/usr/lib/qemu/qemu-bridge-helper" }
> > + { "clear_emulator_capabilities" = "1" }
> > + { "set_process_name" = "1" }
> > + { "max_processes" = "0" }
> > diff -Nru
> > libvirt-1.2.9/debian/patches/Disable-failing-virnetsockettest.patch
> > libvirt-1.2.9/debian/patches/Disable-failing-virnetsockettest.patch
> > --- libvirt-1.2.9/debian/patches/Disable-failing-virnetsockettest.patch
> > 2015-08-24 16:20:54.000000000 +0200
> > +++ libvirt-1.2.9/debian/patches/Disable-failing-virnetsockettest.patch
> > 2016-03-04 14:47:12.000000000 +0100
> > @@ -7,11 +7,25 @@
> > tests/virnetsockettest.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > -diff --git a/tests/virnetsockettest.c b/tests/virnetsockettest.c
> > -index 5d91f26..1f283a3 100644
> > --- a/tests/virnetsockettest.c
> > +++ b/tests/virnetsockettest.c
> > -@@ -501,10 +501,12 @@ mymain(void)
> > +@@ -333,6 +333,7 @@ static int testSocketUNIXAddrs(const voi
> > + return ret;
> > + }
> > +
> > ++#if 0
> > + static int testSocketCommandNormal(const void *data ATTRIBUTE_UNUSED)
> > + {
> > + virNetSocketPtr csock = NULL; /* Client socket */
> > +@@ -383,6 +384,7 @@ static int testSocketCommandFail(const v
> > + virObjectUnref(csock);
> > + return ret;
> > + }
> > ++#endif
>
>
> Why did you disable this one?
>
> > +
> > + struct testSSHData {
> > + const char *nodename;
> > +@@ -501,10 +503,12 @@ mymain(void)
> > if (virtTestRun("Socket UNIX Addrs", testSocketUNIXAddrs, NULL) < 0)
> > ret = -1;
> >
> > diff -Nru
> > libvirt-1.2.9/debian/patches/security/CVE-2015-5313-storage-don-t-allow-in-filesystem-volu.patch
> >
> > libvirt-1.2.9/debian/patches/security/CVE-2015-5313-storage-don-t-allow-in-filesystem-volu.patch
> > ---
> > libvirt-1.2.9/debian/patches/security/CVE-2015-5313-storage-don-t-allow-in-filesystem-volu.patch
> > 1970-01-01 01:00:00.000000000 +0100
> > +++
> > libvirt-1.2.9/debian/patches/security/CVE-2015-5313-storage-don-t-allow-in-filesystem-volu.patch
> > 2016-03-04 14:46:20.000000000 +0100
> > @@ -0,0 +1,72 @@
> > +From 034e47c338b13a95cf02106a3af912c1c5f818d7 Mon Sep 17 00:00:00 2001
> > +Message-Id:
> > <034e47c338b13a95cf02106a3af912c1c5f818d7.1457088964.git.h...@univention.de>
> > +From: Eric Blake <ebl...@redhat.com>
> > +Date: Tue, 8 Dec 2015 17:46:31 -0700
> > +Subject: [PATCH] CVE-2015-5313: storage: don't allow '/' in filesystem
> > volume
> > + names
> > +Organization: Univention GmbH, Bremen, Germany
> > +To: libvir-l...@redhat.com
> > +
> > +The libvirt file system storage driver determines what file to
> > +act on by concatenating the pool location with the volume name.
> > +If a user is able to pick names like "../../../etc/passwd", then
> > +they can escape the bounds of the pool. For that matter,
> > +virStoragePoolListVolumes() doesn't descend into subdirectories,
> > +so a user really shouldn't use a name with a slash.
> > +
> > +Normally, only privileged users can coerce libvirt into creating
> > +or opening existing files using the virStorageVol APIs; and such
> > +users already have full privilege to create any domain XML (so it
> > +is not an escalation of privilege). But in the case of
> > +fine-grained ACLs, it is feasible that a user can be granted
> > +storage_vol:create but not domain:write, and it violates
> > +assumptions if such a user can abuse libvirt to access files
> > +outside of the storage pool.
> > +
> > +Therefore, prevent all use of volume names that contain "/",
> > +whether or not such a name is actually attempting to escape the
> > +pool.
> > +
> > +This changes things from:
> > +
> > +$ virsh vol-create-as default ../../../../../../etc/haha --capacity 128
> > +Vol ../../../../../../etc/haha created
> > +$ rm /etc/haha
> > +
> > +to:
> > +
> > +$ virsh vol-create-as default ../../../../../../etc/haha --capacity 128
> > +error: Failed to create vol ../../../../../../etc/haha
> > +error: Requested operation is not valid: volume name
> > '../../../../../../etc/haha' cannot contain '/'
> > +
> > +Signed-off-by: Eric Blake <ebl...@redhat.com>
> > +---
> > + src/storage/storage_backend_fs.c | 10 +++++++++-
> > + 1 file changed, 9 insertions(+), 1 deletion(-)
> > +
> > +--- a/src/storage/storage_backend_fs.c
> > ++++ b/src/storage/storage_backend_fs.c
> > +@@ -1,7 +1,7 @@
> > + /*
> > + * storage_backend_fs.c: storage backend for FS and directory handling
> > + *
> > +- * Copyright (C) 2007-2014 Red Hat, Inc.
> > ++ * Copyright (C) 2007-2015 Red Hat, Inc.
> > + * Copyright (C) 2007-2008 Daniel P. Berrange
> > + *
> > + * This library is free software; you can redistribute it and/or
> > +@@ -1005,6 +1005,14 @@ virStorageBackendFileSystemVolCreate(vir
> > +
> > + vol->type = VIR_STORAGE_VOL_FILE;
> > +
> > ++ /* Volumes within a directory pools are not recursive; do not
> > ++ * allow escape to ../ or a subdir */
> > ++ if (strchr(vol->name, '/')) {
> > ++ virReportError(VIR_ERR_OPERATION_INVALID,
> > ++ _("volume name '%s' cannot contain '/'"),
> > vol->name);
> > ++ return -1;
> > ++ }
> > ++
> > + VIR_FREE(vol->target.path);
> > + if (virAsprintf(&vol->target.path, "%s/%s",
> > + pool->def->target.path,
> > diff -Nru libvirt-1.2.9/debian/patches/series
> > libvirt-1.2.9/debian/patches/series
> > --- libvirt-1.2.9/debian/patches/series 2015-08-24 16:20:54.000000000
> > +0200
> > +++ libvirt-1.2.9/debian/patches/series 2016-03-05 08:18:07.000000000
> > +0100
> > @@ -31,3 +31,5 @@
> > Allow-access-to-libnl-3-config-files.patch
> > Fix-crash-on-live-migration.patch
> > upstream/Report-original-error-when-QMP-probing-fails-with-ne.patch
> > +security/CVE-2015-5313-storage-don-t-allow-in-filesystem-volu.patch
> > +debian/Debianize-bridge-helper-path.patch
> > diff -Nru
> > libvirt-1.2.9/debian/patches/upstream/Report-original-error-when-QMP-probing-fails-with-ne.patch
> >
> > libvirt-1.2.9/debian/patches/upstream/Report-original-error-when-QMP-probing-fails-with-ne.patch
> > ---
> > libvirt-1.2.9/debian/patches/upstream/Report-original-error-when-QMP-probing-fails-with-ne.patch
> > 2015-08-24 16:20:54.000000000 +0200
> > +++
> > libvirt-1.2.9/debian/patches/upstream/Report-original-error-when-QMP-probing-fails-with-ne.patch
> > 2016-03-04 14:47:12.000000000 +0100
> > @@ -176,7 +176,7 @@
> >
> > if (virQEMUCapsParseHelpStr("QEMU", help, flags,
> > - &version, &is_kvm, &kvm_version, false)
> > == -1)
> > -+ &version, &is_kvm, &kvm_version, false,
> > NULL) == -1) {
> > ++ &version, &is_kvm, &kvm_version, false,
> > NULL) == -1)
> > goto cleanup;
>
> I wonder why this one changed as well.
>
> >
> > # ifndef WITH_YAJL
> > diff -Nru libvirt-1.2.9/debian/README.Debian
> > libvirt-1.2.9/debian/README.Debian
> > --- libvirt-1.2.9/debian/README.Debian 2015-08-24 16:20:54.000000000
> > +0200
> > +++ libvirt-1.2.9/debian/README.Debian 2016-03-05 08:18:07.000000000
> > +0100
> > @@ -51,6 +51,18 @@
> > This makes dnsmasq only bind to the loopback interface by default so
> > libvirtd
> > can handle the virtual bridges.
> >
> > +Bridged network
> > +===============
> > +libvirt can use the qemu-bridge-helper to create bridged network
> > interfaces for
> > +session domains. For this to work the helper must have the capability to
> > create
> > +TUN/TAP devices or must have the SUID permission set.
> > +This can be done by running the following command as the user root:
> > +
> > + setcap cap_net_admin+ep /usr/lib/qemu/qemu-bridge-helper
> > +
> > +The allowed bridges must be configured in the file
> > '/etc/qemu/bridge.conf'. For
> > +each bridge add a line like 'allow br0'.
> > +
> > Access Control
> > ==============
> > Access to the libvirt managing tasks is controlled by PolicyKit. To ease
> > diff -Nru libvirt-1.2.9/debian/rules libvirt-1.2.9/debian/rules
> > --- libvirt-1.2.9/debian/rules 2015-08-24 16:20:54.000000000 +0200
> > +++ libvirt-1.2.9/debian/rules 2016-03-04 13:42:30.000000000 +0100
> > @@ -123,7 +123,7 @@
> > EXAMPLES_DIR =
> > $(CURDIR)/debian/libvirt-doc/usr/share/doc/libvirt-doc/examples/
> >
> > %:
> > - dh $@ --builddirectory=$(DEB_BUILDDIR) --parallel
> > + dh $@ --builddirectory=$(DEB_BUILDDIR) --parallel --with autoreconf
>
> That shouldn't be needed either.
>
> The two proposed fixes for: #808273 and #816602 make a lot of sense but
> I'm worried about the other changes.
So what about the attached debdiff instead that also fixes the autopkg
test in Jessie.
Cheers,
-- Guido
diff --git a/debian/README.Debian b/debian/README.Debian
index 0fa9358..0637b68 100644
--- a/debian/README.Debian
+++ b/debian/README.Debian
@@ -51,6 +51,18 @@ EOF
This makes dnsmasq only bind to the loopback interface by default so libvirtd
can handle the virtual bridges.
+Bridged network
+===============
+libvirt can use the qemu-bridge-helper to create bridged network interfaces for
+session domains. For this to work the helper must have the capability to create
+TUN/TAP devices or must have the SUID permission set.
+This can be done by running the following command as the user root:
+
+ setcap cap_net_admin+ep /usr/lib/qemu/qemu-bridge-helper
+
+The allowed bridges must be configured in the file '/etc/qemu/bridge.conf'. For
+each bridge add a line like 'allow br0'.
+
Access Control
==============
Access to the libvirt managing tasks is controlled by PolicyKit. To ease
diff --git a/debian/changelog b/debian/changelog
index 36eabe4..4d357fe 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,16 @@
+libvirt (1.2.9-9+deb8u2) jessie; urgency=medium
+
+ [ Philipp Hahn ]
+ * [16e52e6] CVE-2015-5313: Don't allow allow '/' in filesystem volume
+ (Closes: #808273)
+ * [e69dd73] libvirt-daemon: Expect qemu-bridge-helper in /usr/lib/qemu
+ like we fixed #790935 in sid. (Closes: #816602)
+
+ [ Guido Günther ]
+ * [72db643] Allow autopkg tests to print to stderr
+
+ -- Guido Günther <a...@sigxcpu.org> Fri, 25 Mar 2016 11:19:45 +0100
+
libvirt (1.2.9-9+deb8u1) jessie; urgency=medium
[ Guido Günther ]
@@ -8,7 +21,7 @@ libvirt (1.2.9-9+deb8u1) jessie; urgency=medium
* [c830a54] Disable test suite due to libxml2 bug #781232 in jessie
* [be70aec] Fix crash on live migration
this supplements 07dbec0a64783f644854a22aa0355720f0328d17.
- Thanks to Eckebrecht von Pappenheim (Closes: #7788171)
+ Thanks to Eckebrecht von Pappenheim (Closes: #788171)
[ Felix Geyer ]
* [9fb6c59] Allow access to libnl-3 configuration (Closes: #786652)
diff --git a/debian/patches/debian/Debianize-bridge-helper-path.patch b/debian/patches/debian/Debianize-bridge-helper-path.patch
new file mode 100644
index 0000000..689741e
--- /dev/null
+++ b/debian/patches/debian/Debianize-bridge-helper-path.patch
@@ -0,0 +1,42 @@
+libvirt-daemon: Expects qemu-bridge-helper in /usr/libexec/
+
+$ strings /usr/lib/libvirt/connection-driver/libvirt_driver_qemu.so | grep bridge-helper
+/usr/libexec/qemu-bridge-helper
+
+$ dpkg -S bridge-helper
+qemu-system-common: /usr/lib/qemu/qemu-bridge-helper
+
+Closes #816602
+--- a/src/qemu/qemu.conf
++++ b/src/qemu/qemu.conf
+@@ -357,7 +357,7 @@
+ # is used to create <source type='bridge'> interfaces when libvirtd is
+ # running unprivileged. libvirt invokes the helper directly, instead
+ # of using "-netdev bridge", for security reasons.
+-#bridge_helper = "/usr/libexec/qemu-bridge-helper"
++#bridge_helper = "/usr/lib/qemu/qemu-bridge-helper"
+
+
+
+--- a/src/qemu/qemu_conf.c
++++ b/src/qemu/qemu_conf.c
+@@ -244,7 +244,7 @@ virQEMUDriverConfigPtr virQEMUDriverConf
+ goto error;
+ }
+
+- if (VIR_STRDUP(cfg->bridgeHelperName, "/usr/libexec/qemu-bridge-helper") < 0)
++ if (VIR_STRDUP(cfg->bridgeHelperName, "/usr/lib/qemu/qemu-bridge-helper") < 0)
+ goto error;
+
+ cfg->clearEmulatorCapabilities = true;
+--- a/src/qemu/test_libvirtd_qemu.aug.in
++++ b/src/qemu/test_libvirtd_qemu.aug.in
+@@ -56,7 +56,7 @@ module Test_libvirtd_qemu =
+ { "auto_dump_bypass_cache" = "0" }
+ { "auto_start_bypass_cache" = "0" }
+ { "hugetlbfs_mount" = "/dev/hugepages" }
+-{ "bridge_helper" = "/usr/libexec/qemu-bridge-helper" }
++{ "bridge_helper" = "/usr/lib/qemu/qemu-bridge-helper" }
+ { "clear_emulator_capabilities" = "1" }
+ { "set_process_name" = "1" }
+ { "max_processes" = "0" }
diff --git a/debian/patches/security/CVE-2015-5313-storage-don-t-allow-in-filesystem-volu.patch b/debian/patches/security/CVE-2015-5313-storage-don-t-allow-in-filesystem-volu.patch
new file mode 100644
index 0000000..90e9610
--- /dev/null
+++ b/debian/patches/security/CVE-2015-5313-storage-don-t-allow-in-filesystem-volu.patch
@@ -0,0 +1,72 @@
+From 034e47c338b13a95cf02106a3af912c1c5f818d7 Mon Sep 17 00:00:00 2001
+Message-Id: <034e47c338b13a95cf02106a3af912c1c5f818d7.1457088964.git.h...@univention.de>
+From: Eric Blake <ebl...@redhat.com>
+Date: Tue, 8 Dec 2015 17:46:31 -0700
+Subject: [PATCH] CVE-2015-5313: storage: don't allow '/' in filesystem volume
+ names
+Organization: Univention GmbH, Bremen, Germany
+To: libvir-l...@redhat.com
+
+The libvirt file system storage driver determines what file to
+act on by concatenating the pool location with the volume name.
+If a user is able to pick names like "../../../etc/passwd", then
+they can escape the bounds of the pool. For that matter,
+virStoragePoolListVolumes() doesn't descend into subdirectories,
+so a user really shouldn't use a name with a slash.
+
+Normally, only privileged users can coerce libvirt into creating
+or opening existing files using the virStorageVol APIs; and such
+users already have full privilege to create any domain XML (so it
+is not an escalation of privilege). But in the case of
+fine-grained ACLs, it is feasible that a user can be granted
+storage_vol:create but not domain:write, and it violates
+assumptions if such a user can abuse libvirt to access files
+outside of the storage pool.
+
+Therefore, prevent all use of volume names that contain "/",
+whether or not such a name is actually attempting to escape the
+pool.
+
+This changes things from:
+
+$ virsh vol-create-as default ../../../../../../etc/haha --capacity 128
+Vol ../../../../../../etc/haha created
+$ rm /etc/haha
+
+to:
+
+$ virsh vol-create-as default ../../../../../../etc/haha --capacity 128
+error: Failed to create vol ../../../../../../etc/haha
+error: Requested operation is not valid: volume name '../../../../../../etc/haha' cannot contain '/'
+
+Signed-off-by: Eric Blake <ebl...@redhat.com>
+---
+ src/storage/storage_backend_fs.c | 10 +++++++++-
+ 1 file changed, 9 insertions(+), 1 deletion(-)
+
+--- a/src/storage/storage_backend_fs.c
++++ b/src/storage/storage_backend_fs.c
+@@ -1,7 +1,7 @@
+ /*
+ * storage_backend_fs.c: storage backend for FS and directory handling
+ *
+- * Copyright (C) 2007-2014 Red Hat, Inc.
++ * Copyright (C) 2007-2015 Red Hat, Inc.
+ * Copyright (C) 2007-2008 Daniel P. Berrange
+ *
+ * This library is free software; you can redistribute it and/or
+@@ -1005,6 +1005,14 @@ virStorageBackendFileSystemVolCreate(vir
+
+ vol->type = VIR_STORAGE_VOL_FILE;
+
++ /* Volumes within a directory pools are not recursive; do not
++ * allow escape to ../ or a subdir */
++ if (strchr(vol->name, '/')) {
++ virReportError(VIR_ERR_OPERATION_INVALID,
++ _("volume name '%s' cannot contain '/'"), vol->name);
++ return -1;
++ }
++
+ VIR_FREE(vol->target.path);
+ if (virAsprintf(&vol->target.path, "%s/%s",
+ pool->def->target.path,
diff --git a/debian/patches/series b/debian/patches/series
index bac1f34..7651164 100644
--- a/debian/patches/series
+++ b/debian/patches/series
@@ -31,3 +31,5 @@ upstream/Teach-virt-aa-helper-to-use-TEMPLATE.qemu-if-the-dom.patch
Allow-access-to-libnl-3-config-files.patch
Fix-crash-on-live-migration.patch
upstream/Report-original-error-when-QMP-probing-fails-with-ne.patch
+security/CVE-2015-5313-storage-don-t-allow-in-filesystem-volu.patch
+debian/Debianize-bridge-helper-path.patch
diff --git a/debian/tests/control b/debian/tests/control
index 5794aa8..353dac6 100644
--- a/debian/tests/control
+++ b/debian/tests/control
@@ -1,2 +1,3 @@
Tests: smoke
Depends: @
+Restrictions: allow-stderr
