On Wed, Mar 31, 2021 at 4:04 AM Paul Moore <paul@xxxxxxxxxxxxxx> wrote:
> On Tue, Mar 30, 2021 at 9:16 AM Ondrej Mosnacek <omosnace@xxxxxxxxxx> wrote:
> >
> > Currently, duplicate_policydb_cond_list() first copies the whole
> > conditional avtab and then tries to link to the correct entries in
> > cond_dup_av_list() using avtab_search(). However, since the conditional
> > avtab may contain multiple entries with the same key, this approach
> > often fails to find the right entry, potentially leading to wrong rules
> > being activated/deactivated when booleans are changed.
> >
> > To fix this, instead start with an empty conditional avtab and add the
> > individual entries one-by-one while building the new av_lists. This
> > approach leads to the correct result, since each entry is present in the
> > av_lists exactly once.
> >
> > The issue can be reproduced with Fedora policy as follows:
> >
> > # sesearch -s ftpd_t -t public_content_rw_t -c dir -p create -A
> > allow ftpd_t non_security_file_type:dir { add_name create getattr ioctl link lock open read remove_name rename reparent rmdir search setattr unlink watch watch_reads write }; [ ftpd_full_access ]:True
> > allow ftpd_t public_content_rw_t:dir { add_name create link remove_name rename reparent rmdir setattr unlink watch watch_reads write }; [ ftpd_anon_write ]:True
> > # setsebool ftpd_anon_write=off ftpd_connect_all_unreserved=off ftpd_connect_db=off ftpd_full_access=off
> >
> > On fixed kernels, the sesearch output is the same after the setsebool
> > command:
> >
> > # sesearch -s ftpd_t -t public_content_rw_t -c dir -p create -A
> > allow ftpd_t non_security_file_type:dir { add_name create getattr ioctl link lock open read remove_name rename reparent rmdir search setattr unlink watch watch_reads write }; [ ftpd_full_access ]:True
> > allow ftpd_t public_content_rw_t:dir { add_name create link remove_name rename reparent rmdir setattr unlink watch watch_reads write }; [ ftpd_anon_write ]:True
> >
> > While on the broken kernels, it will be different:
> >
> > # sesearch -s ftpd_t -t public_content_rw_t -c dir -p create -A
> > allow ftpd_t non_security_file_type:dir { add_name create getattr ioctl link lock open read remove_name rename reparent rmdir search setattr unlink watch watch_reads write }; [ ftpd_full_access ]:True
> > allow ftpd_t non_security_file_type:dir { add_name create getattr ioctl link lock open read remove_name rename reparent rmdir search setattr unlink watch watch_reads write }; [ ftpd_full_access ]:True
> > allow ftpd_t non_security_file_type:dir { add_name create getattr ioctl link lock open read remove_name rename reparent rmdir search setattr unlink watch watch_reads write }; [ ftpd_full_access ]:True
> >
> > Fixes: c7c556f1e81b ("selinux: refactor changing booleans")
> > Signed-off-by: Ondrej Mosnacek <omosnace@xxxxxxxxxx>
> > ---
> > security/selinux/ss/avtab.c | 90 +++++++++----------------------
> > security/selinux/ss/avtab.h | 2 +-
> > security/selinux/ss/conditional.c | 12 ++---
> > 3 files changed, 33 insertions(+), 71 deletions(-)
> >
> > diff --git a/security/selinux/ss/avtab.c b/security/selinux/ss/avtab.c
> > index 6dcb6aa4db7f..11f8f524de98 100644
> > --- a/security/selinux/ss/avtab.c
> > +++ b/security/selinux/ss/avtab.c
> > @@ -305,86 +305,48 @@ void avtab_init(struct avtab *h)
> > h->nel = 0;
> > }
> >
> > -int avtab_alloc(struct avtab *h, u32 nrules)
> > +static int avtab_alloc_common(struct avtab *h, u32 nslot)
> > {
> > - u32 mask = 0;
> > - u32 shift = 0;
> > - u32 work = nrules;
> > - u32 nslot = 0;
> > -
> > - if (nrules == 0)
> > - goto avtab_alloc_out;
> > -
> > - while (work) {
> > - work = work >> 1;
> > - shift++;
> > - }
> > - if (shift > 2)
> > - shift = shift - 2;
> > - nslot = 1 << shift;
> > - if (nslot > MAX_AVTAB_HASH_BUCKETS)
> > - nslot = MAX_AVTAB_HASH_BUCKETS;
> > - mask = nslot - 1;
> > -
> > h->htable = kvcalloc(nslot, sizeof(void *), GFP_KERNEL);
> > if (!h->htable)
> > return -ENOMEM;
>
> Hmmm, do we need to protect against 'nslot == 0'? Unless I missed
> something, a quick dive into kvcalloc() makes it look like it can
> return non-NULL for zero length allocations, at least in the slab
> case.
>
> > - avtab_alloc_out:
> > h->nel = 0;
> > h->nslot = nslot;
> > - h->mask = mask;
> > - pr_debug("SELinux: %d avtab hash slots, %d rules.\n",
> > - h->nslot, nrules);
> > + h->mask = nslot - 1;
>
> This is definitely not good if 'nslot <= 1';
Yeah, in fact the current code doesn't seem to be safe against nslot
== 0 either... I'll insert another patch that fixes this.
>
> > return 0;
> > }
> >
> > -int avtab_duplicate(struct avtab *new, struct avtab *orig)
> > +int avtab_alloc(struct avtab *h, u32 nrules)
> > {
> > - int i;
> > - struct avtab_node *node, *tmp, *tail;
> > -
> > - memset(new, 0, sizeof(*new));
> > + int rc;
> > + u32 shift = 0;
> > + u32 work = nrules;
> > + u32 nslot = 0;
> >
> > - new->htable = kvcalloc(orig->nslot, sizeof(void *), GFP_KERNEL);
> > - if (!new->htable)
> > - return -ENOMEM;
> > - new->nslot = orig->nslot;
> > - new->mask = orig->mask;
> > -
> > - for (i = 0; i < orig->nslot; i++) {
> > - tail = NULL;
> > - for (node = orig->htable[i]; node; node = node->next) {
> > - tmp = kmem_cache_zalloc(avtab_node_cachep, GFP_KERNEL);
> > - if (!tmp)
> > - goto error;
> > - tmp->key = node->key;
> > - if (tmp->key.specified & AVTAB_XPERMS) {
> > - tmp->datum.u.xperms =
> > - kmem_cache_zalloc(avtab_xperms_cachep,
> > - GFP_KERNEL);
> > - if (!tmp->datum.u.xperms) {
> > - kmem_cache_free(avtab_node_cachep, tmp);
> > - goto error;
> > - }
> > - tmp->datum.u.xperms = node->datum.u.xperms;
> > - } else
> > - tmp->datum.u.data = node->datum.u.data;
> > -
> > - if (tail)
> > - tail->next = tmp;
> > - else
> > - new->htable[i] = tmp;
> > -
> > - tail = tmp;
> > - new->nel++;
> > + if (nrules != 0) {
> > + while (work) {
> > + work = work >> 1;
>
> Extra horizontal spaces are awkward and bad.
>
> > + shift++;
> > }
> > + if (shift > 2)
> > + shift = shift - 2;
>
> Since we are getting nit-picky with this code, why not make the
> loop/if a bit more elegant? How about something like below?
>
> u32 shift = 2;
> u32 work = nrules >> 4;
> while (work) {
> work >>= 1;
> shift++;
> }
I think you meant:
u32 shift = 0;
u32 work = nrules >> 2;
while (work) {
work >>= 1;
shift++;
}
...which is equivalent to the current code and yes, I'll use that :)
>
> > + nslot = 1 << shift;
> > + if (nslot > MAX_AVTAB_HASH_BUCKETS)
> > + nslot = MAX_AVTAB_HASH_BUCKETS;
> > }
> >
> > + rc = avtab_alloc_common(h, nslot);
> > + if (rc)
> > + return rc;
> > +
> > + pr_debug("SELinux: %d avtab hash slots, %d rules.\n", nslot, nrules);
> > return 0;
> > -error:
> > - avtab_destroy(new);
> > - return -ENOMEM;
> > +}
> > +
> > +int avtab_alloc_dup(struct avtab *new, const struct avtab *orig)
> > +{
> > + return avtab_alloc_common(new, orig->nslot);
> > }
>
> --
> paul moore
> www.paul-moore.com
>
--
Ondrej Mosnacek
Software Engineer, Linux Security - SELinux kernel
Red Hat, Inc.
