[PATCH v2 1/1] Detect identical genfscon
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, &db->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
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
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
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&search=0x3B6C5F1D2C7B6B02 Dominick Grift signature.asc Description: PGP signature
Re: dbus-daemon patches review
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().