| Message ID | CAJ-EccNNCNrA7to85WrYFjRUP7Z1XC+sJ2JXkargiEvXd11AVg@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] security: Add LSM fixup hooks to set*gid syscalls. | expand |
On 7/31/2018 10:34 AM, Micah Morton wrote: > The set*uid system calls all call an LSM fixup hook called > security_task_fix_setuid, which allows for altering the behavior of those > calls by a security module. Comments explaining the LSM_SETID_* constants > in /include/linux/security.h imply that the constants are to be used for > both the set*uid and set*gid syscalls, but the set*gid syscalls do not > have the relevant hooks, meaning a security module can only alter syscalls > that change user identity attributes but not ones that change group > identity attributes. This patch adds the necessary LSM hook, called > security_task_fix_setgid, and calls the hook from the appropriate places > in the set*gid syscalls.Tested by putting a print statement in the hook and > seeing it triggered from the various set*gid syscalls. > > Signed-off-by: Micah Morton <mortonm@chromium.org <mailto:mortonm@chromium.org>> > Acked-by: Kees Cook <keescook@chromium.org <mailto:keescook@chromium.org>> What security module is going to provide a hook for this? > --- > NOTE: the security_task_fix_setgid line in sys_setfsgid is over 80 > characters, but I figured I'd just follow how it was done in sys_setfsuid > rather than trying to wrap the line, since the functions are nearly > identical. > > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h > index 8f1131c8dd54..a2166c812a97 100644 > --- a/include/linux/lsm_hooks.h > +++ b/include/linux/lsm_hooks.h > @@ -599,6 +599,15 @@ > *@old is the set of credentials that are being replaces > *@flags contains one of the LSM_SETID_* values. > *Return 0 on success. > + * @task_fix_setgid: > + * Update the module's state after setting one or more of the group > + * identity attributes of the current process. The @flags parameter > + * indicates which of the set*gid system calls invoked this hook. > + * @new is the set of credentials that will be installed. Modifications > + * should be made to this rather than to @current->cred. > + * @old is the set of credentials that are being replaced > + * @flags contains one of the LSM_SETID_* values. > + * Return 0 on success. > * @task_setpgid: > *Check permission before setting the process group identifier of the > *process @p to @pgid. > @@ -1587,6 +1596,8 @@ union security_list_options { > enum kernel_read_file_id id); > int (*task_fix_setuid)(struct cred *new, const struct cred *old, > int flags); > +int (*task_fix_setgid)(struct cred *new, const struct cred *old, > +int flags); > int (*task_setpgid)(struct task_struct *p, pid_t pgid); > int (*task_getpgid)(struct task_struct *p); > int (*task_getsid)(struct task_struct *p); > @@ -1876,6 +1887,7 @@ struct security_hook_heads { > struct hlist_head kernel_post_read_file; > struct hlist_head kernel_module_request; > struct hlist_head task_fix_setuid; > +struct hlist_head task_fix_setgid; > struct hlist_head task_setpgid; > struct hlist_head task_getpgid; > struct hlist_head task_getsid; > diff --git a/include/linux/security.h b/include/linux/security.h > index 63030c85ee19..a82d97cf13ab 100644 > --- a/include/linux/security.h > +++ b/include/linux/security.h > @@ -325,6 +325,8 @@ int security_kernel_post_read_file(struct file *file, char *buf, loff_t size, > enum kernel_read_file_id id); > int security_task_fix_setuid(struct cred *new, const struct cred *old, > int flags); > +int security_task_fix_setgid(struct cred *new, const struct cred *old, > + int flags); > int security_task_setpgid(struct task_struct *p, pid_t pgid); > int security_task_getpgid(struct task_struct *p); > int security_task_getsid(struct task_struct *p); > @@ -929,6 +931,13 @@ static inline int security_task_fix_setuid(struct cred *new, > return cap_task_fix_setuid(new, old, flags); > } > > +static inline int security_task_fix_setgid(struct cred *new, > + const struct cred *old, > + int flags) > +{ > +return 0; > +} > + > static inline int security_task_setpgid(struct task_struct *p, pid_t pgid) > { > return 0; > diff --git a/kernel/sys.c b/kernel/sys.c > index 38509dc1f77b..f6ef922c6815 100644 > --- a/kernel/sys.c > +++ b/kernel/sys.c > @@ -392,6 +392,10 @@ long __sys_setregid(gid_t rgid, gid_t egid) > new->sgid = new->egid; > new->fsgid = new->egid; > > +retval = security_task_fix_setgid(new, old, LSM_SETID_RE); > +if (retval < 0) > +goto error; > + > return commit_creds(new); > > error: > @@ -434,6 +438,10 @@ long __sys_setgid(gid_t gid) > else > goto error; > > +retval = security_task_fix_setgid(new, old, LSM_SETID_ID); > +if (retval < 0) > +goto error; > + > return commit_creds(new); > > error: > @@ -755,6 +763,10 @@ long __sys_setresgid(gid_t rgid, gid_t egid, gid_t sgid) > new->sgid = ksgid; > new->fsgid = new->egid; > > +retval = security_task_fix_setgid(new, old, LSM_SETID_RES); > +if (retval < 0) > +goto error; > + > return commit_creds(new); > > error: > @@ -861,7 +873,8 @@ long __sys_setfsgid(gid_t gid) > ns_capable(old->user_ns, CAP_SETGID)) { > if (!gid_eq(kgid, old->fsgid)) { > new->fsgid = kgid; > -goto change_okay; > +if (security_task_fix_setgid(new, old, LSM_SETID_FS) == 0) > +goto change_okay; > } > } > > diff --git a/security/security.c b/security/security.c > index 68f46d849abe..587786fc0aaa 100644 > --- a/security/security.c > +++ b/security/security.c > @@ -1062,6 +1062,12 @@ int security_task_fix_setuid(struct cred *new, const struct cred *old, > return call_int_hook(task_fix_setuid, 0, new, old, flags); > } > > +int security_task_fix_setgid(struct cred *new, const struct cred *old, > + int flags) > +{ > +return call_int_hook(task_fix_setgid, 0, new, old, flags); > +} > + > int security_task_setpgid(struct task_struct *p, pid_t pgid) > { > return call_int_hook(task_setpgid, 0, p, pgid); > > -- > 4.18-rc2 > > -- > <html> <head> <meta http-equiv="Content-Type" content="text/html; charset=utf-8"> </head> <body text="#000000" bgcolor="#FFFFFF"> On 7/31/2018 10:34 AM, Micah Morton wrote:<br> <blockquote type="cite" cite="mid:CAJ-EccNNCNrA7to85WrYFjRUP7Z1XC+sJ2JXkargiEvXd11AVg@mail.gmail.com"> <meta http-equiv="content-type" content="text/html; charset=utf-8"> <div dir="ltr"> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial">The set*uid system calls all call an LSM fixup hook called</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial">security_task_fix_setuid, which allows for altering the behavior of those</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial">calls by a security module. Comments explaining the LSM_SETID_* constants</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial">in /include/linux/security.h imply that the constants are to be used for</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial">both the set*uid and set*gid syscalls, but the set*gid syscalls do not</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial">have the relevant hooks, meaning a security module can only alter syscalls</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial">that change user identity attributes but not ones that change group</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial">identity attributes. This patch adds the necessary LSM hook, called</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial">security_task_fix_setgid, and calls the hook from the appropriate places</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial">in the set*gid syscalls.Tested by putting a print statement in the hook and</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial">seeing it triggered from the various set*gid syscalls.</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial"><br> </div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial">Signed-off-by: Micah Morton <<a href="mailto:mortonm@chromium.org" target="_blank" style="color:rgb(17,85,204)" moz-do-not-send="true">mortonm@chromium.org</a>></div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial"><span style="background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline">Acked-by: Kees Cook <</span><a href="mailto:keescook@chromium.org" target="_blank" style="color:rgb(17,85,204);background-color:rgb(255,255,255)" moz-do-not-send="true">keescook@chromium.org</a><span style="background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline">></span><br> </div> </div> </blockquote> <pre> What security module is going to provide a hook for this? </pre> <blockquote type="cite" cite="mid:CAJ-EccNNCNrA7to85WrYFjRUP7Z1XC+sJ2JXkargiEvXd11AVg@mail.gmail.com"> <div dir="ltr"> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial">---</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial">NOTE: the security_task_fix_setgid line in sys_setfsgid is over 80</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial">characters, but I figured I'd just follow how it was done in sys_setfsuid</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial">rather than trying to wrap the line, since the functions are nearly</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial">identical.</div> <br class="gmail-m_4493492642882813245gmail-Apple-interchange-newline" style="font-size:small;text-decoration-style:initial;text-decoration-color:initial"> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial">diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial">index 8f1131c8dd54..a2166c812a97 100644</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial">--- a/include/linux/lsm_hooks.h</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial">+++ b/include/linux/lsm_hooks.h</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial">@@ -599,6 +599,15 @@</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial"> *<span style="white-space:pre-wrap"> </span>@old is the set of credentials that are being replaces</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial"> *<span style="white-space:pre-wrap"> </span>@flags contains one of the LSM_SETID_* values.</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial"> *<span style="white-space:pre-wrap"> </span>Return 0 on success.</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial">+ * @task_fix_setgid:</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial">+ * Update the module's state after setting one or more of the group</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial">+ * identity attributes of the current process. The @flags parameter</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial">+ * indicates which of the set*gid system calls invoked this hook.</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial">+ * @new is the set of credentials that will be installed. Modifications</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial">+ * should be made to this rather than to @current->cred.</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial">+ * @old is the set of credentials that are being replaced</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial">+ * @flags contains one of the LSM_SETID_* values.</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial">+ * Return 0 on success.</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial"> * @task_setpgid:</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial"> *<span style="white-space:pre-wrap"> </span>Check permission before setting the process group identifier of the</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial"> *<span style="white-space:pre-wrap"> </span>process @p to @pgid.</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial">@@ -1587,6 +1596,8 @@ union security_list_options {</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial"> <span style="white-space:pre-wrap"> </span> enum kernel_read_file_id id);</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial"> <span style="white-space:pre-wrap"> </span>int (*task_fix_setuid)(struct cred *new, const struct cred *old,</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial"> <span style="white-space:pre-wrap"> </span>int flags);</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial">+<span style="white-space:pre-wrap"> </span>int (*task_fix_setgid)(struct cred *new, const struct cred *old,</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial">+<span style="white-space:pre-wrap"> </span>int flags);</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial"> <span style="white-space:pre-wrap"> </span>int (*task_setpgid)(struct task_struct *p, pid_t pgid);</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial"> <span style="white-space:pre-wrap"> </span>int (*task_getpgid)(struct task_struct *p);</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial"> <span style="white-space:pre-wrap"> </span>int (*task_getsid)(struct task_struct *p);</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial">@@ -1876,6 +1887,7 @@ struct security_hook_heads {</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial"> <span style="white-space:pre-wrap"> </span>struct hlist_head kernel_post_read_file;</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial"> <span style="white-space:pre-wrap"> </span>struct hlist_head kernel_module_request;</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial"> <span style="white-space:pre-wrap"> </span>struct hlist_head task_fix_setuid;</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial">+<span style="white-space:pre-wrap"> </span>struct hlist_head task_fix_setgid;</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial"> <span style="white-space:pre-wrap"> </span>struct hlist_head task_setpgid;</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial"> <span style="white-space:pre-wrap"> </span>struct hlist_head task_getpgid;</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial"> <span style="white-space:pre-wrap"> </span>struct hlist_head task_getsid;</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial">diff --git a/include/linux/security.h b/include/linux/security.h</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial">index 63030c85ee19..a82d97cf13ab 100644</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial">--- a/include/linux/security.h</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial">+++ b/include/linux/security.h</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial">@@ -325,6 +325,8 @@ int security_kernel_post_read_file(struct file *file, char *buf, loff_t size,</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial"> <span style="white-space:pre-wrap"> </span> enum kernel_read_file_id id);</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial"> int security_task_fix_setuid(struct cred *new, const struct cred *old,</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial"> <span style="white-space:pre-wrap"> </span> int flags);</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial">+int security_task_fix_setgid(struct cred *new, const struct cred *old,</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial">+<span style="white-space:pre-wrap"> </span> int flags);</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial"> int security_task_setpgid(struct task_struct *p, pid_t pgid);</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial"> int security_task_getpgid(struct task_struct *p);</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial"> int security_task_getsid(struct task_struct *p);</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial">@@ -929,6 +931,13 @@ static inline int security_task_fix_setuid(struct cred *new,</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial"> <span style="white-space:pre-wrap"> </span>return cap_task_fix_setuid(new, old, flags);</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial"> }</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial"> </div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial">+static inline int security_task_fix_setgid(struct cred *new,</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial">+<span style="white-space:pre-wrap"> </span> const struct cred *old,</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial">+<span style="white-space:pre-wrap"> </span> int flags)</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial">+{</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial">+<span style="white-space:pre-wrap"> </span>return 0;</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial">+}</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial">+</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial"> static inline int security_task_setpgid(struct task_struct *p, pid_t pgid)</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial"> {</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial"> <span style="white-space:pre-wrap"> </span>return 0;</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial">diff --git a/kernel/sys.c b/kernel/sys.c</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial">index 38509dc1f77b..f6ef922c6815 100644</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial">--- a/kernel/sys.c</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial">+++ b/kernel/sys.c</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial">@@ -392,6 +392,10 @@ long __sys_setregid(gid_t rgid, gid_t egid)</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial"> <span style="white-space:pre-wrap"> </span>new->sgid = new->egid;</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial"> <span style="white-space:pre-wrap"> </span>new->fsgid = new->egid;</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial"> </div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial">+<span style="white-space:pre-wrap"> </span>retval = security_task_fix_setgid(new, old, LSM_SETID_RE);</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial">+<span style="white-space:pre-wrap"> </span>if (retval < 0)</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial">+<span style="white-space:pre-wrap"> </span>goto error;</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial">+</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial"> <span style="white-space:pre-wrap"> </span>return commit_creds(new);</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial"> </div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial"> error:</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial">@@ -434,6 +438,10 @@ long __sys_setgid(gid_t gid)</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial"> <span style="white-space:pre-wrap"> </span>else</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial"> <span style="white-space:pre-wrap"> </span>goto error;</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial"> </div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial">+<span style="white-space:pre-wrap"> </span>retval = security_task_fix_setgid(new, old, LSM_SETID_ID);</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial">+<span style="white-space:pre-wrap"> </span>if (retval < 0)</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial">+<span style="white-space:pre-wrap"> </span>goto error;</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial">+</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial"> <span style="white-space:pre-wrap"> </span>return commit_creds(new);</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial"> </div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial"> error:</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial">@@ -755,6 +763,10 @@ long __sys_setresgid(gid_t rgid, gid_t egid, gid_t sgid)</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial"> <span style="white-space:pre-wrap"> </span>new->sgid = ksgid;</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial"> <span style="white-space:pre-wrap"> </span>new->fsgid = new->egid;</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial"> </div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial">+<span style="white-space:pre-wrap"> </span>retval = security_task_fix_setgid(new, old, LSM_SETID_RES);</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial">+<span style="white-space:pre-wrap"> </span>if (retval < 0)</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial">+<span style="white-space:pre-wrap"> </span>goto error;</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial">+</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial"> <span style="white-space:pre-wrap"> </span>return commit_creds(new);</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial"> </div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial"> error:</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial">@@ -861,7 +873,8 @@ long __sys_setfsgid(gid_t gid)</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial"> <span style="white-space:pre-wrap"> </span> ns_capable(old->user_ns, CAP_SETGID)) {</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial"> <span style="white-space:pre-wrap"> </span>if (!gid_eq(kgid, old->fsgid)) {</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial"> <span style="white-space:pre-wrap"> </span>new->fsgid = kgid;</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial">-<span style="white-space:pre-wrap"> </span>goto change_okay;</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial">+<span style="white-space:pre-wrap"> </span>if (security_task_fix_setgid(new, old, LSM_SETID_FS) == 0)</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial">+<span style="white-space:pre-wrap"> </span>goto change_okay;</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial"> <span style="white-space:pre-wrap"> </span>}</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial"> <span style="white-space:pre-wrap"> </span>}</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial"> </div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial">diff --git a/security/security.c b/security/security.c</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial">index 68f46d849abe..587786fc0aaa 100644</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial">--- a/security/security.c</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial">+++ b/security/security.c</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial">@@ -1062,6 +1062,12 @@ int security_task_fix_setuid(struct cred *new, const struct cred *old,</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial"> <span style="white-space:pre-wrap"> </span>return call_int_hook(task_fix_setuid, 0, new, old, flags);</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial"> }</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial"> </div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial">+int security_task_fix_setgid(struct cred *new, const struct cred *old,</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial">+<span style="white-space:pre-wrap"> </span> int flags)</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial">+{</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial">+<span style="white-space:pre-wrap"> </span>return call_int_hook(task_fix_setgid, 0, new, old, flags);</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial">+}</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial">+</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial"> int security_task_setpgid(struct task_struct *p, pid_t pgid)</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial"> {</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial"> <span style="white-space:pre-wrap"> </span>return call_int_hook(task_setpgid, 0, p, pgid);</div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial"><br> </div> <div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial"> <div style="font-size:small;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial">--</div> <div style="font-size:small;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial">4.18-rc2<br> </div> <div style="font-size:small;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial"><br> </div> <div style="font-size:small;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial">--</div> </div> <br> </div> </blockquote> <br> </body> </html>
On Tue, 31 Jul 2018, Micah Morton wrote: > +static inline int security_task_fix_setgid(struct cred *new, > + const struct cred *old, > + int flags) > +{ > + return 0; > +} > + This looks whitespace-damaged. Please send patches as plain text.
The ChromiumOS LSM used by ChromeOS will provide a hook for this, in
order to enforce ChromeOS-specific policies regarding which UIDs/GIDs a
process with CAP_SET{UID/GID} can transition to. The
security_task_fix_setuid LSM hook is very helpful in enabling such a feature
for ChromeOS that governs UID transitions, but unfortunately for us it looks
like the equivalent hook that would allow us to govern GID transitions from an
LSM never got added.
Resending with plain text mode enabled.
---
The set*uid system calls all call an LSM fixup hook called
security_task_fix_setuid, which allows for altering the behavior of those
calls by a security module. Comments explaining the LSM_SETID_* constants
in /include/linux/security.h imply that the constants are to be used for
both the set*uid and set*gid syscalls, but the set*gid syscalls do not
have the relevant hooks, meaning a security module can only alter syscalls
that change user identity attributes but not ones that change group
identity attributes. This patch adds the necessary LSM hook, called
security_task_fix_setgid, and calls the hook from the appropriate places
in the set*gid syscalls.Tested by putting a print statement in the hook and
seeing it triggered from the various set*gid syscalls.
Signed-off-by: Micah Morton <mortonm@chromium.org>
Acked-by: Kees Cook <keescook@chromium.org>
---
NOTE: the security_task_fix_setgid line in sys_setfsgid is over 80
characters, but I figured I'd just follow how it was done in sys_setfsuid
rather than trying to wrap the line, since the functions are nearly
identical.
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 8f1131c8dd54..a2166c812a97 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -599,6 +599,15 @@
* @old is the set of credentials that are being replaces
* @flags contains one of the LSM_SETID_* values.
* Return 0 on success.
+ * @task_fix_setgid:
+ * Update the module's state after setting one or more of the group
+ * identity attributes of the current process. The @flags parameter
+ * indicates which of the set*gid system calls invoked this hook.
+ * @new is the set of credentials that will be installed. Modifications
+ * should be made to this rather than to @current->cred.
+ * @old is the set of credentials that are being replaced
+ * @flags contains one of the LSM_SETID_* values.
+ * Return 0 on success.
* @task_setpgid:
* Check permission before setting the process group identifier of the
* process @p to @pgid.
@@ -1587,6 +1596,8 @@ union security_list_options {
enum kernel_read_file_id id);
int (*task_fix_setuid)(struct cred *new, const struct cred *old,
int flags);
+ int (*task_fix_setgid)(struct cred *new, const struct cred *old,
+ int flags);
int (*task_setpgid)(struct task_struct *p, pid_t pgid);
int (*task_getpgid)(struct task_struct *p);
int (*task_getsid)(struct task_struct *p);
@@ -1876,6 +1887,7 @@ struct security_hook_heads {
struct hlist_head kernel_post_read_file;
struct hlist_head kernel_module_request;
struct hlist_head task_fix_setuid;
+ struct hlist_head task_fix_setgid;
struct hlist_head task_setpgid;
struct hlist_head task_getpgid;
struct hlist_head task_getsid;
diff --git a/include/linux/security.h b/include/linux/security.h
index 63030c85ee19..a82d97cf13ab 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -325,6 +325,8 @@ int security_kernel_post_read_file(struct file
*file, char *buf, loff_t size,
enum kernel_read_file_id id);
int security_task_fix_setuid(struct cred *new, const struct cred *old,
int flags);
+int security_task_fix_setgid(struct cred *new, const struct cred *old,
+ int flags);
int security_task_setpgid(struct task_struct *p, pid_t pgid);
int security_task_getpgid(struct task_struct *p);
int security_task_getsid(struct task_struct *p);
@@ -929,6 +931,13 @@ static inline int security_task_fix_setuid(struct
cred *new,
return cap_task_fix_setuid(new, old, flags);
}
+static inline int security_task_fix_setgid(struct cred *new,
+ const struct cred *old,
+ int flags)
+{
+ return 0;
+}
+
static inline int security_task_setpgid(struct task_struct *p, pid_t pgid)
{
return 0;
diff --git a/kernel/sys.c b/kernel/sys.c
index 38509dc1f77b..f6ef922c6815 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -392,6 +392,10 @@ long __sys_setregid(gid_t rgid, gid_t egid)
new->sgid = new->egid;
new->fsgid = new->egid;
+ retval = security_task_fix_setgid(new, old, LSM_SETID_RE);
+ if (retval < 0)
+ goto error;
+
return commit_creds(new);
error:
@@ -434,6 +438,10 @@ long __sys_setgid(gid_t gid)
else
goto error;
+ retval = security_task_fix_setgid(new, old, LSM_SETID_ID);
+ if (retval < 0)
+ goto error;
+
return commit_creds(new);
error:
@@ -755,6 +763,10 @@ long __sys_setresgid(gid_t rgid, gid_t egid, gid_t sgid)
new->sgid = ksgid;
new->fsgid = new->egid;
+ retval = security_task_fix_setgid(new, old, LSM_SETID_RES);
+ if (retval < 0)
+ goto error;
+
return commit_creds(new);
error:
@@ -861,7 +873,8 @@ long __sys_setfsgid(gid_t gid)
ns_capable(old->user_ns, CAP_SETGID)) {
if (!gid_eq(kgid, old->fsgid)) {
new->fsgid = kgid;
- goto change_okay;
+ if (security_task_fix_setgid(new, old, LSM_SETID_FS) == 0)
+ goto change_okay;
}
}
diff --git a/security/security.c b/security/security.c
index 68f46d849abe..587786fc0aaa 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1062,6 +1062,12 @@ int security_task_fix_setuid(struct cred *new,
const struct cred *old,
return call_int_hook(task_fix_setuid, 0, new, old, flags);
}
+int security_task_fix_setgid(struct cred *new, const struct cred *old,
+ int flags)
+{
+ return call_int_hook(task_fix_setgid, 0, new, old, flags);
+}
+
int security_task_setpgid(struct task_struct *p, pid_t pgid)
{
return call_int_hook(task_setpgid, 0, p, pgid);
--
4.18-rc2
--
On Tue, Jul 31, 2018 at 1:09 PM James Morris <jmorris@namei.org> wrote:
>
> On Tue, 31 Jul 2018, Micah Morton wrote:
>
> > +static inline int security_task_fix_setgid(struct cred *new,
> > + const struct cred *old,
> > + int flags)
> > +{
> > + return 0;
> > +}
> > +
>
> This looks whitespace-damaged. Please send patches as plain text.
>
>
> --
> James Morris
> <jmorris@namei.org>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/31/2018 02:47 PM, Micah Morton wrote: > The ChromiumOS LSM used by ChromeOS will provide a hook for this, in > order to enforce ChromeOS-specific policies regarding which UIDs/GIDs a > process with CAP_SET{UID/GID} can transition to. The > security_task_fix_setuid LSM hook is very helpful in enabling such a feature > for ChromeOS that governs UID transitions, but unfortunately for us it looks > like the equivalent hook that would allow us to govern GID transitions from an > LSM never got added. > > Resending with plain text mode enabled. > > --- > > The set*uid system calls all call an LSM fixup hook called > security_task_fix_setuid, which allows for altering the behavior of those > calls by a security module. Comments explaining the LSM_SETID_* constants > in /include/linux/security.h imply that the constants are to be used for > both the set*uid and set*gid syscalls, but the set*gid syscalls do not > have the relevant hooks, meaning a security module can only alter syscalls > that change user identity attributes but not ones that change group > identity attributes. This patch adds the necessary LSM hook, called > security_task_fix_setgid, and calls the hook from the appropriate places > in the set*gid syscalls.Tested by putting a print statement in the hook and > seeing it triggered from the various set*gid syscalls. > > Signed-off-by: Micah Morton <mortonm@chromium.org> > Acked-by: Kees Cook <keescook@chromium.org> > --- > NOTE: the security_task_fix_setgid line in sys_setfsgid is over 80 > characters, but I figured I'd just follow how it was done in sys_setfsuid > rather than trying to wrap the line, since the functions are nearly > identical. > > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h > index 8f1131c8dd54..a2166c812a97 100644 > --- a/include/linux/lsm_hooks.h > +++ b/include/linux/lsm_hooks.h > @@ -599,6 +599,15 @@ > * @old is the set of credentials that are being replaces > * @flags contains one of the LSM_SETID_* values. > * Return 0 on success. > + * @task_fix_setgid: > + * Update the module's state after setting one or more of the group > + * identity attributes of the current process. The @flags parameter > + * indicates which of the set*gid system calls invoked this hook. > + * @new is the set of credentials that will be installed. Modifications > + * should be made to this rather than to @current->cred. > + * @old is the set of credentials that are being replaced > + * @flags contains one of the LSM_SETID_* values. > + * Return 0 on success. > * @task_setpgid: > * Check permission before setting the process group identifier of the > * process @p to @pgid. > @@ -1587,6 +1596,8 @@ union security_list_options { > enum kernel_read_file_id id); > int (*task_fix_setuid)(struct cred *new, const struct cred *old, > int flags); > + int (*task_fix_setgid)(struct cred *new, const struct cred *old, > + int flags); > int (*task_setpgid)(struct task_struct *p, pid_t pgid); > int (*task_getpgid)(struct task_struct *p); > int (*task_getsid)(struct task_struct *p); > @@ -1876,6 +1887,7 @@ struct security_hook_heads { > struct hlist_head kernel_post_read_file; > struct hlist_head kernel_module_request; > struct hlist_head task_fix_setuid; > + struct hlist_head task_fix_setgid; > struct hlist_head task_setpgid; > struct hlist_head task_getpgid; > struct hlist_head task_getsid; > diff --git a/include/linux/security.h b/include/linux/security.h > index 63030c85ee19..a82d97cf13ab 100644 > --- a/include/linux/security.h > +++ b/include/linux/security.h > @@ -325,6 +325,8 @@ int security_kernel_post_read_file(struct file > *file, char *buf, loff_t size, > enum kernel_read_file_id id); > int security_task_fix_setuid(struct cred *new, const struct cred *old, > int flags); > +int security_task_fix_setgid(struct cred *new, const struct cred *old, > + int flags); > int security_task_setpgid(struct task_struct *p, pid_t pgid); > int security_task_getpgid(struct task_struct *p); > int security_task_getsid(struct task_struct *p); > @@ -929,6 +931,13 @@ static inline int security_task_fix_setuid(struct > cred *new, > return cap_task_fix_setuid(new, old, flags); > } > > +static inline int security_task_fix_setgid(struct cred *new, > + const struct cred *old, > + int flags) > +{ > + return 0; > +} > + > static inline int security_task_setpgid(struct task_struct *p, pid_t pgid) > { > return 0; > diff --git a/kernel/sys.c b/kernel/sys.c > index 38509dc1f77b..f6ef922c6815 100644 > --- a/kernel/sys.c > +++ b/kernel/sys.c > @@ -392,6 +392,10 @@ long __sys_setregid(gid_t rgid, gid_t egid) > new->sgid = new->egid; > new->fsgid = new->egid; > > + retval = security_task_fix_setgid(new, old, LSM_SETID_RE); > + if (retval < 0) > + goto error; > + > return commit_creds(new); > > error: > @@ -434,6 +438,10 @@ long __sys_setgid(gid_t gid) > else > goto error; > > + retval = security_task_fix_setgid(new, old, LSM_SETID_ID); > + if (retval < 0) > + goto error; > + > return commit_creds(new); > > error: > @@ -755,6 +763,10 @@ long __sys_setresgid(gid_t rgid, gid_t egid, gid_t sgid) > new->sgid = ksgid; > new->fsgid = new->egid; > > + retval = security_task_fix_setgid(new, old, LSM_SETID_RES); > + if (retval < 0) > + goto error; > + > return commit_creds(new); > > error: > @@ -861,7 +873,8 @@ long __sys_setfsgid(gid_t gid) > ns_capable(old->user_ns, CAP_SETGID)) { > if (!gid_eq(kgid, old->fsgid)) { > new->fsgid = kgid; > - goto change_okay; > + if (security_task_fix_setgid(new, old, LSM_SETID_FS) == 0) > + goto change_okay; > } > } > > diff --git a/security/security.c b/security/security.c > index 68f46d849abe..587786fc0aaa 100644 > --- a/security/security.c > +++ b/security/security.c > @@ -1062,6 +1062,12 @@ int security_task_fix_setuid(struct cred *new, > const struct cred *old, > return call_int_hook(task_fix_setuid, 0, new, old, flags); > } > > +int security_task_fix_setgid(struct cred *new, const struct cred *old, > + int flags) > +{ > + return call_int_hook(task_fix_setgid, 0, new, old, flags); > +} > + > int security_task_setpgid(struct task_struct *p, pid_t pgid) > { > return call_int_hook(task_setpgid, 0, p, pgid); > > -- > 4.18-rc2 > > -- > > On Tue, Jul 31, 2018 at 1:09 PM James Morris <jmorris@namei.org> wrote: >> >> On Tue, 31 Jul 2018, Micah Morton wrote: >> >>> +static inline int security_task_fix_setgid(struct cred *new, >>> + const struct cred *old, >>> + int flags) >>> +{ >>> + return 0; >>> +} >>> + >> >> This looks whitespace-damaged. Please send patches as plain text. Still is ...
On Tue, 31 Jul 2018, Micah Morton wrote: > The ChromiumOS LSM used by ChromeOS will provide a hook for this, in > order to enforce ChromeOS-specific policies regarding which UIDs/GIDs a > process with CAP_SET{UID/GID} can transition to Will you be submitting this LSM to mainline? It's a policy generally of the kernel that we only add features to support in-tree code.
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h index 8f1131c8dd54..a2166c812a97 100644 --- a/include/linux/lsm_hooks.h +++ b/include/linux/lsm_hooks.h @@ -599,6 +599,15 @@ * @old is the set of credentials that are being replaces * @flags contains one of the LSM_SETID_* values. * Return 0 on success. + * @task_fix_setgid: + * Update the module's state after setting one or more of the group + * identity attributes of the current process. The @flags parameter + * indicates which of the set*gid system calls invoked this hook. + * @new is the set of credentials that will be installed. Modifications + * should be made to this rather than to @current->cred. + * @old is the set of credentials that are being replaced + * @flags contains one of the LSM_SETID_* values. + * Return 0 on success. * @task_setpgid: * Check permission before setting the process group identifier of the * process @p to @pgid. @@ -1587,6 +1596,8 @@ union security_list_options { enum kernel_read_file_id id); int (*task_fix_setuid)(struct cred *new, const struct cred *old, int flags); + int (*task_fix_setgid)(struct cred *new, const struct cred *old, + int flags); int (*task_setpgid)(struct task_struct *p, pid_t pgid); int (*task_getpgid)(struct task_struct *p); int (*task_getsid)(struct task_struct *p); @@ -1876,6 +1887,7 @@ struct security_hook_heads { struct hlist_head kernel_post_read_file; struct hlist_head kernel_module_request; struct hlist_head task_fix_setuid; + struct hlist_head task_fix_setgid; struct hlist_head task_setpgid; struct hlist_head task_getpgid; struct hlist_head task_getsid; diff --git a/include/linux/security.h b/include/linux/security.h index 63030c85ee19..a82d97cf13ab 100644 --- a/include/linux/security.h +++ b/include/linux/security.h @@ -325,6 +325,8 @@ int security_kernel_post_read_file(struct file *file, char *buf, loff_t size, enum kernel_read_file_id id); int security_task_fix_setuid(struct cred *new, const struct cred *old, int flags); +int security_task_fix_setgid(struct cred *new, const struct cred *old, + int flags); int security_task_setpgid(struct task_struct *p, pid_t pgid); int security_task_getpgid(struct task_struct *p); int security_task_getsid(struct task_struct *p); @@ -929,6 +931,13 @@ static inline int security_task_fix_setuid(struct cred *new, return cap_task_fix_setuid(new, old, flags); } +static inline int security_task_fix_setgid(struct cred *new, + const struct cred *old, + int flags) +{ + return 0; +} + static inline int security_task_setpgid(struct task_struct *p, pid_t pgid) { return 0; diff --git a/kernel/sys.c b/kernel/sys.c index 38509dc1f77b..f6ef922c6815 100644 --- a/kernel/sys.c +++ b/kernel/sys.c @@ -392,6 +392,10 @@ long __sys_setregid(gid_t rgid, gid_t egid) new->sgid = new->egid; new->fsgid = new->egid; + retval = security_task_fix_setgid(new, old, LSM_SETID_RE); + if (retval < 0) + goto error; + return commit_creds(new); error: @@ -434,6 +438,10 @@ long __sys_setgid(gid_t gid) else goto error; + retval = security_task_fix_setgid(new, old, LSM_SETID_ID); + if (retval < 0) + goto error; + return commit_creds(new); error: @@ -755,6 +763,10 @@ long __sys_setresgid(gid_t rgid, gid_t egid, gid_t sgid) new->sgid = ksgid; new->fsgid = new->egid; + retval = security_task_fix_setgid(new, old, LSM_SETID_RES); + if (retval < 0) + goto error; + return commit_creds(new); error: @@ -861,7 +873,8 @@ long __sys_setfsgid(gid_t gid) ns_capable(old->user_ns, CAP_SETGID)) { if (!gid_eq(kgid, old->fsgid)) { new->fsgid = kgid; - goto change_okay; + if (security_task_fix_setgid(new, old, LSM_SETID_FS) == 0) + goto change_okay; } } diff --git a/security/security.c b/security/security.c index 68f46d849abe..587786fc0aaa 100644 --- a/security/security.c +++ b/security/security.c @@ -1062,6 +1062,12 @@ int security_task_fix_setuid(struct cred *new, const struct cred *old, return call_int_hook(task_fix_setuid, 0, new, old, flags); } +int security_task_fix_setgid(struct cred *new, const struct cred *old, + int flags) +{ + return call_int_hook(task_fix_setgid, 0, new, old, flags); +} + int security_task_setpgid(struct task_struct *p, pid_t pgid) {