Re: [PATCH v3 1/2] selinux: add brief info to policydb

[William Roberts]

On Fri, May 12, 2017 at 3:22 PM, Paul Moore  wrote:
>
> On Thu, May 11, 2017 at 4:45 PM, Casey Schaufler  
> wrote:
> > On 5/11/2017 1:22 PM, Stephen Smalley wrote:
> >> On Thu, 2017-05-11 at 08:56 -0700, Casey Schaufler wrote:
> >>> On 5/11/2017 5:59 AM, Sebastien Buisson wrote:
>  Add policybrief field to struct policydb. It holds a brief info
>  of the policydb, in the following form:
>  <0 or 1 for enforce>:<0 or 1 for checkreqprot>:=
>  Policy brief is computed every time the policy is loaded, and when
>  enforce or checkreqprot are changed.
> 
>  Add security_policy_brief hook to give access to policy brief to
>  the rest of the kernel. Lustre client makes use of this information
>  to detect changes to the policy, and forward it to Lustre servers.
>  Depending on how the policy is enforced on Lustre client side,
>  Lustre servers can refuse connection.
> 
>  Signed-off-by: Sebastien Buisson 
>  ---
>   include/linux/lsm_hooks.h   | 16 
>   include/linux/security.h|  7 
>   security/security.c |  6 +++
>   security/selinux/hooks.c|  7 
>   security/selinux/include/security.h |  2 +
>   security/selinux/selinuxfs.c|  2 +
>   security/selinux/ss/policydb.c  | 76
>  +
>   security/selinux/ss/policydb.h  |  2 +
>   security/selinux/ss/services.c  | 62
>  ++
>   9 files changed, 180 insertions(+)
> 
>  diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
>  index 080f34e..9cac282 100644
>  --- a/include/linux/lsm_hooks.h
>  +++ b/include/linux/lsm_hooks.h
>  @@ -1336,6 +1336,20 @@
>    * @inode we wish to get the security context of.
>    * @ctx is a pointer in which to place the allocated
>  security context.
>    * @ctxlen points to the place to put the length of @ctx.
>  + *
>  + * Security hooks for policy brief
>  + *
>  + * @policy_brief:
>  + *
>  + * Returns a string containing a brief info of the
>  policydb, in the
>  + * following form:
>  + * <0 or 1 for enforce>:<0 or 1 for
>  checkreqprot>:=
> >>>
> >>> This sure looks like SELinux specific information. If the Spiffy
> >>> security module has multiple values for enforcement (e.g. off,
> >>> soft and hard) this interface definition does not work. What is a
> >>> "checkreqprot", and what is it for?
> >>>
> >>> I expect that you have no interest (or incentive) in supporting
> >>> security modules other than SELinux, and that's OK. What's I'm
> >>> after is an interface that another security module could use if
> >>> someone where interested (or inspired) to do so.
> >>>
> >>> Rather than a string with predefined positional values (something
> >>> I was taught not to do when 1 MIPS and 1 MEG was a big computer)
> >>> you might use
> >>> "enforce=:checkreqprot=:hashalg="
> >>
> >> No objection to the above, although it makes his updating code for
> >> enforce/checkreqprot a bit uglier.
> >
> > Sure, but can you imagine trying to use find(1) if the
> > options where positional?
>
> Perhaps I'm suffering from audit induced PTSD, but I think we need to
> operate under the assumption that we are going to need to augment this
> at some point in the future (no good feature goes un-abused) and I'd
> much rather have some sort of name-value pairing to keep my sanity.  I
> also feel rather strongly that we should make it very explicit in the
> comments that the ordering of the fields in the string may change.
>
> >>> for SELinux and define @policy_brief as
> >>>
> >>>  A string containing colon separated name and value pairs
> >>>  that will be parsed and interpreted by the security module
> >>>  or modules.
> >>
> >> Actually, I'm not clear it will be parsed or interpreted by the
> >> security module(s).  I think he is just fetching it and then doing a
> >> simple comparison to check for inconsistencies between clients and the
> >> server, and optionally admins/users can read it and interpret it as
> >> they see fit.
> >
> > OK, in which case human eyes *need* the name as well as the value.
> > That, and strcmp(value, "enforce=0") is no harder than strcmp(value, "0").
>
> The initial use case may not require parsing the string, but who knows
> what will end up using this five years from now.  Once again, I agree
> with Casey, let's make sure it easily parsed and readable by admins;
> imagine this as the output of a file under /proc or /sys.
>
> >>> You already have it right for the "hashalg" field. If you want to
> >>> be really forward looking you could use names field names that
> >>> identify the security module that uses them, such as
> >>>
> >>>  "selinux/enforce=1:selinux/checkreqprot=0:selinux/MD5=8675309"
> >> That seems a bit 

Re: [PATCH v3 1/2] selinux: add brief info to policydb

[William Roberts]

On Fri, May 12, 2017 at 3:22 PM, Paul Moore  wrote:

> On Thu, May 11, 2017 at 4:45 PM, Casey Schaufler 
> wrote:
> > On 5/11/2017 1:22 PM, Stephen Smalley wrote:
> >> On Thu, 2017-05-11 at 08:56 -0700, Casey Schaufler wrote:
> >>> On 5/11/2017 5:59 AM, Sebastien Buisson wrote:
>  Add policybrief field to struct policydb. It holds a brief info
>  of the policydb, in the following form:
>  <0 or 1 for enforce>:<0 or 1 for checkreqprot>:=
>  Policy brief is computed every time the policy is loaded, and when
>  enforce or checkreqprot are changed.
> 
>  Add security_policy_brief hook to give access to policy brief to
>  the rest of the kernel. Lustre client makes use of this information
>  to detect changes to the policy, and forward it to Lustre servers.
>  Depending on how the policy is enforced on Lustre client side,
>  Lustre servers can refuse connection.
> 
>  Signed-off-by: Sebastien Buisson 
>  ---
>   include/linux/lsm_hooks.h   | 16 
>   include/linux/security.h|  7 
>   security/security.c |  6 +++
>   security/selinux/hooks.c|  7 
>   security/selinux/include/security.h |  2 +
>   security/selinux/selinuxfs.c|  2 +
>   security/selinux/ss/policydb.c  | 76
>  +
>   security/selinux/ss/policydb.h  |  2 +
>   security/selinux/ss/services.c  | 62
>  ++
>   9 files changed, 180 insertions(+)
> 
>  diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
>  index 080f34e..9cac282 100644
>  --- a/include/linux/lsm_hooks.h
>  +++ b/include/linux/lsm_hooks.h
>  @@ -1336,6 +1336,20 @@
>    * @inode we wish to get the security context of.
>    * @ctx is a pointer in which to place the allocated
>  security context.
>    * @ctxlen points to the place to put the length of @ctx.
>  + *
>  + * Security hooks for policy brief
>  + *
>  + * @policy_brief:
>  + *
>  + * Returns a string containing a brief info of the
>  policydb, in the
>  + * following form:
>  + * <0 or 1 for enforce>:<0 or 1 for
>  checkreqprot>:=
> >>>
> >>> This sure looks like SELinux specific information. If the Spiffy
> >>> security module has multiple values for enforcement (e.g. off,
> >>> soft and hard) this interface definition does not work. What is a
> >>> "checkreqprot", and what is it for?
> >>>
> >>> I expect that you have no interest (or incentive) in supporting
> >>> security modules other than SELinux, and that's OK. What's I'm
> >>> after is an interface that another security module could use if
> >>> someone where interested (or inspired) to do so.
> >>>
> >>> Rather than a string with predefined positional values (something
> >>> I was taught not to do when 1 MIPS and 1 MEG was a big computer)
> >>> you might use
> >>> "enforce=:checkreqprot=:hashalg="
> >>
> >> No objection to the above, although it makes his updating code for
> >> enforce/checkreqprot a bit uglier.
> >
> > Sure, but can you imagine trying to use find(1) if the
> > options where positional?
>
> Perhaps I'm suffering from audit induced PTSD, but I think we need to
> operate under the assumption that we are going to need to augment this
> at some point in the future (no good feature goes un-abused) and I'd
> much rather have some sort of name-value pairing to keep my sanity.  I
> also feel rather strongly that we should make it very explicit in the
> comments that the ordering of the fields in the string may change.
>
> >>> for SELinux and define @policy_brief as
> >>>
> >>>  A string containing colon separated name and value pairs
> >>>  that will be parsed and interpreted by the security module
> >>>  or modules.
> >>
> >> Actually, I'm not clear it will be parsed or interpreted by the
> >> security module(s).  I think he is just fetching it and then doing a
> >> simple comparison to check for inconsistencies between clients and the
> >> server, and optionally admins/users can read it and interpret it as
> >> they see fit.
> >
> > OK, in which case human eyes *need* the name as well as the value.
> > That, and strcmp(value, "enforce=0") is no harder than strcmp(value,
> "0").
>
> The initial use case may not require parsing the string, but who knows
> what will end up using this five years from now.  Once again, I agree
> with Casey, let's make sure it easily parsed and readable by admins;
> imagine this as the output of a file under /proc or /sys.
>
> >>> You already have it right for the "hashalg" field. If you want to
> >>> be really forward looking you could use names field names that
> >>> identify the security module that uses them, such as
> >>>
> >>>  "selinux/enforce=1:selinux/checkreqprot=0:selinux/MD5=8675309"
> >> That seems a bit 

Re: [PATCH v3 1/2] selinux: add brief info to policydb

[Paul Moore]

On Thu, May 11, 2017 at 4:45 PM, Casey Schaufler  wrote:
> On 5/11/2017 1:22 PM, Stephen Smalley wrote:
>> On Thu, 2017-05-11 at 08:56 -0700, Casey Schaufler wrote:
>>> On 5/11/2017 5:59 AM, Sebastien Buisson wrote:
 Add policybrief field to struct policydb. It holds a brief info
 of the policydb, in the following form:
 <0 or 1 for enforce>:<0 or 1 for checkreqprot>:=
 Policy brief is computed every time the policy is loaded, and when
 enforce or checkreqprot are changed.

 Add security_policy_brief hook to give access to policy brief to
 the rest of the kernel. Lustre client makes use of this information
 to detect changes to the policy, and forward it to Lustre servers.
 Depending on how the policy is enforced on Lustre client side,
 Lustre servers can refuse connection.

 Signed-off-by: Sebastien Buisson 
 ---
  include/linux/lsm_hooks.h   | 16 
  include/linux/security.h|  7 
  security/security.c |  6 +++
  security/selinux/hooks.c|  7 
  security/selinux/include/security.h |  2 +
  security/selinux/selinuxfs.c|  2 +
  security/selinux/ss/policydb.c  | 76
 +
  security/selinux/ss/policydb.h  |  2 +
  security/selinux/ss/services.c  | 62
 ++
  9 files changed, 180 insertions(+)

 diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
 index 080f34e..9cac282 100644
 --- a/include/linux/lsm_hooks.h
 +++ b/include/linux/lsm_hooks.h
 @@ -1336,6 +1336,20 @@
   * @inode we wish to get the security context of.
   * @ctx is a pointer in which to place the allocated
 security context.
   * @ctxlen points to the place to put the length of @ctx.
 + *
 + * Security hooks for policy brief
 + *
 + * @policy_brief:
 + *
 + * Returns a string containing a brief info of the
 policydb, in the
 + * following form:
 + * <0 or 1 for enforce>:<0 or 1 for
 checkreqprot>:=
