[libvirt] [PATCH 2/3] Introduce readonly without explicit deny-write

[Serge Hallyn]

Quoting Guido Günther (agx at sigxcpu.org):
> On Fri, Mar 11, 2016 at 08:07:02PM +0000, Serge Hallyn wrote:
> > [Sorry, the Ubuntu package suggests this came from Cèdric, although
> > I can't quite find this patch on the mailing list.  Those patches
> > which I did see from Cèdric did not have a Signed-off-by, so I didn't
> > add one for him.]
> > 
> > From: Cèdric Bosdonnat <cbosdonnat at suse.com>
> > 
> > Upstream changed get_files to unconditionally be "rw" to allow audit
> > files to be written. Still under discussion but the proposed approach
> > is to have a way of saying readonly but do not implicitely create a
> > write deny rule.
> > 
> > ---
> >  Changelog: do not overwrite const memory, that leads to segv (serge)
> > ---
> >  src/security/virt-aa-helper.c | 23 +++++++++++++++++++----
> >  1 file changed, 19 insertions(+), 4 deletions(-)
> > 
> > diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
> > index b466626..34d08c8 100644
> > --- a/src/security/virt-aa-helper.c
> > +++ b/src/security/virt-aa-helper.c
> > @@ -785,12 +785,19 @@ get_definition(vahControl * ctl, const char *xmlStr)
> >      return rc;
> >  }
> >  
> > +/*
> > + * The permissions allowed are apparmor valid permissions and 'R'. 'R' stands
> > + * for read with no explicit deny rule.
> > + */
> >  static int
> > -vah_add_path(virBufferPtr buf, const char *path, const char *perms, bool recursive)
> > +vah_add_path(virBufferPtr buf, const char *path, const char *inperms, bool recursive)
> >  {
> >      char *tmp = NULL;
> >      int rc = -1;
> >      bool readonly = true;
> > +    bool explicit_deny_rule = true;
> > +    char *perms = strdupa(inperms);
> 
> This would be a newcomer, the rest of the code uses VIR_STRDUP. I'm not
> sure how hard this is being "enforced" at the moment.

Good point - that should be switched over, if Jamie is ok with the patch in
general.  (Note I'm out this week, I can update next week or I don't mind
if someone else take it over)

> > +    char *sub = NULL;
> >  
> >      if (path == NULL)
> >          return rc;
> > @@ -815,8 +822,16 @@ vah_add_path(virBufferPtr buf, const char *path, const char *perms, bool recursi
> >          return rc;
> >      }
> >  
> > -    if (strchr(perms, 'w') != NULL)
> > +    if (strchr(perms, 'w') != NULL) {
> >          readonly = false;
> > +        explicit_deny_rule = false;
> > +    }
> > +
> > +    if ((sub = strchr(perms, 'R')) != NULL) {
> > +        /* Don't write the invalid 'R' permission, replace with 'r' */
> > +        sub[0] = 'r';
> > +        explicit_deny_rule = false;
> > +    }
> >  
> >      rc = valid_path(tmp, readonly);
> >      if (rc != 0) {
> > @@ -831,7 +846,7 @@ vah_add_path(virBufferPtr buf, const char *path, const char *perms, bool recursi
> >          tmp[strlen(tmp) - 1] = '\0';
> >  
> >      virBufferAsprintf(buf, "  \"%s%s\" %s,\n", tmp, recursive ? "/**" : "", perms);
> > -    if (readonly) {
> > +    if (explicit_deny_rule) {
> >          virBufferAddLit(buf, "  # don't audit writes to readonly files\n");
> >          virBufferAsprintf(buf, "  deny \"%s%s\" w,\n", tmp, recursive ? "/**" : "");
> >      }
> > @@ -1130,7 +1145,7 @@ get_files(vahControl * ctl)
> >              /* 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)
> > +            if (vah_add_path(&buf, fs->src, fs->readonly ? "R" : "rw", true) != 0)
> >                  goto cleanup;
> >          }
> >      }
> > -- 
> 
> This otherwise looks good to me (as does the whole series) but Jamie
> probably should comment on it.
> Cheers,
>  -- Guido