On 2015-07-11 12:04:37, John Johansen wrote:
> On 06/25/2015 11:55 AM, Tyler Hicks wrote:
> > On 2015-06-25 01:21:39, Steve Beattie wrote:
> >> Hi,
> >>
> >> When running the apparmor regression tests on wily with the trunk of
> >> the userspace tools, I'm getting the following two failures in the
> >> query_label test:
> >>
> >> Error: query_label failed. Test 'QUERY file (all base perms #1)' was 
> >> expected to 'pass'. Reason for failure 'FAIL: the access should not be 
> >> allowed and should be audited'
> >> Error: query_label failed. Test 'QUERY file (all base perms #2)' was 
> >> expected to 'pass'. Reason for failure 'FAIL: the access should not be 
> >> allowed and should be audited'
> > 
> > Note that the test passes when we run them against the wily apparmor
> > userspace (2.9.2-0ubuntu1). Seems to be something broken specifically in
> > trunk.
> > 
> So after further investigation there are a couple of problems.

Thanks for looking into this. I was lost and wasn't sure what the
solution was. :)

> 
> 1. The test is using the wrong defines: It is using the defines from the
> parser for the packed dfa permissions. This set of permissions is not
> meant to be exposed to the outside world
> 
> 2. The kernel is using the wrong mapping function for the permissions
> in the file class. This results in partially exposing the packed
> permissions, but even then it doesn't fully line up with the packed
> permissions, and is not correct for several of the potential permissions.

Is the kernel only using the wrong mapping function for behind the query
interface or is it using the wrong mapping function in other areas too?

> 
> 
> Attached is a patch that fixes the test, and moves the two tests that
> fail due to the kernel to xpass.

Can the 'xpass' tests be moved back to 'pass' once the kernel fix is in
place?

> 
> ---
> 
> === modified file 'tests/regression/apparmor/query_label.c'
> --- tests/regression/apparmor/query_label.c   2015-05-28 19:48:46 +0000
> +++ tests/regression/apparmor/query_label.c   2015-07-10 20:45:07 +0000
> @@ -35,28 +35,68 @@
>  #define AA_MAY_APPEND                (1 << 3)
>  #endif
>  
> +#ifndef AA_MAY_CREATE
> +#define AA_MAY_CREATE                (1 << 4)
> +#endif
> +
> +#ifndef AA_MAY_DELETE
> +#define AA_MAY_DELETE                (1 << 5)
> +#endif
> +
> +#ifndef AA_MAY_OPEN
> +#define AA_MAY_OPEN          (1 << 6)
> +#endif
> +
> +#ifndef AA_MAY_RENAME
> +#define AA_MAY_RENAME                (1 << 7)
> +#endif
> +
> +#ifndef AA_MAY_SETATTR
> +#define AA_MAY_SETATTR               (1 << 8)
> +#endif
> +
> +#ifndef AA_MAY_GETATTR
> +#define AA_MAY_GETATTR               (1 << 9)
> +#endif
> +
> +#ifndef AA_MAY_SETCRED
> +#define AA_MAY_SETCRED               (1 << 10)
> +#endif
> +
> +#ifndef AA_MAY_GETCRED
> +#define AA_MAY_GETCRED               (1 << 11)
> +#endif
> +
> +#ifndef AA_MAY_CHMOD
> +#define AA_MAY_CHMOD         (1 << 12)
> +#endif
> +
> +#ifndef AA_MAY_CHOWN
> +#define AA_MAY_CHOWN         (1 << 13)
> +#endif
> +
> +#ifndef AA_MAY_LCOK

There's a typo in the line above. _LCOK -> _LOCK

With that fixed,

Acked-by: Tyler Hicks <[email protected]>

Thanks again!

Tyler

