libvirt allows spaces in vm names, there were issues in the past but it
seems not removed so the assumption has to be that spaces are continuing
to be allowed.

Therefore virt-aa-helper should not reject spaces in vm names anymore if
it is goign to be refused causing issues then the parser or xml schema
should do so.
Apparmor rules are in quotes, so a space in a path based on the name works.

Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
---
 src/security/virt-aa-helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
index d1518ea..5f4519d 100644
--- a/src/security/virt-aa-helper.c
+++ b/src/security/virt-aa-helper.c
@@ -449,7 +449,7 @@ valid_name(const char *name)
 {
     /* just try to filter out any dangerous characters in the name that can be
      * used to subvert the profile */
-    const char *bad = " /[]*";
+    const char *bad = "/[]*";
 
     if (strlen(name) == 0)
         return -1;
-- 
2.7.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Wed, 2017-09-20 at 16:59 +0200, Christian Ehrhardt wrote:
> libvirt allows spaces in vm names, there were issues in the past but
> it
> seems not removed so the assumption has to be that spaces are
> continuing
> to be allowed.
> 
> Therefore virt-aa-helper should not reject spaces in vm names anymore
> if
> it is goign to be refused causing issues then the parser or xml
> schema
> should do so.
> Apparmor rules are in quotes, so a space in a path based on the name
> works.
> 
> Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
> ---
>  src/security/virt-aa-helper.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-
> helper.c
> index d1518ea..5f4519d 100644
> --- a/src/security/virt-aa-helper.c
> +++ b/src/security/virt-aa-helper.c
> @@ -449,7 +449,7 @@ valid_name(const char *name)
>  {
>      /* just try to filter out any dangerous characters in the name
> that can be
>       * used to subvert the profile */
> -    const char *bad = " /[]*";
> +    const char *bad = "/[]*";
>  
>      if (strlen(name) == 0)
>          return -1;

Your justification seems reasonable. It does mean that we'll need
always quote rules that use def->name and looking at virt-aa-helper.c,
that seems to be the case.

All that said, I was surprised that tests/virt-aa-helper-test didn't
need to be updated, but, indeed, this is a testing gap.

+1 as is, but perhaps in a follow-up patch you could expand bad to be:

  const char *bad = "/[]{}?^,\"*";

'{', '}', '?', '^', ',' and '"' are characters used in AARE (see
'Globbing' in 'man apparmor.d') and add tests to tests/virt-aa-helper-
test for this.

Thanks!

-- 
Jamie Strandboge             | http://www.canonical.com--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Wed, Oct 25, 2017 at 8:48 PM, Jamie Strandboge <jamie@canonical.com>
wrote:

> On Wed, 2017-09-20 at 16:59 +0200, Christian Ehrhardt wrote:
> > libvirt allows spaces in vm names, there were issues in the past but
> > it
> > seems not removed so the assumption has to be that spaces are
> > continuing
> > to be allowed.
> >
> > Therefore virt-aa-helper should not reject spaces in vm names anymore
> > if
> > it is goign to be refused causing issues then the parser or xml
> > schema
> > should do so.
> > Apparmor rules are in quotes, so a space in a path based on the name
> > works.
> >
> > Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
> > ---
> >  src/security/virt-aa-helper.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-
> > helper.c
> > index d1518ea..5f4519d 100644
> > --- a/src/security/virt-aa-helper.c
> > +++ b/src/security/virt-aa-helper.c
> > @@ -449,7 +449,7 @@ valid_name(const char *name)
> >  {
> >      /* just try to filter out any dangerous characters in the name
> > that can be
> >       * used to subvert the profile */
> > -    const char *bad = " /[]*";
> > +    const char *bad = "/[]*";
> >
> >      if (strlen(name) == 0)
> >          return -1;
>
> Your justification seems reasonable. It does mean that we'll need
> always quote rules that use def->name and looking at virt-aa-helper.c,
> that seems to be the case.
>
> All that said, I was surprised that tests/virt-aa-helper-test didn't
> need to be updated, but, indeed, this is a testing gap.
>
> +1 as is, but perhaps in a follow-up patch you could expand bad to be:
>
>   const char *bad = "/[]{}?^,\"*";
>

Yep, makes sense.
Found a (minor) cleanup along the way - patches will follow shortly as new
thread (no need to splice it into this thread I think)


> '{', '}', '?', '^', ',' and '"' are characters used in AARE (see
> 'Globbing' in 'man apparmor.d') and add tests to tests/virt-aa-helper-
> test for this.
>
> Thanks!
>
> --
> Jamie Strandboge             | http://www.canonical.com




-- 
Christian Ehrhardt
Software Engineer, Ubuntu Server
Canonical Ltd
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list