see more comments below: * richard offer (offerat_private) wrote: > ===== kernel/capability.c 1.2 vs edited ===== looks fine. > ===== kernel/sys.c 1.18 vs edited ===== > @@ -820,6 +813,10 @@ > if (!p) > goto out; > > + err = security_ops->task_ops->setpgid(p, pgid); > + if (err) > + goto out; > + > if (p->p_pptr == current || p->p_opptr == current) { > err = -EPERM; > if (p->session != current->session) > @@ -843,10 +840,6 @@ > } > > ok_pgid: > - err = security_ops->task_ops->setpgid(p, pgid); > - if (err) > - goto out; > - > p->pgrp = pgid; > err = 0; > out: what are you trying to do here? > @@ -1123,7 +1116,7 @@ > asmlinkage long sys_setrlimit(unsigned int resource, struct rlimit *rlim) > { > struct rlimit new_rlim, *old_rlim; > - int retval; > + int retval = 0; > > if (resource >= RLIM_NLIMITS) > return -EINVAL; > @@ -1133,13 +1126,13 @@ > if (((new_rlim.rlim_cur > old_rlim->rlim_max) || > (new_rlim.rlim_max > old_rlim->rlim_max)) && > !capable(CAP_SYS_RESOURCE)) > - return -EPERM; > - if (resource == RLIMIT_NOFILE) { > + retval = -EPERM; > + if (!retval && resource == RLIMIT_NOFILE) { > if (new_rlim.rlim_cur > NR_OPEN || new_rlim.rlim_max > NR_OPEN) ugh, i mentioned this before. the '!retval &&' just looks bad ;-( however, in this case...you have no way to distiguish between i don't have the capability and you are trying to increase beyond hard kernel limit (NR_OPEN). sure you can redo check in hook, but this seems bad. while the NR_OPEN check can return -EPERM, it is pretty close to functional logic (kind of like, is this filesystem read-only). > ===== kernel/module.c 1.16 vs edited ===== > @@ -357,8 +357,6 @@ > unsigned long mod_user_size; > struct module_ref *dep; > > - if (!capable(CAP_SYS_MODULE)) > - return -EPERM; > lock_kernel(); > if ((namelen = get_mod_name(name_user, &name)) < 0) { > error = namelen; am i missing something here? did you just delete the CAP_SYS_MODULE check? this surely isn't right! > @@ -632,7 +627,11 @@ > spin_lock(&unload_lock); > if (!__MOD_IN_USE(mod)) { > /* check that we have permission to do this */ > - error = security_ops->module_ops->delete_module(mod); > + error = 0; > + if (!capable(CAP_SYS_MODULE)) > + error = -EPERM; > + > + error = security_ops->module_ops->delete_module(error,mod); > if (error) { > spin_unlock(&unload_lock); > goto out; > @@ -645,6 +644,15 @@ > spin_unlock(&unload_lock); > } > goto out; > + } else { > + /* check that we have permission to do this */ > + error = 0; > + if (!capable(CAP_SYS_MODULE)) > + error = -EPERM; > + > + error = security_ops->module_ops->delete_module(error, NULL); > + if (error) > + goto out; so now, delete_module is called twice in the automatic reaping case. once with a NULL (struct mod*). i suppose this is better than the alternative of checking CAP_SYS_MODULE for every module during reaping, but it's a little kludgy feeling. and, a change like this would need some corresponding docs...to let module developer know that (struct mod*) may be NULL. > ===== kernel/ptrace.c 1.6 vs edited ===== > @@ -27,20 +27,24 @@ > goto bad; > if (!task->mm) > goto bad; > + /* messier than it should be to ensure we have a code path identical to > + * that pre-LSM */ > + retval = 0; > if(((current->uid != task->euid) || > (current->uid != task->suid) || > (current->uid != task->uid) || > (current->gid != task->egid) || > (current->gid != task->sgid) || > (current->gid != task->gid)) && !capable(CAP_SYS_PTRACE)) > - goto bad; > - rmb(); > - if (!task->mm->dumpable && !capable(CAP_SYS_PTRACE)) > - goto bad; > + retval = -EPERM; > + if ( ! retval ) > + rmb(); > + if (!retval && !task->mm->dumpable && !capable(CAP_SYS_PTRACE)) > + retval = -EPERM; now this is looking very sketchy. to be safe, i'd not make the rmb() conditional, as it is required to order reads for race condition elimination. (yes, ptrace is _always_ a headache!) perhaps nesting conditional would be better here instead of multiple use of !retval. and this is another example of the blurred lines between functional and access control logic... if (task->ptrace & PT_PTRACED). do you really want to hand over a task that is already being traced to our hook? i don't think it is appropriate. > ===== kernel/sched.c 1.16 vs edited ===== looks ok. > ===== kernel/signal.c 1.4 vs edited ===== looks ok. > ===== kernel/sysctl.c 1.12 vs edited ===== looks ok. > ===== include/linux/security.h 1.92 vs edited ===== seems ok. i think a hook is either authoritative or not (i don't think it depends on where hook is called). just happens that some cases the kerror is hardcoded to 0. this is really a hint to the module developer i think. > ===== security/capability_plug.c 1.68 vs edited ===== > --- 1.68/security/capability_plug.c Thu Oct 4 18:34:24 2001 > +++ edited/security/capability_plug.c Thu Oct 11 10:24:09 2001 > @@ -109,14 +109,14 @@ > return 0; > } > > -static int cap_ptrace (struct task_struct *parent, struct task_struct *child) > +static int cap_ptrace (int kerror, struct task_struct *parent, struct task_struct *child) > { > /* Derived from arch/i386/kernel/ptrace.c:sys_ptrace. */ > if (!cap_issubset (child->cap_permitted, current->cap_permitted) && > !capable (CAP_SYS_PTRACE)) > return -EPERM; > else > - return 0; > + return kerror; i don't think this is exactly right. i think it should prefer returning kerror. if (kerror) return kerror; else if (!cap_issubset (child->cap_permitted, current->cap_permitted) && !capable (CAP_SYS_PTRACE)) return -EPERM; else return 0; or whatever... > @@ -129,7 +129,7 @@ > return 0; > } > > -static int cap_capset_check (struct task_struct *target, > +static int cap_capset_check (int kerror, struct task_struct *target, > kernel_cap_t * effective, > kernel_cap_t * inheritable, > kernel_cap_t * permitted) > @@ -154,7 +154,7 @@ > return -EPERM; > } > > - return 0; > + return kerror; ditto. prefer returning kerror. > ===== security/dummy.c 1.6 vs edited ===== > @@ -740,12 +740,12 @@ > return 0; > } > > -static int dummy_module_delete_module (const struct module *mod) > +static int dummy_module_delete_module (int kerror, const struct module *mod) > { > - return 0; > + return kerror; > } > > -static int dummy_ipc_permission (struct kern_ipc_perm *ipcp, short flag) > +static int dummy_ipc_permission (int kerror, struct kern_ipc_perm *ipcp, short flag) > { > return 0; nope, return kerror; > ===== ipc/util.c 1.4 vs edited ===== looks ok. > ===== arch/i386/kernel/ioport.c 1.6 vs edited ===== looks ok. > ===== arch/i386/kernel/ptrace.c 1.13 vs edited ===== looks ok. > ===== arch/ia64/ia32/sys_ia32.c 1.9 vs edited ===== looks ok. > ===== arch/ia64/kernel/ptrace.c 1.9 vs edited ===== looks ok. whew, i think that's it. -chris _______________________________________________ linux-security-module mailing list linux-security-moduleat_private http://mail.wirex.com/mailman/listinfo/linux-security-module
This archive was generated by hypermail 2b30 : Tue Oct 16 2001 - 19:38:58 PDT