RE: add CONFIG_SECURITY_SELINUX_LOAD_ONCE

[Roberts, William C]

Nack… use Booleans

Allow Android to have 1 boolean that init trips, once innit trips it, the allow 
to load policy is removed and also the rule to allow toggling that Boolean is 
removed


From: Seandroid-list [mailto:seandroid-list-boun...@tycho.nsa.gov] On Behalf Of 
Nick Kralevich
Sent: Friday, April 7, 2017 10:34 AM
To: SELinux ; seandroid-l...@tycho.nsa.gov
Subject: add CONFIG_SECURITY_SELINUX_LOAD_ONCE

I wanted to draw people's attention to the following proposed change:

  https://android-review.googlesource.com/367695

In the case of Android, it's common for security policy to be loaded once, and 
never reloaded again. In that case, the locking / unlocking surrounding the 
in-kernel policy is unnecessary and can be avoided. The patch above turns the 
locks into no-ops and ensures that the kernel cannot load a policy more than 
once. End result is that locking and preemption overhead is avoided and there's 
less attack surface / code compiled into the kernel.

I would appreciate comments on the change. This feels like a worthwhile change 
for the entire SELinux community.

-- Nick

--
Nick Kralevich | Android Security | n...@google.com | 
650.214.4037
___
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: CIL Typepermissive Symbol not inside parenthesis

[Roberts, William C]

> -Original Message-
> From: Roberts, William C
> Sent: Thursday, January 26, 2017 10:39 AM
> To: seandroid-l...@tycho.nsa.gov
> Cc: 'Stephen Smalley' <s...@tycho.nsa.gov>; 'Nick Kralevich' 
> <n...@google.com>;
> selinux@tycho.nsa.gov
> Subject: CIL Typepermissive Symbol not inside parenthesis
> 
> Building for Hikey (Android) with a type permissive statement on hci_attach,
> yields this error:
> 
> /bin/bash -c "(out/host/linux-x86/bin/secilc -M true -c 30
> out/target/product/hikey/obj/ETC/plat_sepolicy.cil_intermediates/plat_policy_n
> vr.cil
> out/target/product/hikey/obj/ETC/mapping_sepolicy.cil_intermediates/mapping
> /current.cil
> out/target/product/hikey/obj/ETC/nonplat_sepolicy.cil_intermediates/nonplat_
> policy_nvr.cil  -o
> out/target/product/hikey/obj/ETC/sepolicy_intermediates/sepolicy.tmp ) &&
> (out/host/linux-x86/bin/sepolicy-analyze
> out/target/product/hikey/obj/ETC/sepolicy_intermediates/sepolicy.tmp
> permissive >
> out/target/product/hikey/obj/ETC/sepolicy_intermediates/sepolicy.permissived
> omains ) && (if [ \"userdebug\" = \"user\" -a -s
> out/target/product/hikey/obj/ETC/sepolicy_intermediates/sepolicy.permissived
> omains ]; thenecho \"==\" 1>&2;   echo 
> \"ERROR:
> permissive domains not allowed in user builds\" 1>&2; echo 
> \"List of
> invalid domains:\" 1>&2;  cat
> out/target/product/hikey/obj/ETC/sepolicy_intermediates/sepolicy.permissived
> omains 1>&2;  exit 1; fi ) && (mv
> out/target/product/hikey/obj/ETC/sepolicy_intermediates/sepolicy.tmp
> out/target/product/hikey/obj/ETC/sepolicy_intermediates/sepolicy )"
> Symbol not inside parenthesis at line 1239 of
> out/target/product/hikey/obj/ETC/nonplat_sepolicy.cil_intermediates/nonplat_
> policy_nvr.cil
> 
> To reproduce apply this patch to device/linaro/hikey:
> diff --git a/sepolicy/hci_attach.te b/sepolicy/hci_attach.te index
> d87f444..1990d54 100644
> --- a/sepolicy/hci_attach.te
> +++ b/sepolicy/hci_attach.te
> @@ -1,6 +1,8 @@
>  type hci_attach, domain;
>  type hci_attach_exec, exec_type, file_type;
> 
> +permissive hci_attach;
> +
>  init_daemon_domain(hci_attach)
> 
>  allow hci_attach kernel:system module_request;
> 
> and build sepolicy
> 
> make -j4 sepolicy
> 
> I have no idea what's hgappening, but the statement looks different than all 
> the
> other CIL statements:
> 
> Failing CIL snippet:
> 
> (type hci_attach)
> (roletype object_r hci_attach)
> CIL_TYPEPERMISSIVE (type hci_attach_exec) (roletype object_r hci_attach_exec)
> (type hci_attach_tmpfs)
> 
> 

Some of things call routines like cil_write_roletype() in write_ast.c, but some 
just frpintf(CIL_). Are these features not implemented?

If I apply this hack it works:
diff --git a/libsepol/cil/src/cil_write_ast.c b/libsepol/cil/src/cil_write_ast.c
index 4ebda6a..8a25680 100644
--- a/libsepol/cil/src/cil_write_ast.c
+++ b/libsepol/cil/src/cil_write_ast.c
@@ -1255,7 +1255,7 @@ int __cil_write_node_helper(struct cil_tree_node *node, 
uint32_t *finished, void
fprintf(cil_out, "CIL_TYPEBOUNDS ");
break;
case CIL_TYPEPERMISSIVE:
-   fprintf(cil_out, "CIL_TYPEPERMISSIVE ");
+   fprintf(cil_out, "(typepermissive hci_attach)\n");
break;
case CIL_TYPEATTRIBUTE:

The output looks ok from sepolicy-analyze:

$ sepolicy-analyze $OUT/root/sepolicy permissive
crash_dump
su
hci_attach

___
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: CIL Typepermissive Symbol not inside parenthesis

[Roberts, William C]

> -Original Message-
> From: Roberts, William C
> Sent: Thursday, January 26, 2017 11:17 AM
> To: 'seandroid-l...@tycho.nsa.gov' <seandroid-l...@tycho.nsa.gov>
> Cc: 'Stephen Smalley' <s...@tycho.nsa.gov>; 'Nick Kralevich' 
> <n...@google.com>;
> 'selinux@tycho.nsa.gov' <selinux@tycho.nsa.gov>
> Subject: RE: CIL Typepermissive Symbol not inside parenthesis
> 
> 
> 
> > -Original Message-
> > From: Roberts, William C
> > Sent: Thursday, January 26, 2017 10:39 AM
> > To: seandroid-l...@tycho.nsa.gov
> > Cc: 'Stephen Smalley' <s...@tycho.nsa.gov>; 'Nick Kralevich'
> > <n...@google.com>; selinux@tycho.nsa.gov
> > Subject: CIL Typepermissive Symbol not inside parenthesis
> >
> > Building for Hikey (Android) with a type permissive statement on
> > hci_attach, yields this error:
> >
> > /bin/bash -c "(out/host/linux-x86/bin/secilc -M true -c 30
> > out/target/product/hikey/obj/ETC/plat_sepolicy.cil_intermediates/plat_
> > policy_n
> > vr.cil
> > out/target/product/hikey/obj/ETC/mapping_sepolicy.cil_intermediates/ma
> > pping
> > /current.cil
> > out/target/product/hikey/obj/ETC/nonplat_sepolicy.cil_intermediates/no
> > nplat_
> > policy_nvr.cil  -o
> > out/target/product/hikey/obj/ETC/sepolicy_intermediates/sepolicy.tmp )
> > && (out/host/linux-x86/bin/sepolicy-analyze
> > out/target/product/hikey/obj/ETC/sepolicy_intermediates/sepolicy.tmp
> > permissive >
> > out/target/product/hikey/obj/ETC/sepolicy_intermediates/sepolicy.permi
> > ssived omains ) && (if [ \"userdebug\" = \"user\" -a -s
> >
> out/target/product/hikey/obj/ETC/sepolicy_intermediates/sepolicy.permissived
> > omains ]; then  echo \"==\" 1>&2;   echo
> \"ERROR:
> > permissive domains not allowed in user builds\" 1>&2;   echo
> \"List of
> > invalid domains:\" 1>&2;cat
> >
> out/target/product/hikey/obj/ETC/sepolicy_intermediates/sepolicy.permissived
> > omains 1>&2;exit 1; fi ) && (mv
> > out/target/product/hikey/obj/ETC/sepolicy_intermediates/sepolicy.tmp
> > out/target/product/hikey/obj/ETC/sepolicy_intermediates/sepolicy )"
> > Symbol not inside parenthesis at line 1239 of
> > out/target/product/hikey/obj/ETC/nonplat_sepolicy.cil_intermediates/no
> > nplat_
> > policy_nvr.cil
> >
> > To reproduce apply this patch to device/linaro/hikey:
> > diff --git a/sepolicy/hci_attach.te b/sepolicy/hci_attach.te index
> > d87f444..1990d54 100644
> > --- a/sepolicy/hci_attach.te
> > +++ b/sepolicy/hci_attach.te
> > @@ -1,6 +1,8 @@
> >  type hci_attach, domain;
> >  type hci_attach_exec, exec_type, file_type;
> >
> > +permissive hci_attach;
> > +
> >  init_daemon_domain(hci_attach)
> >
> >  allow hci_attach kernel:system module_request;
> >
> > and build sepolicy
> >
> > make -j4 sepolicy
> >
> > I have no idea what's hgappening, but the statement looks different
> > than all the other CIL statements:
> >
> > Failing CIL snippet:
> >
> > (type hci_attach)
> > (roletype object_r hci_attach)
> > CIL_TYPEPERMISSIVE (type hci_attach_exec) (roletype object_r
> > hci_attach_exec) (type hci_attach_tmpfs)
> >
> >
> 
> Some of things call routines like cil_write_roletype() in write_ast.c, but 
> some just
> frpintf(CIL_). Are these features not implemented?
> 
> If I apply this hack it works:
> diff --git a/libsepol/cil/src/cil_write_ast.c 
> b/libsepol/cil/src/cil_write_ast.c
> index 4ebda6a..8a25680 100644
> --- a/libsepol/cil/src/cil_write_ast.c
> +++ b/libsepol/cil/src/cil_write_ast.c
> @@ -1255,7 +1255,7 @@ int __cil_write_node_helper(struct cil_tree_node
> *node, uint32_t *finished, void
> fprintf(cil_out, "CIL_TYPEBOUNDS ");
> break;
> case CIL_TYPEPERMISSIVE:
> -   fprintf(cil_out, "CIL_TYPEPERMISSIVE ");
> +   fprintf(cil_out, "(typepermissive hci_attach)\n");
> break;
> case CIL_TYPEATTRIBUTE:
> 
> The output looks ok from sepolicy-analyze:
> 
> $ sepolicy-analyze $OUT/root/sepolicy permissive crash_dump su hci_attach

FYI This does not affect upstream SE Linux, it looks like Dan Cashman over at 
Google authored the file,
So ill drop common selinux mailing listr on further responses. I'll take a look 
at fixing this today...

___
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.

CIL Typepermissive Symbol not inside parenthesis

[Roberts, William C]

Building for Hikey (Android) with a type permissive statement on hci_attach, 
yields this error:

/bin/bash -c "(out/host/linux-x86/bin/secilc -M true -c 30 
out/target/product/hikey/obj/ETC/plat_sepolicy.cil_intermediates/plat_policy_nvr.cil
 
out/target/product/hikey/obj/ETC/mapping_sepolicy.cil_intermediates/mapping/current.cil
 
out/target/product/hikey/obj/ETC/nonplat_sepolicy.cil_intermediates/nonplat_policy_nvr.cil
  -o out/target/product/hikey/obj/ETC/sepolicy_intermediates/sepolicy.tmp ) && 
(out/host/linux-x86/bin/sepolicy-analyze 
out/target/product/hikey/obj/ETC/sepolicy_intermediates/sepolicy.tmp permissive 
> 
out/target/product/hikey/obj/ETC/sepolicy_intermediates/sepolicy.permissivedomains
 ) && (if [ \"userdebug\" = \"user\" -a -s 
out/target/product/hikey/obj/ETC/sepolicy_intermediates/sepolicy.permissivedomains
 ]; then echo \"==\" 1>&2;   echo \"ERROR: 
permissive domains not allowed in user builds\" 1>&2; echo \"List 
of invalid domains:\" 1>&2; cat 
out/target/product/hikey/obj/ETC/sepolicy_intermediates/sepolicy.permissivedomains
 1>&2;exit 1; fi ) && (mv
  out/target/product/hikey/obj/ETC/sepolicy_intermediates/sepolicy.tmp 
out/target/product/hikey/obj/ETC/sepolicy_intermediates/sepolicy )"
Symbol not inside parenthesis at line 1239 of 
out/target/product/hikey/obj/ETC/nonplat_sepolicy.cil_intermediates/nonplat_policy_nvr.cil

To reproduce apply this patch to device/linaro/hikey:
diff --git a/sepolicy/hci_attach.te b/sepolicy/hci_attach.te
index d87f444..1990d54 100644
--- a/sepolicy/hci_attach.te
+++ b/sepolicy/hci_attach.te
@@ -1,6 +1,8 @@
 type hci_attach, domain;
 type hci_attach_exec, exec_type, file_type;
 
+permissive hci_attach;
+
 init_daemon_domain(hci_attach)
 
 allow hci_attach kernel:system module_request;

and build sepolicy

make -j4 sepolicy

I have no idea what's hgappening, but the statement looks different than all 
the other CIL statements:

Failing CIL snippet:

(type hci_attach)
(roletype object_r hci_attach)
CIL_TYPEPERMISSIVE (type hci_attach_exec)
(roletype object_r hci_attach_exec)
(type hci_attach_tmpfs)






___
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] quick selinux support for tracefs

[Roberts, William C]

Yes it was submitted to Android’s Gerrit Here by you:
https://android-review.googlesource.com/#/c/220895/

I already modified the commit message and got an Ack on V3 from Stephen:
http://marc.info/?l=selinux=148105682211911=2


From: YongQin Liu [mailto:yongqin@linaro.org]
Sent: Tuesday, December 6, 2016 11:06 PM
To: Roberts, William C <william.c.robe...@intel.com>
Cc: Stephen Smalley <s...@tycho.nsa.gov>; n...@google.com; 
selinux@tycho.nsa.gov; Paul Moore <p...@paul-moore.com>
Subject: Re: [PATCH] quick selinux support for tracefs

Hi, Roberts

On 7 December 2016 at 02:05, Roberts, William C 
<william.c.robe...@intel.com<mailto:william.c.robe...@intel.com>> wrote:


> -Original Message-
> From: Stephen Smalley [mailto:s...@tycho.nsa.gov<mailto:s...@tycho.nsa.gov>]
> Sent: Tuesday, December 6, 2016 10:01 AM
> To: Roberts, William C 
> <william.c.robe...@intel.com<mailto:william.c.robe...@intel.com>>; 
> n...@google.com<mailto:n...@google.com>;
> selinux@tycho.nsa.gov<mailto:selinux@tycho.nsa.gov>
> Cc: Yongqin Liu <yongqin@linaro.org<mailto:yongqin@linaro.org>>; Paul 
> Moore <p...@paul-moore.com<mailto:p...@paul-moore.com>>
> Subject: Re: [PATCH] quick selinux support for tracefs
>
> On 12/06/2016 12:50 PM, Roberts, William C wrote:
> >
> >
> >> -Original Message-
> >> From: Stephen Smalley 
> >> [mailto:s...@tycho.nsa.gov<mailto:s...@tycho.nsa.gov>]
> >> Sent: Tuesday, December 6, 2016 9:41 AM
> >> To: Roberts, William C 
> >> <william.c.robe...@intel.com<mailto:william.c.robe...@intel.com>>; 
> >> n...@google.com<mailto:n...@google.com>;
> >> selinux@tycho.nsa.gov<mailto:selinux@tycho.nsa.gov>
> >> Cc: Yongqin Liu <yongqin@linaro.org<mailto:yongqin@linaro.org>>; 
> >> Paul Moore
> >> <p...@paul-moore.com<mailto:p...@paul-moore.com>>
> >> Subject: Re: [PATCH] quick selinux support for tracefs
> >>
> >> On 12/06/2016 12:24 PM, 
> >> william.c.robe...@intel.com<mailto:william.c.robe...@intel.com> wrote:
> >>> From: Yongqin Liu <yongqin@linaro.org<mailto:yongqin@linaro.org>>
> >>>
> >>> Here is just the quick fix for tracefs with selinux.
> >>> just add tracefs to the list of whitelisted filesystem types in
> >>> selinux_is_sblabel_mnt(), but the right fix would be to generalize
> >>> this logic as described in the last item on the todo list,
> >>> https://bitbucket.org/seandroid/wiki/wiki/ToDo
> >>>
> >>> Change-Id: I2aa803ccffbcd2802a7287514da7648e6b364157
> >>
> >> Please rewrite the subject line and patch description per the
> >> kernel's submission guidelines, drop the Change-Id and the link to
> >> the SEAndroid todo list, and don't say that this is a quick fix but
> >
> > Why would anyone do that for this patch when below you say it won’t be
> > merged unless we fix issue #2?
>
> I didn't say it couldn't be merged; I said it isn't a good idea to say "this 
> is a quick fix
> but the right fix is X" in an upstream patch submission if you want it to be
> merged, unless it is for a serious security or stability bug that needs to be 
> fixed
> right away.
Oh sure, but I didn't right that patch message, Liu Yonggin is tha author, ill 
fix up the
message and resubmit preserving him as the author.

This change was submitted to http://android-review.googlesource.com/ via gerrit 
instructions before,
and now seems that it is going to be submitted via the instructions for kernel 
changes which I am not familiar.

If you like, please help to update the patch message and submit the patch.

Or you could share me the instructions on how to do that, and then I will 
submit the changes with new message following your instructions.

Thanks,
Yongqin Liu

>
> >
> > the right fix is something else if you want this to
> >> actually be merged.  Because in that case, you ought to just
> >> implement the right fix.  There is now an upstream kernel issue for the 
> >> right
> fix:
> >> https://github.com/SELinuxProject/selinux-kernel/issues/2
> >
> > The other question here is tracefs safe to label in this fashion, I would 
> > assume
> yes.
> > Looking through I didn't see any eviction code.
>
> Yes, the inodes are pinned.



--
Best Regards,
Yongqin Liu
---
#mailing list
linaro-andr...@lists.linaro.org<mailto:linaro-...@lists.linaro.org>
http://lists.linaro.org/mailman/listinfo/linaro-android
___
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 v2] libsepol: fix checkpolicy dontaudit compiler bug

[Roberts, William C]

