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

[Christian Boltz]

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

> 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.


> 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."


> --- /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.

> --- /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.

> --- /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.

>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).

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,
}


> --- /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} ;-)

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


Regards,

Christian Boltz
-- 
<sarnold> it's been on my todo list for eight or nine years,
          I'm sure I'll get around to it right quick :)
[from #apparmor]