[apparmor] [PATCH 2/3] Fix permission mapping for change_profile onexec

[John Johansen]

On 03/22/2012 03:35 PM, Seth Arnold wrote:
> I'm always worried when I see shared magic numbers. If AA_ONEXEC is supposed to share with AA_CHANGE_HAT, please define one in terms of the other or provide a comment to warn the future. Thanks :)

Well in fact they aren't exactly the same and could change. A boring
explanation follows

AA_CHANGE_HAT can be set in a base entry. Think of a dfa encoding
  change_profile /a,

  Start -> state1 -> state2
        /         a

  with the change_profile flag set on the permissions hanging off of state2

AA_ONEXEC can only be set on the second in a pair entry.
  change_profile /e -> /a,

  Start -> state1 -> state2 -> state3 -> state4 -> state5
        /         a         \0        /         e

  with the onexec info hanging off of state5


\0 is not a valid match character for paths, so no path will be able to
step across and start matching the exec portion of the rule.  The kernel
makes a deliberate \0 transition to get into the second match and then
tests the exec.

The permissions in the second match part don't have to be the same as
those in the first, and in fact are not.



> -----Original Message-----
> From: John Johansen <john.johansen at canonical.com>
> Sender: apparmor-bounces at lists.ubuntu.com
> Date: Thu, 22 Mar 2012 11:44:54 
> To: <apparmor at lists.ubuntu.com>
> Subject: [apparmor] [PATCH 2/3] Fix permission mapping for change_profile
> 	onexec
> 
> The kernel has an extended test for change_profile when used with
> onexec, that allows it to only work against set executables.
> 
> The parser is not correctly mapping change_profile for this test
> update the mapping so change_onexec will work when confined.
> 
> Note: the parser does not currently support the extended syntax
> that the kernel test allows for, this just enables it to work
> for the generic case.
> 
> Signed-off-by: John Johansen <john.johansen at canonical.com>
> ---
>  parser/immunix.h      |    1 +
>  parser/parser_regex.c |   26 +++++++++++++++++---------
>  2 files changed, 18 insertions(+), 9 deletions(-)
> 
> diff --git a/parser/immunix.h b/parser/immunix.h
> index 8dc157a..ebb2d2e 100644
> --- a/parser/immunix.h
> +++ b/parser/immunix.h
> @@ -61,6 +61,7 @@
>  #define AA_PTRACE_PERMS			(AA_USER_PTRACE | AA_OTHER_PTRACE)
>  
>  #define AA_CHANGE_HAT			(1 << 30)
> +#define AA_ONEXEC			(1 << 30)
>  #define AA_CHANGE_PROFILE		(1 << 31)
>  #define AA_SHARED_PERMS			(AA_CHANGE_HAT | AA_CHANGE_PROFILE)
>  
> diff --git a/parser/parser_regex.c b/parser/parser_regex.c
> index 8c34799..d0293e1 100644
> --- a/parser/parser_regex.c
> +++ b/parser/parser_regex.c
> @@ -510,19 +510,27 @@ static int process_dfa_entry(aare_ruleset_t *dfarules, struct cod_entry *entry)
>  			return FALSE;
>  	}
>  	if (entry->mode & AA_CHANGE_PROFILE) {
> +		char *vec[3];
> +		char lbuf[PATH_MAX + 8];
> +		int index = 1;
> +
> +		/* allow change_profile for all execs */
> +		vec[0] = "/[^/\x00]*";
> +
>  		if (entry->namespace) {
> -			char *vec[2];
> -			char lbuf[PATH_MAX + 8];
>  			int pos;
>  			ptype = convert_aaregex_to_pcre(entry->namespace, 0, lbuf, PATH_MAX + 8, &pos);
> -			vec[0] = lbuf;
> -			vec[1] = tbuf;
> -			if (!aare_add_rule_vec(dfarules, 0, AA_CHANGE_PROFILE, 0, 2, vec, dfaflags))
> -			    return FALSE;
> -		} else {
> -		  if (!aare_add_rule(dfarules, tbuf, 0, AA_CHANGE_PROFILE, 0, dfaflags))
> -				return FALSE;
> +			vec[index++] = lbuf;
>  		}
> +		vec[index++] = tbuf;
> +
> +		/* regular change_profile rule */
> +		if (!aare_add_rule_vec(dfarules, 0, AA_CHANGE_PROFILE, 0, index -1, &vec[1], dfaflags))
> +			return FALSE;
> +		/* onexec rule - both rules are needed for onexec */
> +		if (!aare_add_rule_vec(dfarules, 0, AA_ONEXEC, 0, index, vec, dfaflags))
> +			return FALSE;
> +
>  	}
>  	if (entry->mode & (AA_USER_PTRACE | AA_OTHER_PTRACE)) {
>  		int mode = entry->mode & (AA_USER_PTRACE | AA_OTHER_PTRACE);