>>>
>>> This sure looks like SELinux specific information. If the Spiffy
>>> security module has multiple values for enforcement (e.g. off,
>>> soft and hard) this interface definition does not work. What is a
>>> "checkreqprot", and what is it for?
>>>
>>> I expect that you have no interest (or incentive) in supporting
>>> security modules other than SELinux, and that's OK. What's I'm
>>> after is an interface that another security module could use if
>>> someone where interested (or inspired) to do so.
>>>
>>> Rather than a string with predefined positional values (something
>>> I was taught not to do when 1 MIPS and 1 MEG was a big computer)
>>> you might use
>>> "enforce=:checkreqprot=:hashalg="
>>
>> No objection to the above, although it makes his updating code for
>> enforce/checkreqprot a bit uglier.
>
> Sure, but can you imagine trying to use find(1) if the
> options where positional?

Perhaps I'm suffering from audit induced PTSD, but I think we need to
operate under the assumption that we are going to need to augment this
at some point in the future (no good feature goes un-abused) and I'd
much rather have some sort of name-value pairing to keep my sanity.  I
also feel rather strongly that we should make it very explicit in the
comments that the ordering of the fields in the string may change.

>>> for SELinux and define @policy_brief as
>>>
>>>  A string containing colon separated name and value pairs
>>>  that will be parsed and interpreted by the security module
>>>  or modules.
>>
>> Actually, I'm not clear it will be parsed or interpreted by the
>> security module(s).  I think he is just fetching it and then doing a
>> simple comparison to check for inconsistencies between clients and the
>> server, and optionally admins/users can read it and interpret it as
>> they see fit.
>
> OK, in which case human eyes *need* the name as well as the value.
> That, and strcmp(value, "enforce=0") is no harder than strcmp(value, "0").

The initial use case may not require parsing the string, but who knows
what will end up using this five years from now.  Once again, I agree
with Casey, let's make sure it easily parsed and readable by admins;
imagine this as the output of a file under /proc or /sys.

>>> You already have it right for the "hashalg" field. If you want to
>>> be really forward looking you could use names field names that
>>> identify the security module that uses them, such as
>>>
>>>  "selinux/enforce=1:selinux/checkreqprot=0:selinux/MD5=8675309"
>> That seems a bit verbose, particularly the duplicated selinux/ bit.
>
> True that, on the other hand
>
> "selinux(enforce=1:checkreqprot=0:MD5=8675309)"
>
> would be harder to parse. Either works for me.

I'm not sure I care too much about how the name-value pairs get
namespaced, but 

Re: [PATCH] procattr.c: Use __BIONIC__ instead of __ANDROID__

[William Roberts]

On Fri, May 12, 2017 at 11:01 AM, Tom Cherry  wrote:

> On Fri, May 12, 2017 at 6:22 AM, Stephen Smalley 
> wrote:
> > On Thu, 2017-05-11 at 16:50 -0700, Tom Cherry via Selinux wrote:
> >> This check is not specific to Android devices. If libselinux were
> >> used
> >> with Bionic on a normal Linux system this check would still be
> >> needed.
> >>
> >> Signed-off-by: Tom Cherry 
> >
> > Thanks, applied.  This was actually switched from ANDROID to _ANDROID__
> >  by nnk in 044f6ef104c8a9d8f42faa8756e71a0525198f5b.  We don't appear
> > to have any other uses of __ANDROID__, but we do have a number of uses
> > of ANDROID.  Offhand though these other uses of ANDROID don't appear to
> > be related to bionic but instead reflect differences in SELinux
> > userspace integration in Android vs GNU/Linux.  If however your
> > bionic/Linux system integrates SELinux support in an Android-like
> > manner (e.g. kernel policy file is /sepolicy, no /etc/selinux/config,
> > ...), then you might need to generalize those as well.
>
> Thanks!  I took a look at the other ANDROID ifdefs in the code and I'm not
> sure
> how or if they would need to be changed.  This change is needed to get
> libselinux building with bionic on the host which is just the first step.
>

Thanks for checking, and I am glad (and surprised) this was the only issue
to get
it to build for a host using bionic.


