RE: [PATCH 1/3] selinux: detect invalid ebitmap

[Roberts, William C]

> -Original Message-
> From: Paul Moore [mailto:p...@paul-moore.com]
> Sent: Monday, August 29, 2016 4:21 PM
> To: Roberts, William C 
> Cc: selinux@tycho.nsa.gov; seandroid-l...@tycho.nsa.gov; Stephen Smalley
> 
> Subject: Re: [PATCH 1/3] selinux: detect invalid ebitmap
> 
> On Tue, Aug 23, 2016 at 4:49 PM,   wrote:
> > From: William Roberts 
> >
> > When count is 0 and the highbit is not zero, the ebitmap is not valid
> > and the internal node is not allocated. This causes issues when
> > routines, like mls_context_isvalid() attempt to use the
> > ebitmap_for_each_bit() and ebitmap_node_get_bit() as they assume a
> > highbit > 0 will have a node allocated.
> > ---
> >  security/selinux/ss/ebitmap.c | 3 +++
> >  1 file changed, 3 insertions(+)
> 
> Hi William,
> 
> This patch looks good to me, but do I have your permission to add your 
> sign-off?

Yes, I guess I missed it. Just so it's easy for you to copy paste:
Signed-off-by: William Roberts 


> 
> > diff --git a/security/selinux/ss/ebitmap.c
> > b/security/selinux/ss/ebitmap.c index 894b6cd..7d10e5d 100644
> > --- a/security/selinux/ss/ebitmap.c
> > +++ b/security/selinux/ss/ebitmap.c
> > @@ -374,6 +374,9 @@ int ebitmap_read(struct ebitmap *e, void *fp)
> > goto ok;
> > }
> >
> > +   if (e->highbit && !count)
> > +   goto bad;
> > +
> > for (i = 0; i < count; i++) {
> > rc = next_entry(, fp, sizeof(u32));
> > if (rc < 0) {
> > --
> > 1.9.1
> >
> > ___
> > Selinux mailing list
> > Selinux@tycho.nsa.gov
> > To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
> > To get help, send an email containing "help" to 
> > selinux-requ...@tycho.nsa.gov.
> 
> 
> 
> --
> paul moore
> www.paul-moore.com

___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.

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

[Paul Moore]

On Tue, Aug 23, 2016 at 4:49 PM,   wrote:
> From: William Roberts 
>
> Throughout the SE Linux LSM, values taken from sepolicy are
> used in places where length == 0 or length == 
> matter, find and fix these.
>
> Signed-off-by: William Roberts 
> ---
>  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 000..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" ;)

-- 
paul moore
www.paul-moore.com
___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.

Re: [PATCH 1/3] selinux: detect invalid ebitmap

[Paul Moore]

On Tue, Aug 23, 2016 at 4:49 PM,   wrote:
> From: William Roberts 
>
> When count is 0 and the highbit is not zero, the ebitmap is not
> valid and the internal node is not allocated. This causes issues
> when routines, like mls_context_isvalid() attempt to use the
> ebitmap_for_each_bit() and ebitmap_node_get_bit() as they assume
> a highbit > 0 will have a node allocated.
> ---
>  security/selinux/ss/ebitmap.c | 3 +++
>  1 file changed, 3 insertions(+)

Hi William,

This patch looks good to me, but do I have your permission to add your sign-off?

> diff --git a/security/selinux/ss/ebitmap.c b/security/selinux/ss/ebitmap.c
> index 894b6cd..7d10e5d 100644
> --- a/security/selinux/ss/ebitmap.c
> +++ b/security/selinux/ss/ebitmap.c
> @@ -374,6 +374,9 @@ int ebitmap_read(struct ebitmap *e, void *fp)
> goto ok;
> }
>
> +   if (e->highbit && !count)
> +   goto bad;
> +
> for (i = 0; i < count; i++) {
> rc = next_entry(, fp, sizeof(u32));
> if (rc < 0) {
> --
> 1.9.1
>
> ___
> Selinux mailing list
> Selinux@tycho.nsa.gov
> To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
> To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.



-- 
paul moore
www.paul-moore.com
___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.

Re: [PATCH] security: lsm_audit: print pid and tid

[Paul Moore]

On Wed, Aug 17, 2016 at 4:58 PM, Paul Moore  wrote:
> On Tue, Jul 26, 2016 at 10:54 AM, Jeff Vander Stoep  wrote:
>> dump_common_audit_data() currently contains a field for pid, but the
>> value printed is actually the thread ID, tid. Update this value to
>> return the task group ID. Add a new field for tid. With this change
>> the values printed by audit now match the values returned by the
>> getpid() and gettid() syscalls.
>>
>> Signed-off-by: Jeff Vander Stoep 
>> ---
>>  security/lsm_audit.c | 7 +--
>>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> Hi Jeff,
>
> Have you tested this against the audit-testsuite[1]?  We don't have an
> explicit PID test yet, but at least two of the tests do test it as a
> side effect.
>
> Steve, I don't see the thread ID listed in the field dictionary, are
> you okay with using "tid" for this?
>
> However, as far as I can see, the biggest problem with this patch is
> that it adds a field in the middle of a record which will likely cause
> the audit userspace tools to explode (or so I've been warned in the
> past).  Steve, what say you about the userspace?
>
> [1] https://github.com/linux-audit/audit-testsuite
> [2] 
> https://github.com/linux-audit/audit-documentation/blob/master/specs/fields/field-dictionary.csv

Steve?

>> diff --git a/security/lsm_audit.c b/security/lsm_audit.c
>> index cccbf30..57f26c1 100644
>> --- a/security/lsm_audit.c
>> +++ b/security/lsm_audit.c
>> @@ -220,7 +220,8 @@ static void dump_common_audit_data(struct audit_buffer 
>> *ab,
>>  */
>> BUILD_BUG_ON(sizeof(a->u) > sizeof(void *)*2);
>>
>> -   audit_log_format(ab, " pid=%d comm=", task_pid_nr(current));
>> +   audit_log_format(ab, " pid=%d tid=%d comm=", task_tgid_vnr(tsk),
>> +   task_pid_vnr(tsk));
>> audit_log_untrustedstring(ab, memcpy(comm, current->comm, 
>> sizeof(comm)));
>>
>> switch (a->type) {
>> @@ -294,10 +295,12 @@ static void dump_common_audit_data(struct audit_buffer 
>> *ab,
>> case LSM_AUDIT_DATA_TASK: {
>> struct task_struct *tsk = a->u.tsk;
>> if (tsk) {
>> -   pid_t pid = task_pid_nr(tsk);
>> +   pid_t pid = task_tgid_vnr(tsk);
>> if (pid) {
>> char comm[sizeof(tsk->comm)];
>> audit_log_format(ab, " opid=%d ocomm=", pid);
>> +   audit_log_format(ab, " opid=%d otid=%d 
>> ocomm=",
>> +   pid, task_pid_vnr(tsk));
>> audit_log_untrustedstring(ab,
>> memcpy(comm, tsk->comm, sizeof(comm)));
>> }
>
> --
> paul moore
> www.paul-moore.com

-- 
paul moore
www.paul-moore.com
___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.

[PATCH] [RFC] nodups_specs: speedup

[william . c . roberts]

From: William Roberts 

I noticed, via gprof, that the time spent in nodups_specs()
accounts for 100% of the label_open() call.

It seems as though the N^2 comparison using strcmp is very
slow.

Do two major things:
1. move the rec->validating check to the left, check the simple
   thing first before runnning the expensive strcmp(). strcmp()
   is used to check string equality, check lengths first.
2. strlen() is used all over to calculate lengths, just store
   it in a struct with the string so its usable elsewhere, rather
   than recalculating it.

text 21.4% speedup:
before text: 248
after text: 195

binary 24.6% speedup:
before bin: 236
after bin: 178

Some things to ponder:
1. We can use C ABI safe pointer instead of len_str structure
   https://bitbucket.org/billcroberts/twist
   There are pros and cons to this approach, namely if someone
   calls free(x) instead of twist_free(x)
   It also currently has 0 support for stack based strings (simple
   enough to add). I think this approach is overkill here.

2. The location of the str_len struct and routines should likely
   move elsewhere.

3. The impact on Android is currently unmeasured, that's next.
   Also, bionic uses something from label_file.h for processing
   property_contexts for the Android property subsystem... so
   need to ensure that all works as advertised still.

Things to do:
1. Cleanup the code locations, likely a util.h or a len_str.h,
   a better name would be nice.
2. Spell check this commit message

Signed-off-by: William Roberts 
---
 libselinux/src/label.c  |  8 ++--
 libselinux/src/label_android_property.c | 44 +--
 libselinux/src/label_db.c   |  7 +--
 libselinux/src/label_file.c | 63 ++
 libselinux/src/label_file.h | 78 +++--
 libselinux/src/label_internal.h | 22 +-
 libselinux/src/label_media.c|  5 ++-
 libselinux/src/label_support.c  | 32 --
 libselinux/src/label_x.c|  5 ++-
 libselinux/src/matchpathcon.c   |  2 +-
 10 files changed, 159 insertions(+), 107 deletions(-)

diff --git a/libselinux/src/label.c b/libselinux/src/label.c
index 963bfcb..11324ea 100644
--- a/libselinux/src/label.c
+++ b/libselinux/src/label.c
@@ -209,7 +209,7 @@ int selabel_validate(struct selabel_handle *rec,
if (!rec->validating || contexts->validated)
goto out;
 
-   rc = selinux_validate(>ctx_raw);
+   rc = selinux_validate(>ctx_raw.str);
if (rc < 0)
goto out;
 
@@ -248,7 +248,7 @@ static int selabel_fini(struct selabel_handle *rec,
return -1;
 
if (translating && !lr->ctx_trans &&
-   selinux_raw_to_trans_context(lr->ctx_raw, >ctx_trans))
+   selinux_raw_to_trans_context(lr->ctx_raw.str, >ctx_trans))
return -1;
 
return 0;
@@ -369,7 +369,7 @@ int selabel_lookup_raw(struct selabel_handle *rec, char 
**con,
if (!lr)
return -1;
 
-   *con = strdup(lr->ctx_raw);
+   *con = strdup(lr->ctx_raw.str);
return *con ? 0 : -1;
 }
 
@@ -429,7 +429,7 @@ int selabel_lookup_best_match_raw(struct selabel_handle 
*rec, char **con,
if (!lr)
return -1;
 
-   *con = strdup(lr->ctx_raw);
+   *con = strdup(lr->ctx_raw.str);
return *con ? 0 : -1;
 }
 
diff --git a/libselinux/src/label_android_property.c 
b/libselinux/src/label_android_property.c
index 290b438..af0b9a8 100644
--- a/libselinux/src/label_android_property.c
+++ b/libselinux/src/label_android_property.c
@@ -16,7 +16,7 @@
 /* A property security context specification. */
 typedef struct spec {
struct selabel_lookup_rec lr;   /* holds contexts for lookup result */
-   char *property_key; /* property key string */
+   struct len_str property_key;/* property key string */
 } spec_t;
 
 /* Our stored configuration */
@@ -33,13 +33,13 @@ static int cmp(const void *A, const void *B)
 {
const struct spec *sp1 = A, *sp2 = B;
 
-   if (strncmp(sp1->property_key, "*", 1) == 0)
+   if (strncmp(sp1->property_key.str, "*", 1) == 0)
return 1;
-   if (strncmp(sp2->property_key, "*", 1) == 0)
+   if (strncmp(sp2->property_key.str, "*", 1) == 0)
return -1;
 
-   size_t L1 = strlen(sp1->property_key);
-   size_t L2 = strlen(sp2->property_key);
+   size_t L1 = sp1->property_key.len;
+   size_t L2 = sp2->property_key.len;
 
return (L1 < L2) - (L1 > L2);
 }
@@ -56,23 +56,23 @@ static int nodups_specs(struct saved_data *data, const char 
*path)
for (ii = 0; ii < data->nspec; ii++) {
curr_spec = _arr[ii];
for (jj = ii + 1; jj < data->nspec; jj++) {
-   if (!strcmp(spec_arr[jj].property_key,
-