fsfreeze-hook script is misplaced in qemu-guest-agent

[Pierre Gaxatte]

[Impact]

 * The feature of qemu-guest agent to run freeze hooks is used rarely but
   where used important to get data consistency right. Due to a path error
   when packaging the script (from upstream) it is currently not working
   at all (it calls a non existing path and even if you'd use the current
   path it would fail to find the potential drop-ins in its .d directory).

 * Without the fix users need to fix up things manually, but with it this
   would finally "just work". So move it to the place qemu-guest-agent
   expects the script.

[Test Case]

 * 1. check the systemd service that no options are set (the default)
      /lib/systemd/system/qemu-guest-agent.service
      It will look like:
        ExecStart=-/usr/sbin/qemu-ga
   2. without options set at #1 it will look for the script at the path:
      /etc/qemu/fsfreeze-hook
      => check that the executable script is at that path

   Before fix: it is a directory of the same name with the file in it
   After fix: location of the file is now as expected

Extended test (optional):
  If you set up qemu guest agent [1] then you can drop executable snippets
  in /etc/qemu/fsfreeze-hook.d
  Those have to be executed when you trigger them from the host e.g. via
    $ virsh domfsfreeze <domainname>

[1]: https://wiki.libvirt.org/page/Qemu_guest_agent

[Regression Potential]

 * If users have manually fixed this up in unexpected ways then placing it
   at the correct place might affect their fixup. Chances are high that
   they will then see at least a conffile prompt (chosen the new path) or
   that the fixup code in the maintainer scripts will take an early exit
   (old path not existing) - both cases are safe.
   In general qemu-guest-agent is rarely used and even if it is, then
   freeze hooks again only affect a sub-portion of these users. For those
   it is important to work, but even if it would be broken it will not
   affect a major share of our overall qemu users and use-cases.

[Other Info]

 * The fact that the new and required path of the file is in the old
   version a directory breaks some requirements of mv_conffile and makes
   this slightly more complex than one would think at first.

---

The fsfreeze-hook in the package is installed on the path /etc/qemu/fsfreeze-hook/fsfreeze-hook but the default path is /etc/qemu/fsfreeze-hook.

This means that to make it work, I had to:
- add "-F/etc/qemu/fsfreeze-hook/fsfreeze-hook" to the DAEMON_ARGS in /etc/default/qemu-guest-agent
- create a symlink /etc/qemu/fsfreeze-hook/fsfreeze-hook.d -> /etc/qemu/fsfreeze-hook.d to allow the fsfreeze-hook to find the scripts (it looks for them in FSFREEZE_D=$(dirname -- "$0")/fsfreeze-hook.d)

To fix this I propose to either move the fsfreeze-hook to /etc/qemu/fsfreeze-hook as qemu-guest-agent expects it or add "-F/etc/qemu/fsfreeze-hook/fsfreeze-hook" to the init script and change the fsfreeze-hook accordingly.

[Christian Ehrhardt  (paelzer)]

Thanks a lot for that bug report Pierre.
You are right and we want to solve that before Disco is released to not have to mess around too much with confile moving around between releases. (The rules are relaxed for not yet released Ubuntu releases and in this case it makes sense - usage will be very low and the file is usually unmodified).

Let me prep something ...

[Christian Ehrhardt  (paelzer)]

Current:

ll /etc/qemu/fsfreeze-hook/fsfreeze-hook
-rwxr-xr-x 1 root root 1274 Dec 11 17:44 /etc/qemu/fsfreeze-hook/fsfreeze-hook

Yeah that should only be
ll /etc/qemu/fsfreeze-hook/fsfreeze-hook
-rwxr-xr-x 1 root root 1274 Dec 11 17:44 /etc/qemu/fsfreeze-hook

[Christian Ehrhardt  (paelzer)]

/etc/qemu/fsfreeze-hook.d path is correct

[Christian Ehrhardt  (paelzer)]

Broken in Debian as well since they picked my change, I'll report the fix there too

[Christian Ehrhardt  (paelzer)]

