Re: [PATCH v3] selinux: simplify mls_context_to_sid()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 11/13/18 10:14 PM, Paul Moore wrote:
On Tue, Nov 13, 2018 at 4:10 PM Stephen Smalley <sds@xxxxxxxxxxxxx> wrote:
On 11/12/18 6:44 AM, Ondrej Mosnacek wrote:
This function has only two callers, but only one of them actually needs
the special logic at the beginning. Factoring this logic out into
string_to_context_struct() allows us to drop the arguments 'oldc', 's',
and 'def_sid'.

Signed-off-by: Ondrej Mosnacek <omosnace@xxxxxxxxxx>
---

Changes in v3:
   - correct the comment about policy read lock

Changes in v2:
   - also drop unneeded #include's from mls.c

   security/selinux/ss/mls.c      | 49 +++++-----------------------------
   security/selinux/ss/mls.h      |  5 +---
   security/selinux/ss/services.c | 32 +++++++++++++++++++---
   3 files changed, 36 insertions(+), 50 deletions(-)

diff --git a/security/selinux/ss/mls.c b/security/selinux/ss/mls.c
index 2fe459df3c85..d1da928a7e77 100644
--- a/security/selinux/ss/mls.c
+++ b/security/selinux/ss/mls.c
@@ -24,10 +24,7 @@
   #include <linux/string.h>
   #include <linux/errno.h>
   #include <net/netlabel.h>
-#include "sidtab.h"
   #include "mls.h"
-#include "policydb.h"
-#include "services.h"

   /*
    * Return the length in bytes for the MLS fields of the
@@ -223,20 +220,12 @@ int mls_context_isvalid(struct policydb *p, struct context *c)
    * This function modifies the string in place, inserting
    * NULL characters to terminate the MLS fields.
    *
- * If a def_sid is provided and no MLS field is present,
- * copy the MLS field of the associated default context.
- * Used for upgraded to MLS systems where objects may lack
- * MLS fields.
- *
- * Policy read-lock must be held for sidtab lookup.
+ * Policy read-lock must be held for policy data lookup.
    *
    */
   int mls_context_to_sid(struct policydb *pol,
-                    char oldc,
                      char *scontext,
-                    struct context *context,
-                    struct sidtab *s,
-                    u32 def_sid)
+                    struct context *context)
   {
       char *sensitivity, *cur_cat, *next_cat, *rngptr;
       struct level_datum *levdatum;
@@ -244,29 +233,6 @@ int mls_context_to_sid(struct policydb *pol,
       int l, rc, i;
       char *rangep[2];

-     if (!pol->mls_enabled) {
-             if ((def_sid != SECSID_NULL && oldc) || (*scontext) == '\0')
-                     return 0;
-             return -EINVAL;
-     }
-
-     /*
-      * No MLS component to the security context, try and map to
-      * default if provided.
-      */
-     if (!oldc) {
-             struct context *defcon;
-
-             if (def_sid == SECSID_NULL)
-                     return -EINVAL;
-
-             defcon = sidtab_search(s, def_sid);
-             if (!defcon)
-                     return -EINVAL;
-
-             return mls_context_cpy(context, defcon);
-     }
-
       /*
        * If we're dealing with a range, figure out where the two parts
        * of the range begin.
@@ -364,14 +330,11 @@ int mls_from_string(struct policydb *p, char *str, struct context *context,
               return -EINVAL;

       tmpstr = kstrdup(str, gfp_mask);
-     if (!tmpstr) {
-             rc = -ENOMEM;
-     } else {
-             rc = mls_context_to_sid(p, ':', tmpstr, context,
-                                     NULL, SECSID_NULL);
-             kfree(tmpstr);
-     }
+     if (!tmpstr)
+             return -ENOMEM;

+     rc = mls_context_to_sid(p, tmpstr, context);
+     kfree(tmpstr);
       return rc;
   }

diff --git a/security/selinux/ss/mls.h b/security/selinux/ss/mls.h
index 67093647576d..e2498f78e100 100644
--- a/security/selinux/ss/mls.h
+++ b/security/selinux/ss/mls.h
@@ -33,11 +33,8 @@ int mls_range_isvalid(struct policydb *p, struct mls_range *r);
   int mls_level_isvalid(struct policydb *p, struct mls_level *l);

   int mls_context_to_sid(struct policydb *p,
-                    char oldc,
                      char *scontext,
-                    struct context *context,
-                    struct sidtab *s,
-                    u32 def_sid);
+                    struct context *context);

   int mls_from_string(struct policydb *p, char *str, struct context *context,
                   gfp_t gfp_mask);
diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index 12e414394530..ccad4334f99d 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -1425,9 +1425,35 @@ static int string_to_context_struct(struct policydb *pol,

       ctx->type = typdatum->value;

-     rc = mls_context_to_sid(pol, oldc, p, ctx, sidtabp, def_sid);
-     if (rc)
-             goto out;
+     if (!pol->mls_enabled) {
+             rc = -EINVAL;
+             if ((def_sid == SECSID_NULL || !oldc) && (*p) != '\0')
+                     goto out;

I don't think this is your bug, but unless I'm mistaken, p could be OOB
and be dereferenced here.  Seems to have been introduced by
95ffe194204ae3cef88d0b59be209204bbe9b3be.  Likely not caught in testing
since both Linux distributions and Android enable MLS to use the
category sets for isolation.

Yep, and we should fix that in v4.20-rcX independent of this patch.  I
think if we simply remove the "(*scontext) == '\0'" from the check we
should be okay; I believe the only time we would want to return 0 when
not running a MLS policy would be when there is something in the MLS
portion of the label.

Yes, that should restore the previous behavior. The desired behavior is that if MLS is disabled, we reject the context as invalid if it has a MLS field unless the context was fetched from an xattr, which we only permit to provide filesystem compatibility between MLS and non-MLS systems.



[Index of Archives]     [Selinux Refpolicy]     [Linux SGX]     [Fedora Users]     [Fedora Desktop]     [Yosemite Photos]     [Yosemite Camping]     [Yosemite Campsites]     [KDE Users]     [Gnome Users]

  Powered by Linux