Re: [AppArmor #3 0/12] AppArmor security module

[John Johansen <john.johansen@canonical.com>]

Tetsuo Handa wrote:
> Hello.
> 
> Sorry for late response.
np. we are all busy and any time taken to review code is greatly appreciated

> I browsed apparmorfs-24.c apparmorfs.c audit.c capability.c context.c domain.c .
> Comments are shown below.
> 
> 
> 
>> static int aa_audit_base(int type, struct aa_profile *profile,
>> 			   struct aa_audit *sa, struct audit_context *audit_cxt,
>> 			   void (*cb) (struct audit_buffer *, void *))
> (...snipped...)
>> if (profile && PROFILE_KILL(profile) && type == AUDIT_APPARMOR_DENIED)
>> 	type = AUDIT_APPARMOR_KILL;
> 
> PROFILE_KILL(profile) includes profile != NULL checks.
> Are you doing
> 
> 	profile && PROFILE_KILL(profile)
> 
> in order to ignore aa_g_profile_mode == APPARMOR_KILL if profile == NULL?
> 
yes.  The profile kill flag should not be set unless there is a profile
confining the task.  This is confusing and really should be reworked, by
either renaming PROFILE_KILL and adding a comment here, or reworking the
PROFILE_KILL check, and its callers.  In any case I need to go through
and document which functions require filtered vs. unfiltered profiles
(ie. profile != NULL) I'll update this for the next posting.

> 
> 
>> int aa_restore_previous_profile(u64 token)
>> {
>> 	struct aa_task_context *cxt;
>> 	struct cred *new = prepare_creds();
>> 	if (!new)
>> 		return -ENOMEM;
>>
>> 	cxt = new->security;
>> 	if (cxt->sys.token != token) {
>> 		abort_creds(new);
> 
> I think simply returning -EACCES when trying to escape from some profile
> gives hijacked process chances to do brute force attack.
> Don't you need to kill the current process?
> 
>> 		return -EACCES;
>> 	}
> 
> 
correct and we do.  It is done by the caller (aa_change_hat()) by setting the
kill flag in the audit structure.
	} else if (previous_profile) {
		sa.name = previous_profile->fqname;
		sa.base.error = aa_restore_previous_profile(token);
		sa.perms.kill = AA_MAY_CHANGEHAT;

> 
>> static struct aa_profile *x_to_profile(struct aa_namespace *ns,
>> 					 struct aa_profile *profile,
>> 					 const char *name, u16 xindex)
> (...snipped...)
>> 	case AA_X_TABLE:
>> 		if (index > profile->file.trans.size) {
> 
> profile->file.trans.table[0] is not permitted if profile->file.trans.size == 0,
> is it?
No it isn't.
> Did you mean index >= profile->file.trans.size ?
> 
No, a file.trans.size == 0 means there are no transition entries, in which case the
type should not be AA_X_TABLE.  It is a consistency check that should never occur
if the user space tools properly generate the policy.  This check could, and probably
should be moved to a late pass in the policy unpack, so the check is only ever done
once for a given profile.


>> 			AA_ERROR("Invalid named transition\n");
>> 			return ERR_PTR(-EACCES);
>> 		}
>> 		name = profile->file.trans.table[index];
> (...snipped...)
>> 	} else if (*name == ':') {
>> 		/* switching namespace */
>> 		const char *ns_name = name + 1;
>> 		name = xname = ns_name + strlen(ns_name) + 1;
>> 		if (!*xname)
> 
> Isn't *xname undefined because it is beyond '\0'?
> 
No, however this needs to be better documented, and perhaps pulled out
into its own function.  The encoding of the names:
if there is a namespace the general case is
  :<namespace>\0<name>\0
if there is a namespace and the name is not specified, it becomes
  :<namespace>\0\0
else if the name does not begin with a : it is just a name
  <name>\0

  
>> 			/* no name so use profile name */
>> 			xname = profile->fqname;
> 
> (...snipped...)
> 
>> 	} else if (*name == '@') {
>> 		/* TODO: variable support */
>>
> 
> You want "continue;" here in order to avoid doing strstr(NULL, "//")
> inside aa_find_profile().
> 
yep, thanks for catching that

>> 	} else {
> 
> 
> 
>> int apparmor_bprm_set_creds(struct linux_binprm *bprm)
>> {
>> 	struct aa_task_context *cxt;
>> 	struct aa_profile *profile, *new_profile = NULL;
>> 	struct aa_namespace *ns;
>> 	char *buffer = NULL;
>> 	unsigned int state = DFA_START;
>> 	struct path_cond cond = {
>> 		bprm->file->f_path.dentry->d_inode->i_uid,
>> 		bprm->file->f_path.dentry->d_inode->i_mode
>> 	};
>> 	struct aa_audit_file sa = {
>> 		.base.operation = "exec",
>> 		.base.gfp_mask = GFP_KERNEL,
>> 		.request = MAY_EXEC,
>> 		.cond = &cond,
>> 	};
>>
>> 	sa.base.error = cap_bprm_set_creds(bprm);
>> 	if (sa.base.error)
>> 		return sa.base.error;
>>
>> 	if (bprm->cred_prepared)
>> 		return 0;
>>
>> 	cxt = bprm->cred->security;
>> 	BUG_ON(!cxt);
>>
>> 	profile = aa_confining_profile(cxt->sys.profile);
>> 	ns = profile->ns;
> 
> aa_confining_profile() may return NULL.
> According to apparmor-kermic tree, it is
> 
> 	ns = cxt->sys.profile->ns;
> 
yep, I introduced this error when reworking the profile filtering, while
cleaning up the removed profile checks.  I really should have done it
as a separate patch, and I only caught it after I pushed this patch set.
It is already fixed for the next push.

>> 	/* buffer freed below, name is pointer inside of buffer */
>> 	sa.base.error = aa_get_name(&bprm->file->f_path, 0, &buffer,
>> 				    (char **)&sa.name);
>> 	if (sa.base.error) {
>> 		if (!profile || profile->flags & PFLAG_IX_ON_NAME_ERROR)
>> 			sa.base.error = 0;
>> 		sa.base.info = "Exec failed name resolution";
>> 		sa.name = bprm->filename;
>> 		goto audit;
>> 	}
>>
>> 	if (!profile) {
>> 		/* unconfined task - attach profile if one matches */
>> 		new_profile = aa_sys_find_attach(&ns->base, sa.name);
>> 		if (!new_profile)
>> 			goto cleanup;
>> 		goto apply;
>> 	} else if (cxt->sys.onexec) {
>> 		/*
>> 		 * onexec permissions are stored in a pair, rewalk the
>> 		 * dfa to get start of the exec path match.
>> 		 */
>> 		sa.perms = change_profile_perms(profile, cxt->sys.onexec->ns,
>> 						sa.name, &state);
>> 		state = aa_dfa_null_transition(profile->file.dfa, state);
>> 	}
>> 	sa.perms = aa_str_perms(profile->file.dfa, state, sa.name, &cond, NULL);> 
>> 	if (cxt->sys.onexec && sa.perms.allowed & AA_MAY_ONEXEC) {
>> 		new_profile = cxt->sys.onexec;
>> 		cxt->sys.onexec = NULL;
>> 		sa.base.info = "change_profile onexec";
>> 	} else if (sa.perms.allowed & MAY_EXEC) {
>> 		new_profile = x_to_profile(ns, profile, sa.name,
>> 					   sa.perms.xindex);
>> 		if (IS_ERR(new_profile)) {
>> 			if (sa.perms.xindex & AA_X_INHERIT) {
>> 				/* (p|c|n)ix - don't change profile */
>> 				sa.base.info = "ix fallback";
>> 				goto x_clear;
>> 			} else if (sa.perms.xindex & AA_X_UNCONFINED) {
>> 				new_profile = aa_get_profile(ns->unconfined);
>> 				sa.base.info = "ux fallback";
> 
> IS_ERR(new_profile) is true but new_profile == NULL is false.
> What I worry is that you sometimes embed error values into pointer but you
> are sometimes checking only NULL.
> 
Right, the use of ERR_PTR is limited but it is easy to mess up, and/or not follow
and maintenance could be problematic.  It might be best to rework so none of
the functions return ERR_PTR, avoiding any potential problems here.

>> 			} else {
>> 				sa.base.error = PTR_ERR(new_profile);
>> 				if (sa.base.error == -ENOENT)
>> 					sa.base.info = "profile not found";
>> 				new_profile = NULL;
>> 			}
>> 		}
>> 	} else if (PROFILE_COMPLAIN(profile)) {
>> 		new_profile = aa_alloc_null_profile(profile, 0);
>> 		sa.base.error = -EACCES;
>> 		if (!new_profile)
>> 			sa.base.error = -ENOMEM;
>> 		sa.name2 = new_profile->fqname;
> 
> This will oops if new_profile == NULL. Please fix apparmor-karmic as well.
> 
ouch :(, thanks again

>> 		sa.perms.xindex |= AA_X_UNSAFE;
>> 	} else {
>> 		sa.base.error = -EACCES;
>> 	}
>>
>> 	if (!new_profile)
>> 		goto audit;
> 
> You want to do
> 
> 	if (!new_profile || IS_ERR(new_profile))
> 
> rather than
> 
> 	if (!new_profile)
> 
> Please fix apparmor-karmic as well.
> 
Well it shouldn't ever get to this code with an ERR_PTR, it is handled by
		new_profile = x_to_profile(ns, profile, sa.name,
					   sa.perms.xindex);
		if (IS_ERR(new_profile)) {
above, but it is too easy to miss this, or break in the future so I will
rework to drop the use of ERR_PTR with profile.

>> 	if (profile == new_profile) {
>> 		aa_put_profile(new_profile);
> 
> aa_put_profile() with error pointer, which will be fixed by above change.
> 
dito

>> 		goto audit;
>> 	}
>>
>> 	if (bprm->unsafe & LSM_UNSAFE_SHARE) {
>> 		/* FIXME: currently don't mediate shared state */
>> 		;
>> 	}
>>
>> 	if (bprm->unsafe & (LSM_UNSAFE_PTRACE | LSM_UNSAFE_PTRACE_CAP)) {
>> 		sa.base.error = aa_may_change_ptraced_domain(current,
>> 							     new_profile);
> 
> aa_may_change_ptraced_domain() with error pointer, which will be fixed by
> above change.
> 
dito

>> 		if (sa.base.error)
>> 			goto audit;
>> 	}
>>
>> 	/* Determine if secure exec is needed.
>> 	 * Can be at this point for the following reasons:
>> 	 * 1. unconfined switching to confined
>> 	 * 2. confined switching to different confinement
>> 	 * 3. confined switching to unconfined
>> 	 *
>> 	 * Cases 2 and 3 are marked as requiring secure exec
>> 	 * (unless policy specified "unsafe exec")
>> 	 *
>> 	 * bprm->unsafe is used to cache the AA_X_UNSAFE permission
>> 	 * to avoid having to recompute in secureexec
>> 	 */
>> 	if (!(sa.perms.xindex & AA_X_UNSAFE))
>> 		bprm->unsafe |= AA_SECURE_X_NEEDED;
>>
>> apply:
>> 	sa.name2 = new_profile->fqname;
> 
> error pointer, which will be fixed by above change.
> 
dito

>> 	/* When switching namespace ensure its part of audit message */
>> 	if (new_profile->ns != ns)
>> 		sa.name3 = new_profile->ns->base.name;
>>
>> 	/* when transitioning profiles clear unsafe personality bits */
>> 	bprm->per_clear |= PER_CLEAR_ON_SETID;
>>
>> 	aa_put_profile(cxt->sys.profile);
>> 	/* transfer new profile reference will be released when cxt is freed */
>> 	cxt->sys.profile = new_profile;
>>
>> x_clear:
>> 	aa_put_profile(cxt->sys.previous);
>> 	aa_put_profile(cxt->sys.onexec);
>> 	cxt->sys.previous = NULL;
>> 	cxt->sys.onexec = NULL;
>> 	cxt->sys.token = 0;
>>
>> audit:
>> 	sa.base.error = aa_audit_file(profile, &sa);
> 
> aa_audit_file() might return 0 if PROFILE_COMPLAIN(profile) == true even if
> sa.base.error != 0 . I think regarding execve(), we should not ignore errors
> like -EACCES, -ENOMEM etc. if something went wrong before auditing.
> Otherwise, current process might continue execve() with unexpected profile.
> 
true enough, it should only be EPERM, EACCES

>> cleanup:
>> 	kfree(buffer);
>>
>> 	return sa.base.error;
>> }
> 
> 
> 
>> int aa_change_hat(const char *hat_name, u64 token, int permtest)
> (...snipped...)
>> 			/* freed below */
>> 			name = new_compound_name(root->fqname, hat_name);
>>
> 
> Audit log lacks "name=%s" part if name == NULL.
> 
hrmm, yeah that is less than ideal, will fix

>> 			sa.name = name;
>> 			sa.base.info = "hat not found";
>> 			sa.base.error = -ENOENT;
> 
> 
> 
>> int aa_change_profile(const char *ns_name, const char *fqname, int onexec,
>> 		      int permtest)
> (...snipped...)
>> 	/* released below */
>> 	target = aa_find_profile(ns, fqname);
>> 	if (!target) {
>> 		sa.base.info = "profile not found";
>> 		sa.base.error = -ENOENT;
>> 		if (permtest || !PROFILE_COMPLAIN(profile))
>> 			goto audit;
>> 		/* release below */
>> 		target = aa_alloc_null_profile(profile, 0);
> 
> aa_alloc_null_profile() will oops if profile == NULL.

right this is actually caught by the !PROFILE_COMPLAIN(profile) part of
>> 		if (permtest || !PROFILE_COMPLAIN(profile))
>> 			goto audit;
immediately above but that is less than obvious and needs to be documented.

thanks again
john