virt-aa-helper should resolve symlinks and use only resolved paths

[Christian Ehrhardt ]

I happened to run into issues with virt-aa-helper.
The behavior is the same since essentially forever, but we could improve on it.

TL;DR
if you use a path like
    <interface type='vhostuser'>
      <source type='unix' path='/var/run/vhostuserclient/vhost-user-client-1' mode='server'/

Then virt-aa-helper will be kind and generate a rule for it to allow access.

But in some cases like in this this isn't sufficient, as there can be symlinks
 /var/run -> /run

But to avoid attacks via symlinks apparmor resolves them before matching.

That way the above will be checked against:
  /run/vhostuserclient/vhost-user-client-1

And due to that fail.

virt-aa-helper should on adding a path resolve all symlinks in said path and use the final path for the rules.

Can be tested with symlinks for the image files as well, which should be easier.

[Christian Ehrhardt  (paelzer)]

Test notes:

test file:
<domain type='kvm'>
    <name>symlink-test</name>
    <uuid>deadbeef-dead-beef-dead-beefdeadbeef</uuid>
    <memory unit='KiB'>1048576</memory>
    <vcpu>1</vcpu>
    <os>
        <type arch='x86_64'>hvm</type>
        <boot dev='hd'/>
    </os>
    <devices>
        <disk type='file' device='disk'>
            <driver name='qemu' type='qcow2'/>
            <source file='/var/run/symlinkdisk'/>
            <target dev='hda' bus='ide'/>
            <address type='drive' controller='0' bus='0' target='0' unit='0'/>
        </disk>
        <interface type='vhostuser'>
            <model type='virtio'/>
            <source type='unix' path='/var/run/symlinknet' mode='server'/>
        </interface>
        <channel type='unix'>
            <source mode='bind' path='/var/run/symlinksocket'/>
            <target type='virtio' name='org.qemu.guest_agent.0'/>
        </channel>
    </devices>
</domain>

And /var/run being a symlink to /run (as it is by default in Ubuntu)
$ readlink /var/run
/run

Without fix that creates:
$ ./src/virt-aa-helper -u libvirt-deadbeef-dead-beef-dead-beefdeadbeef -r --dryrun < /tmp/symlink-test.xml
/etc/apparmor.d/libvirt/libvirt-deadbeef-dead-beef-dead-beefdeadbeef.files
[...]
  "/var/run/symlinkdisk" rwk,
  "/var/run/symlinksocket" rw,
  "/var/run/symlinknet" rw,
[...]

None of the rules have any effect due to apparmor checking is vs /run/... (the resolved symlink).

[Christian Ehrhardt  (paelzer)]