> -Original Message-
> From: Selinux [mailto:selinux-boun...@tycho.nsa.gov] On Behalf Of Roberts,
> William C
> Sent: Monday, November 14, 2016 10:44 AM
> To: Stephen Smalley <s...@tycho.nsa.gov>; selinux@tycho.nsa.gov
> Subject: RE: [PATCH v2] libsepol: fix checkpolicy dontaudit compiler bug
> 
> 
> 
> > -Original Message-
> > From: Selinux [mailto:selinux-boun...@tycho.nsa.gov] On Behalf Of
> > Stephen Smalley
> > Sent: Monday, November 14, 2016 9:48 AM
> > To: selinux@tycho.nsa.gov
> > Cc: Stephen Smalley <s...@tycho.nsa.gov>
> > Subject: [PATCH v2] libsepol: fix checkpolicy dontaudit compiler bug
> >
> > The combining logic for dontaudit rules was wrong, causing a dontaudit
> > A B:C *; rule to be clobbered by a dontaudit A B:C p; rule.
> >
> > Reported-by: Nick Kralevich <n...@google.com>
> > Signed-off-by: Stephen Smalley <s...@tycho.nsa.gov>
> > ---
> >  libsepol/src/expand.c | 16 
> >  1 file changed, 12 insertions(+), 4 deletions(-)
> >
> > diff --git a/libsepol/src/expand.c b/libsepol/src/expand.c index
> > 004a029..d7adbf8
> > 100644
> > --- a/libsepol/src/expand.c
> > +++ b/libsepol/src/expand.c
> > @@ -1604,7 +1604,8 @@ static int expand_range_trans(expand_state_t *
> > state, static avtab_ptr_t find_avtab_node(sepol_handle_t * handle,
> >avtab_t * avtab, avtab_key_t * key,
> >cond_av_list_t ** cond,
> > -  av_extended_perms_t *xperms)
> > +  av_extended_perms_t *xperms,
> > +  char *alloced)
> >  {
> > avtab_ptr_t node;
> > avtab_datum_t avdatum;
> > @@ -1658,6 +1659,11 @@ static avtab_ptr_t
> > find_avtab_node(sepol_handle_t * handle,
> > nl->next = *cond;
> > *cond = nl;
> > }
> > +   if (alloced)
> > +   *alloced = 1;
> > +   } else {
> > +   if (alloced)
> > +   *alloced = 0;
> > }
> >
> > return node;
> > @@ -1750,7 +1756,7 @@ static int expand_terule_helper(sepol_handle_t *
> > handle,
> > return EXPAND_RULE_CONFLICT;
> > }
> >
> > -   node = find_avtab_node(handle, avtab, , cond, NULL);
> > +   node = find_avtab_node(handle, avtab, , cond, NULL,
> > NULL);
> > if (!node)
> > return -1;
> > if (enabled) {
> > @@ -1790,6 +1796,7 @@ static int expand_avrule_helper(sepol_handle_t *
> > handle,
> > class_perm_node_t *cur;
> > uint32_t spec = 0;
> > unsigned int i;
> > +   char alloced;
> >
> > if (specified & AVRULE_ALLOWED) {
> > spec = AVTAB_ALLOWED;
> > @@ -1824,7 +1831,8 @@ static int expand_avrule_helper(sepol_handle_t *
> > handle,
> > avkey.target_class = cur->tclass;
> > avkey.specified = spec;
> >
> > -   node = find_avtab_node(handle, avtab, , cond,
> > extended_perms);
> > +   node = find_avtab_node(handle, avtab, , cond,
> > +  extended_perms, );
> > if (!node)
> > return EXPAND_RULE_ERROR;
> > if (enabled) {
> > @@ -1850,7 +1858,7 @@ static int expand_avrule_helper(sepol_handle_t *
> > handle,
> >  */
> > avdatump->data &= cur->data;
> > } else if (specified & AVRULE_DONTAUDIT) {
> > -   if (avdatump->data)
> > +   if (!alloced)
> > avdatump->data &= ~cur->data;
> > else
> > avdatump->data = ~cur->data;
> 
> This seems awkward to me. If the insertion created a new empty node why
> wouldn't !avdump->data be true (note the addition of the not operator)?

I misstated that a bit, but the !avdump->data was the else case. I am really
saying why didn't this work before? In my mind, we don't care if its allocated
we care if it's set or not.

> 
> Or perhaps a mechanism to actual set the data on allocation, this way the 
> logic is
> Just &=.
> 
> > --
> > 2.7.4
> >
> > ___
> > 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.
> 
> ___
> 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.

___
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 v2] libsepol: fix checkpolicy dontaudit compiler bug

[Roberts, William C]

> -Original Message-
> From: Selinux [mailto:selinux-boun...@tycho.nsa.gov] On Behalf Of Stephen
> Smalley
> Sent: Monday, November 14, 2016 9:48 AM
> To: selinux@tycho.nsa.gov
> Cc: Stephen Smalley 
> Subject: [PATCH v2] libsepol: fix checkpolicy dontaudit compiler bug
> 
> The combining logic for dontaudit rules was wrong, causing a dontaudit A B:C 
> *;
> rule to be clobbered by a dontaudit A B:C p; rule.
> 
> Reported-by: Nick Kralevich 
> Signed-off-by: Stephen Smalley 
> ---
>  libsepol/src/expand.c | 16 
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/libsepol/src/expand.c b/libsepol/src/expand.c index 
> 004a029..d7adbf8
> 100644
> --- a/libsepol/src/expand.c
> +++ b/libsepol/src/expand.c
> @@ -1604,7 +1604,8 @@ static int expand_range_trans(expand_state_t * state,
> static avtab_ptr_t find_avtab_node(sepol_handle_t * handle,
>  avtab_t * avtab, avtab_key_t * key,
>  cond_av_list_t ** cond,
> -av_extended_perms_t *xperms)
> +av_extended_perms_t *xperms,
> +char *alloced)
>  {
>   avtab_ptr_t node;
>   avtab_datum_t avdatum;
> @@ -1658,6 +1659,11 @@ static avtab_ptr_t find_avtab_node(sepol_handle_t *
> handle,
>   nl->next = *cond;
>   *cond = nl;
>   }
> + if (alloced)
> + *alloced = 1;
> + } else {
> + if (alloced)
> + *alloced = 0;
>   }
> 
>   return node;
> @@ -1750,7 +1756,7 @@ static int expand_terule_helper(sepol_handle_t *
> handle,
>   return EXPAND_RULE_CONFLICT;
>   }
> 
> - node = find_avtab_node(handle, avtab, , cond, NULL);
> + node = find_avtab_node(handle, avtab, , cond, NULL,
> NULL);
>   if (!node)
>   return -1;
>   if (enabled) {
> @@ -1790,6 +1796,7 @@ static int expand_avrule_helper(sepol_handle_t *
> handle,
>   class_perm_node_t *cur;
>   uint32_t spec = 0;
>   unsigned int i;
> + char alloced;
> 
>   if (specified & AVRULE_ALLOWED) {
>   spec = AVTAB_ALLOWED;
> @@ -1824,7 +1831,8 @@ static int expand_avrule_helper(sepol_handle_t *
> handle,
>   avkey.target_class = cur->tclass;
>   avkey.specified = spec;
> 
> - node = find_avtab_node(handle, avtab, , cond,
> extended_perms);
> + node = find_avtab_node(handle, avtab, , cond,
> +extended_perms, );
>   if (!node)
>   return EXPAND_RULE_ERROR;
>   if (enabled) {
> @@ -1850,7 +1858,7 @@ static int expand_avrule_helper(sepol_handle_t *
> handle,
>*/
>   avdatump->data &= cur->data;
>   } else if (specified & AVRULE_DONTAUDIT) {
> - if (avdatump->data)
> + if (!alloced)
>   avdatump->data &= ~cur->data;
>   else
>   avdatump->data = ~cur->data;

This seems awkward to me. If the insertion created a new empty node
why wouldn't !avdump->data be true (note the addition of the not operator)?

Or perhaps a mechanism to actual set the data on allocation, this way the logic 
is
Just &=.

> --
> 2.7.4
> 
> ___
> 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.

___
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: GPF in sidtab_context_to_sid

[Roberts, William C]

> -Original Message-
> From: Paul Moore [mailto:p...@paul-moore.com]
> Sent: Tuesday, November 8, 2016 12:57 PM
> To: Roberts, William C <william.c.robe...@intel.com>
> Cc: selinux@tycho.nsa.gov
> Subject: Re: GPF in sidtab_context_to_sid
> 
> On Tue, Nov 8, 2016 at 1:26 PM, Roberts, William C 
> <william.c.robe...@intel.com>
> wrote:
> > I found a very similar oops online:
> >
> > http://oops.kernel.org/oops/general-protection-fault-in-sidtab_context
> > _to_sid/
> >
> > Has anyone encountered this bug?
> >
> > I had something reported to me very similar where the faulting
> > instruction
> > was:
> >
> > 0x8133c81e <+174>:   mov0x14(%r12),%eax
> >
> > Addr2line on vmlinux produced:
> >
> > $ addr2line -f -e ./vmlinux 8133c81e context_cmp
> > kernel/cht/security/selinux/ss/context.h:152
> 
> I'm guessing you don't have a reproducer?

Supposedly, I am digging that slowly out of the reporters. If I can use it to
reproduce, I'll let you know.

> 
> It looks like both these kernels are older (3.x), have you seen this on 
> anything
> recent?

No.

> 
> --
> 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.

GPF in sidtab_context_to_sid

[Roberts, William C]

I found a very similar oops online:
http://oops.kernel.org/oops/general-protection-fault-in-sidtab_context_to_sid/

Has anyone encountered this bug?

I had something reported to me very similar where the faulting instruction was:

0x8133c81e <+174>:   mov0x14(%r12),%eax

Addr2line on vmlinux produced:
$ addr2line -f -e ./vmlinux 8133c81e
context_cmp
kernel/cht/security/selinux/ss/context.h:152

Actual Dump:

[131436.409639] general protection fault:  [#1] PREEMPT SMP
[131436.416085] Modules linked in: tcp_diag inet_diag
atomisp_css2401a0_v21 videobuf_vmalloc videobuf_core bt_lpm
rfkill_gpio 8723bs(O) cfg80211 ov2680 ov8858_driver silead_ts ltr501
bmg160 ak09911 kxcjk_1013
[131436.436623] CPU: 3 PID: 3177 Comm: SettingsProvide Tainted: G
  W  O 3.14.70-x86_64-02246-g49319b8 #1
[131436.447500] Hardware name: XXX
CHTMRD.A6.002.016 09/20/2016
[131436.456542] task: 88006039cb30 ti: 88005e2ea000 task.ti:
88005e2ea000
[131436.465000] RIP: 0010:[]
[131436.469579]  [] sidtab_context_to_sid+0xae/0x480
[131436.476783] RSP: 0018:88005e2ebae0  EFLAGS: 00010286
[131436.482814] RAX: fff9f9f9 RBX: 82776540 RCX:

[131436.490884] RDX:  RSI:  RDI:
82776540
[131436.498953] RBP: 88005e2ebb28 R08: 88005e2ebb88 R09:

[131436.507022] R10: 88007826c000 R11: 2f2f2f2f2f2f2f2f R12:
fff9f9f9fff9f9f9
[131436.515091] R13: 88005e2ebba0 R14: 88005e2ebbb8 R15:
0068
[131436.523160] FS:  d1efbe00(006b) GS:88007938(0063)
knlGS:d1a77960
[131436.532297] CS:  0010 DS: 002b ES: 002b CR0: 80050033
[131436.538813] CR2: 72e67750 CR3: 5e1ba000 CR4:
001007e0
[131436.546883] Last Branch Records:
[131436.550590]to: [] general_protection+0x0/0x80
[131436.557700]  from: [] sidtab_context_to_sid+0xae/0x480
[131436.565292]to: [] sidtab_context_to_sid+0xa0/0x480
[131436.572885]  from: [] sidtab_context_to_sid+0x96/0x480
[131436.580478]to: [] sidtab_context_to_sid+0x90/0x480
[131436.588070]  from: [] sidtab_context_to_sid+0xb5/0x480
[131436.595662]to: [] sidtab_context_to_sid+0xa0/0x480
[131436.603255]  from: [] sidtab_context_to_sid+0xd2/0x480
[131436.610847]to: [] sidtab_context_to_sid+0xa0/0x480
[131436.618439]  from: [] sidtab_context_to_sid+0xd2/0x480
[131436.626031]to: [] sidtab_context_to_sid+0xa0/0x480
[131436.633624]  from: [] sidtab_context_to_sid+0xd2/0x480
[131436.641216]to: [] sidtab_context_to_sid+0xae/0x480
[131436.648810]  from: [] sidtab_context_to_sid+0x7f/0x480
[131436.656401]to: [] sidtab_context_to_sid+0x75/0x480
[131436.663994]  from: [] sidtab_context_to_sid+0x34b/0x480
[131436.671684] Stack:
[131436.674023]  88005e2ebb88 88005e2ebb08 8134938e
88005e2ebc3c
[131436.682416]   88005e2ebb88 0010
880060371ea8
[131436.690809]  8800716d4968 88005e2ebbf8 8134372f
0006
[131436.699204] Call Trace:
[131436.702036]  [] ? mls_context_isvalid+0x2e/0xb0
[131436.708944]  [] security_compute_sid.part.10+0x43f/0x550
[131436.716727]  [] ? search_dir+0x40/0x120
[131436.722851]  [] security_compute_sid+0x4e/0x50
[131436.729660]  [] security_transition_sid+0x2d/0x40
[131436.736762]  [] may_create+0x96/0x100
[131436.742699]  [] selinux_inode_create+0x13/0x20
[131436.749509]  [] security_inode_create+0x1f/0x30
[131436.756417]  [] vfs_create+0x8e/0x140
[131436.762353]  [] do_last+0x7e1/0x1210
[131436.768192]  [] ? link_path_walk+0x8c/0xfb0
[131436.774712]  [] ? kmem_cache_alloc_trace+0xe1/0x1d0
[131436.782008]  [] ? selinux_file_alloc_security+0x3c/0x60
[131436.789692]  [] path_openat+0xbb/0x6d0
[131436.795724]  [] ? SYSC_renameat+0xe8/0x3f0
[131436.802146]  [] do_filp_open+0x3a/0xa0
[131436.808179]  [] ? _raw_spin_unlock+0x18/0x40
[131436.814795]  [] ? __alloc_fd+0xa7/0x130
[131436.820925]  [] do_sys_open+0x12c/0x220
[131436.827056]  [] compat_SyS_openat+0x11/0x20
[131436.833574]  [] sysenter_dispatch+0x7/0x1f
[131436.839997]  [] ? trace_hardirqs_on_thunk+0x3a/0x3c
[131436.847289] Code: 02 00 00 66 2e 0f 1f 84 00 00 00 00 00 41 8b 50
0c 85 d2 74 08 39 d0 0f 84 70 02 00 00 4d 8b 64 24 50 4d 85 e4 0f 84
92 02 00 00 <41> 8b 44 24 14 85 c0 75 d9 41 8b 48 0c 85 c9 75 e1 49 8b
00 49
[131436.869023] RIP
[131436.870977]  [] sidtab_context_to_sid+0xae/0x480
[131436.878180]  RSP 
[131436.882285] ---[ end trace 4c33bfa820f020fe ]---

___
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: POSIX mqueues

[Roberts, William C]

> -Original Message-
> From: Selinux [mailto:selinux-boun...@tycho.nsa.gov] On Behalf Of Stephen
> Smalley
> Sent: Tuesday, October 25, 2016 9:33 AM
> To: David Graziano ;
> selinux@tycho.nsa.gov
> Subject: Re: POSIX mqueues
> 
> On 10/24/2016 03:25 PM, David Graziano wrote:
> > I am attempting to write policy for a set of applications which use
> > POSIX mqueues using named type_transistion rules to uniquely label the
> > mqueue files in the /dev/mqueue directory then controlling access
> > based on the types. Standard type transition rules seem to work but I
> > cannot seem to get the named type transitions to apply the proper
> > label. Are named type transitions not supported by the mqueue file
> > system? I’m on a 3.14 series kernel with policy version 28 if that
> > helps. I’d like to avoid needing to do a restorecon after a new queue
> > is created. Named type transistions seem to work on other file systems
> > like tmp and jffs2.
> 
> You would need to patch the kernel to support that; the filesystem
> implementation must call security_inode_init_security() and pass the 
> >d_name in order to support name-based transitions.
> 

Interesting, is anyone currently working on that, David, are you going to do 
that? If no one
Wants it, I'll do it ;-P

> 
> ___
> 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.

___
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] [RFC] nodups_specs: speedup

[Roberts, William C]

Following up on:
http://marc.info/?l=selinux=147249024230263=2

The speedup only occurs when you have things with very long
And common prefix's that cause strcmp() to run for long periods.

Under actual usecases, like refpolicy and Android, no measurable speedup was
discovered.

Unless anyone else has a use that they would like analyzed, I'll consider this
dead.

-- Bill

___
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.

speeding up nodups_specs, need large fc file.

[Roberts, William C]

I was looking back at my speedup patch for nodups specs...
http://marc.info/?l=selinux=147249024230263=2

I was testing before with a large, generated file_context file. I was wondering 
what would be a good source for
A desktop version of a file_contexts (textual preference as I can run 
sefcontext_compile on it) file as well as a binary
policy file

Should I just use refpolicy?

Bill
___
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.

read_spec_entry question

[Roberts, William C]

Should read_spec_entry every return 0 and not have found in entry?

It currently only allocates the entry argument if processing the line occurred 
in
A way where it found a valid string. It states that on success, it returns 0 
and *entry
Is allocated, but it seems that its possible that len is 0 and *entry is never 
allocated
But it still returns 0.

What should the behavior be?

/*
 * Read an entry from a spec file (e.g. file_contexts)
 * entry - Buffer to allocate for the entry.
 * ptr - current location of the line to be processed.
 * returns  - 0 on success and *entry is set to be a null
 *terminated value. On Error it returns -1 and
 *errno will be set.
 *
 */
static inline int read_spec_entry(qstr **entry, char **ptr, const char **errbuf)


___
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.

unlocked stdio

[Roberts, William C]

What was the purpose of using unlocked stdio in libselinux? Bionic doesn't 
support it, is it really necessary?

Bill

___
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: unlocked stdio

[Roberts, William C]

Correction, it's just fgets_unlocked, it appears to support the others.

From: Roberts, William C
Sent: Wednesday, September 21, 2016 12:53 PM
To: 'seandroid-l...@tycho.nsa.gov' <seandroid-l...@tycho.nsa.gov>; 
'selinux@tycho.nsa.gov' <selinux@tycho.nsa.gov>; 's...@tycho.nsa.gov' 
<s...@tycho.nsa.gov>
Subject: unlocked stdio

What was the purpose of using unlocked stdio in libselinux? Bionic doesn't 
support it, is it really necessary?

Bill

___
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: [RFC] mmap file_contexts and property_contexts:

[Roberts, William C]

> > -Original Message-
> > From: Roberts, William C
> > Sent: Monday, September 19, 2016 2:45 PM
> > To: selinux@tycho.nsa.gov; seandroid-l...@tycho.nsa.gov;
> > s...@tycho.nsa.gov; jda...@google.com
> > Cc: Roberts, William C <william.c.robe...@intel.com>
> > Subject: [RFC] mmap file_contexts and property_contexts:
> >
> > From: William Roberts <william.c.robe...@intel.com>
> >
> > THIS IS WIP...
> >
> > Rather than using stdio and making copies, just mmap the files and use
> > the pointers in place. The affect of this change, is that text file
> > load time is now faster than binary load time by 4.7% when testing
> > with a file_contexts file from the Android tree. Note that the Android 
> > doesn't
> use monstrous regexs.
> >
> > Times are the average of 3 runs.
> >
> > BEFORE:
> > Text file allocs: 114803
> > Text file load time: 0.266101
> > Bin file allocs: 93073
> > Bin file load time: 0.248757667
> >
> > AFTER:
> > Text file allocs: 103933
> > Text file load time: 0.236192667
> > Bin file allocs: 87645
> > Bin file load time: .247607333

Another potential issue, is that mmaping with textfiles also has the sideffect
of mapping in all the spaces or tabs, etc used, between fields, so on files with
heavy whitespace, it could be really memory intensive. 

I think Janis's patch that has -r option is a more useful way to go. Without 
precompiled
Regex's its essentially a textfile without the whitespace, and it automatically 
would
get pulled into the mmap interface.

I'm abandoning this, since we already get this functionality, without the 
negatives.
Janis, IIRC, you are re-configuring your patch so -r omits pre-compiled regex's 
(submitted
I believe on the list and awaiting other patches) and adding an arch string so 
we can
Detect when we need to recompile the regular expressions if they are supplied 
but
unusable.

I think we may want to investigate to have the "omit pre-compiled regex's" work 
on PCRE
library variants as well, but I doubt it would be used since Android is moving 
to PCRE2.




___
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: [RFC] mmap file_contexts and property_contexts:

[Roberts, William C]

> -Original Message-
> From: Stephen Smalley [mailto:s...@tycho.nsa.gov]
> Sent: Tuesday, September 20, 2016 5:56 AM
> To: Roberts, William C <william.c.robe...@intel.com>; selinux@tycho.nsa.gov;
> seandroid-l...@tycho.nsa.gov; jda...@google.com
> Subject: Re: [RFC] mmap file_contexts and property_contexts:
> 
> On 09/19/2016 05:45 PM, william.c.robe...@intel.com wrote:
> > From: William Roberts <william.c.robe...@intel.com>
> >
> > THIS IS WIP...
> >
> > Rather than using stdio and making copies, just mmap the files and use
> > the pointers in place. The affect of this change, is that text file

s/affect/effect

> > load time is now faster than binary load time by 4.7% when testing
> > with a file_contexts file from the Android tree. Note that the Android
> > doesn't use monstrous regexs.
> >
> > Times are the average of 3 runs.
> >
> > BEFORE:
> > Text file allocs: 114803
> > Text file load time: 0.266101
> > Bin file allocs: 93073
> > Bin file load time: 0.248757667
> >
> > AFTER:
> > Text file allocs: 103933
> > Text file load time: 0.236192667
> > Bin file allocs: 87645
> > Bin file load time: .247607333
> >
> > THINGS TO DO:
> > 1. What's arm performance like?
> > 2. What interfaces to backends are busted by this (if any)?
> > 3. Test Android Properties
> > 4. Im pretty sure this breaks sefcontext_compile, fix.
> > 5. Test with PCRE2 enabled.
> > 6. Spell check this message!
> > 7. Run checkpatch
> >
> > Signed-off-by: William Roberts <william.c.robe...@intel.com>
> > ---
> >  libselinux/src/label.c  |   2 -
> >  libselinux/src/label_android_property.c |  22 ++---
> >  libselinux/src/label_file.c | 140 
> > +++-
> >  libselinux/src/label_file.h |  66 +--
> >  libselinux/src/label_internal.h |   3 +-
> >  libselinux/src/label_support.c  |  79 --
> >  6 files changed, 172 insertions(+), 140 deletions(-)
> >
> 
> > diff --git a/libselinux/src/label_file.c b/libselinux/src/label_file.c
> > index 7156825..4dc440e 100644
> > --- a/libselinux/src/label_file.c
> > +++ b/libselinux/src/label_file.c
> > @@ -96,43 +96,64 @@ static int nodups_specs(struct saved_data *data, const
> char *path)
> > return rc;
> >  }
> >
> > -static int process_text_file(FILE *fp, const char *prefix,
> > -struct selabel_handle *rec, const char *path)
> > +static inline struct saved_data *rec_to_data(struct selabel_handle
> > +*rec) {
> > +   return (struct saved_data *)rec->data; }
> > +
> > +static char *mmap_area_get_line(struct mmap_area *area) {
> > +   size_t len = area->next_len;
> > +   if (!len)
> > +   return NULL;
> > +
> > +   char *start = area->next_addr;
> > +   char *end = memchr(start, '\n', len);
> > +
> > +   /* the file may not end with a newline */
> > +   if (!end)
> > +   end = (char *)area->next_addr + len - 1;
> > +
> > +   *end = '\0';
> 
> Couldn't this clobber the last character in the file if the file does not end 
> with a
> newline?

Yeah I guess I really can't handle this case without increasing the mapping 
size or allocating
A new buffer to copy into. Considering, that the only time a file not ending 
with a newline
was my own stupid mistake, I am not too worried about the not ending newline, 
especially
considering that the Android build process handles it if it's missing.

Perhaps I could always add a terminating \0 to the mapping by growing it by one 
during mmap
And explicitly setting the byte.

> 
> > +   /* len includes null byte */
> > +   len = end - start;
> > +
> > +   area->next_len -= len + 1;
> > +   area->next_addr = ++end;
> > +   return start;
> > +}
> > +
> > +static int process_text_file(const char *path, const char *prefix,
> > +   struct selabel_handle *rec)
> >  {
> > int rc;
> > -   size_t line_len;
> > unsigned int lineno = 0;
> > -   char *line_buf = NULL;
> > +   char *line_buf;
> > +   struct mmap_area *areas = rec_to_data(rec)->mmap_areas;
> > +
> > +   /* mmap_area_get_line() and process_line() require mutable string
> pointers */
> > +   rc = mprotect(areas->addr, areas->len, PROT_READ | PROT_WRITE);
> 
> Just map it with PROT_READ|PROT_WRITE in the beginning.

Yep.


___
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: [RFC] mmap file_contexts and property_contexts:

[Roberts, William C]

> -Original Message-
> From: Stephen Smalley [mailto:s...@tycho.nsa.gov]
> Sent: Tuesday, September 20, 2016 6:18 AM
> To: Roberts, William C <william.c.robe...@intel.com>; selinux@tycho.nsa.gov;
> seandroid-l...@tycho.nsa.gov; jda...@google.com
> Subject: Re: [RFC] mmap file_contexts and property_contexts:
> 
> On 09/19/2016 05:51 PM, Roberts, William C wrote:
> > FYI I only tested this with checkfc...
> 
> Evidently.  matchpathcon and sefcontext_compile both report calls to
> free() on invalid pointers and abort.

That doesn’t surprise me, I only tested the checkfc usages. Hence #4 in todo's

> 
> >
> >> -Original Message-
> >> From: Roberts, William C
> >> Sent: Monday, September 19, 2016 2:45 PM
> >> To: selinux@tycho.nsa.gov; seandroid-l...@tycho.nsa.gov;
> >> s...@tycho.nsa.gov; jda...@google.com
> >> Cc: Roberts, William C <william.c.robe...@intel.com>
> >> Subject: [RFC] mmap file_contexts and property_contexts:
> >>
> >> From: William Roberts <william.c.robe...@intel.com>
> >>
> >> THIS IS WIP...
> >>
> >> Rather than using stdio and making copies, just mmap the files and
> >> use the pointers in place. The affect of this change, is that text
> >> file load time is now faster than binary load time by 4.7% when
> >> testing with a file_contexts file from the Android tree. Note that the 
> >> Android
> doesn't use monstrous regexs.
> >>
> >> Times are the average of 3 runs.
> >>
> >> BEFORE:
> >> Text file allocs: 114803
> >> Text file load time: 0.266101
> >> Bin file allocs: 93073
> >> Bin file load time: 0.248757667
> >>
> >> AFTER:
> >> Text file allocs: 103933
> >> Text file load time: 0.236192667
> >> Bin file allocs: 87645
> >> Bin file load time: .247607333
> >>
> >> THINGS TO DO:
> >> 1. What's arm performance like?
> >> 2. What interfaces to backends are busted by this (if any)?
> >> 3. Test Android Properties
> >> 4. Im pretty sure this breaks sefcontext_compile, fix.
> >> 5. Test with PCRE2 enabled.
> >> 6. Spell check this message!
> >> 7. Run checkpatch
> >>
> >> Signed-off-by: William Roberts <william.c.robe...@intel.com>
> >> ---
> >>  libselinux/src/label.c  |   2 -
> >>  libselinux/src/label_android_property.c |  22 ++---
> >>  libselinux/src/label_file.c | 140 
> >> +++-
> >>  libselinux/src/label_file.h |  66 +--
> >>  libselinux/src/label_internal.h |   3 +-
> >>  libselinux/src/label_support.c  |  79 --
> >>  6 files changed, 172 insertions(+), 140 deletions(-)
> >>
> >> diff --git a/libselinux/src/label.c b/libselinux/src/label.c index
> >> 963bfcb..d767b49
> >> 100644
> >> --- a/libselinux/src/label.c
> >> +++ b/libselinux/src/label.c
> >> @@ -15,8 +15,6 @@
> >>  #include "callbacks.h"
> >>  #include "label_internal.h"
> >>
> >> -#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
> >> -
> >>  typedef int (*selabel_initfunc)(struct selabel_handle *rec,
> >>const struct selinux_opt *opts,
> >>unsigned nopts);
> >> diff --git a/libselinux/src/label_android_property.c
> >> b/libselinux/src/label_android_property.c
> >> index 290b438..2aac394 100644
> >> --- a/libselinux/src/label_android_property.c
> >> +++ b/libselinux/src/label_android_property.c
> >> @@ -85,13 +85,19 @@ static int process_line(struct selabel_handle *rec,
> >>int pass, unsigned lineno)
> >>  {
> >>int items;
> >> -  char *prop = NULL, *context = NULL;
> >> +  union {
> >> +  struct {
> >> +  char *prop;
> >> +  char *context;
> >> +  };
> >> +  char *array[2];
> >> +  } found = { .array = { 0 } };
> >>struct saved_data *data = (struct saved_data *)rec->data;
> >>spec_t *spec_arr = data->spec_arr;
> >>unsigned int nspec = data->nspec;
> >>const char *errbuf = NULL;
> >>
> >> -  items = read_spec_entries(line_buf, , 2, , );
> >> +  items = read_spec_entries(line_buf, ,
> >> +ARRAY_SIZE(found.array), found.array);
> &

RE: [RFC] mmap file_contexts and property_contexts:

[Roberts, William C]

FYI I only tested this with checkfc...

> -Original Message-
> From: Roberts, William C
> Sent: Monday, September 19, 2016 2:45 PM
> To: selinux@tycho.nsa.gov; seandroid-l...@tycho.nsa.gov; s...@tycho.nsa.gov;
> jda...@google.com
> Cc: Roberts, William C <william.c.robe...@intel.com>
> Subject: [RFC] mmap file_contexts and property_contexts:
> 
> From: William Roberts <william.c.robe...@intel.com>
> 
> THIS IS WIP...
> 
> Rather than using stdio and making copies, just mmap the files and use the
> pointers in place. The affect of this change, is that text file load time is 
> now faster
> than binary load time by 4.7% when testing with a file_contexts file from the
> Android tree. Note that the Android doesn't use monstrous regexs.
> 
> Times are the average of 3 runs.
> 
> BEFORE:
> Text file allocs: 114803
> Text file load time: 0.266101
> Bin file allocs: 93073
> Bin file load time: 0.248757667
> 
> AFTER:
> Text file allocs: 103933
> Text file load time: 0.236192667
> Bin file allocs: 87645
> Bin file load time: .247607333
> 
> THINGS TO DO:
> 1. What's arm performance like?
> 2. What interfaces to backends are busted by this (if any)?
> 3. Test Android Properties
> 4. Im pretty sure this breaks sefcontext_compile, fix.
> 5. Test with PCRE2 enabled.
> 6. Spell check this message!
> 7. Run checkpatch
> 
> Signed-off-by: William Roberts <william.c.robe...@intel.com>
> ---
>  libselinux/src/label.c  |   2 -
>  libselinux/src/label_android_property.c |  22 ++---
>  libselinux/src/label_file.c | 140 
> +++-
>  libselinux/src/label_file.h |  66 +--
>  libselinux/src/label_internal.h |   3 +-
>  libselinux/src/label_support.c  |  79 --
>  6 files changed, 172 insertions(+), 140 deletions(-)
> 
> diff --git a/libselinux/src/label.c b/libselinux/src/label.c index 
> 963bfcb..d767b49
> 100644
> --- a/libselinux/src/label.c
> +++ b/libselinux/src/label.c
> @@ -15,8 +15,6 @@
>  #include "callbacks.h"
>  #include "label_internal.h"
> 
> -#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
> -
>  typedef int (*selabel_initfunc)(struct selabel_handle *rec,
>   const struct selinux_opt *opts,
>   unsigned nopts);
> diff --git a/libselinux/src/label_android_property.c
> b/libselinux/src/label_android_property.c
> index 290b438..2aac394 100644
> --- a/libselinux/src/label_android_property.c
> +++ b/libselinux/src/label_android_property.c
> @@ -85,13 +85,19 @@ static int process_line(struct selabel_handle *rec,
>   int pass, unsigned lineno)
>  {
>   int items;
> - char *prop = NULL, *context = NULL;
> + union {
> + struct {
> + char *prop;
> + char *context;
> + };
> + char *array[2];
> + } found = { .array = { 0 } };
>   struct saved_data *data = (struct saved_data *)rec->data;
>   spec_t *spec_arr = data->spec_arr;
>   unsigned int nspec = data->nspec;
>   const char *errbuf = NULL;
> 
> - items = read_spec_entries(line_buf, , 2, , );
> + items = read_spec_entries(line_buf, , ARRAY_SIZE(found.array),
> +found.array);
>   if (items < 0) {
>   items = errno;
>   selinux_log(SELINUX_ERROR,
> @@ -108,18 +114,14 @@ static int process_line(struct selabel_handle *rec,
>   selinux_log(SELINUX_ERROR,
>   "%s:  line %u is missing fields\n", path,
>   lineno);
> - free(prop);
>   errno = EINVAL;
>   return -1;
>   }
> 
> - if (pass == 0) {
> - free(prop);
> - free(context);
> - } else if (pass == 1) {
> + if (pass == 1) {
>   /* On the second pass, process and store the specification in
> spec. */
> - spec_arr[nspec].property_key = prop;
> - spec_arr[nspec].lr.ctx_raw = context;
> + spec_arr[nspec].property_key = found.prop;
> + spec_arr[nspec].lr.ctx_raw = found.context;
> 
>   if (rec->validating) {
>   if (selabel_validate(rec, _arr[nspec].lr) < 0) { 
> @@ -
> 234,7 +236,7 @@ static void closef(struct selabel_handle *rec)
>   for (i = 0; i < data->nspec; i++) {
>   spec = >spec_arr[i];
>   free(spec->property_key);
> - free(spec->lr.ctx_raw);
> + //free(spec->lr.c

RE: [PATCH v2] libselinux: clean up process file

[Roberts, William C]

> > > +static FILE *open_file(const char *path, const char *suffix,
> > > +char *save_path, size_t len, struct stat *sb) {
> > > + unsigned i;
> > > + int rc;
> > > + char stack_path[len];
> >
> > Ew, what is this?  C99 magic.  Probably just make it PATH_MAX and be
> > done with it.
> 
> You already have C99 magic with mixed code and data declarations, so I don't 
> see
> the problem.
> https://gcc.gnu.org/onlinedocs/gcc/Mixed-Declarations.html
> https://gcc.gnu.org/onlinedocs/gcc/Variable-Length.html
> 

You also seem to have more magic with inline, when passing
CFLAGS='-std=c90' to make it complains about:

cc -std=c90 -I../include -I/usr/include -D_GNU_SOURCE   -c -o booleans.o 
booleans.c
In file included from avc.c:14:0:
avc_internal.h:36:15: error: expected ‘=’, ‘,’, ‘;’, ‘asm’ or ‘__attribute__’ 
before ‘void’
 static inline void set_callbacks(const struct avc_memory_callback *mem_cb,

Looking at:
https://gcc.gnu.org/onlinedocs/gcc/Inline.html
http://stackoverflow.com/questions/12151168/does-ansi-c-not-know-the-inline-keyword

It appears you need something like __inline__

If this code base was met to compile C90, it already failed and is only C99 
compliant + GNUC extensions.
So I don't see the harm in using the C99 capabilities.




___
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 v2] libselinux: clean up process file

[Roberts, William C]

> -Original Message-
> From: Stephen Smalley [mailto:s...@tycho.nsa.gov]
> Sent: Thursday, September 8, 2016 12:41 PM
> To: Roberts, William C <william.c.robe...@intel.com>; selinux@tycho.nsa.gov;
> seandroid-l...@tycho.nsa.gov; jwca...@tycho.nsa.gov
> Subject: Re: [PATCH v2] libselinux: clean up process file
> 
> On 09/08/2016 03:30 PM, Roberts, William C wrote:
> > 
> >
> >>>> +/* Append any given suffix */
> >>>> +char *to = stpcpy([current_size], ".");
> >>>
> >>> Simpler as:
> >>>   char *to = current + current_size;
> >>>   *to++ = '.';
> >>
> >> I don't think this is simpler, but I'll do it.
> >
> > Doing that as is gets us this:
> > ==26050== Conditional jump or move depends on uninitialised value(s)
> > ==26050==at 0x4C2DD9A: strcat (in /usr/lib/valgrind/vgpreload_memcheck-
> amd64-linux.so)
> > ==26050==by 0x4E4B6D8: rolling_append (label_file.c:429)
> > ==26050==by 0x4E4B8A3: open_file (label_file.c:472)
> > ==26050==by 0x4E4BAC6: process_file (label_file.c:519)
> > ==26050==by 0x4E4BE6D: init (label_file.c:582)
> > ==26050==by 0x4E4D02B: selabel_file_init (label_file.c:965)
> > ==26050==by 0x4E481F5: selabel_open (label.c:340)
> > ==26050==by 0x4E513E0: matchpathcon_init_prefix (matchpathcon.c:322)
> > ==26050==by 0x4E51725: matchpathcon (matchpathcon.c:413)
> > ==26050==by 0x400D86: printmatchpathcon (matchpathcon.c:26)
> > ==26050==by 0x40141F: main (matchpathcon.c:196)
> >
> > Because strcat() needs to fastforward to the null byte, this would
> > need to change to an strcpy.
> 
> Yes, sorry - switch the strcat() to strcpy() too.  Simpler and more 
> efficient, and if
> you had to append further components, you could use
> stpcpy() rather than strcpy() each time until the last one.
> 
FYI: That change actually dropped off aprox 100 bytes on the obj code.


___
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 v2] libselinux: clean up process file

[Roberts, William C]

> > > + /* Append any given suffix */
> > > + char *to = stpcpy([current_size], ".");
> >
> > Simpler as:
> > char *to = current + current_size;
> > *to++ = '.';
> 
> I don't think this is simpler, but I'll do it.

Doing that as is gets us this:
==26050== Conditional jump or move depends on uninitialised value(s)
==26050==at 0x4C2DD9A: strcat (in 
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==26050==by 0x4E4B6D8: rolling_append (label_file.c:429)
==26050==by 0x4E4B8A3: open_file (label_file.c:472)
==26050==by 0x4E4BAC6: process_file (label_file.c:519)
==26050==by 0x4E4BE6D: init (label_file.c:582)
==26050==by 0x4E4D02B: selabel_file_init (label_file.c:965)
==26050==by 0x4E481F5: selabel_open (label.c:340)
==26050==by 0x4E513E0: matchpathcon_init_prefix (matchpathcon.c:322)
==26050==by 0x4E51725: matchpathcon (matchpathcon.c:413)
==26050==by 0x400D86: printmatchpathcon (matchpathcon.c:26)
==26050==by 0x40141F: main (matchpathcon.c:196)

Because strcat() needs to fastforward to the null byte, this would need to
change to an strcpy.




___
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 v2] libselinux: clean up process file

[Roberts, William C]

> -Original Message-
> From: Stephen Smalley [mailto:s...@tycho.nsa.gov]
> Sent: Thursday, September 8, 2016 8:15 AM
> To: Roberts, William C <william.c.robe...@intel.com>; selinux@tycho.nsa.gov;
> seandroid-l...@tycho.nsa.gov; jwca...@tycho.nsa.gov
> Subject: Re: [PATCH v2] libselinux: clean up process file
> 
> On 09/06/2016 08:07 PM, william.c.robe...@intel.com wrote:
> > From: William Roberts <william.c.robe...@intel.com>
> >
> > The current process_file() code will open the file twice on the case
> > of a binary file, correct this.
> >
> > The general flow through process_file() was a bit difficult to read,
> > streamline the routine to be more readable.
> >
> > Detailed statistics of before and after:
> >
> > Source lines of code reported by cloc on modified files:
> > before: 735
> > after: 740
> >
> > Object size difference:
> > before: 195530 bytes
> > after:  195529 bytes
> >
> > Signed-off-by: William Roberts <william.c.robe...@intel.com>
> > ---
> >  libselinux/src/label_file.c | 263
> > 
> >  1 file changed, 145 insertions(+), 118 deletions(-)
> >
> > diff --git a/libselinux/src/label_file.c b/libselinux/src/label_file.c
> > index c89bb35..6839a77 100644
> > --- a/libselinux/src/label_file.c
> > +++ b/libselinux/src/label_file.c
> > @@ -97,62 +97,43 @@ static int nodups_specs(struct saved_data *data, const
> char *path)
> > return rc;
> >  }
> >
> > -static int load_mmap(struct selabel_handle *rec, const char *path,
> > -   struct stat *sb, bool isbinary,
> > -   struct selabel_digest *digest)
> > +static int process_text_file(FILE *fp, const char *prefix, struct
> > +selabel_handle *rec, const char *path) {
> > +   /*
> > +* Then do detailed validation of the input and fill the spec array
> > +*/
> 
> You can drop this comment; it is no longer helpful/relevant.
> 
> > +   int rc;
> > +   size_t line_len;
> > +   unsigned lineno = 0;
> > +   char *line_buf = NULL;
> > +
> > +   while (getline(_buf, _len, fp) > 0) {
> > +   rc = process_line(rec, path, prefix, line_buf, ++lineno);
> > +   if (rc)
> > +   goto out;
> > +   }
> > +   rc = 0;
> > +out:
> > +   free(line_buf);
> > +   return rc;
> > +}
> > +
> > +static int load_mmap(FILE *fp, size_t len, struct selabel_handle
> > +*rec, const char *path)
> >  {
> > struct saved_data *data = (struct saved_data *)rec->data;
> > -   char mmap_path[PATH_MAX + 1];
> > -   int mmapfd;
> > int rc;
> > -   struct stat mmap_stat;
> > char *addr, *str_buf;
> > -   size_t len;
> > int *stem_map;
> > struct mmap_area *mmap_area;
> > uint32_t i, magic, version;
> > uint32_t entry_len, stem_map_len, regex_array_len;
> >
> > -   if (isbinary) {
> > -   len = strlen(path);
> > -   if (len >= sizeof(mmap_path))
> > -   return -1;
> > -   strcpy(mmap_path, path);
> > -   } else {
> > -   rc = snprintf(mmap_path, sizeof(mmap_path), "%s.bin", path);
> > -   if (rc >= (int)sizeof(mmap_path))
> > -   return -1;
> > -   }
> > -
> > -   mmapfd = open(mmap_path, O_RDONLY | O_CLOEXEC);
> > -   if (mmapfd < 0)
> > -   return -1;
> > -
> > -   rc = fstat(mmapfd, _stat);
> > -   if (rc < 0) {
> > -   close(mmapfd);
> > -   return -1;
> > -   }
> > -
> > -   /* if mmap is old, ignore it */
> > -   if (mmap_stat.st_mtime < sb->st_mtime) {
> > -   close(mmapfd);
> > -   return -1;
> > -   }
> > -
> > -   /* ok, read it in... */
> > -   len = mmap_stat.st_size;
> > -   len += (sysconf(_SC_PAGE_SIZE) - 1);
> > -   len &= ~(sysconf(_SC_PAGE_SIZE) - 1);
> > -
> > mmap_area = malloc(sizeof(*mmap_area));
> > if (!mmap_area) {
> > -   close(mmapfd);
> > return -1;
> > }
> >
> > -   addr = mmap(NULL, len, PROT_READ, MAP_PRIVATE, mmapfd, 0);
> > -   close(mmapfd);
> > +   addr = mmap(NULL, len, PROT_READ, MAP_PRIVATE, fileno(fp), 0);
> > if (addr == MAP_FAILED) {
> > free(mmap_area);
> > perror("mmap");
&g

RE: [PATCH] libselinux: add support for pcre2

[Roberts, William C]

> -Original Message-
> From: Janis Danisevskis [mailto:jda...@android.com]
> Sent: Thursday, September 8, 2016 8:52 AM
> To: selinux@tycho.nsa.gov; seandroid-l...@tycho.nsa.gov; s...@tycho.nsa.gov;
> jwca...@tycho.nsa.gov
> Cc: Janis Danisevskis <jda...@google.com>; Roberts, William C
> <william.c.robe...@intel.com>
> Subject: [PATCH] libselinux: add support for pcre2
> 
> From: Janis Danisevskis <jda...@google.com>
> 
> This patch moves all pcre1/2 dependencies into the new files regex.h and 
> regex.c
> implementing the common denominator of features needed by libselinux. The
> compiler flag -DUSE_PCRE2 toggles between the used implementations.
> 
> As of this patch libselinux supports either pcre or pcre2 but not both at the 
> same
> time. The persistently stored file contexts information differs. This means
> libselinux can only load file context files generated by sefcontext_compile 
> build
> with the same pcre variant.
> 
> Also, for pcre2 the persistent format is architecture dependent.
> Stored precompiled regular expressions can only be used on the same
> architecture they were generated on. If pcre2 is used and sefcontext_compile
> shall generate portable output, it and libselinux must be compiled with -
> DNO_PERSISTENTLY_STORED_PATTERNS, at the cost of having to recompile the
> regular expressions at load time.
> 
> Signed-off-by: Janis Danisevskis <jda...@google.com>
> 
> This patch includes includes:

Double includes

> 
> libselinux: fix memory leak on pcre2
> 
> Introduced a malloc on pcre_version(). Libselinux expected this to be static, 
> just
> use a static internal buffer.
> 
> Signed-off-by: William Roberts <william.c.robe...@intel.com>

You can remove any attribution since its squashed down, this really doesn't
Apply, so don't feel obligated to include any of this information.

> ---
>  libselinux/Makefile   |  13 +
>  libselinux/src/Makefile   |   4 +-
>  libselinux/src/label_file.c   |  93 ++-
>  libselinux/src/label_file.h   |  59 ++---
>  libselinux/src/regex.c| 461 
> ++
>  libselinux/src/regex.h| 169 +
>  libselinux/utils/Makefile |   4 +-
>  libselinux/utils/sefcontext_compile.c |  55 +---
>  8 files changed, 697 insertions(+), 161 deletions(-)  create mode 100644
> libselinux/src/regex.c  create mode 100644 libselinux/src/regex.h
> 
> diff --git a/libselinux/Makefile b/libselinux/Makefile index 6142b60..15d051e
> 100644
> --- a/libselinux/Makefile
> +++ b/libselinux/Makefile
> @@ -24,6 +24,19 @@ ifeq ($(DISABLE_SETRANS),y)  endif  export DISABLE_AVC
> DISABLE_SETRANS DISABLE_RPM DISABLE_BOOL EMFLAGS
> 
> +USE_PCRE2 ?= n
> +DISABLE_PERSISTENTLY_STORED_REGEX_PATTERNS ?= n ifeq ($(USE_PCRE2),y)
> + PCRE_CFLAGS := -DUSE_PCRE2 -DPCRE2_CODE_UNIT_WIDTH=8
> + ifeq ($(DISABLE_PERSISTENTLY_STORED_REGEX_PATTERNS), y)
> + PCRE_CFLAGS += -DNO_PERSISTENTLY_STORED_PATTERNS
> + endif
> + PCRE_LDFLAGS := -lpcre2-8
> +else
> + PCRE_LDFLAGS := -lpcre
> +endif
> +export PCRE_CFLAGS PCRE_LDFLAGS
> +
>  all install relabel clean distclean indent:
>   @for subdir in $(SUBDIRS); do \
>   (cd $$subdir && $(MAKE) $@) || exit 1; \ diff --git
> a/libselinux/src/Makefile b/libselinux/src/Makefile index 37d01af..66687e6
> 100644
> --- a/libselinux/src/Makefile
> +++ b/libselinux/src/Makefile
> @@ -74,7 +74,7 @@ CFLAGS ?= -O -Wall -W -Wundef -Wformat-y2k -Wformat-
> security -Winit-self -Wmissi
>-fipa-pure-const -Wno-suggest-attribute=pure -Wno-suggest-
> attribute=const \
>-Werror -Wno-aggregate-return -Wno-redundant-decls
> 
> -override CFLAGS += -I../include -I$(INCLUDEDIR) -D_GNU_SOURCE $(EMFLAGS)
> +override CFLAGS += -I../include -I$(INCLUDEDIR) -D_GNU_SOURCE
> +$(EMFLAGS) $(PCRE_CFLAGS)
> 
>  SWIG_CFLAGS += -Wno-error -Wno-unused-variable -Wno-unused-but-set-
> variable -Wno-unused-parameter \
>   -Wno-shadow -Wno-uninitialized -Wno-missing-prototypes -
> Wno-missing-declarations @@ -113,7 +113,7 @@ $(LIBA): $(OBJS)
>   $(RANLIB) $@
> 
>  $(LIBSO): $(LOBJS)
> - $(CC) $(CFLAGS) -shared -o $@ $^ -lpcre -ldl $(LDFLAGS) -L$(LIBDIR) 
> -Wl,-
> soname,$(LIBSO),-z,defs,-z,relro
> + $(CC) $(CFLAGS) -shared -o $@ $^ $(PCRE_LDFLAGS) -ldl $(LDFLAGS)
> +-L$(LIBDIR) -Wl,-soname,$(LIBSO),-z,defs,-z,relro
>   ln -sf $@ $(TARGET)
> 
>  $(LIBPC): $(LIBPC).in ../VERSION
> diff --git a/libselinux/src/label_file.c b/libselinux/src/label_file.c index
> c89bb35..e41c351 100644
> --- a/libselinux/src/label_file.c
> +

RE: [PATCH] libselinux: clean up process file

[Roberts, William C]

> -Original Message-
> From: Roberts, William C
> Sent: Friday, September 2, 2016 10:34 AM
> To: 'Stephen Smalley' <s...@tycho.nsa.gov>; 'selinux@tycho.nsa.gov'
> <selinux@tycho.nsa.gov>; 'jwca...@tycho.nsa.gov' <jwca...@tycho.nsa.gov>;
> 'seandroid-l...@tycho.nsa.gov' <seandroid-l...@tycho.nsa.gov>
> Subject: RE: [PATCH] libselinux: clean up process file
> 
> 
> > since M, I think it makes sense to have this in the following order:
> > 1. patch to only open the file once
> > 2. patch to switch to stdio fmemopen
> 
> Actually looking at this, it would be write it once for item 1, then discard 
> that as
> Most of it would change for item two. So I see no value in splitting this up.
> I think we just implement item 2 and as a sideffect it won't open the file up
> multiple times.

Looking at this in the context of fmemopen() or even some of the others like 
open_memstream()
They both seem to copy data into an internal buffer, so I did not see a way, 
for instance to just
set pointers into the mapping. The hand rolled getline like routine that worked 
on the area
struct should really not be allocating and using the pointers into the map, 
that was a poor
decision.

> 
> 

___
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] libselinux: clean up process file

[Roberts, William C]

> since M, I think it makes sense to have this in the following order:
> 1. patch to only open the file once
> 2. patch to switch to stdio fmemopen

Actually looking at this, it would be write it once for item 1, then discard 
that as
Most of it would change for item two. So I see no value in splitting this up.
I think we just implement item 2 and as a sideffect it won't open the file up
multiple times.



___
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

[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 <william.c.robe...@intel.com>
> Cc: selinux@tycho.nsa.gov; seandroid-l...@tycho.nsa.gov; Stephen Smalley
> <s...@tycho.nsa.gov>
> Subject: Re: [PATCH 1/3] selinux: detect invalid ebitmap
> 
> On Tue, Aug 23, 2016 at 4:49 PM,  <william.c.robe...@intel.com> wrote:
> > From: William Roberts <william.c.robe...@intel.com>
> >
> > 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 <william.c.robe...@intel.com>


> 
> > 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 1/2] libsepol: calloc all the *_to_val_structs

[Roberts, William C]

> -Original Message-
> From: Stephen Smalley [mailto:s...@tycho.nsa.gov]
> Sent: Friday, August 19, 2016 6:06 AM
> To: Roberts, William C <william.c.robe...@intel.com>; selinux@tycho.nsa.gov;
> jwca...@tycho.nsa.gov; seandroid-l...@tycho.nsa.gov
> Subject: Re: [PATCH 1/2] libsepol: calloc all the *_to_val_structs
> 
> On 08/18/2016 04:54 PM, william.c.robe...@intel.com wrote:
> > From: William Roberts <william.c.robe...@intel.com>
> >
> > The usage patterns between these structures seem similair to
> > role_val_to_struct usages. Calloc these up to prevent any unitialized
> > usages.
> >
> > Signed-off-by: William Roberts <william.c.robe...@intel.com>
> > ---
> >  libsepol/src/mls.c  | 2 +-
> >  libsepol/src/policydb.c | 6 +++---
> >  libsepol/src/users.c| 9 -
> >  3 files changed, 12 insertions(+), 5 deletions(-)
> >
> > diff --git a/libsepol/src/mls.c b/libsepol/src/mls.c index
> > 2dc5f2b..8047d91 100644
> > --- a/libsepol/src/mls.c
> > +++ b/libsepol/src/mls.c
> > @@ -312,7 +312,7 @@ int mls_context_isvalid(const policydb_t * p, const
> context_struct_t * c)
> > if (!c->user || c->user > p->p_users.nprim)
> > return 0;
> > usrdatum = p->user_val_to_struct[c->user - 1];
> > -   if (!mls_range_contains(usrdatum->exp_range, c->range))
> > +   if (!usrdatum || !mls_range_contains(usrdatum->exp_range, c->range))
> > return 0;   /* user may not be associated with range */
> >
> > return 1;
> > diff --git a/libsepol/src/policydb.c b/libsepol/src/policydb.c index
> > c225ac6..5f888d3 100644
> > --- a/libsepol/src/policydb.c
> > +++ b/libsepol/src/policydb.c
> > @@ -1074,7 +1074,7 @@ int policydb_index_others(sepol_handle_t *
> > handle,
> >
> > free(p->user_val_to_struct);
> > p->user_val_to_struct = (user_datum_t **)
> > -   malloc(p->p_users.nprim * sizeof(user_datum_t *));
> > +   calloc(p->p_users.nprim, sizeof(user_datum_t *));
> > if (!p->user_val_to_struct)
> > return -1;
> >
> > @@ -4006,12 +4006,12 @@ int policydb_reindex_users(policydb_t * p)
> > free(p->sym_val_to_name[i]);
> >
> > p->user_val_to_struct = (user_datum_t **)
> > -   malloc(p->p_users.nprim * sizeof(user_datum_t *));
> > +   calloc(p->p_users.nprim, sizeof(user_datum_t *));
> > if (!p->user_val_to_struct)
> > return -1;
> >
> > p->sym_val_to_name[i] = (char **)
> > -   malloc(p->symtab[i].nprim * sizeof(char *));
> > +   calloc(p->symtab[i].nprim, sizeof(char *));
> > if (!p->sym_val_to_name[i])
> > return -1;
> >
> > diff --git a/libsepol/src/users.c b/libsepol/src/users.c index
> > ce54c2b..3ffb166 100644
> > --- a/libsepol/src/users.c
> > +++ b/libsepol/src/users.c
> > @@ -19,12 +19,17 @@ static int user_to_record(sepol_handle_t * handle,
> >
> > const char *name = policydb->p_user_val_to_name[user_idx];
> > user_datum_t *usrdatum = policydb->user_val_to_struct[user_idx];
> > -   ebitmap_t *roles = &(usrdatum->roles.roles);
> > +   ebitmap_t *roles;
> > ebitmap_node_t *rnode;
> > unsigned bit;
> >
> > sepol_user_t *tmp_record = NULL;
> >
> > +   if (!usrdatum)
> > +   goto err;
> > +
> > +   roles = &(usrdatum->roles.roles);
> > +
> > if (sepol_user_create(handle, _record) < 0)
> > goto err;
> >
> > @@ -234,6 +239,7 @@ int sepol_user_modify(sepol_handle_t * handle,
> > if (!tmp_ptr)
> > goto omem;
> > policydb->user_val_to_struct = tmp_ptr;
> > +   policydb->user_val_to_struct[policydb->p_users.nprim] = NULL;
> >
> > tmp_ptr = realloc(policydb->sym_val_to_name[SYM_USERS],
> >   (policydb->p_users.nprim +
> > @@ -241,6 +247,7 @@ int sepol_user_modify(sepol_handle_t * handle,
> > if (!tmp_ptr)
> > goto omem;
> > policydb->sym_val_to_name[SYM_USERS] = tmp_ptr;
> > +   policydb->p_user_val_to_name[policydb->p_users.nprim] =
> NULL;
> 
> This one is wrong.
> 
> >
> > /* Need to copy the user name */
> > name = strdup(cname);
> >

After looking at the email and the patch again, that’s just a context hunk, I 
see no + or - next it,
And I verified it still exists in my source tree. I never touched that hunk or  
am I missing
Some subtle interaction?


___
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 v4 7/7] libsepol: fix overflow and 0 length allocations

[Roberts, William C]

> -Original Message-
> From: Roberts, William C
> Sent: Tuesday, August 16, 2016 10:29 AM
> To: selinux@tycho.nsa.gov; jwca...@tycho.nsa.gov; seandroid-
> l...@tycho.nsa.gov; s...@tycho.nsa.gov
> Cc: Roberts, William C <william.c.robe...@intel.com>
> Subject: [PATCH v4 7/7] libsepol: fix overflow and 0 length allocations
> 
> From: William Roberts <william.c.robe...@intel.com>
> 
> Throughout libsepol, values taken from sepolicy are used in places where 
> length
> == 0 or length ==  matter, find and fix these.
> 
> Also, correct any type mismatches noticed along the way.
> 
> Signed-off-by: William Roberts <william.c.robe...@intel.com>
> ---
>  libsepol/src/conditional.c|  3 +-
>  libsepol/src/context.c| 16 +++---
>  libsepol/src/context_record.c | 72 +--
> 
>  libsepol/src/module.c | 13 
>  libsepol/src/module_to_cil.c  |  4 ++-
>  libsepol/src/policydb.c   | 52 +--
>  libsepol/src/private.h|  3 ++
>  7 files changed, 130 insertions(+), 33 deletions(-)
> 
> diff --git a/libsepol/src/conditional.c b/libsepol/src/conditional.c index
> ea47cdd..8680eb2 100644
> --- a/libsepol/src/conditional.c
> +++ b/libsepol/src/conditional.c
> @@ -589,7 +589,8 @@ int cond_read_bool(policydb_t * p,
>   goto err;
> 
>   len = le32_to_cpu(buf[2]);
> -
> + if (zero_or_saturated(len))
> + goto err;
>   key = malloc(len + 1);
>   if (!key)
>   goto err;
> diff --git a/libsepol/src/context.c b/libsepol/src/context.c index 
> 420ee16..a88937f
> 100644
> --- a/libsepol/src/context.c
> +++ b/libsepol/src/context.c
> @@ -10,6 +10,7 @@
>  #include "context.h"
>  #include "handle.h"
>  #include "mls.h"
> +#include "private.h"
> 
>  /* - Compatibility  */
>  int policydb_context_isvalid(const policydb_t * p, const context_struct_t * 
> c)
> @@ -297,10 +298,18 @@ int context_from_string(sepol_handle_t * handle,
>   char *con_cpy = NULL;
>   sepol_context_t *ctx_record = NULL;
> 
> + if (zero_or_saturated(con_str_len)) {
> + ERR(handle, "Invalid context length");
> + goto err;
> + }
> +
>   /* sepol_context_from_string expects a NULL-terminated string */
>   con_cpy = malloc(con_str_len + 1);
> - if (!con_cpy)
> - goto omem;
> + if (!con_cpy) {
> + ERR(handle, "out of memory");
> + goto err;
> + }
> +
>   memcpy(con_cpy, con_str, con_str_len);
>   con_cpy[con_str_len] = '\0';
> 
> @@ -315,9 +324,6 @@ int context_from_string(sepol_handle_t * handle,
>   sepol_context_free(ctx_record);
>   return STATUS_SUCCESS;
> 
> -  omem:
> - ERR(handle, "out of memory");
> -
>err:
>   ERR(handle, "could not create context structure");
>   free(con_cpy);
> diff --git a/libsepol/src/context_record.c b/libsepol/src/context_record.c 
> index
> ac2884a..0a8bbf6 100644
> --- a/libsepol/src/context_record.c
> +++ b/libsepol/src/context_record.c
> @@ -5,6 +5,7 @@
> 
>  #include "context_internal.h"
>  #include "debug.h"
> +#include "private.h"
> 
>  struct sepol_context {
> 
> @@ -279,44 +280,69 @@ int sepol_context_from_string(sepol_handle_t *
> handle,
> 
>  hidden_def(sepol_context_from_string)
> 
> +static inline int safe_sum(size_t *sum, const size_t augends[], const
> +size_t cnt) {
> +
> + size_t a, i;
> +
> + *sum = 0;
> + for(i=0; i < cnt; i++) {
> + /* sum should not be smaller than the addend */
> + a = augends[i];
> + *sum += a;
> + if (*sum < a) {
> + return i;

Damn it...this does not work when index 0 is the bad one... I'll wait till
Tomorrow and aggregate responses again.

> + }
> + }
> +
> + return 0;
> +}
> +
>  int sepol_context_to_string(sepol_handle_t * handle,
>   const sepol_context_t * con, char **str_ptr)  {
> 
>   int rc;
> - const int user_sz = strlen(con->user);
> - const int role_sz = strlen(con->role);
> - const int type_sz = strlen(con->type);
> - const int mls_sz = (con->mls) ? strlen(con->mls) : 0;
> - const int total_sz = user_sz + role_sz + type_sz +
> - mls_sz + ((con->mls) ? 3 : 2);
> -
> - char *str = (char *)malloc(total_sz + 1);
> - if (!str)
> - goto omem;
> + char *str 

RE: [PATCH] libsepol: fix memory leak in expand.c

[Roberts, William C]

> >
> > -  out:
> > + out:
> > +   rc = 0;
> >
> > -   ebitmap_destroy();
> > + err_neg:
> > ebitmap_destroy(_types);
> > + err_types:
> > +   ebitmap_destroy();
> >
> > -   return 0;
> > +   return rc;
> >  }

I just noticed the indenting on the label changed, I went through the
File and it has:
1. labels with 7 leading spaces
2. labels left justified

Which one is the way you want them, left justified?

Bill


___
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 v2] module_to_cil: fix use of uninitialized variables

[Roberts, William C]

> -Original Message-
> From: Steve Lawrence [mailto:slawre...@tresys.com]
> Sent: Wednesday, August 3, 2016 2:51 PM
> To: Roberts, William C <william.c.robe...@intel.com>; selinux@tycho.nsa.gov;
> seandroid-l...@tycho.nsa.gov; s...@tycho.nsa.gov
> Subject: Re: [PATCH v2] module_to_cil: fix use of uninitialized variables
> 
> On 08/03/2016 05:42 PM, william.c.robe...@intel.com wrote:
> > From: William Roberts <william.c.robe...@intel.com>
> >
> > Correct errors like these reported by gcc:
> >
> > module_to_cil.c: In function ‘block_to_cil’:
> > module_to_cil.c:229:20: error: ‘attr_list’ may be used uninitialized in this
> function [-Werror=maybe-uninitialized]
> >   struct list_node *curr = (*attr_list)->head;
> >
> > Usages of attr_list_destroy() were called when list_init() fails. Just
> > bail early on failure.
> >
> > stack_init() and stack_destroy() also suffered from the aforementioned
> > issue.
> >
> > Signed-off-by: William Roberts <william.c.robe...@intel.com>
> > ---
> >  libsepol/src/module_to_cil.c | 8 
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/libsepol/src/module_to_cil.c
> > b/libsepol/src/module_to_cil.c index b9a4af7..9d0d064 100644
> > --- a/libsepol/src/module_to_cil.c
> > +++ b/libsepol/src/module_to_cil.c
> > @@ -1307,7 +1307,7 @@ static int cond_list_to_cil(int indent, struct
> > policydb *pdb, struct cond_node *
> >
> > rc = list_init(_list);
> > if (rc != 0) {
> > -   goto exit;
> > +   return rc;
> > }
> >
> > for (cond = cond_list; cond != NULL; cond = cond->next) { @@ -3488,7
> > +3488,7 @@ static int block_to_cil(struct policydb *pdb, struct
> > avrule_block *block, struct
> >
> > rc = list_init(_list);
> > if (rc != 0) {
> > -   goto exit;
> > +   return rc;
> > }
> >
> > rc = typealiases_to_cil(indent, pdb, block, stack); @@ -3635,7
> > +3635,7 @@ static int blocks_to_cil(struct policydb *pdb)
> >
> > rc = stack_init();
> > if (rc != 0) {
> > -   goto exit;
> > +   return rc;
> > }
> >
> > block = pdb->global;
> > @@ -3703,7 +3703,7 @@ static int linked_blocks_to_cil(struct policydb
> > *pdb)
> >
> > rc = stack_init();
> > if (rc != 0) {
> > -   goto exit;
> > +   return rc;
> > }
> >
> > block = pdb->global;
> >
> 
> I would recommend just initializing the variables to NULL and keeping the 
> "goto
> exit"'s. That would maintain the single return point, allows for extra 
> cleanup code
> to be run in the future if necessary, and is consistent with the rest of the
> module_to_cil code.

That's fine, but requiring only one exit point isn't consistent with the rest 
of libsepol,
some do it this way, some don’t. There will be slightly more overhead In the 
approach
of initializing it first and then jumping on failure, then checking if its NULL 
in the
destruction routine, then finally returning versus just checking the init 
routine
and returning.

> 
> - Steve
> 


___
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] module_to_cil: fix possible use of initialized value

[Roberts, William C]

> -Original Message-
> From: Roberts, William C
> Sent: Wednesday, August 3, 2016 2:37 PM
> To: selinux@tycho.nsa.gov; seandroid-l...@tycho.nsa.gov; s...@tycho.nsa.gov
> Cc: Roberts, William C <william.c.robe...@intel.com>
> Subject: [PATCH] module_to_cil: fix possible use of initialized value

Let's try this commit message again... unitialized!

> 
> From: William Roberts <william.c.robe...@intel.com>
> 
> Correct errors like these reported by gcc:
> 
> module_to_cil.c: In function ‘block_to_cil’:
> module_to_cil.c:229:20: error: ‘attr_list’ may be used uninitialized in this 
> function
> [-Werror=maybe-uninitialized]
>   struct list_node *curr = (*attr_list)->head;
> 
> Usages of attr_list_destroy() were called when list_init() fails. Just bail 
> early on
> failure.
> 
> stack_init() and stack_destroy() also suffered from the aforementioned issue.
> 
> Signed-off-by: William Roberts <william.c.robe...@intel.com>
> ---
>  libsepol/src/module_to_cil.c | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/libsepol/src/module_to_cil.c b/libsepol/src/module_to_cil.c index
> b9a4af7..9d0d064 100644
> --- a/libsepol/src/module_to_cil.c
> +++ b/libsepol/src/module_to_cil.c
> @@ -1307,7 +1307,7 @@ static int cond_list_to_cil(int indent, struct policydb
> *pdb, struct cond_node *
> 
>   rc = list_init(_list);
>   if (rc != 0) {
> - goto exit;
> + return rc;
>   }
> 
>   for (cond = cond_list; cond != NULL; cond = cond->next) { @@ -3488,7
> +3488,7 @@ static int block_to_cil(struct policydb *pdb, struct avrule_block
> *block, struct
> 
>   rc = list_init(_list);
>   if (rc != 0) {
> - goto exit;
> + return rc;
>   }
> 
>   rc = typealiases_to_cil(indent, pdb, block, stack); @@ -3635,7 +3635,7
> @@ static int blocks_to_cil(struct policydb *pdb)
> 
>   rc = stack_init();
>   if (rc != 0) {
> - goto exit;
> + return rc;
>   }
> 
>   block = pdb->global;
> @@ -3703,7 +3703,7 @@ static int linked_blocks_to_cil(struct policydb *pdb)
> 
>   rc = stack_init();
>   if (rc != 0) {
> - goto exit;
> + return rc;
>   }
> 
>   block = pdb->global;
> --
> 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.

RE: RFC Fuzzing SE Linux interfaces

[Roberts, William C]

> -Original Message-
> From: Stephen Smalley [mailto:s...@tycho.nsa.gov]
> Sent: Monday, July 18, 2016 6:21 AM
> To: Roberts, William C <william.c.robe...@intel.com>; selinux@tycho.nsa.gov
> Subject: Re: RFC Fuzzing SE Linux interfaces
> 
> On 07/15/2016 04:18 PM, Roberts, William C wrote:
> >
> >
> > A quick google search didn’t yield much, neither did a grep of the
> > selinux-testsuite, but is their currently any fuzzing work being done
> > on the selinux interfaces?
> 
> Not AFAIK.  There are general system call fuzzers for Linux such trinity and
> syzkaller; if you want to do full fledged fuzzing, you probably want to use 
> one of
> those frameworks rather than rolling your own in selinux-testsuite.  On the 
> other

I planned on using one of the frameworks, not sure which yet. I didn't plan on 
adding
Any fuzzing tests into selinux-testsuite. However, if I find issues, I'll 
likely take the malformed
Input and create a test case on that one, that way we can at least detect 
regressions on
Known bad inputs.

> hand, if you just want to write some specific tests of the selinuxfs and
> /proc/pid/attr interfaces and add them to selinux-testsuite, that's fine too.
> 
> > Also, I noticed that the test suite has some ToDo’s and I didn’t see
> > tests surrounding ioctlcmd there, are their some implemented?
> 
> Not implemented yet, but they are mentioned in the ToDo list:
> $ grep ioctl ToDo
> ioctl: Test new ioctl whitelisting feature.

IMHO we should probably not take new features without a tests.

> 
> You'll need Fedora 24 or newer in order to have the corresponding
> libsepol/checkpolicy support.
> 


___
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.

RFC Fuzzing SE Linux interfaces

[Roberts, William C]

A quick google search didn't yield much, neither did a grep of the 
selinux-testsuite, but is their currently any fuzzing work being done on the 
selinux interfaces?

Also, I noticed that the test suite has some ToDo's and I didn't see tests 
surrounding ioctlcmd there, are their some implemented?

Bill
___
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] selinux: print leading 0x on ioctlcmd audits

[Roberts, William C]

> -Original Message-
> From: Steve Grubb [mailto:sgr...@redhat.com]
> Sent: Friday, July 15, 2016 12:42 PM
> To: Roberts, William C <william.c.robe...@intel.com>
> Cc: Paul Moore <pmo...@redhat.com>; William Roberts
> <bill.c.robe...@gmail.com>; seandroid-l...@tycho.nsa.gov;
> selinux@tycho.nsa.gov; linux-au...@redhat.com
> Subject: Re: [PATCH] selinux: print leading 0x on ioctlcmd audits
> 
> On Friday, July 15, 2016 7:33:09 PM EDT Roberts, William C wrote:
> > 
> >
> > > > This is important so that people don't make up new ones that do
> > > > the same thing. The ioctlcmd field name should be recorded. Are
> > > > there more that need documenting?
> > >
> > > Steve/William, one of you want to send a patch/PR for the field
> > > dictionary?
> >
> > I'll send it over.
> 
> I also asked some other questions.  Is this the ioctl number? As in syscall 
> arg a1? I
> need to know if its the same thing so that I can hook up its translation if 
> so.

Yes, per man ioctl, it's the "request number".  Assuming a0 is the file 
descriptor, then a1 is the
Ioctlcmd value.


> 
> Thanks,
> -Steve

___
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] selinux: print leading 0x on ioctlcmd audits

[Roberts, William C]

> -Original Message-
> From: Steve Grubb [mailto:sgr...@redhat.com]
> Sent: Friday, July 15, 2016 11:54 AM
> To: Paul Moore <p...@paul-moore.com>
> Cc: Roberts, William C <william.c.robe...@intel.com>; selinux@tycho.nsa.gov;
> seandroid-l...@tycho.nsa.gov; Stephen Smalley <s...@tycho.nsa.gov>; linux-
> au...@redhat.com
> Subject: Re: [PATCH] selinux: print leading 0x on ioctlcmd audits
> 
> On Thursday, July 14, 2016 6:17:32 PM EDT Paul Moore wrote:
> > Re: [PATCH] selinux: print leading 0x on ioctlcmd audits
> > From:   Paul Moore <p...@paul-moore.com>
> > To: william.c.robe...@intel.com
> > CC: selinux@tycho.nsa.gov, seandroid-l...@tycho.nsa.gov, Stephen Smalley
> > <s...@tycho.nsa.gov>, Me, linux-au...@redhat.com Date:  Yesterday 6:17
> PM
> >
> > On Thu, Jul 14, 2016 at 3:29 PM,  <william.c.robe...@intel.com> wrote:
> > > From: William Roberts <william.c.robe...@intel.com>
> > >
> > > ioctlcmd is currently printing hex numbers, but their is no leading
> > > 0x. Thus things like ioctlcmd=1234 are misleading, as the base is
> > > not evident.
> > >
> > > Correct this by adding 0x as a prefix, so ioctlcmd=1234 becomes
> > > ioctlcmd=0x1234.
> > >
> > > Signed-off-by: William Roberts <william.c.robe...@intel.com>
> > > ---
> > > security/lsm_audit.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > NOTE: adding Steve Grubb and the audit mailing list to the CC line
> >
> > Like it or not, I believe the general standard/convention when it
> > comes to things like this is to leave off the "0x" prefix; the idea
> > being that is saves precious space in the audit logs and the value is
> > only ever going to be in hex anyway.
> 
> We normally like the 0x prefix on anything that is hex so that stroul can 
> figure it
> out itself. And since AVC's should in theory be rare or occassional, log 
> space is not
> a concern.

Does this mean then the patch will be applied?

> 
> That said, what is this ioctlcmd field name? Is this the ioctl number? As in 
> syscall
> arg a1? If so, it should be hooked up to the interpretation for that.
> 
> Also, we have a field dictionary with some basic info about each field used in
> audit events:
> 
> http://people.redhat.com/sgrubb/audit/field-dictionary.txt
> 
> This is important so that people don't make up new ones that do the same 
> thing.
> The ioctlcmd field name should be recorded. Are there more that need
> documenting?
> 
> -Steve

___
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] libsepol: Change which attributes CIL keeps in the binary policy

[Roberts, William C]

> > >> On May 6, 2016 11:58 AM, "James Carter"  wrote:
> > >>>
> > >>> The removal of attributes that are only used in neverallow rules
> > >>> is hindering AOSP adoption of the CIL compiler. This is because
> > >>> AOSP extracts neverallow rules from its policy.conf for use in the
> > >>> Android compatibility test suite. These neverallow rules are
> > >>> applied against the binary policy being tested to check for a
> > >>> violation. Any neverallow rules with an attribute that has been
> > >>> removed cannot be
> > checked.
> > >>>
> > >>> Now attributes are kept unless they are not used in any allow rule
> > >>> and they are auto-generated or named "cil_gen_require" or do not
> > >>> have any types associated with them.
> > >>
> > >> I see now, you’re keeping them unless they are generated or marked.
> > >
> > > I'm still not convinced  this does what's on the tin. In the case of
> > > AOSP, the Attributes are not used in any allow rules, they are not
> > > auto-generated or named cil_gen_require And they will not have any
> > > types
> > associated with them. So I see them being discarded.
> > >
> >
> > If they don't have types associated with them, then I misunderstood
> > the problem and we will have to keep the attributes without types around.
> 
> This is pretty much all my understanding on this issue is from this link:
> 
> Per sds:
> https://android-review.googlesource.com/#/c/145034/
> 
> "CIL also deletes unused attributes from the kernel policy, which is a 
> problem for
> the Android neverallow checking.  It considers an attribute that is only 
> referenced
> by a neverallow rule to be unused since neverallows are not included in the
> kernel policy, so this drops data_file_type from the kernel policy.  But we 
> then
> would lose checking of the various neverallows on data_file_type by CTS." - 
> sds
> 
> Looking through the policy in file.te:
> 
> file.te:151:type app_data_file, file_type, data_file_type; file.te:153:type
> system_app_data_file, file_type, data_file_type, mlstrustedobject;
> file.te:169:type asec_apk_file, file_type, data_file_type, mlstrustedobject;
> file.te:171:type asec_public_file, file_type, data_file_type; file.te:173:type
> asec_image_file, file_type, data_file_type; file.te:175:type backup_data_file,
> file_type, data_file_type, mlstrustedobject;
> 
> Data file type does have type associations, but it does NOT have allow rules.
> 
> So when this gets generated to cil, how is indicated in cil intermediary not 
> to
> discard that data_file_type attribute?
> 

That’s not the question I meant to ask. The cardinality check would be 
sufficient there.

In other words, this LGTM. I am going to restore sds's changes and try this.

Thanks.



___
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] libsepol: Change which attributes CIL keeps in the binary policy

[Roberts, William C]

From: Selinux [mailto:selinux-boun...@tycho.nsa.gov] On Behalf Of William 
Roberts
Sent: Friday, May 6, 2016 12:16 PM
To: James Carter 
Cc: selinux@tycho.nsa.gov
Subject: Re: [PATCH] libsepol: Change which attributes CIL keeps in the binary 
policy


On May 6, 2016 11:58 AM, "James Carter"  wrote:
>
> The removal of attributes that are only used in neverallow rules is
> hindering AOSP adoption of the CIL compiler. This is because AOSP
> extracts neverallow rules from its policy.conf for use in the Android
> compatibility test suite. These neverallow rules are applied against
> the binary policy being tested to check for a violation. Any neverallow
> rules with an attribute that has been removed cannot be checked.
>
> Now attributes are kept unless they are not used in any allow rule and
> they are auto-generated or named "cil_gen_require" or do not have any
> types associated with them.

I see now, you’re keeping them unless they are generated or marked.

>
> Signed-off-by: James Carter 
> ---
>  libsepol/cil/src/cil_post.c  | 27 +++
>  libsepol/src/module_to_cil.c |  8 +---
>  2 files changed, 32 insertions(+), 3 deletions(-)
>
> diff --git a/libsepol/cil/src/cil_post.c b/libsepol/cil/src/cil_post.c
> index a694b33..f8447c9 100644
> --- a/libsepol/cil/src/cil_post.c
> +++ b/libsepol/cil/src/cil_post.c
> @@ -47,6 +47,9 @@
>  #include "cil_verify.h"
>  #include "cil_symtab.h"
>
> +#define GEN_REQUIRE_ATTR "cil_gen_require" /* Also in 
> libsepol/src/module_to_cil.c */
> +#define TYPEATTR_INFIX "_typeattr_"        /* Also in 
> libsepol/src/module_to_cil.c */
> +
>  static int __cil_expr_to_bitmap(struct cil_list *expr, ebitmap_t *out, int 
> max, struct cil_db *db);
>  static int __cil_expr_list_to_bitmap(struct cil_list *expr_list, ebitmap_t 
> *out, int max, struct cil_db *db);
>
> @@ -1186,6 +1189,27 @@ exit:
>         return SEPOL_ERR;
>  }
>
> +static int cil_typeattribute_used(struct cil_typeattribute *cil_attr)
> +{
> +       if (cil_attr->used) {
> +               return CIL_TRUE;
> +       }
> +
> +       if (strcmp(DATUM(cil_attr)->name, GEN_REQUIRE_ATTR) == 0) {
> +               return CIL_FALSE;
Just by reading this patch with 0 knowledge of cil, I would imagine this would 
be CIL_TRUE on a match with GEN_REQUIRED_ATTR. Especially since cardinality 0 
below returns false.
> +       }
> +
> +       if (strstr(DATUM(cil_attr)->name,TYPEATTR_INFIX) != NULL) {
> +               return CIL_FALSE;
> +       }
> +
> +       if (ebitmap_cardinality(cil_attr->types) == 0) {
> +               return CIL_FALSE;
> +       }
> +
> +       return CIL_TRUE;
> +}
> +
>  static int __cil_post_db_attr_helper(struct cil_tree_node *node, uint32_t 
> *finished, void *extra_args)
>  {
>         int rc = SEPOL_ERR;
> @@ -1208,6 +1232,9 @@ static int __cil_post_db_attr_helper(struct 
> cil_tree_node *node, uint32_t *finis
>                 if (attr->types == NULL) {
>                         rc = __evaluate_type_expression(attr, db);
>                         if (rc != SEPOL_OK) goto exit;
> +                       if (cil_typeattribute_used(attr)) {
> +                               attr->used = CIL_TRUE;
> +                       }
>                 }
>                 break;
>         }
> diff --git a/libsepol/src/module_to_cil.c b/libsepol/src/module_to_cil.c
> index b9a4af7..bcbb4de 100644
> --- a/libsepol/src/module_to_cil.c
> +++ b/libsepol/src/module_to_cil.c
> @@ -58,7 +58,9 @@ FILE *out_file;
>  #define STACK_SIZE 16
>  #define DEFAULT_LEVEL "systemlow"
>  #define DEFAULT_OBJECT "object_r"
> -#define GEN_REQUIRE_ATTR "cil_gen_require"
> +#define GEN_REQUIRE_ATTR "cil_gen_require" /* Also in 
> libsepol/cil/src/cil_post.c */
> +#define TYPEATTR_INFIX "_typeattr_"        /* Also in 
> libsepol/cil/src/cil_post.c */
> +#define ROLEATTR_INFIX "_roleattr_"
>
>  __attribute__ ((format(printf, 1, 2)))
>  static void log_err(const char *fmt, ...)
> @@ -626,9 +628,9 @@ static int set_to_cil_attr(struct policydb *pdb, int 
> is_type, char ***names, uin
>         num_attrs++;
>
>         if (is_type) {
> -               attr_infix = "_typeattr_";
> +               attr_infix = TYPEATTR_INFIX;
>         } else {
> -               attr_infix = "_roleattr_";
> +               attr_infix = ROLEATTR_INFIX;
>         }
>
>         len = strlen(pdb->name) + strlen(attr_infix) + num_digits(num_attrs) 
> + 1;
> --
> 2.5.5
>
> ___
> 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.

___
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.

initial_sid context via libsepol

[Roberts, William C]

How can one obtain the same value as /sys/fs/selinux/initial_contexts/file via 
libsepol?

I've been digging around libsepol and its not quite clear to me.

It looks as though the record is here:
context_struct_t *a = &((policydb_t 
*)pol.db)->ocontexts[OCON_ISID]->context[0];
context_struct_t *b = &((policydb_t 
*)pol.db)->ocontexts[OCON_ISID]->context[1];

printf("%u\n", a->type);
printf("%u\n",b->type);

Prints:
185
0

Not sure if this is right, and how to format the context struct to a string. I 
didn't see any helpers.

Thanks,
Bill
___
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: New SELinux userspace release supporting extended ioctl permissions?

[Roberts, William C]

> -Original Message-
> From: Selinux [mailto:selinux-boun...@tycho.nsa.gov] On Behalf Of Jeffrey
> Vander Stoep
> Sent: Friday, November 6, 2015 9:51 AM
> To: Paul Moore ; SELinux 
> Subject: Re: New SELinux userspace release supporting extended ioctl
> permissions?
> 
> > Applying ioctl whitelisting on GNU/Linux systems looks to me pretty
> > hard to do though. Many drivers, and their ioctls to support.
> 
> Agreed.
> 
> On Android we use ioctl whitelisting only in a targeted manner. I think the 
> same
> approach could (should) be applied to GNU/Linux.
> 
> The example that went out in the M release focused on restricting the leakage 
> of
> privacy sensitive information from socket ioctls. Apps are blocked from using
> socket ioctls to access the wifi MAC address, wifi SSID and layer 2 encryption
> protocol. I could see a similar policy applied to the GNU/Linux browser. Ioctl
> policies on GPU access also seem practical albeit device specific.
> 
> Constructing a comprehensive list of ioctls and the source/target/class sets 
> that
> require access does not strike me as practical. In many cases these ioctls are
> already properly restricted via the ioctl permission.

I think a good example of what could be targeted are perhaps video devices 
implementing
The Video 4 Linux and V4L2 api's. Since those ioctl's are well documented, 
hopefully things
adhere to them. However, based on my experience, there is the standard and then 
what
people do, and they are not one in the same.


> ___
> 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.

___
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: av_decision on audit callback

[Roberts, William C]

> -Original Message-
> From: Stephen Smalley [mailto:s...@tycho.nsa.gov]
> Sent: Friday, October 2, 2015 1:13 PM
> To: Roberts, William C; seandroid-l...@tycho.nsa.gov; selinux@tycho.nsa.gov
> Subject: Re: av_decision on audit callback
> 
> On 10/02/2015 04:07 PM, Roberts, William C wrote:
> >
> >
> >> -Original Message-
> >> From: Stephen Smalley [mailto:s...@tycho.nsa.gov]
> >> Sent: Friday, October 2, 2015 12:12 PM
> >> To: Roberts, William C; seandroid-l...@tycho.nsa.gov;
> >> selinux@tycho.nsa.gov
> >> Subject: Re: av_decision on audit callback
> >>
> >> On 10/02/2015 02:54 PM, Stephen Smalley wrote:
> >>> On 10/02/2015 02:48 PM, Roberts, William C wrote:
> >>>> I would like to be able to gather the result of permissive mode per
> >>>> domain from a check_access() call for the userspace object managers
> >>>> on Android.
> >>>>
> >>>>   From what I can tell check_access() calls avc_has_perm with a
> >>>> NULL 5th argument. That argument is for the struct avc_entry_ref.
> >>>>
> >>>> That structure has a pointer to an opaque type, avc_entry. Which
> >>>> contains struct av_decision.
> >>>>
> >>>> Which contains flags that have a permissive flag:
> >>>>
> >>>> struct av_decision {
> >>>>
> >>>>   access_vector_t allowed;
> >>>>
> >>>>   access_vector_t decided;
> >>>>
> >>>>   access_vector_t auditallow;
> >>>>
> >>>>   access_vector_t auditdeny;
> >>>>
> >>>>   unsigned int seqno;
> >>>>
> >>>>   unsigned int flags;
> >>>>
> >>>> };
> >>>>
> >>>> /* Definitions of av_decision.flags */
> >>>>
> >>>> #define SELINUX_AVD_FLAGS_PERMISSIVE0x0001
> >>>>
> >>>> It looks like if check_access just passes this structure and then
> >>>> avc_has_perm() when it calls avc_audit, it could supply the
> >>>> av_decision structure to the avc_suppl_audit() call. We could then
> >>>> have an audit2 callback that takes this parameter.
> >>>>
> >>>> Is this mostly right, seem sane? Better way to do this?
> >>>
> >>> It doesn't need to be exposed at that level; the libselinux
> >>> avc_audit() routine can log it, similar to what is done in the kernel.
> >>> It already has the av_decision structure available to it.
> >>
> >> To clarify, anything directly known to the AVC, like the permissive
> >> flag, can be directly logged by it.  The audit callback is for
> >> logging auxiliary audit information not known to the AVC (the pid of the 
> >> client
> process being a good example).
> >
> > I was wondering if we could just dump permissive=0|1 from the AVC
> > logging routine, but that would affect everyone.  I guess then you
> > would be ok with that? Does order matter with the fields wrt parsing?
> > I don't want to break any desktop tooling I am aware of, would we upstream
> this change as well?
> 
> Just put it at the end (i.e. log_append() after the avc_dump_query() call), 
> like we
> do in the kernel.  Shouldn't be a problem.  Technically order shouldn't 
> matter but
> safer to just append it to the current fields.
> 
> Yes, we'd upstream it.
> 
Done. Ill post back with a patch on android-review. And once merged there I can 
send one to the
Mailing list or you can cherry-pick. Are you ok with that?


___
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.