[apparmor] [PATCH] Add support for variable expansion in profile names, and attachments

Mon Jun 8 23:29:03 UTC 2015

On 06/08/2015 02:23 PM, Christian Boltz wrote:
> Hello,
> 
> Am Montag, 8. Juni 2015 schrieb John Johansen:
>> allow
>>   @{FOO}=bar
>>   /foo@{FOO} { }
>>
>> to be expanded into
>>   /foobar { }
>>
>> and
>>   @{FOO}=bar baz
>>   /foo@{FOO} { }
>>
>> to be expanded into
>>   /foo{bar,baz} { }
>> which is used as a regular expression for attachment purposes
> 
> Yes, it makes sense to have the variable-expanded name displayed as 
> profile name or attachment.
> 
> That said - profile authors should "hide" the variable in the attachment 
> and give the profile a nice name, so IMHO the recommended way for 
> profile names containing a variable would be
> 
>     profile foo /usr/@{multiarch}/foo
> 
> and not
> 
>     /usr/@{multiarch}/foo
>

sure I recommned all profiles do that, in fact I don't like having the name be
the attachment.  We really should be doing
  profile firefox /.... { }

for everything

>> Further allow variable expansion in attachment specifications
>>   profile foo /foo@{FOO} { }
>> profile name (if begun with profile keyword) and attachments to begin
>> with a variable
>>   profile @{FOO} { }
>>   profile /foo @{FOO} { }
>>   profile @{FOO} @{BAR} {}
>>
>> hats
>>   ^@{FOO}
>>   hat @{FOO}
>>
>> and for subprofiles as well
> 
> So basically everywhere. Makes sense.
> 
> 
>> diff --git a/parser/parser_variable.c b/parser/parser_variable.c
>> index ac334dc..7250c0b 100644
>> --- a/parser/parser_variable.c
>> +++ b/parser/parser_variable.c
>> @@ -275,12 +275,29 @@ static int process_variables_in_rules(Profile
>> &prof) return 0;
>>  }
>>
>> +static int process_variables_in_name(Profile &prof)
>> +{
>> +	/* this needs to be done before alias expansion, ie. altnames are
>> +	 * setup
>> +	 */
> 
> That reminds me - can you also apply this to file rules, please? 
> There is a quite old bug (still open IIRC) that breaks /{var/,}run/ with 
> my /var/ -> /home/sys-var/ symlink even if I have an alias for it.
> 
that is a different issue, and can only be solved in the backend.

Basically it needs the long delayed alias fixes I started on for Tails

> 
>> diff --git a/parser/parser_yacc.y b/parser/parser_yacc.y
>> index b3083d5..68bdede 100644
>> --- a/parser/parser_yacc.y
>> +++ b/parser/parser_yacc.y
> ...
>> +		if ($2 && !($2[0] == '/' || strncmp($2, "@{", 2) == 0))
>>  			yyerror(_("Profile attachment must begin with a '/'."));
> 
> An updated error message would be nice - "... with a '/' or a variable."
> 
ack

> 
>> --- /dev/null
>> +++ b/parser/tst/simple_tests/vars/vars_profile_name_12.sd
>> @@ -0,0 +1,9 @@
>> +#=DESCRIPTION profiles declared with the profile keyword can begin
>> with var +#=EXRESULT PASS
>> +
>> +@{FOO}=bar baz
>> +@{BAR}=baz foo
>> +
>> +profile /does/not/exist@{BAR} @{FOO} {
>> +  /does/not/exist r,
>> +}
> 
> As discussed on IRC: The attachment will expand to {bar,baz} - and 
> that's not a valid attachment (not starting with /), so this test should 
> fail.
> 
Nope, as discussed this needs to be fixed in a different patch.  And the
simple_tests don't have a way to encode an xfail so that the tests won't
fail while that patch is being worked on

Basically we go with PASS for now, and when the fix is done it will cause
this test to FAIL, and need to be patched at that point.

The only other options are fixing the tests to accept an xfail, sorry I
am not doing that atm, changing it to disabled, or dropping the test
which I would rather not do.

dropping the test means it will get lost, setting it to disabled means
it will get lost as well as it will be forgotten then the patch finally
lands, and not get updated.

We can add a comment so that when that patch lands and this starts
failing it is explained why

>> --- /dev/null
>> +++ b/parser/tst/simple_tests/vars/vars_profile_name_13.sd
>> @@ -0,0 +1,9 @@
>> +#=DESCRIPTION reference variables in rules that also have
>> alternations +#=EXRESULT PASS
>> +
>> +@{FOO}=bar
>> +@{BAR}=baz
>> +
>> +profile @{BAR} @{FOO} {
>> +  /does/not/exist r,
>> +}
> 
> That expands to "profile baz bar {", which again means an invalid 
> attachment (not starting with /) that should fail.
> 
nope expands to an alternation

profile {baz,bar} and we can't properly deal with the leading /
on alternations as a semantic check yet.

>> --- /dev/null
>> +++ b/parser/tst/simple_tests/vars/vars_profile_name_14.sd
>> @@ -0,0 +1,9 @@
>> +#=DESCRIPTION reference variables in rules that also have
>> alternations +#=EXRESULT PASS
>> +
>> +@{FOO}=bar baz
>> +@{BAR}=baz
>> +
>> +profile @{BAR} @{FOO} {
>> +  /does/not/exist r,
>> +}
> 
> Same here.
> 
>> --- /dev/null
>> +++ b/parser/tst/simple_tests/vars/vars_profile_name_15.sd
>> @@ -0,0 +1,9 @@
>> +#=DESCRIPTION profiles declared with the profile keyword can begin
>> with var +#=EXRESULT PASS
>> +
>> +@{FOO}=bar baz
>> +@{BAR}=baz foo
>> +
>> +profile @{BAR} @{FOO} {
>> +  /does/not/exist r,
>> +}
> 
> And here.
> 
>> --- /dev/null
>> +++ b/parser/tst/simple_tests/vars/vars_profile_name_7.sd
>> @@ -0,0 +1,8 @@
>> +#=DESCRIPTION profiles declared with the profile keyword can begin
>> with var +#=EXRESULT PASS
>> +
>> +@{FOO}=bar
>> +
>> +profile /does/not/exist @{FOO} {
>> +  /does/not/exist r,
>> +}
> 
> And here.
> 
>> diff --git a/parser/tst/simple_tests/vars/vars_profile_name_8.sd
>> b/parser/tst/simple_tests/vars/vars_profile_name_8.sd new file mode
>> 100644
>> index 0000000..433d355
>> --- /dev/null
>> +++ b/parser/tst/simple_tests/vars/vars_profile_name_8.sd
>> @@ -0,0 +1,8 @@
>> +#=DESCRIPTION profiles declared with the profile keyword can begin
>> with var +#=EXRESULT PASS
>> +
>> +@{FOO}=bar baz
>> +
>> +profile /does/not/exist @{FOO} {
>> +  /does/not/exist r,
>> +}
> 
> And once more.
> 
> Please change these tests to EXRESULT FAIL.
> 
No, as explained above

> From the discussion on IRC, I understand that checking if the expanded 
> attachment is valid is something for another patch, so a temporary 
> solution would be EXRESULT FAIL + #=DISABLED (which is still much better 
> than wrong EXRESULT PASS).
> 
again DISABLED means the test won't get updated

> BTW: For obvious reasons, all expanded attachment variants need to start 
> with /, so here's another test you should add:
> 
> 
> #=DESCRIPTION all attachment expansions must start with /
> #=EXRESULT FAIL
> 
> @{FOO}=/bar baz
> 
> profile /does/not/exist @{FOO} {
>   /does/not/exist r,
> }
> 
yep thanks

> 
>> --- /dev/null
>> +++ b/parser/tst/simple_tests/vars/vars_profile_name_bad_2.sd
>> @@ -0,0 +1,6 @@
>> +#=DESCRIPTION special @{profile_name} not defined for profile name
>> declaration +#=EXRESULT FAIL
>> +
>> +profile @{profile_name} {
>> +  /does/not/exist r,
>> +}
> 
> A similar test with an undefined variable for the attachment would be 
> nice. And since @{profile_name} has a special handling in the parser, 
> please also add a test with @{undefined_foo} ;-)
> 
good idea

> Also, what happens when a variable is defined as empty? That might also 
> be worth some tests (for profile and attachment).
> 
> 
yep

> Regards,
> 
> Christian Boltz
> 