john.johansen at canonical
Nov 23, 2009, 2:10 AM
Post #7 of 9
(254 views)
Permalink
|
|
Re: [AppArmor #3 0/12] AppArmor security module
[In reply to]
|
|
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
|
