[04/10] LSM: SafeSetID: refactor safesetid_security_capable()
diff mbox series

Message ID 20190410165541.210809-1-mortonm@chromium.org
State New
Headers show
Series
  • [01/10] LSM: SafeSetID: fix pr_warn() to include newline
Related show

Commit Message

Micah Morton April 10, 2019, 4:55 p.m. UTC
From: Jann Horn <jannh@google.com>

At the moment, safesetid_security_capable() has two nested conditional
blocks, and one big comment for all the logic. Chop it up and reduce the
amount of indentation.

Signed-off-by: Jann Horn <jannh@google.com>
Signed-off-by: Micah Morton <mortonm@chromium.org>
---
 security/safesetid/lsm.c | 41 +++++++++++++++++++++++++---------------
 1 file changed, 26 insertions(+), 15 deletions(-)

Comments

Kees Cook April 10, 2019, 5:13 p.m. UTC | #1
On Wed, Apr 10, 2019 at 9:55 AM Micah Morton <mortonm@chromium.org> wrote:
>
> From: Jann Horn <jannh@google.com>
>
> At the moment, safesetid_security_capable() has two nested conditional
> blocks, and one big comment for all the logic. Chop it up and reduce the
> amount of indentation.
>
> Signed-off-by: Jann Horn <jannh@google.com>
> Signed-off-by: Micah Morton <mortonm@chromium.org>

Reviewed-by: Kees Cook <keescook@chromium.org>

-Kees

