diff mbox

[3/3] selinux: fix overflow and 0 length allocations

Message ID 1471985365-1197-3-git-send-email-william.c.roberts@intel.com (mailing list archive)
State Changes Requested
Headers show

Commit Message

Roberts, William C Aug. 23, 2016, 8:49 p.m. UTC 
From: William Roberts <william.c.roberts@intel.com>

Throughout the SE Linux LSM, values taken from sepolicy are
used in places where length == 0 or length == <saturated>
matter, find and fix these.

Signed-off-by: William Roberts <william.c.roberts@intel.com>
---
 security/selinux/ss/conditional.c | 3 +++
 security/selinux/ss/policydb.c    | 4 ++++
 security/selinux/ss/private.h     | 7 +++++++
 3 files changed, 14 insertions(+)
 create mode 100644 security/selinux/ss/private.h

Comments

Paul Moore Aug. 23, 2016, 9:55 p.m. UTC | #1 
On Tue, Aug 23, 2016 at 4:49 PM,  <william.c.roberts@intel.com> wrote:
> From: William Roberts <william.c.roberts@intel.com>
>
> Throughout the SE Linux LSM, values taken from sepolicy are

I'll take a closer look at this patchset after LinuxCon/LSS, but
thanks for doing this ... however, one little bikeshed thing that
drives me crazy is the use of "SE Linux" instead of "SELinux".  Don't
change anything you've submitted, but in the future please use SELinux
;)

