[PATCH v2 1/1] Detect identical genfscon

[Pierre-Hugues Husson]

From: Pierre-Hugues Husson 

Currently secilc doesn't deal with duplicate genfscon rules

This commit fixes this, and implements multiple_decls behaviour.

To reduce the code changes, the compare function returns in its LSB
whether the rules are only a matching rule match, or a full match.
---
 libsepol/cil/src/cil_post.c | 34 --
 1 file changed, 32 insertions(+), 2 deletions(-)

diff --git a/libsepol/cil/src/cil_post.c b/libsepol/cil/src/cil_post.c
index a2122454..c054e9ce 100644
--- a/libsepol/cil/src/cil_post.c
+++ b/libsepol/cil/src/cil_post.c
@@ -53,6 +53,26 @@
 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);
 
+/* compare function returns whether a,b have the same context in the LSB */
+static int compact(void* array, uint32_t *count, int len, int (*compare)(const 
void *, const void *), int multiple_decls) {
+   char *a = (char*)array;
+   uint32_t i, j = 0;
+   int c;
+   for(i=1; i<*count; i++) {
+   c = compare(a+i*len, a+j*len);
+   /* If LSB is set, it means the rules match except for the 
context
+* We never want this */
+   if(c&1) return SEPOL_ERR;
+
+   if(!multiple_decls && c == 0) return SEPOL_ERR;
+
+   if(c) j++;
+   if(i != j) memcpy(a+j*len, a+i*len, len);
+   }
+   *count = j;
+   return SEPOL_OK;
+}
+
 static int cil_verify_is_list(struct cil_list *list, enum cil_flavor flavor)
 {
struct cil_list_item *curr;
@@ -202,9 +222,14 @@ int cil_post_genfscon_compare(const void *a, const void *b)
struct cil_genfscon *agenfscon = *(struct cil_genfscon**)a;
struct cil_genfscon *bgenfscon = *(struct cil_genfscon**)b;
 
-   rc = strcmp(agenfscon->fs_str, bgenfscon->fs_str);
+   rc = 2*strcmp(agenfscon->fs_str, bgenfscon->fs_str);
if (rc == 0) {
-   rc = strcmp(agenfscon->path_str, bgenfscon->path_str);
+   rc = 2*strcmp(agenfscon->path_str, bgenfscon->path_str);
+   if(rc == 0) {
+   rc = strcmp(agenfscon->context_str, 
bgenfscon->context_str);
+   if(rc > 0) rc = 1;
+   if(rc < 0) rc = -1;
+   }
}
 
return rc;
@@ -2118,6 +2143,11 @@ static int cil_post_db(struct cil_db *db)
 
qsort(db->netifcon->array, db->netifcon->count, 
sizeof(db->netifcon->array), cil_post_netifcon_compare);
qsort(db->genfscon->array, db->genfscon->count, 
sizeof(db->genfscon->array), cil_post_genfscon_compare);
+   rc = compact(db->genfscon->array, >genfscon->count, 
sizeof(db->genfscon->array), cil_post_genfscon_compare, db->multiple_decls);
+   if (rc != SEPOL_OK) {
+   cil_log(CIL_INFO, "Failed to de-dupe genfscon\n");
+   goto exit;
+   }
qsort(db->ibpkeycon->array, db->ibpkeycon->count, 
sizeof(db->ibpkeycon->array), cil_post_ibpkeycon_compare);
qsort(db->ibendportcon->array, db->ibendportcon->count, 
sizeof(db->ibendportcon->array), cil_post_ibendportcon_compare);
qsort(db->portcon->array, db->portcon->count, 
sizeof(db->portcon->array), cil_post_portcon_compare);
-- 
2.15.1

[PATCH v2 0/1] Detect identical genfscon

[Pierre-Hugues Husson]

Currently secilc doesn't deal with duplicate genfscon rules.

This commit fixes this, and implements multiple_decls behaviour.

To reduce the code changes, the compare function returns in its LSB
whether the rules are only a matching rule match, or a full match.

One usecase is Android/Project Treble:
With Project Treble, vendor might include rules included in later
in framework.
In order to be able to update the framework in this case, we need
to remove identical rules.

This is a RFC version, this hasn't been properly tested.

v2:
- Respect multiple_decls behaviour
- Fail merge when context is different
- genfscon compare function returns partial or full match

Pierre-Hugues Husson (1):
  Detect identical genfscon

 libsepol/cil/src/cil_post.c | 34 --
 1 file changed, 32 insertions(+), 2 deletions(-)

-- 
2.15.1

Re: [PATCH 0/1] Support multiple identical genfscon

[jwcart2]

On 03/19/2018 02:47 PM, Pierre-Hugues Husson wrote:

secilc has a multiple_decls option to allow for multiple type
declarations.
The next step is to allow multiple samples of the same rules.
This commit does this on genfscon

One usecase is Android/Project Treble:
With Project Treble, vendor might include rules included in later
in framework.
In order to be able to update the framework in this case, we need
to remove identical rules.

I have several pending questions before considering merging:

Should the "compact" function be somewhere else? Or perhaps there is already
some variant available?


Where you put it is fine. There is no other variant.


Should the "compact" function simply take a cil_sort rather than a C array?
Should we compact all types indifferently?


It looks like secilc is not checking for duplicates right now for any of the 
ocontext rules which is a problem.


I am assuming that if the genfscon is different only in the context, then that 
should be an error. Is that correct?


So the following should be an error:
(genfscon FS1 / (U R T1 ((S) (S
(genfscon FS1 / (U R T2 ((S) (S

but if they both had T1, then it would be ok, but the second rule would not be 
added to the policy.


I think the right approach in the compact function is to return an error if the 
compare function returns 0 and the multiple-decls flag has not been used or the 
contexts of the two rules are not the same. If the rule is exactly the same and 
the multiple-decls flag is set, then skip the duplicate rule.



If so, we need to guarantee that the _compare function returns 0 only when the
types rules are identical, and not just the same match rule. Is this already
the case?
How is memory allocation done/will compact impact the release of the memory?
In my understanding this is just one big chunk, so the size isn't used when
free-ing, so it should be ok


Yes. It is one big chunk.

Thanks,
Jim



Pierre-Hugues Husson (1):
   Delete identical genfscon-s

  libsepol/cil/src/cil_post.c | 11 +++
  1 file changed, 11 insertions(+)




--
James Carter 
National Security Agency

Re: dbus-daemon patches review

[Dominick Grift]

On Thu, Mar 22, 2018 at 12:09:08PM -0400, Stephen Smalley wrote:
> On 03/21/2018 07:58 AM, Laurent Bigonville wrote:
> > Hello,
> > 
> > Could somebody have a quick look at the two patches that I opened for two 
> > dbus bugs:
> > 
> > https://bugs.freedesktop.org/show_bug.cgi?id=92831 (stop using avc_init())
> > 
> > https://bugs.freedesktop.org/attachment.cgi?id=138021 (stop using 
> > selinux_set_mapping())
> > 
> > I'm also wondering whether the call to avc_add_callback() shouldn't be 
> > replaced by selinux_set_callback(), an opinion on this?
> 
> Patches look sane to me although I'm not really familiar with dbus code.
> 
> Looks like the callback is only used to trigger a reload of the dbus 
> configuration (for dbus_contexts updates), and thus 
> selinux_set_callback(SELINUX_CB_POLICYLOAD) is more appropriate than 
> avc_add_callback(AVC_CALLBACK_RESET), since the latter is called upon 
> setenforce 1 as well.  However, if it were truly only for that purpose, one 
> might argue that it ought to be a watch on the dbus_contexts file instead and 
> not be tied to selinux at all.
> 
> NB This still won't fix the case where dbusd has already performed a 
> string_to_security_class/av_perm lookup and the result has been cached by the 
> libselinux class cache and then a subsequent policy update alters those 
> values.  That is what was fixed for systemd's usage of selinux_check_access() 
> by selinux commit b408d72ca9104cb0c1bc4e154d8732cc7c0a9190.  Offhand, I'm now 
> wondering why I didn't just call flush_class_cache() from avc_reset() itself. 
>  That would fix it for other users of the AVC.  You can't directly call 
> flush_class_cache() from the dbus selinux policyload callback because it is 
> hidden presently.  If we can fix it directly in libselinux, then that is 
> better.  If not, we'd need to export it and probably give it a more unique 
> name, ala selinux_flush_class_cache().

dbus-broker also uses selinux_access_check: 
https://github.com/bus1/dbus-broker/issues/16

> 
> 

-- 
Key fingerprint = 5F4D 3CDB D3F8 3652 FBD8  02D5 3B6C 5F1D 2C7B 6B02
https://sks-keyservers.net/pks/lookup?op=get=0x3B6C5F1D2C7B6B02
Dominick Grift


signature.asc
Description: PGP signature

Re: dbus-daemon patches review

[Stephen Smalley]

On 03/21/2018 07:58 AM, Laurent Bigonville wrote:
> Hello,
> 
> Could somebody have a quick look at the two patches that I opened for two 
> dbus bugs:
> 
> https://bugs.freedesktop.org/show_bug.cgi?id=92831 (stop using avc_init())
> 
> https://bugs.freedesktop.org/attachment.cgi?id=138021 (stop using 
> selinux_set_mapping())
> 
> I'm also wondering whether the call to avc_add_callback() shouldn't be 
> replaced by selinux_set_callback(), an opinion on this?

Patches look sane to me although I'm not really familiar with dbus code.

Looks like the callback is only used to trigger a reload of the dbus 
configuration (for dbus_contexts updates), and thus 
selinux_set_callback(SELINUX_CB_POLICYLOAD) is more appropriate than 
avc_add_callback(AVC_CALLBACK_RESET), since the latter is called upon 
setenforce 1 as well.  However, if it were truly only for that purpose, one 
might argue that it ought to be a watch on the dbus_contexts file instead and 
not be tied to selinux at all.

NB This still won't fix the case where dbusd has already performed a 
string_to_security_class/av_perm lookup and the result has been cached by the 
libselinux class cache and then a subsequent policy update alters those values. 
 That is what was fixed for systemd's usage of selinux_check_access() by 
selinux commit b408d72ca9104cb0c1bc4e154d8732cc7c0a9190.  Offhand, I'm now 
wondering why I didn't just call flush_class_cache() from avc_reset() itself.  
That would fix it for other users of the AVC.  You can't directly call 
flush_class_cache() from the dbus selinux policyload callback because it is 
hidden presently.  If we can fix it directly in libselinux, then that is 
better.  If not, we'd need to export it and probably give it a more unique 
name, ala selinux_flush_class_cache().