Re: [PATCH v2] security: Add LSM fixup hooks to set*gid syscalls.

[Casey Schaufler]

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@xxxxxxxxxxxx>

Acked-by: Kees Cook <keescook@xxxxxxxxxxxx>

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

--