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

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 ...

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

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

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

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.

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

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 ...

Bionic to Disco are affected - adding tasks

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 :-)

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 ...

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.

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!

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.

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.

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

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 ...

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.

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

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

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.

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.