[apparmor] [patch 02/12] parser: Add support for unix domain socket rules.

John Johansen john.johansen at canonical.com
Tue Aug 26 11:57:45 UTC 2014


On 08/26/2014 02:44 AM, Tyler Hicks wrote:
> I've found a few issues with this patch through testing.
> 
>  1) It segfaults on bare unix rules ("unix,"). However, this is fixed
>     later in patch 12. I have not looked into the cause or the fix.
> 
right this is due to the permission mapping issues, its deliberately not
fixed here

>  2) It doesn't allow the policy author to only specify a permission. For
>     example you can't create a blanket rule for all AF_UNIX create
>     permissions ("unix create,"). The create operation is denied unless
>     you specify the type ("unix create type=stream,"). This can be
>     manually demonstrated using a simple profile to confine dbus-send
>     ("profile test { file, dbus, unix create, }") but I also hope to
>     have regression tests ready within the next day or two to test these
>     types of things.
> 
shoot, thanks for catching.

>  3) It messes up the stub rule creation and causes some rule types to
>     not be enforced. I'll point out where I think the problem area is
>     below and I'll also respond with a proposed fix.
> 
ouch, sorry about that

> On 2014-08-25 17:06:07, john.johansen at canonical.com wrote:
>> This patch implements parsing of fine grained mediation for unix domain
>> sockets, that have abstract and anonymous paths. Sockets with file
>> system paths are handled by regular file access rules.
> 
> <snip>
> 
>> --- 2.9-test.orig/parser/parser_regex.c
>> +++ 2.9-test/parser/parser_regex.c
>> @@ -665,14 +665,17 @@
>>  	return TRUE;
>>  }
>>  
>> -#define MAKE_STR(X) #X
>> -#define CLASS_STR(X) "\\d" MAKE_STR(X)
>> +#define MAKE_STR(A) #A
>> +#define CLASS_STR(X) "\\000\\d" MAKE_STR(X)
ugh!!!, wth????

this can straight up revert to what it was

>> +#define CLASS_SUB_STR(X, Y) MAKE_STR(X) MAKE_STR(Y)
> 
this should be the single byte class followed by the 2 byte AF in big endian.
So \000 followed by the single byte of the af, since af all fit in a single
byte currently.

Realistically we could collapse this to a single byte.

> This changes doesn't look correct and I've verified that it causes some
> rule types to not be enforced. I'll make a guess at what the magic byte
> sequence should be and respond with a tested patch. However, I'd like
> for you to verify that the byte sequence I picked is correct.
>

> Tyler
> 
>>  
>>  static const char *mediates_file = CLASS_STR(AA_CLASS_FILE);
>>  static const char *mediates_mount = CLASS_STR(AA_CLASS_MOUNT);
>>  static const char *mediates_dbus =  CLASS_STR(AA_CLASS_DBUS);
>>  static const char *mediates_signal =  CLASS_STR(AA_CLASS_SIGNAL);
>>  static const char *mediates_ptrace =  CLASS_STR(AA_CLASS_PTRACE);
>> +static const char *mediates_extended_net = CLASS_STR(AA_CLASS_NET);
>> +static const char *mediates_net_unix = CLASS_SUB_STR(AA_CLASS_NET, AF_UNIX);
>>  
>>  int process_profile_policydb(Profile *prof)
>>  {
>> @@ -689,7 +692,7 @@
>>  	 * to be supported
>>  	 */
>>  
>> -	/* note: this activates unix domain sockets mediation on connect */
>> +	/* note: this activates fs based unix domain sockets mediation on connect */
>>  	if (kernel_abi_version > 5 &&
>>  	    !prof->policy.rules->add_rule(mediates_file, 0, AA_MAY_READ, 0, dfaflags))
>>  		goto out;
>> @@ -705,6 +708,10 @@
>>  	if (kernel_supports_ptrace &&
>>  	    !prof->policy.rules->add_rule(mediates_ptrace, 0, AA_MAY_READ, 0, dfaflags))
>>  		goto out;
>> +	if (kernel_supports_unix &&
>> +	    (!prof->policy.rules->add_rule(mediates_extended_net, 0, AA_MAY_READ, 0, dfaflags) ||
>> +	     !prof->policy.rules->add_rule(mediates_net_unix, 0, AA_MAY_READ, 0, dfaflags)))
>> +		goto out;
>>  
>>  	if (prof->policy.rules->rule_count > 0) {
>>  		prof->policy.dfa = prof->policy.rules->create_dfa(&prof->policy.size, dfaflags);




More information about the AppArmor mailing list