> ---
>  security/safesetid/lsm.c | 41 +++++++++++++++++++++++++---------------
>  1 file changed, 26 insertions(+), 15 deletions(-)
>
> diff --git a/security/safesetid/lsm.c b/security/safesetid/lsm.c
> index 15cd13b5a211..ab429e1816c5 100644
> --- a/security/safesetid/lsm.c
> +++ b/security/safesetid/lsm.c
> @@ -55,21 +55,32 @@ static int safesetid_security_capable(const struct cred *cred,
>                                       int cap,
>                                       unsigned int opts)
>  {
> -       if (cap == CAP_SETUID &&
> -           setuid_policy_lookup(cred->uid, INVALID_UID) != SIDPOL_DEFAULT) {
> -               if (!(opts & CAP_OPT_INSETID)) {
> -                       /*
> -                        * Deny if we're not in a set*uid() syscall to avoid
> -                        * giving powers gated by CAP_SETUID that are related
> -                        * to functionality other than calling set*uid() (e.g.
> -                        * allowing user to set up userns uid mappings).
> -                        */
> -                       pr_warn("Operation requires CAP_SETUID, which is not available to UID %u for operations besides approved set*uid transitions\n",
> -                               __kuid_val(cred->uid));
> -                       return -1;
> -               }
> -       }
> -       return 0;
> +       /* We're only interested in CAP_SETUID. */
> +       if (cap != CAP_SETUID)
> +               return 0;
> +
> +       /*
> +        * If CAP_SETUID is currently used for a set*uid() syscall, we want to
> +        * let it go through here; the real security check happens later, in the
> +        * task_fix_setuid hook.
> +        */
> +       if ((opts & CAP_OPT_INSETID) != 0)
> +               return 0;
> +
> +       /*
> +        * If no policy applies to this task, allow the use of CAP_SETUID for
> +        * other purposes.
> +        */
> +       if (setuid_policy_lookup(cred->uid, INVALID_UID) == SIDPOL_DEFAULT)
> +               return 0;
> +
> +       /*
> +        * Reject use of CAP_SETUID for functionality other than calling
> +        * set*uid() (e.g. setting up userns uid mappings).
> +        */
> +       pr_warn("Operation requires CAP_SETUID, which is not available to UID %u for operations besides approved set*uid transitions\n",
> +               __kuid_val(cred->uid));
> +       return -1;
>  }
>
>  /*
> --
> 2.21.0.392.gf8f6787159e-goog
>
Micah Morton May 7, 2019, 3:01 p.m. UTC | #2
Ready for merge.

On Wed, Apr 10, 2019 at 10:14 AM Kees Cook <keescook@chromium.org> wrote:
>
> On Wed, Apr 10, 2019 at 9:55 AM Micah Morton <mortonm@chromium.org> wrote:
> >
> > From: Jann Horn <jannh@google.com>
> >
> > At the moment, safesetid_security_capable() has two nested conditional
> > blocks, and one big comment for all the logic. Chop it up and reduce the
> > amount of indentation.
> >
> > Signed-off-by: Jann Horn <jannh@google.com>
> > Signed-off-by: Micah Morton <mortonm@chromium.org>
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
>
> -Kees
>
> > ---
> >  security/safesetid/lsm.c | 41 +++++++++++++++++++++++++---------------
> >  1 file changed, 26 insertions(+), 15 deletions(-)
> >
> > diff --git a/security/safesetid/lsm.c b/security/safesetid/lsm.c
> > index 15cd13b5a211..ab429e1816c5 100644
> > --- a/security/safesetid/lsm.c
> > +++ b/security/safesetid/lsm.c
> > @@ -55,21 +55,32 @@ static int safesetid_security_capable(const struct cred *cred,
> >                                       int cap,
> >                                       unsigned int opts)
> >  {
> > -       if (cap == CAP_SETUID &&
> > -           setuid_policy_lookup(cred->uid, INVALID_UID) != SIDPOL_DEFAULT) {
> > -               if (!(opts & CAP_OPT_INSETID)) {
> > -                       /*
> > -                        * Deny if we're not in a set*uid() syscall to avoid
> > -                        * giving powers gated by CAP_SETUID that are related
> > -                        * to functionality other than calling set*uid() (e.g.
> > -                        * allowing user to set up userns uid mappings).
> > -                        */
> > -                       pr_warn("Operation requires CAP_SETUID, which is not available to UID %u for operations besides approved set*uid transitions\n",
> > -                               __kuid_val(cred->uid));
> > -                       return -1;
> > -               }
> > -       }
> > -       return 0;
> > +       /* We're only interested in CAP_SETUID. */
> > +       if (cap != CAP_SETUID)
> > +               return 0;
> > +
> > +       /*
> > +        * If CAP_SETUID is currently used for a set*uid() syscall, we want to
> > +        * let it go through here; the real security check happens later, in the
> > +        * task_fix_setuid hook.
> > +        */
> > +       if ((opts & CAP_OPT_INSETID) != 0)
> > +               return 0;
> > +
> > +       /*
> > +        * If no policy applies to this task, allow the use of CAP_SETUID for
> > +        * other purposes.
> > +        */
> > +       if (setuid_policy_lookup(cred->uid, INVALID_UID) == SIDPOL_DEFAULT)
> > +               return 0;
> > +
> > +       /*
> > +        * Reject use of CAP_SETUID for functionality other than calling
> > +        * set*uid() (e.g. setting up userns uid mappings).
> > +        */
> > +       pr_warn("Operation requires CAP_SETUID, which is not available to UID %u for operations besides approved set*uid transitions\n",
> > +               __kuid_val(cred->uid));
> > +       return -1;
> >  }
> >
> >  /*
> > --
> > 2.21.0.392.gf8f6787159e-goog
> >
>
>
> --
> Kees Cook

Patch
diff mbox series

diff --git a/security/safesetid/lsm.c b/security/safesetid/lsm.c
index 15cd13b5a211..ab429e1816c5 100644
--- a/security/safesetid/lsm.c
+++ b/security/safesetid/lsm.c
@@ -55,21 +55,32 @@  static int safesetid_security_capable(const struct cred *cred,
 				      int cap,
 				      unsigned int opts)
 {
-	if (cap == CAP_SETUID &&
-	    setuid_policy_lookup(cred->uid, INVALID_UID) != SIDPOL_DEFAULT) {
-		if (!(opts & CAP_OPT_INSETID)) {
-			/*
-			 * Deny if we're not in a set*uid() syscall to avoid
-			 * giving powers gated by CAP_SETUID that are related
-			 * to functionality other than calling set*uid() (e.g.
-			 * allowing user to set up userns uid mappings).
-			 */
-			pr_warn("Operation requires CAP_SETUID, which is not available to UID %u for operations besides approved set*uid transitions\n",
-				__kuid_val(cred->uid));
-			return -1;
-		}
-	}
-	return 0;
+	/* We're only interested in CAP_SETUID. */
+	if (cap != CAP_SETUID)
+		return 0;
+
+	/*
+	 * If CAP_SETUID is currently used for a set*uid() syscall, we want to
+	 * let it go through here; the real security check happens later, in the
+	 * task_fix_setuid hook.
+	 */
+	if ((opts & CAP_OPT_INSETID) != 0)
+		return 0;
+
+	/*
+	 * If no policy applies to this task, allow the use of CAP_SETUID for
+	 * other purposes.
+	 */
+	if (setuid_policy_lookup(cred->uid, INVALID_UID) == SIDPOL_DEFAULT)
+		return 0;
+
+	/*
+	 * Reject use of CAP_SETUID for functionality other than calling
+	 * set*uid() (e.g. setting up userns uid mappings).
+	 */
+	pr_warn("Operation requires CAP_SETUID, which is not available to UID %u for operations besides approved set*uid transitions\n",
+		__kuid_val(cred->uid));
+	return -1;
 }
 
 /*