[libvirt] [PATCH 2/3] virt-aa-helper: don't deny writes to readonly mounts

[Jamie Strandboge]

On 11/26/2015 02:18 AM, Cedric Bosdonnat wrote:
> Hi all,
> 
> Has that patch been skipped in the review process?
> 
> --
> Cedric
> 
> On Tue, 2015-11-17 at 15:14 +0100, Cédric Bosdonnat wrote:
>> There is no need to deny writes on a readonly mount: write still
>> won't be accepted, even if the user remounts the folder as RW in
>> the guest as qemu sets the 9p mount as ro.
>>
>> This deny rule was leading to problems for example with readonly /:
>> The qemu process had to write to a bunch of files in / like logs,
>> sockets, etc. This deny rule was also preventing auditing of these
>> denials, making it harder to debug.
>> ---
>>  src/security/virt-aa-helper.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
>> index 5de56e5..a2d7226 100644
>> --- a/src/security/virt-aa-helper.c
>> +++ b/src/security/virt-aa-helper.c
>> @@ -1127,7 +1127,10 @@ get_files(vahControl * ctl)
>>                  ctl->def->fss[i]->src) {
>>              virDomainFSDefPtr fs = ctl->def->fss[i];
>>  
>> -            if (vah_add_path(&buf, fs->src, fs->readonly ? "r" : "rw", true) != 0)
>> +            /* We don't need to add deny rw rules for readonly mounts,
>> +             * this can only lead to troubles when mounting / readonly.
>> +             */
>> +            if (vah_add_path(&buf, fs->src, "rw", true) != 0)
>>                  goto cleanup;
>>          }
>>      }

NAK (though it seems this was already committed-- I apologize I missed this when
it was initially discussed).

The logic before was if fs->readonly was set, add an 'r' rule (which happened to
also add a 'deny w' rule) and if fs->readonly is not set, add an 'rw' rule. This
patch unconditionally changes that logic and adds a 'rw' regardless of
fs->readonly which is wrong.

Instead, adjust vah_add_path() to conditionally not add the deny rule. One way
to do this is use 'R' for skipping the explicit deny write rule:

static int
vah_add_path(virBufferPtr buf, ...)
{
    ...
    /*bool readonly = true;*/
    bool explicit_deny_write = true;
    ...

    if (strchr(perms, 'w') != NULL || strchr(perms, 'R') != NULL)
        explicit_deny_write = false;

    ...

    /*if (readonly) {*/
    if (explicit_deny_write) {
        virBufferAddLit(buf, "  # don't audit writes to readonly files\n");
        virBufferAsprintf(buf, "  deny \"%s%s\" w,\n", tmp, recursive ? "/**" : "");
    }
    ...
}

...

static int
get_files(vahControl * ctl)
{
    ...
    /* We don't need explicit deny write rules for readonly mounts,
     * this can only lead to troubles when mounting / readonly.
     */
    if (vah_add_path(&buf, fs->src, fs->readonly ? "R" : "rw", true) != 0)
        goto cleanup;


I left out the bit about converting the 'R' in perms to 'r' before adding the
rule for clarity of approach but this would have to be done too since 'R' isn't
a valid permission in apparmor. Also, with this change it would be best to add a
comment before the vah_add_path() function to say what valid values of perms are
and that 'R' is special.

There are certainly other ways of conditionally adding the deny rule.

-- 
Jamie Strandboge                 http://www.ubuntu.com/

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20160115/a059de16/attachment-0001.sig>