Actually the call to realpath should already do that (but it doesn't)

[Christian Ehrhardt  (paelzer)]

Check why it doesn't
$ libtool --mode=execute gdb ./src/virt-aa-helper
(gdb) b vah_add_path
(gdb) run -u libvirt-deadbeef-dead-beef-dead-beefdeadbeef -r --dryrun < /tmp/symlink-test.xml
[...]
Breakpoint 1, vah_add_path (buf=0x7fffffffd500, path=0x55555578b000 "/var/run/symlinkdisk", perms=0x55555555ff53 "rwk", recursive=false)

It seems this fails:
  virFileExists(path)

Due to that realpath is never executed and the path is taken as-is.

Breakpoint 2, virFileExists (path=0x55555578b000 "/var/run/symlinkdisk") at util/virfile.c:1860
1860 return access(path, F_OK) == 0;
(gdb) finish
Value returned is $5 = false

Ok, now things make sense.
The file does not exist so it does not try to derive.
That also is the explanation why it wasn't an issue all the time (=it mostly worked)

But there are cases - like the vhostuser mode=server - where qemu is the one that has to CREATE the file.
So the check will fail, and the path not be translated.

[Christian Ehrhardt  (paelzer)]

Actually realpath can resolve non existing file, as long as it is on existing paths

$ ll /var/run/symlinkdisk /var/run/foobar/symlinkdisk
ls: cannot access '/var/run/symlinkdisk': No such file or directory
ls: cannot access '/var/run/foobar/symlinkdisk': No such file or directory

$ realpath /var/run/symlinkdisk /var/run/foobar/symlinkdisk
/run/symlinkdisk
realpath: /var/run/foobar/symlinkdisk: No such file or directory

So the virFileExists guard in the code can be weakened just a bit to make it work in general.
That also matches what qemu will do, it will create a socket or file, but would not mkdir.

[Christian Ehrhardt  (paelzer)]

Extended Testcase for existing and non-existing Files as well as non existing paths (can't be dereferenced).

<domain type='kvm'>
    <name>symlink-test</name>
    <uuid>deadbeef-dead-beef-dead-beefdeadbeef</uuid>
    <memory unit='KiB'>1048576</memory>
    <vcpu>1</vcpu>
    <os>
        <type arch='x86_64'>hvm</type>
        <boot dev='hd'/>
    </os>
    <devices>
        <disk type='file' device='disk'>
            <driver name='qemu' type='qcow2'/>
            <source file='/var/run/symlinkdisk-doesexist'/>
            <target dev='hda' bus='ide'/>
        </disk>
        <disk type='file' device='disk'>
            <driver name='qemu' type='qcow2'/>
            <source file='/var/run/symlinkdisk-doesnotexist'/>
            <target dev='hdb' bus='ide'/>
        </disk>
        <interface type='vhostuser'>
            <model type='virtio'/>
            <source type='unix' path='/var/run/symlinknet-doesexist' mode='server'/>
        </interface>
        <interface type='vhostuser'>
            <model type='virtio'/>
            <source type='unix' path='/var/run/symlinknet-doesnotexist' mode='server'/>
        </interface>
        <channel type='unix'>
            <source mode='bind' path='/var/run/symlinksocket-doesexist'/>
            <target type='virtio' name='org.qemu.guest_agent.1'/>
        </channel>
        <channel type='unix'>
            <source mode='bind' path='/var/run/symlinksocket-doesnotexist'/>
            <target type='virtio' name='org.qemu.guest_agent.0'/>
        </channel>
        <channel type='unix'>
            <source mode='bind' path='/var/run/pathdoesnotexist/symlinksocket'/>
            <target type='virtio' name='org.qemu.guest_agent.0'/>
        </channel>
    </devices>
</domain>

[Christian Ehrhardt  (paelzer)]

virt-aa-helper right now:
  "/run/symlinkdisk-doesexist" rwk,
  "/var/run/symlinkdisk-doesnotexist" rwk,
  "/run/symlinksocket-doesexist" rw,
  "/var/run/symlinksocket-doesnotexist" rw,
  "/var/run/pathdoesnotexist/symlinksocket" rw,
  "/run/symlinknet-doesexist" rw,
  "/var/run/symlinknet-doesnotexist" rw,

With fix:
  "/run/symlinkdisk-doesexist" rwk,
  "/run/symlinkdisk-doesnotexist" rwk,
  "/run/symlinksocket-doesexist" rw,
  "/run/symlinksocket-doesnotexist" rw,
  "/var/run/pathdoesnotexist/symlinksocket" rw,
  "/run/symlinknet-doesexist" rw,
  "/run/symlinknet-doesnotexist" rw,

That is already much better.
I wonder if instead splitting dir/file we should walk the path and resolve the longest existing path and append all not yet existing path parts.

[Christian Ehrhardt  (paelzer)]

Implemented the walking code.
Also adding this as another case (that can't be translated / accessed):
        <interface type='vhostuser'>
            <model type='virtio'/>
            <source type='unix' path='/nothing/of/this/exists' mode='server'/>
        </interface>

Generates:
  "/run/symlinkdisk-doesexist" rwk,
  "/run/symlinkdisk-doesnotexist" rwk,
  "/run/symlinksocket-doesexist" rw,
  "/run/symlinksocket-doesnotexist" rw,
  "/run/pathdoesnotexist/symlinksocket" rw,
  "/run/symlinknet-doesexist" rw,
  "/run/symlinknet-doesnotexist" rw,
  "/nothing/of/this/exists" rw,

Which is exactly what we want.

Will look at syntax/style checks after lunch...

[Christian Ehrhardt  (paelzer)]

Patch up for review - https://www.redhat.com/archives/libvir-list/2018-March/msg00022.html

[Christian Ehrhardt  (paelzer)]

Pushed after slight changes due to upstream feedback.

[Launchpad Janitor (janitor)]

This bug was fixed in the package libvirt - 4.0.0-1ubuntu5

---------------
libvirt (4.0.0-1ubuntu5) bionic; urgency=medium

  * run dnsmasq as libvirt-dnsmasq (LP: #1743718)
    - d/libvirt-daemon-system.postinst: add libvirt-dnsmasq user and group
    - d/libvirt-daemon-system.postrm: remove libvirt-dnsmasq user and group on
      purge
    - d/p/ubuntu/dnsmasq-as-priv-user: write dnsmas config with user
      libvirt-dnsmasq and adapt the self tests to expect that config
    - d/libvirt-daemon-system.postinst: fix old libvirt-dnsmasq users
  * Backport from recent upstream to stabilize libvirt (LP: #1754352)
    - d/p/stable/0024-qemu-blockcopy-Add-check-for-bandwidth.patch
    - d/p/stable/0025-conf-move-generated-member-from-virMacAddr-to-virDom.patch
    - d/p/stable/0026-lxc-Drop-useless-check-in-live-device-update.patch
    - d/p/stable/0027-Pass-oldDev-to-virDomainDefCompatibleDevice-on-devic.patch
    - d/p/stable/0028-qemu-Fix-updating-device-with-boot-order.patch
    - d/p/stable/0030-daemon-fix-rpc-event-leak-on-error-path-in-remoteDis.patch
    - d/p/stable/0029-lxc-fix-rpc-event-leak-on-error-path-in-virLXCContro.patch
    - d/p/stable/0031-qemu-fix-memory-leak-of-vporttype-during-migration.patch
    - d/p/stable/0032-virsh-fixing-segfault-by-pool-autocompleter-function.patch
  * d/p/ubuntu-aa/0041-apparmor-add-ro-rule-for-sasl-GSSAPI-
    plugin-on-etc-g.patch fix issues if sasl is configured (LP: #1696471)
  * d/p/ubuntu-aa/0042-virt-aa-helper-resolve-yet-to-be-created-paths.patch
    ensure symlinks are resolved to get valid rules if interim parts of a path
    are a symlink (LP: #1752361)

 -- Christian Ehrhardt <email address hidden> Tue, 27 Feb 2018 12:04:02 +0100