[prev in list] [next in list] [prev in thread] [next in thread] 

List:       apparmor-dev
Subject:    Re: [apparmor] [PATCH 2/3] Fix permission mapping for change_profile onexec
From:       John Johansen <john.johansen () canonical ! com>
Date:       2012-03-22 22:55:40
Message-ID: 4F6BADEC.4020105 () canonical ! com
[Download RAW message or body]

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@canonical.com>
> Sender: apparmor-bounces@lists.ubuntu.com
> Date: Thu, 22 Mar 2012 11:44:54 
> To: <apparmor@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@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);


-- 
AppArmor mailing list
AppArmor@lists.ubuntu.com
Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor


[prev in list] [next in list] [prev in thread] [next in thread]