The trivial update has troubles with the old path colliding.
Let modify that to use mv conffile - maybe I also need a tweak to properly handle the new path changing from a DIR to a FILE as being a conffile would not remove it by default on an upgrade.

[Christian Ehrhardt  (paelzer)]

Without such fixed it looks like this:
dpkg: warning: qemu-guest-agent: conffile '/etc/qemu/fsfreeze-hook' is not a plain file or symlink (= '/etc/qemu/fsfreeze-hook')

And then leaves a dpkg-new file:
ubuntu@disco:~$ ll /etc/qemu
total 20
drwxr-xr-x 4 root root 4096 Mar 18 09:34 ./
drwxr-xr-x 87 root root 4096 Mar 18 09:34 ../
drwxr-xr-x 2 root root 4096 Mar 18 09:34 fsfreeze-hook/
drwxr-xr-x 2 root root 4096 Feb 19 05:43 fsfreeze-hook.d/
-rwxr-xr-x 1 root root 1274 Dec 11 17:44 fsfreeze-hook.dpkg-new*

Using mv_conffile now but I'm afraid even that might not be enough, I started a discussion on IRC to poll for best practices for that specific case

[Christian Ehrhardt  (paelzer)]

It turns out the renaming is unnecessarily complex in this case for the reasons I outlined before.
Furthermore the change differs per release.

Lets approach this in a different way ...

[Christian Ehrhardt  (paelzer)]

Bionic to Disco are affected - adding tasks

[Christian Ehrhardt  (paelzer)]

Chances are that affected people have fixed it up in
  /etc/default/qemu-guest-agent
just as you have or in latter releases modified (the defaults file was dropped)
  /lib/systemd/system/qemu-guest-agent.service

Now exactly those (few) people that would have fixed the path up that way would be broken by an update moving the file.

While I admit the path is wrong, changing it might cause more pain than it would help.
So instead we should leave the path as is, but instead change the default config in a way that works.

That means adding the path that it has on disk to the default-config/systemd file.

I'm only 90% sure on this, so I beg your pardon if I make up my mind differently later on :-)

[Christian Ehrhardt  (paelzer)]

I went a bit back and forth on this, but after further checks I realized that due to the code expecting "fsfreeze-hook.d" to be below the path where the main script is located this has to be fixed by moving the script around.
Otherwise we'd also have to change the code of the initial fsfreeze-hook.

The special case of the new path being a DIR in the former version is some fun for the upgrade scripts, but hopefully we can resolve that ...

[Christian Ehrhardt  (paelzer)]

Test cases:
- no modification -> move silently (ok)
- modified old file -> move with modification (ok)
- old file removed -> old dir and non-existing file untouched, ...dpkg-new is placed (ok)

Ok that should work, I'll pass that (The Disco variant) to Team review.

[Pierre Gaxatte (pierre-gaxatte)]

Thank you Christian!
It looks good to me.

It seems like a lot of trouble to move a file in the place of the directory with the same name!

[Christian Ehrhardt  (paelzer)]

Yes it is Pierre, but that is the only way to do it right :-/

Review was great to make it more resilient, testing the new version (one more testcase) ...

- no modification -> move silently (ok)
- modified old file -> move with modification and place dpkg-new file (ok)
- old file removed -> old dir kept, new file not placed (to not enable what was menat to be disabled by removing before), dpkg-new file placed (ok)
- extra file placed in the old DIR -> behaves like the file removed case (ok)

We could think on fixing up the last case even more, but there is an arbitrary amount of even more special cases and we can't fix all.
The scripts are hardened to the extend that they won't fail/deny the upgrade which is important.
The user gets warnings if this happens to make him aware of the cleanup:
  rmdir: failed to remove '/etc/qemu/fsfreeze-hook': Directory not empty
  failed to remove /etc/qemu/fsfreeze-hook
  dpkg: warning: qemu-guest-agent: conffile '/etc/qemu/fsfreeze-hook' is not a plain file or symlink (= '/etc/qemu/fsfreeze-hook')