> used in places where length == 0 or length == <saturated>
> matter, find and fix these.
>
> Signed-off-by: William Roberts <william.c.roberts@intel.com>
> ---
>  security/selinux/ss/conditional.c | 3 +++
>  security/selinux/ss/policydb.c    | 4 ++++
>  security/selinux/ss/private.h     | 7 +++++++
>  3 files changed, 14 insertions(+)
>  create mode 100644 security/selinux/ss/private.h
>
> diff --git a/security/selinux/ss/conditional.c b/security/selinux/ss/conditional.c
> index 456e1a9..ecc0fb6 100644
> --- a/security/selinux/ss/conditional.c
> +++ b/security/selinux/ss/conditional.c
> @@ -16,6 +16,7 @@
>  #include "security.h"
>  #include "conditional.h"
>  #include "services.h"
> +#include "private.h"
>
>  /*
>   * cond_evaluate_expr evaluates a conditional expr
> @@ -242,6 +243,8 @@ int cond_read_bool(struct policydb *p, struct hashtab *h, void *fp)
>                 goto err;
>
>         len = le32_to_cpu(buf[2]);
> +       if (zero_or_saturated(len))
> +               goto err;
>
>         rc = -ENOMEM;
>         key = kmalloc(len + 1, GFP_KERNEL);
> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
> index 4b24385..0e881f3 100644
> --- a/security/selinux/ss/policydb.c
> +++ b/security/selinux/ss/policydb.c
> @@ -38,6 +38,7 @@
>  #include "conditional.h"
>  #include "mls.h"
>  #include "services.h"
> +#include "private.h"
>
>  #define _DEBUG_HASHES
>
> @@ -1094,6 +1095,9 @@ static int str_read(char **strp, gfp_t flags, void *fp, u32 len)
>         int rc;
>         char *str;
>
> +       if (zero_or_saturated(len))
> +               return -EINVAL;
> +
>         str = kmalloc(len + 1, flags);
>         if (!str)
>                 return -ENOMEM;
> diff --git a/security/selinux/ss/private.h b/security/selinux/ss/private.h
> new file mode 100644
> index 0000000..0e81a78
> --- /dev/null
> +++ b/security/selinux/ss/private.h
> @@ -0,0 +1,7 @@
> +#ifndef PRIVATE_H_
> +#define PRIVATE_H_
> +
> +#define is_saturated(x) (x == (typeof(x))-1)
> +#define zero_or_saturated(x) ((x == 0) || is_saturated(x))
> +
> +#endif
> --
> 1.9.1
>
> _______________________________________________
> Selinux mailing list
> Selinux@tycho.nsa.gov
> To unsubscribe, send email to Selinux-leave@tycho.nsa.gov.
> To get help, send an email containing "help" to Selinux-request@tycho.nsa.gov.
Paul Moore Aug. 29, 2016, 11:55 p.m. UTC | #2 
On Tue, Aug 23, 2016 at 4:49 PM,  <william.c.roberts@intel.com> wrote:
> From: William Roberts <william.c.roberts@intel.com>
>
> Throughout the SE Linux LSM, values taken from sepolicy are
> used in places where length == 0 or length == <saturated>
> matter, find and fix these.
>
> Signed-off-by: William Roberts <william.c.roberts@intel.com>
> ---
>  security/selinux/ss/conditional.c | 3 +++
>  security/selinux/ss/policydb.c    | 4 ++++
>  security/selinux/ss/private.h     | 7 +++++++
>  3 files changed, 14 insertions(+)
>  create mode 100644 security/selinux/ss/private.h
>
> diff --git a/security/selinux/ss/conditional.c b/security/selinux/ss/conditional.c
> index 456e1a9..ecc0fb6 100644
> --- a/security/selinux/ss/conditional.c
> +++ b/security/selinux/ss/conditional.c
> @@ -16,6 +16,7 @@
>  #include "security.h"
>  #include "conditional.h"
>  #include "services.h"
> +#include "private.h"
>
>  /*
>   * cond_evaluate_expr evaluates a conditional expr
> @@ -242,6 +243,8 @@ int cond_read_bool(struct policydb *p, struct hashtab *h, void *fp)
>                 goto err;
>
>         len = le32_to_cpu(buf[2]);
> +       if (zero_or_saturated(len))
> +               goto err;
>
>         rc = -ENOMEM;
>         key = kmalloc(len + 1, GFP_KERNEL);
> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
> index 4b24385..0e881f3 100644
> --- a/security/selinux/ss/policydb.c
> +++ b/security/selinux/ss/policydb.c
> @@ -38,6 +38,7 @@
>  #include "conditional.h"
>  #include "mls.h"
>  #include "services.h"
> +#include "private.h"
>
>  #define _DEBUG_HASHES
>
> @@ -1094,6 +1095,9 @@ static int str_read(char **strp, gfp_t flags, void *fp, u32 len)
>         int rc;
>         char *str;
>
> +       if (zero_or_saturated(len))
> +               return -EINVAL;
> +
>         str = kmalloc(len + 1, flags);
>         if (!str)
>                 return -ENOMEM;
> diff --git a/security/selinux/ss/private.h b/security/selinux/ss/private.h
> new file mode 100644
> index 0000000..0e81a78
> --- /dev/null
> +++ b/security/selinux/ss/private.h
> @@ -0,0 +1,7 @@
> +#ifndef PRIVATE_H_
> +#define PRIVATE_H_
> +
> +#define is_saturated(x) (x == (typeof(x))-1)
> +#define zero_or_saturated(x) ((x == 0) || is_saturated(x))
> +
> +#endif

While I'm not opposed to the idea of using a macro for this purpose,
e.g. is_saturated() and zero_or_saturated(), I don't see much value in
creating this macro buried in the SELinux directory.  Especially if we
end up creating a new header file just for these macros.  Something as
generic as this should be something we inherit from the generic kernel
code so we can leverage existing conventions.

If you can find a macro like these in the core kernel code, go ahead
and use it, otherwise please respin this with the bound checks open
coded.

Oh, and use "SELinux" ;)
William Roberts Aug. 30, 2016, 5:54 a.m. UTC | #3 
On Aug 29, 2016 16:56, "Paul Moore" <paul@paul-moore.com> wrote:
>
> On Tue, Aug 23, 2016 at 4:49 PM,  <william.c.roberts@intel.com> wrote:
> > From: William Roberts <william.c.roberts@intel.com>
> >
> > Throughout the SE Linux LSM, values taken from sepolicy are
> > used in places where length == 0 or length == <saturated>
> > matter, find and fix these.
> >
> > Signed-off-by: William Roberts <william.c.roberts@intel.com>
> > ---
> >  security/selinux/ss/conditional.c | 3 +++
> >  security/selinux/ss/policydb.c    | 4 ++++
> >  security/selinux/ss/private.h     | 7 +++++++
> >  3 files changed, 14 insertions(+)
> >  create mode 100644 security/selinux/ss/private.h
> >
> > diff --git a/security/selinux/ss/conditional.c
b/security/selinux/ss/conditional.c
> > index 456e1a9..ecc0fb6 100644
> > --- a/security/selinux/ss/conditional.c
> > +++ b/security/selinux/ss/conditional.c
> > @@ -16,6 +16,7 @@
> >  #include "security.h"
> >  #include "conditional.h"
> >  #include "services.h"
> > +#include "private.h"
> >
> >  /*
> >   * cond_evaluate_expr evaluates a conditional expr
> > @@ -242,6 +243,8 @@ int cond_read_bool(struct policydb *p, struct
hashtab *h, void *fp)
> >                 goto err;
> >
> >         len = le32_to_cpu(buf[2]);
> > +       if (zero_or_saturated(len))
> > +               goto err;
> >
> >         rc = -ENOMEM;
> >         key = kmalloc(len + 1, GFP_KERNEL);
> > diff --git a/security/selinux/ss/policydb.c
b/security/selinux/ss/policydb.c
> > index 4b24385..0e881f3 100644
> > --- a/security/selinux/ss/policydb.c
> > +++ b/security/selinux/ss/policydb.c
> > @@ -38,6 +38,7 @@
> >  #include "conditional.h"
> >  #include "mls.h"
> >  #include "services.h"
> > +#include "private.h"
> >
> >  #define _DEBUG_HASHES
> >
> > @@ -1094,6 +1095,9 @@ static int str_read(char **strp, gfp_t flags,
void *fp, u32 len)
> >         int rc;
> >         char *str;
> >
> > +       if (zero_or_saturated(len))
> > +               return -EINVAL;
> > +
> >         str = kmalloc(len + 1, flags);
> >         if (!str)
> >                 return -ENOMEM;
> > diff --git a/security/selinux/ss/private.h
b/security/selinux/ss/private.h
> > new file mode 100644
> > index 0000000..0e81a78
> > --- /dev/null
> > +++ b/security/selinux/ss/private.h
> > @@ -0,0 +1,7 @@
> > +#ifndef PRIVATE_H_
> > +#define PRIVATE_H_
> > +
> > +#define is_saturated(x) (x == (typeof(x))-1)
> > +#define zero_or_saturated(x) ((x == 0) || is_saturated(x))
> > +
> > +#endif
>
> While I'm not opposed to the idea of using a macro for this purpose,
> e.g. is_saturated() and zero_or_saturated(), I don't see much value in
> creating this macro buried in the SELinux directory.  Especially if we
> end up creating a new header file just for these macros.  Something as
> generic as this should be something we inherit from the generic kernel
> code so we can leverage existing conventions.

It made more sense when I wanted to keep the change similar to libsepol.
But the str_read() function really reduced the number of checks. I ended up
porting that to libsepol. With that said, the macro serves little purpose
with the exception of readability.

>
> If you can find a macro like these in the core kernel code, go ahead

Ill look, doubt I'll find anything.

> and use it, otherwise please respin this with the bound checks open
> code

Sure

>
> Oh, and use "SELinux" ;)
>
> --
> paul moore
> www.paul-moore.com
> _______________________________________________
> Selinux mailing list
> Selinux@tycho.nsa.gov
> To unsubscribe, send email to Selinux-leave@tycho.nsa.gov.
> To get help, send an email containing "help" to
Selinux-request@tycho.nsa.gov.
diff mbox

Patch

diff --git a/security/selinux/ss/conditional.c b/security/selinux/ss/conditional.c
index 456e1a9..ecc0fb6 100644
--- a/security/selinux/ss/conditional.c
+++ b/security/selinux/ss/conditional.c
@@ -16,6 +16,7 @@ 
 #include "security.h"
 #include "conditional.h"
 #include "services.h"
+#include "private.h"
 
 /*
  * cond_evaluate_expr evaluates a conditional expr
@@ -242,6 +243,8 @@  int cond_read_bool(struct policydb *p, struct hashtab *h, void *fp)
 		goto err;
 
 	len = le32_to_cpu(buf[2]);
+	if (zero_or_saturated(len))
+		goto err;
 
 	rc = -ENOMEM;
 	key = kmalloc(len + 1, GFP_KERNEL);
diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
index 4b24385..0e881f3 100644
--- a/security/selinux/ss/policydb.c
+++ b/security/selinux/ss/policydb.c
@@ -38,6 +38,7 @@ 
 #include "conditional.h"
 #include "mls.h"
 #include "services.h"
+#include "private.h"
 
 #define _DEBUG_HASHES
 
@@ -1094,6 +1095,9 @@  static int str_read(char **strp, gfp_t flags, void *fp, u32 len)
 	int rc;
 	char *str;
 
+	if (zero_or_saturated(len))
+		return -EINVAL;
+
 	str = kmalloc(len + 1, flags);
 	if (!str)
 		return -ENOMEM;
diff --git a/security/selinux/ss/private.h b/security/selinux/ss/private.h
new file mode 100644
index 0000000..0e81a78
--- /dev/null
+++ b/security/selinux/ss/private.h
@@ -0,0 +1,7 @@ 
+#ifndef PRIVATE_H_
+#define PRIVATE_H_
+
+#define is_saturated(x) (x == (typeof(x))-1)
+#define zero_or_saturated(x) ((x == 0) || is_saturated(x))
+
+#endif