>
> >
> >> ---
> >>  libselinux/src/procattr.c | 4 ++--
> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/libselinux/src/procattr.c b/libselinux/src/procattr.c
> >> index ebc0adec..48dd8aff 100644
> >> --- a/libselinux/src/procattr.c
> >> +++ b/libselinux/src/procattr.c
> >> @@ -22,8 +22,8 @@ static pthread_key_t destructor_key;
> >>  static int destructor_key_initialized = 0;
> >>  static __thread char destructor_initialized;
> >>
> >> -#ifndef __ANDROID__
> >> -/* Android declares this in unistd.h and has a definition for it */
> >> +#ifndef __BIONIC__
> >> +/* Bionic declares this in unistd.h and has a definition for it */
> >>  static pid_t gettid(void)
> >>  {
> >>   return syscall(__NR_gettid);
>



-- 
Respectfully,

William C Roberts

Re: Possible use after free in selabel_subs_init

[William Roberts]

On Fri, May 12, 2017 at 1:26 PM, Nicolas Iooss 
wrote:

> Hi,
>
> Currently libselinux/src/label.c defines selabel_subs_init() like this [1]:
>
> struct selabel_sub *selabel_subs_init(/* ... */)
> {
> /* ... */
> while (fgets_unlocked(buf, sizeof(buf) - 1, cfg)) {
> /* ... allocate and fill "sub" ... */
> sub->next = list;
> list = sub;
> }
>
> if (digest_add_specfile(digest, cfg, NULL, sb.st_size, path) < 0)
> goto err;
>
> out:
> fclose(cfg);
> return list;
> err:
> if (sub)
> free(sub->src);
> free(sub);
> goto out;
> }
>
> When digest_add_specfile() fails, sub is freed (in the err label), but
> as list=sub, it means that the return value, list, is freed. This
> leads to a use-after-free when this value is used.
>
> What should selabel_subs_init() do (and return) when
> digest_add_specfile() fails?
>
> Nicolas
>
> [1] https://github.com/SELinuxProject/selinux/blob/
> 9cc62ce35d099acf7897b6259228479737521709/libselinux/src/label.c#L94
>
>
>From what I can tell, and I may be wrong, is that the only callers are from
init() in label_file.c. So the first two failure returns where it returns
list,
list i think would be NULL there. Which makes sense since the first
iteration, the linked list next pointer would be NULL. I think we would
want to free
the whole list and return NULL and have callers check for NULL. This seems
like a fatal error path. But I am wrong frequently :-)

Possible use after free in selabel_subs_init

[Nicolas Iooss]

Hi,

Currently libselinux/src/label.c defines selabel_subs_init() like this [1]:

struct selabel_sub *selabel_subs_init(/* ... */)
{
/* ... */
while (fgets_unlocked(buf, sizeof(buf) - 1, cfg)) {
/* ... allocate and fill "sub" ... */
sub->next = list;
list = sub;
}

if (digest_add_specfile(digest, cfg, NULL, sb.st_size, path) < 0)
goto err;

out:
fclose(cfg);
return list;
err:
if (sub)
free(sub->src);
free(sub);
goto out;
}

When digest_add_specfile() fails, sub is freed (in the err label), but
as list=sub, it means that the return value, list, is freed. This
leads to a use-after-free when this value is used.

What should selabel_subs_init() do (and return) when
digest_add_specfile() fails?

Nicolas

[1] 
https://github.com/SELinuxProject/selinux/blob/9cc62ce35d099acf7897b6259228479737521709/libselinux/src/label.c#L94

[PATCH 1/2] libsepol/cil: do not use an uninitialized value in __cil_fqn_qualify_blocks

[Nicolas Iooss]

In __cil_fqn_qualify_blocks(), when newlen >= CIL_MAX_NAME_LENGTH,
cil_tree_log() is called with child_args.node as argument but this value
has not been initialized yet. Use local variable node instead, which is
initialized early enough in the function.

This issue has been found using clang's static analyzer.

Signed-off-by: Nicolas Iooss 
---
 libsepol/cil/src/cil_fqn.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libsepol/cil/src/cil_fqn.c b/libsepol/cil/src/cil_fqn.c
