Re: [RFC PATCH v2 2/4] [squash] do not store entry for SECSID_NULL

[Stephen Smalley <sds@tycho.nsa.gov>]

On 11/27/18 12:00 PM, Stephen Smalley wrote:
> On 11/27/18 5:36 AM, Ondrej Mosnacek wrote:
>> This patch is kept separate only for review. Eventually it will be
>> folded into the previous patch.
> 
> This one triggers a lot of warnings (security_compute_av: unrecognized 
> SID 0, security_sid_to_context_core: unrecognized SID 0) and some 
> failures during selinux-testsuite inet_socket tests.  While the policy 
> doesn't provide an entry for SECSID_NULL, the sidtab search logic was 
> remapping it to the unlabeled context and that was apparently being 
> relied upon by the labeled networking code IIUC.

NB I had active ssh sessions to the system at the time of the test (and 
was in fact running the testsuite in one of the ssh sessions).  All of 
the sessions froze during the inet_sockets tests, presumably when 
labeled networking was configured, and then came back to life later, 
presumably once labeled network was un-configured.

> 
> 
>>
>> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
>> ---
>>   security/selinux/ss/policydb.c |  2 +-
>>   security/selinux/ss/sidtab.c   | 25 ++++++++++++++++---------
>>   security/selinux/ss/sidtab.h   |  3 ++-
>>   3 files changed, 19 insertions(+), 11 deletions(-)
>>
>> diff --git a/security/selinux/ss/policydb.c 
>> b/security/selinux/ss/policydb.c
>> index 59359fa0bd74..a50d625e7946 100644
>> --- a/security/selinux/ss/policydb.c
>> +++ b/security/selinux/ss/policydb.c
>> @@ -912,7 +912,7 @@ int policydb_load_isids(struct policydb *p, struct 
>> sidtab *s)
>>               sidtab_destroy(s);
>>               goto out;
>>           }
>> -        if (c->sid[0] > SECINITSID_NUM) {
>> +        if (c->sid[0] == SECSID_NULL || c->sid[0] > SECINITSID_NUM) {
>>               pr_err("SELinux:  Initial SID %s out of range.\n",
>>                   c->u.name);
>>               sidtab_destroy(s);
>> diff --git a/security/selinux/ss/sidtab.c b/security/selinux/ss/sidtab.c
>> index fd8115b211a6..e157d8240cf1 100644
>> --- a/security/selinux/ss/sidtab.c
>> +++ b/security/selinux/ss/sidtab.c
>> @@ -23,7 +23,7 @@ int sidtab_init(struct sidtab *s)
>>       if (!s->htable)
>>           return -ENOMEM;
>> -    for (i = 0; i <= SECINITSID_NUM; i++)
>> +    for (i = 0; i < SECINITSID_NUM; i++)
>>           s->isids[i].set = 0;
>>       for (i = 0; i < SIDTAB_SIZE; i++)
>> @@ -86,8 +86,15 @@ static int sidtab_insert(struct sidtab *s, u32 sid, 
>> struct context *context)
>>   int sidtab_set_initial(struct sidtab *s, u32 sid, struct context 
>> *context)
>>   {
>> -    struct sidtab_isid_entry *entry = &s->isids[sid];
>> -    int rc = context_cpy(&entry->context, context);
>> +    struct sidtab_isid_entry *entry;
>> +    int rc;
>> +
>> +    if (sid == 0 || sid > SECINITSID_NUM)
>> +        return -EINVAL;
>> +
>> +    entry = &s->isids[sid - 1];
>> +
>> +    rc = context_cpy(&entry->context, context);
>>       if (rc)
>>           return rc;
>> @@ -116,19 +123,19 @@ static struct context *sidtab_search_core(struct 
>> sidtab *s, u32 sid, int force)
>>       struct context *context;
>>       struct sidtab_isid_entry *entry;
>> -    if (!s)
>> +    if (!s || sid == 0)
>>           return NULL;
>>       if (sid > SECINITSID_NUM) {
>>           context = sidtab_lookup(s, sid - (SECINITSID_NUM + 1));
>>       } else {
>> -        entry = &s->isids[sid];
>> +        entry = &s->isids[sid - 1];
>>           context = entry->set ? &entry->context : NULL;
>>       }
>>       if (context && (!context->len || force))
>>           return context;
>> -    entry = &s->isids[SECINITSID_UNLABELED];
>> +    entry = &s->isids[SECINITSID_UNLABELED - 1];
>>       return entry->set ? &entry->context : NULL;
>>   }
>> @@ -283,11 +290,11 @@ int sidtab_context_to_sid(struct sidtab *s, 
>> struct context *context, u32 *sid)
>>       int rc;
>>       u32 i;
>> -    for (i = 0; i <= SECINITSID_NUM; i++) {
>> +    for (i = 0; i < SECINITSID_NUM; i++) {
>>           struct sidtab_isid_entry *entry = &s->isids[i];
>>           if (entry->set && context_cmp(context, &entry->context)) {
>> -            *sid = i;
>> +            *sid = i + 1;
>>               return 0;
>>           }
>>       }
>> @@ -334,7 +341,7 @@ void sidtab_destroy(struct sidtab *s)
>>       if (!s)
>>           return;
>> -    for (i = 0; i <= SECINITSID_NUM; i++)
>> +    for (i = 0; i < SECINITSID_NUM; i++)
>>           if (s->isids[i].set)
>>               context_destroy(&s->isids[i].context);
>> diff --git a/security/selinux/ss/sidtab.h b/security/selinux/ss/sidtab.h
>> index dc0a80bc8894..e657ae6bf996 100644
>> --- a/security/selinux/ss/sidtab.h
>> +++ b/security/selinux/ss/sidtab.h
>> @@ -36,7 +36,8 @@ struct sidtab {
>>       struct sidtab_node *cache[SIDTAB_CACHE_LEN];
>>       spinlock_t lock;
>> -    struct sidtab_isid_entry isids[SECINITSID_NUM + 1];
>> +    /* index == SID - 1 (no entry for SECSID_NULL) */
>> +    struct sidtab_isid_entry isids[SECINITSID_NUM];
>>   };
>>   int sidtab_init(struct sidtab *s);
>>
>