[apparmor] [PATCH 12/16] apparmor: ensure the target profile name is always audited

[Seth Arnold]

On Wed, Apr 20, 2016 at 11:52:54PM -0700, John Johansen wrote:
> The target profile name was not being correctly audited in a few
> cases because the target variable was not being set and gotos
> passed the code to set it at apply:
> 
> Since it is always based on new_profile just drop the target var
> and conditionally report based on new_profile.
> 
> Signed-off-by: John Johansen <john.johansen at canonical.com>

Acked-by: Seth Arnold <seth.arnold at canonical.com>

Thanks

> ---
>  security/apparmor/domain.c | 20 +++++++++-----------
>  1 file changed, 9 insertions(+), 11 deletions(-)
> 
> diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
> index 67a7418..fc3036b 100644
> --- a/security/apparmor/domain.c
> +++ b/security/apparmor/domain.c
> @@ -346,7 +346,7 @@ int apparmor_bprm_set_creds(struct linux_binprm *bprm)
>  		file_inode(bprm->file)->i_uid,
>  		file_inode(bprm->file)->i_mode
>  	};
> -	const char *name = NULL, *target = NULL, *info = NULL;
> +	const char *name = NULL, *info = NULL;
>  	int error = 0;
>  
>  	if (bprm->cred_prepared)
> @@ -399,6 +399,7 @@ int apparmor_bprm_set_creds(struct linux_binprm *bprm)
>  	if (cxt->onexec) {
>  		struct file_perms cp;
>  		info = "change_profile onexec";
> +		new_profile = aa_get_newest_profile(cxt->onexec);
>  		if (!(perms.allow & AA_MAY_ONEXEC))
>  			goto audit;
>  
> @@ -413,7 +414,6 @@ int apparmor_bprm_set_creds(struct linux_binprm *bprm)
>  
>  		if (!(cp.allow & AA_MAY_ONEXEC))
>  			goto audit;
> -		new_profile = aa_get_newest_profile(cxt->onexec);
>  		goto apply;
>  	}
>  
> @@ -445,10 +445,8 @@ int apparmor_bprm_set_creds(struct linux_binprm *bprm)
>  		if (!new_profile) {
>  			error = -ENOMEM;
>  			info = "could not create null profile";
> -		} else {
> +		} else
>  			error = -EACCES;
> -			target = new_profile->base.hname;
> -		}
>  		perms.xindex |= AA_X_UNSAFE;
>  	} else
>  		/* fail exec */
> @@ -459,7 +457,6 @@ int apparmor_bprm_set_creds(struct linux_binprm *bprm)
>  	 * fail the exec.
>  	 */
>  	if (bprm->unsafe & LSM_UNSAFE_NO_NEW_PRIVS) {
> -		aa_put_profile(new_profile);
>  		error = -EPERM;
>  		goto cleanup;
>  	}
> @@ -474,10 +471,8 @@ int apparmor_bprm_set_creds(struct linux_binprm *bprm)
>  
>  	if (bprm->unsafe & (LSM_UNSAFE_PTRACE | LSM_UNSAFE_PTRACE_CAP)) {
>  		error = may_change_ptraced_domain(new_profile);
> -		if (error) {
> -			aa_put_profile(new_profile);
> +		if (error)
>  			goto audit;
> -		}
>  	}
>  
>  	/* Determine if secure exec is needed.
> @@ -498,7 +493,6 @@ int apparmor_bprm_set_creds(struct linux_binprm *bprm)
>  		bprm->unsafe |= AA_SECURE_X_NEEDED;
>  	}
>  apply:
> -	target = new_profile->base.hname;
>  	/* when transitioning profiles clear unsafe personality bits */
>  	bprm->per_clear |= PER_CLEAR_ON_SETID;
>  
> @@ -506,15 +500,19 @@ x_clear:
>  	aa_put_profile(cxt->profile);
>  	/* transfer new profile reference will be released when cxt is freed */
>  	cxt->profile = new_profile;
> +	new_profile = NULL;
>  
>  	/* clear out all temporary/transitional state from the context */
>  	aa_clear_task_cxt_trans(cxt);
>  
>  audit:
>  	error = aa_audit_file(profile, &perms, GFP_KERNEL, OP_EXEC, MAY_EXEC,
> -			      name, target, cond.uid, info, error);
> +			      name,
> +			      new_profile ? new_profile->base.hname : NULL,
> +			      cond.uid, info, error);
>  
>  cleanup:
> +	aa_put_profile(new_profile);
>  	aa_put_profile(profile);
>  	kfree(buffer);
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20160427/387a8c98/attachment-0001.pgp>