And then finally this didn't come up for ~12 months as an issue and we know it isn't th emost used feature. So we don't have to care for too arcane cases IMHO.

Overall the user visible messages improved as well with the last changes - e.g.:
"Preserving user changes to /etc/qemu/fsfreeze-hook (renamed from /etc/qemu/fsfreeze-hook/fsfreeze-hook)..." which makes more sense than what we had before.

[Christian Ehrhardt  (paelzer)]

Re-Review and tests are complete.
I have opened a PR in Debian as well to fix my mistake in all places where it is affecting users :-/
=> https://salsa.debian.org/qemu-team/qemu/merge_requests/5

I'll give Michael a chance to reply so that we can try to handle it the "same way" in Debian/Ubuntu. But OTOH tomorrow is the last day I can easily upload to Disco, so I'm likely going to do so then.

[Launchpad Janitor (janitor)]

This bug was fixed in the package qemu - 1:3.1+dfsg-2ubuntu3

---------------
qemu (1:3.1+dfsg-2ubuntu3) disco; urgency=medium

  * qemu-guest-agent: fix path of fsfreeze-hook (LP: #1820291)
    - d/qemu-guest-agent.install: use correct path for fsfreeze-hook
    - d/qemu-guest-agent.pre{rm|inst}/.postrm: special handling for
      mv_conffile since the new path is a directory in the old package
      version which can not be handled by mv_conffile.
  * i2c-ddc-fix-oob-read-CVE-2019-3812.patch fixes
    OOB read in hw/i2c/i2c-ddc.c which allows for memory disclosure.
    Closes: #922635 (Thanks to Gerd Hoffmann and Michael Tokarev)
    CVE-2019-3812

 -- Christian Ehrhardt <email address hidden> Mon, 18 Mar 2019 09:20:07 +0100

[Christian Ehrhardt  (paelzer)]

I have prepared a PPA and MPs for this to become an SRU.
I'm not yet sure if I'll upload this on its own, but OTOH there is an unaccepted SRU still waiting so it could be bundled for Bionic.

Adding SRU Template now ...

[Christian Ehrhardt  (paelzer)]

MPs are reviewed and this most likely goes in bundled with a security updated to not kill all that is in the SRU pipe already.

[Launchpad Janitor (janitor)]

This bug was fixed in the package qemu - 1:2.12+dfsg-3ubuntu8.6

---------------
qemu (1:2.12+dfsg-3ubuntu8.6) cosmic-security; urgency=medium

  [ Marc Deslauriers ]
  * SECURITY UPDATE: multiple pvrdma security issues
    - debian/patches/split_pvrdma.patch: split PVRDMA from RDMA in
      configure, hw/rdma/Makefile.objs.
    - debian/control*: completely disable pvrdma to fix security issues
    - CVE-2018-20123
    - CVE-2018-20124
    - CVE-2018-20125
    - CVE-2018-20126
    - CVE-2018-20191
    - CVE-2018-20216
  * SECURITY UPDATE: path traversal issue in MTP
    - debian/patches/CVE-2018-16867.patch: check for slash in
      hw/usb/dev-mtp.c.
    - CVE-2018-16867
  * SECURITY UPDATE: TOCTTOU in MTP
    - debian/patches/CVE-2018-16872.patch: use O_NOFOLLOW and O_CLOEXEC in
      hw/usb/dev-mtp.c.
    - CVE-2018-16872
  * SECURITY UPDATE: race during file renaming in v9fs_wstat
    - debian/patches/CVE-2018-19489.patch: add locks to hw/9pfs/9p.c.
    - CVE-2018-19489
  * SECURITY UPDATE: out-of-bounds read via i2 commands
    - debian/patches/CVE-2019-3812.patch: add bounds check to
      hw/i2c/i2c-ddc.c.
    - CVE-2019-3812
  * SECURITY UPDATE: heap based buffer overflow in slirp
    - debian/patches/CVE-2019-6778.patch: check data length while emulating
      ident function in slirp/tcp_subr.c.
    - CVE-2019-6778

  [ Christian Ehrhardt ]
  * qemu-guest-agent: fix path of fsfreeze-hook (LP: #1820291)
    - d/qemu-guest-agent.install: use correct path for fsfreeze-hook
    - d/qemu-guest-agent.pre{rm|inst}/.postrm: special handling for
      mv_conffile since the new path is a directory in the old package
      version which can not be handled by mv_conffile

 -- Marc Deslauriers <email address hidden> Mon, 25 Mar 2019 08:37:14 -0400

[Launchpad Janitor (janitor)]

This bug was fixed in the package qemu - 1:2.11+dfsg-1ubuntu7.12

---------------
qemu (1:2.11+dfsg-1ubuntu7.12) bionic-security; urgency=medium

  [ Marc Deslauriers ]
  * SECURITY UPDATE: TOCTTOU in MTP
    - debian/patches/CVE-2018-16872.patch: use O_NOFOLLOW and O_CLOEXEC in
      hw/usb/dev-mtp.c.
    - CVE-2018-16872
  * SECURITY UPDATE: race during file renaming in v9fs_wstat
    - debian/patches/CVE-2018-19489.patch: add locks to hw/9pfs/9p.c.
    - CVE-2018-19489
  * SECURITY UPDATE: out-of-bounds read via i2 commands
    - debian/patches/CVE-2019-3812.patch: add bounds check to
      hw/i2c/i2c-ddc.c.
    - CVE-2019-3812
  * SECURITY UPDATE: heap based buffer overflow in slirp
    - debian/patches/CVE-2019-6778.patch: check data length while emulating
      ident function in slirp/tcp_subr.c.
    - CVE-2019-6778

  [ Christian Ehrhardt ]
  * fix crash when performing block pull on partial cluster (LP: #1818264)
    - d/p/ubuntu/lp-1818264-block-Fix-copy-on-read-crash-with-partial.patch
  * qemu-guest-agent: fix path of fsfreeze-hook (LP: #1820291)
    - d/qemu-guest-agent.install: use correct path for fsfreeze-hook
    - d/qemu-guest-agent.pre{rm|inst}/.postrm: special handling for
      mv_conffile since the new path is a directory in the old package
      version which can not be handled by mv_conffile

 -- Marc Deslauriers <email address hidden> Mon, 25 Mar 2019 08:32:58 -0400

[Webflow (wflow)]

Hi,

just wanted to add: This bug also crashes unattended-upgrade and thus prevents security updates on 18.04:

root@mailin1:~# unattended-upgrade
Traceback (most recent call last):
  File "/usr/bin/unattended-upgrade", line 1998, in <module>
    sys.exit(main(options))
  File "/usr/bin/unattended-upgrade", line 1714, in main
    if conffile_prompt(item.destfile):
  File "/usr/bin/unattended-upgrade", line 929, in conffile_prompt
    with open(prefix + conf_file, 'rb') as fp:
IsADirectoryError: [Errno 21] Is a directory: '/etc/qemu/fsfreeze-hook'

Basically, unattended-upgrade wants to compare old/new conffiles and doesn't like it when the old conffile turns out to be a directory...

This prevents security updates to be installed. The unattended-upgrade logs do not contain the trace, only the messages:

2019-04-05 13:24:24,851 INFO Initial blacklisted packages:
2019-04-05 13:24:24,853 INFO Initial whitelisted packages:
2019-04-05 13:24:24,853 INFO Starting unattended upgrades script
2019-04-05 13:24:24,854 INFO Allowed origins are: o=Ubuntu,a=bionic, o=Ubuntu,a=bionic-security, o=UbuntuESM,a=bionic'

And that's it.

You can't really get out of this without manually running 'apt-get install qemu-guest-agent' because at this point, unattended-upgrade can't update itself anymore.

[Christian Ehrhardt  (paelzer)]

Arrr, this issue keeps on giving.
Thanks webflow for the hint, I'll have to look into that in a new bug - I filed bug 1823872 for that.

Please consider subscribing and chiming in there.