[apparmor] query_label regression test failures

Mon Jul 13 06:55:53 UTC 2015

On 07/12/2015 08:36 PM, Tyler Hicks wrote:
> 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?
> 
It is only using the wrong mapping on the interface

>>
>>
>> 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?
> 
yes, and we will make sure to add some extra info to the exported
abi/features so this bug can be detected from user space and we can be
confident the 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 <tyhicks at canonical.com>
> 
> 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
>>