> +#define AA_MAY_LOCK          0x8000
> +#endif
> +
> +#ifndef AA_EXEC_MMAP
> +#define AA_EXEC_MMAP         0x10000
> +#endif
> +
>  #ifndef AA_MAY_LINK
> -#define AA_MAY_LINK          (1 << 4)
> -#endif
> -
> -#ifndef AA_MAY_LOCK
> -#define AA_MAY_LOCK          (1 << 5)
> -#endif
> -
> -#ifndef AA_EXEC_MMAP
> -#define AA_EXEC_MMAP         (1 << 6)
> -#endif
> -
> -#ifndef AA_EXEC_PUX
> -#define AA_EXEC_PUX          (1 << 7)
> -#endif
> -
> -#ifndef AA_EXEC_UNSAFE
> -#define AA_EXEC_UNSAFE               (1 << 8)
> -#endif
> -
> -#ifndef AA_EXEC_INHERIT
> -#define AA_EXEC_INHERIT              (1 << 9)
> +#define AA_MAY_LINK          0x40000
> +#endif
> +
> +#ifndef AA_LINK_SUBSET               /* overlayed perm in pair */
> +#define AA_LINK_SUBSET               AA_MAY_LOCK
> +#endif
> +
> +#ifndef AA_MAY_ONEXEC
> +#define AA_MAY_ONEXEC                0x20000000
> +#endif
> +
> +#ifndef AA_MAY_CHANGE_PROFILE
> +#define AA_MAY_CHANGE_PROFILE        0x40000000
>  #endif
>  
>  static char *progname = NULL;
> @@ -148,18 +188,26 @@
>                       *mask |= AA_MAY_READ;
>               else if (!strcmp(perm, "append"))
>                       *mask |= AA_MAY_APPEND;
> +             else if (!strcmp(perm, "create"))
> +                     *mask |= AA_MAY_CREATE;
> +             else if (!strcmp(perm, "delete"))
> +                     *mask |= AA_MAY_DELETE;
> +             else if (!strcmp(perm, "setattr"))
> +                     *mask |= AA_MAY_SETATTR;
> +             else if (!strcmp(perm, "getattr"))
> +                     *mask |= AA_MAY_GETATTR;
> +             else if (!strcmp(perm, "chmod"))
> +                     *mask |= AA_MAY_CHMOD;
> +             else if (!strcmp(perm, "chown"))
> +                     *mask |= AA_MAY_CHOWN;
>               else if (!strcmp(perm, "link"))
>                       *mask |= AA_MAY_LINK;
>               else if (!strcmp(perm, "lock"))
>                       *mask |= AA_MAY_LOCK;
> +             else if (!strcmp(perm, "linksubset"))
> +                     *mask |= AA_LINK_SUBSET;
>               else if (!strcmp(perm, "exec_mmap"))
>                       *mask |= AA_EXEC_MMAP;
> -             else if (!strcmp(perm, "exec_pux"))
> -                     *mask |= AA_EXEC_PUX;
> -             else if (!strcmp(perm, "exec_unsafe"))
> -                     *mask |= AA_EXEC_UNSAFE;
> -             else if (!strcmp(perm, "exec_inherit"))
> -                     *mask |= AA_EXEC_INHERIT;
>               else {
>                       fprintf(stderr, "FAIL: unknown perm: %s\n", perm);
>                       return 1;
> @@ -264,8 +312,8 @@
>           (allowed == should_allow && audited == should_audit)) {
>               printf("PASS\n");
>       } else {
> -             fprintf(stderr, "FAIL: the access should %sbe allowed and 
> should %sbe audited\n",
> -                     allowed ? "" : "not ", audited ? "" : "not ");
> +             fprintf(stderr, "FAIL: the access should %sbe allowed and 
> should %sbe audited. mask 0x%x\n",
> +                     allowed ? "" : "not ", audited ? "" : "not ", mask);
>               exit(1);
>       }
>  
> 
> === modified file 'tests/regression/apparmor/query_label.sh'
> --- tests/regression/apparmor/query_label.sh  2015-05-28 19:48:53 +0000
> +++ tests/regression/apparmor/query_label.sh  2015-07-11 18:54:55 +0000
> @@ -212,9 +212,9 @@
>  
>  genqueryprofile "file,"
>  expect allow
> -perms file exec,write,read,append,link,lock
> -querytest "QUERY file (all base perms #1)" pass /anything
> -querytest "QUERY file (all base perms #2)" pass /everything
> +perms file 
> exec,write,read,append,create,delete,setattr,getattr,chmod,chown,link,linksubset,lock,exec_mmap
> +querytest "QUERY file (all base perms #1)" xpass /anything
> +querytest "QUERY file (all base perms #2)" xpass /everything
>  
>  genqueryprofile "/etc/passwd r,"
>  expect allow
> 

Attachment: signature.asc
Description: Digital signature

-- 
AppArmor mailing list
[email protected]
Modify settings or unsubscribe at: 
https://lists.ubuntu.com/mailman/listinfo/apparmor

Reply via email to