On Fri, Dec 9, 2016 at 4:21 PM, Stephen Smalley <sds@xxxxxxxxxxxxx> wrote:
> SELinux was sometimes using the task "objective" credentials when
> it could/should use the "subjective" credentials. This was sometimes
> hidden by the fact that we were unnecessarily passing around pointers
> to the current task, making it appear as if the task could be something
> other than current, so eliminate all such passing. Given that
> tasks may only alter their own credentials, we likely should move
> the check from selinux_setprocattr() to security_setprocattr() or even
> to proc_pid_attr_write() and drop the task argument to the security hook
> altogether; it can only serve to confuse things.
>
> Signed-off-by: Stephen Smalley <sds@xxxxxxxxxxxxx>
> ---
> security/selinux/hooks.c | 154 +++++++++++++++++++++++------------------------
> 1 file changed, 76 insertions(+), 78 deletions(-)
No substantial objections, just some style nit picks below.
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 8a90a0b..3f99480 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -1696,23 +1696,27 @@ static int cred_has_perm(const struct cred *actor,
> }
>
> /*
> - * Check permission between a pair of tasks, e.g. signal checks,
> - * fork check, ptrace check, etc.
> - * tsk1 is the actor and tsk2 is the target
> - * - this uses the default subjective creds of tsk1
> + * Check permission between a task and the current task.
> */
> -static int task_has_perm(const struct task_struct *tsk1,
> - const struct task_struct *tsk2,
> - u32 perms)
> +static int task_has_perm_to_current(const struct task_struct *tsk,
> + u32 perms)
> {
> - const struct task_security_struct *__tsec1, *__tsec2;
> - u32 sid1, sid2;
> + u32 ssid, tsid;
>
> - rcu_read_lock();
> - __tsec1 = __task_cred(tsk1)->security; sid1 = __tsec1->sid;
> - __tsec2 = __task_cred(tsk2)->security; sid2 = __tsec2->sid;
> - rcu_read_unlock();
> - return avc_has_perm(sid1, sid2, SECCLASS_PROCESS, perms, NULL);
> + ssid = task_sid(tsk);
> + tsid = current_sid();
> + return avc_has_perm(ssid, tsid, SECCLASS_PROCESS, perms, NULL);
> +}
Why not just get rid of both ssid and tsid and turn this into a one
line function?
avc_has_perm(task_sid(tsk), current_sid(), ...);
> +/*
> + * Check permission between current and itself.
> + */
> +static int self_has_perm(u32 perms)
> +{
> + u32 sid;
> +
> + sid = current_sid();
Nit picky, but if you're respining the patch for the above, why not
change this too ...
u32 sid = current_sid();
> + return avc_has_perm(sid, sid, SECCLASS_PROCESS, perms, NULL);
> }
>
> /*
> @@ -1772,13 +1776,10 @@ static int cred_has_capability(const struct cred *cred,
> return rc;
> }
>
> -/* Check whether a task is allowed to use a system operation. */
> -static int task_has_system(struct task_struct *tsk,
> - u32 perms)
> +/* Check whether the current task is allowed to use a system operation. */
> +static int current_has_system(u32 perms)
We should have a little more naming consistentcy between
current_has_system() and self_has_perm(). Something like
current_has_process/current_has_system,
self_has_process/self_has_system, or something else along the lines
... I think you get the idea.
> {
> - u32 sid = task_sid(tsk);
> -
> - return avc_has_perm(sid, SECINITSID_KERNEL,
> + return avc_has_perm(current_sid(), SECINITSID_KERNEL,
> SECCLASS_SYSTEM, perms, NULL);
> }
--
paul moore
www.paul-moore.com
_______________________________________________
Selinux mailing list
Selinux@xxxxxxxxxxxxx
To unsubscribe, send email to Selinux-leave@xxxxxxxxxxxxx.
To get help, send an email containing "help" to Selinux-request@xxxxxxxxxxxxx.
