Hi Adam,
Thanks for your careful review.
On 08/11/2012 04:41 AM, Adam D. Barratt wrote:
> On Fri, 2012-08-10 at 14:25 +0800, Thomas Goirand wrote:
>> Please unblock the nova package. This fixes CVE-2012-3447, which is a
>> file injection vulnerability in the host filesystem, using a specially
>> crafted guest image.
>>
>> The relevant diff is available here:
>> http://anonscm.debian.org/gitweb/?p=openstack/nova.git;a=commitdiff;h=55e78f9cbaa1c4657a97c6b20797a94968030e75
>
> Please don't do that. It needs a context switch, doesn't work when
> reading mail offline and means that the list archive doesn't stand alone
> as a historical, well, archive of what was okayed. There's a reason
> that the freeze policy explicitly asks for debdiffs.
I'm sorry, I wont do it again. I've attached the corresponding diff file
for this unblock request.
>> The patch comes directly from upstream, as per the patch header (I just
>> applied it manually, then did dpkg-source --commit).
>>
>> Note that this also includes a (needed) tweak in the configuration files
>> as per this commit:
>> http://anonscm.debian.org/gitweb/?p=openstack/nova.git;a=commitdiff;h=4cd725c5d164484a3ddb6bf95f37fb715cb51169
>
> Two questions:
>
> 1) Why is there no mention of the above changes in the changelog?
>
> 2) Why does "Add nova-compute.conf files to nova-compute init if exist"
> require
>
> -DAEMON_ARGS="--flagfile=/etc/nova/nova.conf"
> +DAEMON_ARGS="--config-file=/etc/nova/nova.conf"
>
> and a bunch of
>
> +[DEFAULT]
>
> ?
What happened is that CVE-2012-3447 was embargoed. Ghe Rivero asked me
to take care of it, and upload the patch on the 7th of July, since he
was planing on going in holidays at that time. I'm not sure until when
he is away, he didn't send a mail to -private and I didn't ask him until
when he would go away. Ghe could you send a [VAC] message next time, please?
So I did take care of it, and was expecting to see no change in our Git.
So I did add the upstream patch for this CVE, built, then uploaded to SID.
But I was wrong, as Ghe did this commit, and didn't tell about it. He
didn't fill debian/changelog, which is why I didn't notice it either.
I hate pointing fingers at people, but here, I don't think I'm the one
to blame.
Anyway, let me explain what I believe this patch does. Previously, we
had only a single configuration file, called /etc/nova/nova.conf. But we
changed that, and we are now using /etc/nova/nova-compute.conf also,
which has hypervisor specific flags (for example, nova-compute-kvm will
have libvirt_type=kvm when nova-compute-xen will have
connection_type=xenapi).
So the important bit isn't:
-DAEMON_ARGS="--flagfile=/etc/nova/nova.conf"
+DAEMON_ARGS="--config-file=/etc/nova/nova.conf"
but this:
+test -f '/etc/nova/nova-compute.conf' && DAEMON_ARGS=${DAEMON_ARGS}"
--config-file=/etc/nova/nova-compute.conf"
which is necessary so that our new configuration files are used.
I believe that using --flagfile or --config-file does the exact same
thing. --flagfile was the old option, which has been replaced by
--config-file (and --flagfile is now deprecated). It's a good thing to
do that, so that it matches future releases of Openstack nova.
As for the [default] thing, I don't think that changes much anything,
and to be honest, I'm not really sure why Ghe has added this.
Unfortunately, it's impossible for me to ask him right away now.
Also, it seem to me that it's missing a [default] tag in
debian/nova-compute-xen.conf.dist (that one is only stored in
/usr/share/doc/nova-compute-xen, which is why it has a .dist extension
in the debian folder: /etc/nova/nova-compute.conf is maintained using
debconf in the case of nova-compute-xen). So if that has been forgotten
and is 100% necessary, then we will need to upload a fix and ask for
another unblock later on, I believe.
So, to Ghe, could you, in the future:
1/ Document your changes in debian/changelog *at the same time* as you
commit the rest of in our Git?
2/ Try to limit your changes, since we are frozen, or at least talk
about it in our Alioth list, so that I'm not in an uncomfortable
position like now? Was the addition of the [default] thing completely
necessary?
Anyway, I'm deeply concerned about this CVE. A lot more than these small
changes in the configuration files. I believe it is necessary to
unblock, even if I can't comment as much as I should on the above
changes. Holding the package to enter testing can be harmful to some users.
>> Also, Ubuntu folks already fixed the issue in 12.04.
>
> How is that at all relevant to the Debian freeze?
This isn't relevant to the freeze, but to the patch for CVE-2012-3447.
I'm just saying that it has been applied in 12.04 and that no user
complained about its accuracy, which is reassuring of the quality of the
patch. Sorry if I didn't make it clear enough.
One last thing: in our Git, I have already a debian/po/es.po update. I
didn't upload the package with it, because of the urgency=high. Was this
the correct thing to do (eg: plan for a later upload then unblock), or
should I have include the template update? Please give me the release
team view on this, so I know how to handle such situation later on.
Also, is it ok to amend the debian/changelog for this release (eg:
2012.1.1-6) on the next upload?
Cheers,
Thomas Goirand (zigo)
diff --git a/debian/changelog b/debian/changelog
index b700e89..dbe0464 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,9 @@
+nova (2012.1.1-6) unstable; urgency=high
+
+ * CVE-2012-3447: file injection writing to host filesystem (Closes: #684256).
+
+ -- Thomas Goirand <z...@debian.org> Tue, 07 Aug 2012 05:12:35 +0000
+
nova (2012.1.1-5) unstable; urgency=high
* Fix CVE-2012-3371. Closes: #681301
diff --git a/debian/nova-compute-kvm.conf b/debian/nova-compute-kvm.conf
index 732e89e..51b91e4 100644
--- a/debian/nova-compute-kvm.conf
+++ b/debian/nova-compute-kvm.conf
@@ -1,2 +1,3 @@
+[DEFAULT]
connection_type=libvirt
libvirt_type=kvm
diff --git a/debian/nova-compute-lxc.conf b/debian/nova-compute-lxc.conf
index 044fad3..9aac5ee 100644
--- a/debian/nova-compute-lxc.conf
+++ b/debian/nova-compute-lxc.conf
@@ -1,2 +1,3 @@
+[DEFAULT]
connection_type=libvirt
libvirt_type=lxc
diff --git a/debian/nova-compute-qemu.conf b/debian/nova-compute-qemu.conf
index 7e969c1..8907367 100644
--- a/debian/nova-compute-qemu.conf
+++ b/debian/nova-compute-qemu.conf
@@ -1,2 +1,3 @@
+[DEFAULT]
connection_type=libvirt
libvirt_type=qemu
diff --git a/debian/nova-compute-uml.conf b/debian/nova-compute-uml.conf
index 7529d87..5bcfa19 100644
--- a/debian/nova-compute-uml.conf
+++ b/debian/nova-compute-uml.conf
@@ -1,2 +1,3 @@
+[DEFAULT]
connection_type=libvirt
libvirt_type=uml
diff --git a/debian/nova-compute.init b/debian/nova-compute.init
index 1eca0a4..0956d39 100644
--- a/debian/nova-compute.init
+++ b/debian/nova-compute.init
@@ -19,7 +19,7 @@ PATH=/sbin:/usr/sbin:/bin:/usr/bin
DESC="OpenStack Compute"
NAME=nova-compute
DAEMON=/usr/bin/nova-compute
-DAEMON_ARGS="--flagfile=/etc/nova/nova.conf"
+DAEMON_ARGS="--config-file=/etc/nova/nova.conf"
PIDFILE=/var/run/$NAME.pid
SCRIPTNAME=/etc/init.d/$NAME
NOVA_USER=nova
@@ -34,6 +34,8 @@ chown ${NOVA_USER} ${LOCK_DIR}
. /lib/lsb/init-functions
+test -f '/etc/nova/nova-compute.conf' && DAEMON_ARGS=${DAEMON_ARGS}" --config-file=/etc/nova/nova-compute.conf"
+
# Read configuration variable file if it is present
if [ -s $DEFAULTS_FILE ]; then
. $DEFAULTS_FILE
diff --git a/debian/patches/CVE-2012-3447_compute-node-file-injection.patch b/debian/patches/CVE-2012-3447_compute-node-file-injection.patch
new file mode 100644
index 0000000..ad354c1
--- /dev/null
+++ b/debian/patches/CVE-2012-3447_compute-node-file-injection.patch
@@ -0,0 +1,82 @@
+Description: Prohibit file injection writing to host filesystem
+ This is a refinement of the previous fix in commit 2427d4a,
+ which does the file name canonicalization as the root user.
+ This is required so that guest images could not for example,
+ protect malicious symlinks in a directory only readable by root.
+Author: A1draig Brady <pbr...@redhat.com>
+Bug-Debian: http://bugs.debian.org/684256
+Origin: https://github.com/openstack/nova/commit/d9577ce9f266166a297488445b5b0c93c1ddb368
+Bug: 1031311, CVE-2012-3447
+Bug-Ubuntu: https://launchpad.net/bugs/1031311
+Last-Update: 2012-08-07
+
+--- nova-2012.1.1.orig/nova/tests/test_virt.py
++++ nova-2012.1.1/nova/tests/test_virt.py
+@@ -18,6 +18,7 @@
+ from nova import exception
+ from nova import flags
+ from nova import test
++from nova import utils
+ from nova.virt.disk import api as disk_api
+ from nova.virt import driver
+
+@@ -86,6 +87,17 @@ class TestVirtDriver(test.TestCase):
+
+
+ class TestVirtDisk(test.TestCase):
++ def setUp(self):
++ super(TestVirtDisk, self).setUp()
++
++ real_execute = utils.execute
++
++ def nonroot_execute(*cmd_parts, **kwargs):
++ kwargs.pop('run_as_root', None)
++ return real_execute(*cmd_parts, **kwargs)
++
++ self.stubs.Set(utils, 'execute', nonroot_execute)
++
+ def test_check_safe_path(self):
+ ret = disk_api._join_and_check_path_within_fs('/foo', 'etc',
+ 'something.conf')
+--- nova-2012.1.1.orig/nova/tests/test_xenapi.py
++++ nova-2012.1.1/nova/tests/test_xenapi.py
+@@ -597,9 +597,13 @@ class XenAPIVMTestCase(test.TestCase):
+ self._tee_executed = True
+ return '', ''
+
++ def _readlink_handler(cmd_parts, **kwargs):
++ return os.path.realpath(cmd_parts[2]), ''
++
+ fake_utils.fake_execute_set_repliers([
+ # Capture the tee .../etc/network/interfaces command
+ (r'tee.*interfaces', _tee_handler),
++ (r'readlink -nm.*', _readlink_handler),
+ ])
+ self._test_spawn(glance_stubs.FakeGlance.IMAGE_MACHINE,
+ glance_stubs.FakeGlance.IMAGE_KERNEL,
+--- nova-2012.1.1.orig/nova/rootwrap/compute.py
++++ nova-2012.1.1/nova/rootwrap/compute.py
+@@ -182,6 +182,10 @@ filterlist = [
+ # nova/virt/libvirt/utils.py: 'qemu-img'
+ filters.CommandFilter("/usr/bin/qemu-img", "root"),
+
++ # nova/virt/disk/api.py: 'readlink', '-e'
++ filters.CommandFilter("/usr/bin/readlink", "root"),
++ filters.CommandFilter("/bin/readlink", "root"),
++
+ # nova/virt/disk/api.py: 'touch', target
+ filters.CommandFilter("/usr/bin/touch", "root"),
+
+--- nova-2012.1.1.orig/nova/virt/disk/api.py
++++ nova-2012.1.1/nova/virt/disk/api.py
+@@ -314,7 +314,9 @@ def _join_and_check_path_within_fs(fs, *
+ mounted guest fs. Trying to be clever and specifying a
+ path with '..' in it will hit this safeguard.
+ '''
+- absolute_path = os.path.realpath(os.path.join(fs, *args))
++ absolute_path, _err = utils.execute('readlink', '-nm',
++ os.path.join(fs, *args),
++ run_as_root=True)
+ if not absolute_path.startswith(os.path.realpath(fs) + '/'):
+ raise exception.Invalid(_('injected file path not valid'))
+ return absolute_path
diff --git a/debian/patches/series b/debian/patches/series
index 2b207f6..b76e110 100644
--- a/debian/patches/series
+++ b/debian/patches/series
@@ -3,4 +3,5 @@ path-to-the-xenhost.conf-fixup.patch
CVE-2012-3360_CVE-2012-3361.patch
stable_essex_20120710.patch
iscsiadm_path.patch
-CVE-2012-3371.patch
\ No newline at end of file
+CVE-2012-3371.patch
+CVE-2012-3447_compute-node-file-injection.patch