index dad13474c823..717358a24eb7 100644
--- a/libsepol/cil/src/cil_fqn.c
+++ b/libsepol/cil/src/cil_fqn.c
@@ -121,7 +121,7 @@ static int __cil_fqn_qualify_blocks(__attribute__((unused)) 
hashtab_key_t k, has
 
 exit:
if (rc != SEPOL_OK) {
-   cil_tree_log(child_args.node, CIL_ERR,"Problem qualifying names 
in block");
+   cil_tree_log(node, CIL_ERR,"Problem qualifying names in block");
}
 
return rc;
-- 
2.12.2

Re: [PATCH] procattr.c: Use __BIONIC__ instead of __ANDROID__

[Tom Cherry via Selinux]

On Fri, May 12, 2017 at 6:22 AM, Stephen Smalley  wrote:
> On Thu, 2017-05-11 at 16:50 -0700, Tom Cherry via Selinux wrote:
>> This check is not specific to Android devices. If libselinux were
>> used
>> with Bionic on a normal Linux system this check would still be
>> needed.
>>
>> Signed-off-by: Tom Cherry 
>
> Thanks, applied.  This was actually switched from ANDROID to _ANDROID__
>  by nnk in 044f6ef104c8a9d8f42faa8756e71a0525198f5b.  We don't appear
> to have any other uses of __ANDROID__, but we do have a number of uses
> of ANDROID.  Offhand though these other uses of ANDROID don't appear to
> be related to bionic but instead reflect differences in SELinux
> userspace integration in Android vs GNU/Linux.  If however your
> bionic/Linux system integrates SELinux support in an Android-like
> manner (e.g. kernel policy file is /sepolicy, no /etc/selinux/config,
> ...), then you might need to generalize those as well.

Thanks!  I took a look at the other ANDROID ifdefs in the code and I'm not sure
how or if they would need to be changed.  This change is needed to get
libselinux building with bionic on the host which is just the first step.

>
>> ---
>>  libselinux/src/procattr.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/libselinux/src/procattr.c b/libselinux/src/procattr.c
>> index ebc0adec..48dd8aff 100644
>> --- a/libselinux/src/procattr.c
>> +++ b/libselinux/src/procattr.c
>> @@ -22,8 +22,8 @@ static pthread_key_t destructor_key;
>>  static int destructor_key_initialized = 0;
>>  static __thread char destructor_initialized;
>>
>> -#ifndef __ANDROID__
>> -/* Android declares this in unistd.h and has a definition for it */
>> +#ifndef __BIONIC__
>> +/* Bionic declares this in unistd.h and has a definition for it */
>>  static pid_t gettid(void)
>>  {
>>   return syscall(__NR_gettid);

Re: [PATCH 7/9] semanage: Update semanage to allow runtime labeling of Infiniband Pkeys

[Stephen Smalley]

On Thu, 2017-05-11 at 22:51 +, Daniel Jurgens wrote:
> On 5/10/2017 2:22 PM, Stephen Smalley wrote:
> > On Tue, 2017-05-09 at 23:50 +0300, Dan Jurgens wrote:
> > > From: Daniel Jurgens 
> > > 
> > > 
> > >  libsepol/src/ibpkeys.c|  264
> > > ++
> > >  python/semanage/semanage  |   60 +++-
> > >  python/semanage/seobject.py   |  253
> > > +
> > >  28 files changed, 2159 insertions(+), 17 deletions(-)
> > 
> > That's a lot of code.  Did you look at whether you could generalize
> > the
> > port record stuff at all to see if we could factor out common
> > helpers
> > or anything?  I guess this is consistent with the current code, but
> > it
> > seems like a lot of very similar code being duplicated and then
> > slightly tweaked.
> 
> I don't see a good way to generalize.  The high/low pkey/port part
> overlaps, but all that code is compact anyway.  To make it work for
> both would complicate it to figure out the correct key/ocontext to
> use.  The protocol and subnet_prefix handling is most of the code,
> and it's very different.

Yes, looking at it more closely, I agree.  Thanks for looking at it
anyway.

> > 
> > >  
> > >   create_dir(newroot_path(), 0o755)
> > > diff --git a/libsepol/VERSION b/libsepol/VERSION
> > > index 5154b3f..e70b452 100644
> > > --- a/libsepol/VERSION
> > > +++ b/libsepol/VERSION
> > > @@ -1 +1 @@
> > > -2.6
> > > +2.6.0
> > 
> > Extraneous change?
> 
> Yes.
> > > +struct sepol_ibpkey {
> > > + /* Subnet prefix */
> > > + char *subnet_prefix;
> > > + size_t subnet_prefix_sz;
> > 
> > Do we need support for variable-length subnet prefix?  Can it
> > change?
> 
> It doesn't need to be variable.  I'll remove.
> > > +#ifdef DARWIN
> > > + memcpy(subnet_prefix_bytes, in_addr.s6_addr, 16);
> > > +#else
> > > + memcpy(subnet_prefix_bytes, in_addr.s6_addr32, 16);
> > > +#endif
> > 
> > Just reduce to always using s6_addr
> 
> Done
> > > +static int ibpkey_alloc_subnet_prefix(sepol_handle_t *handle,
> > > +   char **subnet_prefix,
> > > +   size_t *subnet_prefix_sz)
> > > +{
> > > + char *tmp_subnet_prefix = malloc(16);
> > > + size_t tmp_subnet_prefix_sz = 16;
> > 
> > No magic constants, and definitely not repeatedly used.
> 
> Done

[PATCH v2] selinux: log policy capability state when a policy is loaded

[Stephen Smalley]

Log the state of SELinux policy capabilities when a policy is loaded.
For each policy capability known to the kernel, log an informational
message with the policy capability name and the value set in the policy.
For policy capabilities that are set in the loaded policy but unknown
to the kernel, log a warning with the policy capability index, since
this is the only information presently available in the policy.

Sample output with a policy created with a new capability defined
that is not known to the kernel:
[   43.812265] SELinux:  policy capability network_peer_controls=1
[   43.812266] SELinux:  policy capability open_perms=1
[   43.812266] SELinux:  policy capability extended_socket_class=1
[   43.812267] SELinux:  policy capability always_check_network=0
[   43.812267] SELinux:  policy capability cgroup_seclabel=0
[   43.812268] SELinux:  unknown policy capability 5

Signed-off-by: Stephen Smalley 
---
v2 drops the Resolves line since I think we are not supposed to include
bug tracking info in upstream kernel commit messages (correct me if wrong).

 security/selinux/include/security.h |  2 ++
 security/selinux/selinuxfs.c| 13 ++---
 security/selinux/ss/services.c  | 23 +++
 3 files changed, 27 insertions(+), 11 deletions(-)

diff --git a/security/selinux/include/security.h 
b/security/selinux/include/security.h
index f979c35..c4224bb 100644
--- a/security/selinux/include/security.h
+++ b/security/selinux/include/security.h
@@ -76,6 +76,8 @@ enum {
 };
 #define POLICYDB_CAPABILITY_MAX (__POLICYDB_CAPABILITY_MAX - 1)
 
+extern char *selinux_policycap_names[__POLICYDB_CAPABILITY_MAX];
+
 extern int selinux_policycap_netpeer;
 extern int selinux_policycap_openperm;
 extern int selinux_policycap_extsockclass;
diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
index ce71718..ea2da91 100644
--- a/security/selinux/selinuxfs.c
+++ b/security/selinux/selinuxfs.c
@@ -41,15 +41,6 @@
 #include "objsec.h"
 #include "conditional.h"
 
-/* Policy capability filenames */
-static char *policycap_names[] = {
-   "network_peer_controls",
-   "open_perms",
-   "extended_socket_class",
-   "always_check_network",
-   "cgroup_seclabel"
-};
-
 unsigned int selinux_checkreqprot = CONFIG_SECURITY_SELINUX_CHECKREQPROT_VALUE;
 
 static int __init checkreqprot_setup(char *str)
@@ -1750,9 +1741,9 @@ static int sel_make_policycap(void)
sel_remove_entries(policycap_dir);
 
for (iter = 0; iter <= POLICYDB_CAPABILITY_MAX; iter++) {
-   if (iter < ARRAY_SIZE(policycap_names))
+   if (iter < ARRAY_SIZE(selinux_policycap_names))
dentry = d_alloc_name(policycap_dir,
- policycap_names[iter]);
+ selinux_policycap_names[iter]);
else
dentry = d_alloc_name(policycap_dir, "unknown");
 
diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index 60d9b02..0abdec2 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -70,6 +70,15 @@
 #include "ebitmap.h"
 #include "audit.h"
 
+/* Policy capability names */
+char *selinux_policycap_names[__POLICYDB_CAPABILITY_MAX] = {
+   "network_peer_controls",
+   "open_perms",
+   "extended_socket_class",
+   "always_check_network",
+   "cgroup_seclabel"
+};
+
 int selinux_policycap_netpeer;
 int selinux_policycap_openperm;
 int selinux_policycap_extsockclass;
@@ -1986,6 +1995,9 @@ static int convert_context(u32 key,
 
 static void security_load_policycaps(void)
 {
+   unsigned int i;
+   struct ebitmap_node *node;
+
selinux_policycap_netpeer = ebitmap_get_bit(,
  POLICYDB_CAPABILITY_NETPEER);
selinux_policycap_openperm = ebitmap_get_bit(,
@@ -1997,6 +2009,17 @@ static void security_load_policycaps(void)
selinux_policycap_cgroupseclabel =
ebitmap_get_bit(,
POLICYDB_CAPABILITY_CGROUPSECLABEL);
+
+   for (i = 0; i < ARRAY_SIZE(selinux_policycap_names); i++)
+   pr_info("SELinux:  policy capability %s=%d\n",
+   selinux_policycap_names[i],
+   ebitmap_get_bit(, i));
+
+   ebitmap_for_each_positive_bit(, node, i) {
+   if (i >= ARRAY_SIZE(selinux_policycap_names))
+   pr_warn("SELinux:  unknown policy capability %u\n",
+   i);
+   }
 }
 
 static int security_preserve_bools(struct policydb *p);
-- 
2.9.3

[PATCH] selinux: do not check open permission on sockets

[Stephen Smalley]

open permission is currently only defined for files in the kernel
(COMMON_FILE_PERMS rather than COMMON_FILE_SOCK_PERMS). Construction of
an artificial test case that tries to open a socket via /proc/pid/fd will
generate a recvfrom avc denial because recvfrom and open happen to map to
the same permission bit in socket vs file classes.

open of a socket via /proc/pid/fd is not supported by the kernel regardless
and will ultimately return ENXIO. But we hit the permission check first and
can thus produce these odd/misleading denials.  Omit the open check when
operating on a socket.

Signed-off-by: Stephen Smalley 
---
 security/selinux/hooks.c | 10 +++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index e67a526..dd356ed 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -2063,8 +2063,9 @@ static inline u32 file_to_av(struct file *file)
 static inline u32 open_file_to_av(struct file *file)
 {
u32 av = file_to_av(file);
+   struct inode *inode = file_inode(file);
 
-   if (selinux_policycap_openperm)
+   if (selinux_policycap_openperm && inode->i_sb->s_magic != SOCKFS_MAGIC)
av |= FILE__OPEN;
 
return av;
@@ -3059,6 +3060,7 @@ static int selinux_inode_permission(struct inode *inode, 
int mask)
 static int selinux_inode_setattr(struct dentry *dentry, struct iattr *iattr)
 {
const struct cred *cred = current_cred();
+   struct inode *inode = d_backing_inode(dentry);
unsigned int ia_valid = iattr->ia_valid;
__u32 av = FILE__WRITE;
 
@@ -3074,8 +3076,10 @@ static int selinux_inode_setattr(struct dentry *dentry, 
struct iattr *iattr)
ATTR_ATIME_SET | ATTR_MTIME_SET | ATTR_TIMES_SET))
return dentry_has_perm(cred, dentry, FILE__SETATTR);
 
-   if (selinux_policycap_openperm && (ia_valid & ATTR_SIZE)
-   && !(ia_valid & ATTR_FILE))
+   if (selinux_policycap_openperm &&
+   inode->i_sb->s_magic != SOCKFS_MAGIC &&
+   (ia_valid & ATTR_SIZE) &&
+   !(ia_valid & ATTR_FILE))
av |= FILE__OPEN;
 
return dentry_has_perm(cred, dentry, av);
-- 
2.9.3

Re: [RFC][PATCH] selinux: add a map permission check for mmap

[Stephen Smalley]

On Fri, 2017-05-05 at 09:14 -0400, Stephen Smalley wrote:
> Add a map permission check on mmap so that we can distinguish memory
> mapped
> access (since it has different implications for revocation). When a
> file
> is opened and then read or written via syscalls like
> read(2)/write(2),
> we revalidate access on each read/write operation via
> selinux_file_permission() and therefore can revoke access if the
> process context, the file context, or the policy changes in such a
> manner that access is no longer allowed. When a file is opened and
> then
> memory mapped via mmap(2) and then subsequently read or written
> directly
> in memory, we presently have no way to revalidate or revoke access.
> The purpose of a separate map permission check on mmap(2) is to
> permit
> policy to prohibit memory mapping of specific files for which we need
> to ensure that every access is revalidated, particularly useful for
> scenarios where we expect the file to be relabeled at runtime in
> order
> to reflect state changes (e.g. cross-domain solution, assured
> pipeline
> without data copying).
> 
> Signed-off-by: Stephen Smalley 

Unless you have any comments/objections, feel free to apply this as is
- I believe it is good to go, and I have also posted the corresponding
selinux-testsuite patch.

> ---
> NB I chose not to define a new policy capability for this permission,
> since it is adequately covered by handle_unknown for compatibility
> and
> others seemed to agree that this does not fall into the category of
> changes requiring a new policy capability.  I also chose to define
> the
> permission for socket classes in addition to file classes and let it
> be checked for both.
> 
>  security/selinux/hooks.c| 12 
>  security/selinux/include/classmap.h |  2 +-
>  2 files changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index e67a526..5432628 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -3550,6 +3550,18 @@ static int selinux_mmap_addr(unsigned long
> addr)
>  static int selinux_mmap_file(struct file *file, unsigned long
> reqprot,
>    unsigned long prot, unsigned long
> flags)
>  {
> + struct common_audit_data ad;
> + int rc;
> +
> + if (file) {
> + ad.type = LSM_AUDIT_DATA_FILE;
> + ad.u.file = file;
> + rc = inode_has_perm(current_cred(),
> file_inode(file),
> + FILE__MAP, );
> + if (rc)
> + return rc;
> + }
> +
>   if (selinux_checkreqprot)
>   prot = reqprot;
>  
> diff --git a/security/selinux/include/classmap.h
> b/security/selinux/include/classmap.h
> index 1e0cc9b..3e49a78 100644
> --- a/security/selinux/include/classmap.h
> +++ b/security/selinux/include/classmap.h
> @@ -1,7 +1,7 @@
>  #include 
>  
>  #define COMMON_FILE_SOCK_PERMS "ioctl", "read", "write", "create", \
> -"getattr", "setattr", "lock", "relabelfrom", "relabelto",
> "append"
> +"getattr", "setattr", "lock", "relabelfrom", "relabelto",
> "append", "map"
>  
>  #define COMMON_FILE_PERMS COMMON_FILE_SOCK_PERMS, "unlink", "link",
> \
>  "rename", "execute", "quotaon", "mounton", "audit_access", \

[PATCH] selinux: log policy capability state when a policy is loaded

[Stephen Smalley]

Log the state of SELinux policy capabilities when a policy is loaded.
For each policy capability known to the kernel, log an informational
message with the policy capability name and the value set in the policy.
For policy capabilities that are set in the loaded policy but unknown
to the kernel, log a warning with the policy capability index, since
this is the only information presently available in the policy.

Sample output with a policy created with a new capability defined
that is not known to the kernel:
[   43.812265] SELinux:  policy capability network_peer_controls=1
[   43.812266] SELinux:  policy capability open_perms=1
[   43.812266] SELinux:  policy capability extended_socket_class=1
[   43.812267] SELinux:  policy capability always_check_network=0
[   43.812267] SELinux:  policy capability cgroup_seclabel=0
[   43.812268] SELinux:  unknown policy capability 5

Resolves: #32
Signed-off-by: Stephen Smalley 
---
 security/selinux/include/security.h |  2 ++
 security/selinux/selinuxfs.c| 13 ++---
 security/selinux/ss/services.c  | 23 +++
 3 files changed, 27 insertions(+), 11 deletions(-)

diff --git a/security/selinux/include/security.h 
b/security/selinux/include/security.h
index f979c35..c4224bb 100644
--- a/security/selinux/include/security.h
+++ b/security/selinux/include/security.h
@@ -76,6 +76,8 @@ enum {
 };
 #define POLICYDB_CAPABILITY_MAX (__POLICYDB_CAPABILITY_MAX - 1)
 
+extern char *selinux_policycap_names[__POLICYDB_CAPABILITY_MAX];
+
 extern int selinux_policycap_netpeer;
 extern int selinux_policycap_openperm;
 extern int selinux_policycap_extsockclass;
diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
index ce71718..ea2da91 100644
--- a/security/selinux/selinuxfs.c
+++ b/security/selinux/selinuxfs.c
@@ -41,15 +41,6 @@
 #include "objsec.h"
 #include "conditional.h"
 
-/* Policy capability filenames */
-static char *policycap_names[] = {
-   "network_peer_controls",
-   "open_perms",
-   "extended_socket_class",
-   "always_check_network",
-   "cgroup_seclabel"
-};
-
 unsigned int selinux_checkreqprot = CONFIG_SECURITY_SELINUX_CHECKREQPROT_VALUE;
 
 static int __init checkreqprot_setup(char *str)
@@ -1750,9 +1741,9 @@ static int sel_make_policycap(void)
sel_remove_entries(policycap_dir);
 
for (iter = 0; iter <= POLICYDB_CAPABILITY_MAX; iter++) {
-   if (iter < ARRAY_SIZE(policycap_names))
+   if (iter < ARRAY_SIZE(selinux_policycap_names))
dentry = d_alloc_name(policycap_dir,
- policycap_names[iter]);
+ selinux_policycap_names[iter]);
else
dentry = d_alloc_name(policycap_dir, "unknown");
 
diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index 60d9b02..0abdec2 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -70,6 +70,15 @@
 #include "ebitmap.h"
 #include "audit.h"
 
+/* Policy capability names */
+char *selinux_policycap_names[__POLICYDB_CAPABILITY_MAX] = {
+   "network_peer_controls",
+   "open_perms",
+   "extended_socket_class",
+   "always_check_network",
+   "cgroup_seclabel"
+};
+
 int selinux_policycap_netpeer;
 int selinux_policycap_openperm;
 int selinux_policycap_extsockclass;
@@ -1986,6 +1995,9 @@ static int convert_context(u32 key,
 
 static void security_load_policycaps(void)
 {
+   unsigned int i;
+   struct ebitmap_node *node;
+
selinux_policycap_netpeer = ebitmap_get_bit(,
  POLICYDB_CAPABILITY_NETPEER);
selinux_policycap_openperm = ebitmap_get_bit(,
@@ -1997,6 +2009,17 @@ static void security_load_policycaps(void)
selinux_policycap_cgroupseclabel =
ebitmap_get_bit(,
POLICYDB_CAPABILITY_CGROUPSECLABEL);
+
+   for (i = 0; i < ARRAY_SIZE(selinux_policycap_names); i++)
+   pr_info("SELinux:  policy capability %s=%d\n",
+   selinux_policycap_names[i],
+   ebitmap_get_bit(, i));
+
+   ebitmap_for_each_positive_bit(, node, i) {
+   if (i >= ARRAY_SIZE(selinux_policycap_names))
+   pr_warn("SELinux:  unknown policy capability %u\n",
+   i);
+   }
 }
 
 static int security_preserve_bools(struct policydb *p);
-- 
2.9.3

Re: [PATCH 2/9] libsepol: Add ibpkey ocontext handling

[Daniel Jurgens]

On 5/11/2017 10:18 AM, James Carter wrote:
> libsepol now has the functionality to write cil or a policy.conf from a 
> kernel 
> policy, so kernel_to_cil.c and kernel_to_conf.c need to be updated as well. 
> Doing that shouldn't be any more complicated than what was done for 
> module_to_c.
>
> Jim
Added.  Thanks for reviewing, completely missed when those files were added.
>
> On 05/09/2017 04:50 PM, Dan Jurgens wrote:
>> From: Daniel Jurgens 
>>
>> Add support for reading, writing, and copying Infinabinda Pkey ocontext
>> data. Also add support for querying a Pkey sid to checkpolicy.
>>

Re: [PATCH] procattr.c: Use __BIONIC__ instead of __ANDROID__

[Stephen Smalley]

On Thu, 2017-05-11 at 16:50 -0700, Tom Cherry via Selinux wrote:
> This check is not specific to Android devices. If libselinux were
> used
> with Bionic on a normal Linux system this check would still be
> needed.
> 
> Signed-off-by: Tom Cherry 

Thanks, applied.  This was actually switched from ANDROID to _ANDROID__
 by nnk in 044f6ef104c8a9d8f42faa8756e71a0525198f5b.  We don't appear
to have any other uses of __ANDROID__, but we do have a number of uses
of ANDROID.  Offhand though these other uses of ANDROID don't appear to
be related to bionic but instead reflect differences in SELinux
userspace integration in Android vs GNU/Linux.  If however your
bionic/Linux system integrates SELinux support in an Android-like
manner (e.g. kernel policy file is /sepolicy, no /etc/selinux/config,
...), then you might need to generalize those as well.

> ---
>  libselinux/src/procattr.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/libselinux/src/procattr.c b/libselinux/src/procattr.c
> index ebc0adec..48dd8aff 100644
> --- a/libselinux/src/procattr.c
> +++ b/libselinux/src/procattr.c
> @@ -22,8 +22,8 @@ static pthread_key_t destructor_key;
>  static int destructor_key_initialized = 0;
>  static __thread char destructor_initialized;
>  
> -#ifndef __ANDROID__
> -/* Android declares this in unistd.h and has a definition for it */
> +#ifndef __BIONIC__
> +/* Bionic declares this in unistd.h and has a definition for it */
>  static pid_t gettid(void)
>  {
>   return syscall(__NR_gettid);

Re: [PATCH] procattr.c: Use __BIONIC__ instead of __ANDROID__

[enh via Selinux]

don't feel bad: the Android tree has the same problem --- even bionic
itself didn't always get this right :-)

we're spotting these now because we're actually trying to use bionic
on non-Android Linux too.

On Thu, May 11, 2017 at 5:18 PM, William Roberts
 wrote:
> On Thursday, May 11, 2017, Tom Cherry via Selinux 
> wrote:
>>
>> This check is not specific to Android devices. If libselinux were used
>> with Bionic on a normal Linux system this check would still be needed.
>>
>> Signed-off-by: Tom Cherry 
>> ---
>>  libselinux/src/procattr.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/libselinux/src/procattr.c b/libselinux/src/procattr.c
>> index ebc0adec..48dd8aff 100644
>> --- a/libselinux/src/procattr.c
>> +++ b/libselinux/src/procattr.c
>> @@ -22,8 +22,8 @@ static pthread_key_t destructor_key;
>>  static int destructor_key_initialized = 0;
>>  static __thread char destructor_initialized;
>>
>> -#ifndef __ANDROID__
>> -/* Android declares this in unistd.h and has a definition for it */
>> +#ifndef __BIONIC__
>> +/* Bionic declares this in unistd.h and has a definition for it */
>>  static pid_t gettid(void)
>>  {
>> return syscall(__NR_gettid);
>> --
>> 2.13.0.rc2.291.g57267f2277-goog
>
>
>
> Ack.. thanks for this. There might be other places I used Android when I
> should have used bionic
>
>
>
>
> --
> Respectfully,
>
> William C Roberts
>
>



-- 
Elliott Hughes - http://who/enh - http://jessies.org/~enh/
Android native code/tools questions? Mail me/drop by/add me as a reviewer.