[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