| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <4B839D0D.203@canonical.com>
Date: Tue, 23 Feb 2010 01:17:01 -0800
From: John Johansen <john.johansen@...onical.com>
To: Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
CC: john.johansen@...onical.com, linux-security-module@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [AppArmor #4 0/12] AppArmor security module
Tetsuo Handa wrote:
> Regarding audit.c
>
> 53 static int aa_audit_base(int type, struct aa_profile *profile,
> 54 struct aa_audit *sa, struct audit_context *audit_cxt,
> 55 void (*cb) (struct audit_buffer *, struct aa_audit *))
> (...snipped...)
> 93 pid_t pid = task->real_parent->pid;
> I think you need to protect this dereference with RCU
> (see SYSCALL_DEFINE0(getppid)).
>
yep
>
>
>
>
> Regarding domain.c
>
> 54 static int aa_may_change_ptraced_domain(struct task_struct *task,
> 55 struct aa_profile *to_profile)
> (...snipped...)
> 71 /* not ptraced */
> 72 if (!tracer || unconfined(tracerp))
> 73 goto out;
> unconfined() does not accept NULL.
> What guarantees that tracer's profile != NULL?
>
every context needs to have a valid profile now. That change happened when
AppArmor made the switch to creds, but internally the change wasn't originally rolled into all the code so NULL was used in a lot of places
for the unconfined profile. That should be finally cleaned up in this
submission.
>
>
>
>
> 197 static const char *separate_fqname(const char *fqname, const char **ns_name)(...snipped...)
> 201 if (fqname[0] == ':') {
> 202 *ns_name = fqname + 1; /* skip : */
> 203 name = *ns_name + strlen(*ns_name) + 1;
> 204 if (!*name)
> This will go beyond \0 if fqname was ":" . Is fqname checked somewhere else?
> 205 name = NULL;
>
yes. unpack_trans_table in policy_unpack.c verifies the transition table
strings. But thanks for asking because its check isn't quite right.
>
>
>
>
> 229 static struct aa_profile *x_to_profile(struct aa_profile *profile,
> 230 const char *name, u16 xindex)
> (...snipped...)
> 297 out:
> This label seems unused.
> 298 /* released by caller */
> 299 return new_profile;
>
well it is used by
case AA_X_NAME:
if (xindex & AA_X_CHILD)
/* released by caller */
new_profile = aa_find_attach(ns,
&profile->base.profiles,
name);
else
/* released by caller */
new_profile = aa_find_attach(ns, &ns->base.profiles,
name);
goto out;
but that goto could be replaced by a return
>
>
>
>
> 308 int apparmor_bprm_set_creds(struct linux_binprm *bprm)
> (...snipped...)
> 336 profile = aa_newest_version(cxt->profile);
> profile == NULL if cxt->profile == NULL.
> What guarantees that cxt->profile != NULL?
>
Well every cxt is supposed to point to a valid profile and fn's should fail
if they violate this. That being said there really should be some BUG_ONs
in the code for this, just like there is for the cxt.
>
>
>
>
> 534 int aa_change_hat(const char *hats[], int count, u64 token, bool permtest)
> (...snipped...)
> 565 /* find first matching hat */
> 566 for (i = 0; i < count && !hat; i++)
> 567 /* released below */
> 568 hat = aa_find_child(root, hats[i]);
> 569 if (!hat) {
> 570 if (!COMPLAIN_MODE(root) || permtest) {
> 571 sa.base.info = "hat not found";
> 572 if (list_empty(&root->base.profiles))
> 573 sa.base.error = -ECHILD;
> 574 else
> 575 sa.base.error = -ENOENT;
> 576 goto out;
> 577 }
> 578 /* freed below */
> 579 name = new_compound_name(root->base.hname, hats[0]);
> Is this hats[0] and not hats[i - 1]?
>
Yep, I know it seems strange and deserves a comment. This is an artifact
of what userspace is expecting as logging output. The traditional
behavior of learning mode is the first hat request to fail cause switching
into a learning profile and the failure is logged. If the requests succeeds
then the transition is made. At this point all requests have failed so
we log the first as expected.
There is a planned improvement here where we log more information but this
is sufficient for the moment.
>
> You have a lot of
>
> sa.base.info = ...;
> sa.base.error = -E...;
>
> Passing sa.base to functions might simplify the code.
>
Hrmm, yeah I have played around with this some and haven't hit on the right
combination yet.
>
>
>
>
> Regarding file.c
>
> 61 static void file_audit_cb(struct audit_buffer *ab, struct aa_audit *va)
> 62 {
> 63 struct aa_audit_file *sa = container_of(va, struct aa_audit_file, base);
> 64 u16 denied = sa->request & ~sa->perms.allowed;
> 65 uid_t fsuid;
> 66
> 67 if (sa->base.task)
> 68 fsuid = task_uid(sa->base.task);
> Is task_struct pointed by sa->base.task guaranteed to exist until now?
>
well yes but only because sa->base.task isn't used :),
>
>
>
>
> Regarding lsm.c
>
> 135 static int apparmor_capable(struct task_struct *task, const struct cred *cred,
> 136 int cap, int audit)
> 137 {
> 138 struct aa_profile *profile;
> 139 /* cap_capable returns 0 on success, else -EPERM */
> 140 int error = cap_capable(task, cred, cap, audit);
> 141
> 142 profile = aa_cred_profile(cred);
> aa_cred_profile() returns NULL but unconfined() and aa_capable() don't accept
> profile == NULL case.
> 143 if (!error && !unconfined(profile))
> 144 error = aa_capable(task, profile, cap, audit);
>
> For me, it is difficult to check "whether parameter may be NULL or not"
> "whether function may return NULL or not".
> Adding "Maybe NULL." / "Never NULL." to function parameter's comment line and
> "May return NULL" / "Never returns NULL" for function's return value would be
> helpful.
okay, will do
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists