Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <CAJ-EccNNCNrA7to85WrYFjRUP7Z1XC+sJ2JXkargiEvXd11AVg@mail.gmail.com>
 <alpine.LRH.2.21.1808010608080.8072@namei.org>
In-Reply-To: <alpine.LRH.2.21.1808010608080.8072@namei.org>
From:   Micah Morton <mortonm@chromium.org>
Date:   Tue, 31 Jul 2018 14:47:02 -0700
Message-ID: <CAJ-EccOMdg370kMfU2=gMbSEyOE3sRVLpca86BHgr+i0_C=J9Q@mail.gmail.com>
Subject: Re: [PATCH v2] security: Add LSM fixup hooks to set*gid syscalls.
To:     jmorris@namei.org
Cc:     linux-security-module@vger.kernel.org, serge@hallyn.com,
        Kees Cook <keescook@chromium.org>, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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