[PATCH v12 23/25] NET: Add SO_PEERCONTEXT for multiple LSMs

[Stephen Smalley]

On 12/18/19 1:28 PM, Stephen Smalley wrote:
> On 12/16/19 5:36 PM, Casey Schaufler wrote:
>> The getsockopt SO_PEERSEC provides the LSM based security
>> information for a single module, but for reasons of backward
>> compatibility cannot include the information for multiple
>> modules. A new option SO_PEERCONTEXT is added to report the
>> security "context" of multiple modules using a "compound" format
>>
>>          lsm1\0value\0lsm2\0value\0
>>
>> This is expected to be used by system services, including dbus-daemon.
>> The exact format of a compound context has been the subject of
>> considerable debate. This format was suggested by Simon McVittie,
>> a dbus maintainer with a significant stake in the format being
>> usable.
>>
>> Signed-off-by: Casey Schaufler <casey at schaufler-ca.com>
>> cc: netdev at vger.kernel.org
> 
> Requires ack by netdev and linux-api.  A couple of comments below.

Also, have you tested this new interface?  I may be doing something 
wrong, but a trivial attempt to use SO_PEERCONTEXT with both SELinux and 
AppArmor enabled only appeared to return the SELinux portion of the 
label 
(selinux\0unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023\0), 
whereas /proc/self/attr/context returned a compound context (the same 
but with apparmor\0unconfined\n\0 appended).

> 
>> ---
> 
>> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
>> index 2bf82e1cf347..2ae10e7f81a7 100644
>> --- a/include/linux/lsm_hooks.h
>> +++ b/include/linux/lsm_hooks.h
>> @@ -880,8 +880,8 @@
>>    *    SO_GETPEERSEC.  For tcp sockets this can be meaningful if the
>>    *    socket is associated with an ipsec SA.
>>    *    @sock is the local socket.
>> - *    @optval userspace memory where the security state is to be copied.
>> - *    @optlen userspace int where the module should copy the actual 
>> length
>> + *    @optval memory where the security state is to be copied.
> 
> This is misleading; it suggests that the caller is providing an 
> allocated buffer into which the security module copies its data. Instead 
> it is just a pointer to a pointer that is then set by the security 
> module to a buffer the module allocates.
> 
>> diff --git a/include/linux/security.h b/include/linux/security.h
>> index 536db4dbfcbb..b72bb90b1903 100644
>> --- a/include/linux/security.h
>> +++ b/include/linux/security.h
>> @@ -181,7 +181,7 @@ struct lsmblob {
>>   #define LSMBLOB_NEEDED        -2    /* Slot requested on 
>> initialization */
>>   #define LSMBLOB_NOT_NEEDED    -3    /* Slot not requested */
>>   #define LSMBLOB_DISPLAY        -4    /* Use the "display" slot */
>> -#define LSMBLOB_FIRST        -5    /* Use the default "display" slot */
>> +#define LSMBLOB_COMPOUND    -5    /* A compound "display" */
> 
> I'm puzzled by the removal of LSMBLOB_FIRST by this patch; it suggests 
> it was never needed in the first place by the patch that introduced it. 
> But more below.
> 
>> diff --git a/security/security.c b/security/security.c
>> index d0b57a7c3b31..1afe245f3246 100644
>> --- a/security/security.c
>> +++ b/security/security.c
>> @@ -723,6 +723,42 @@ static void __init lsm_early_task(struct 
>> task_struct *task)
>>           panic("%s: Early task alloc failed.\n", __func__);
>>   }
>> +/**
>> + * append_ctx - append a lsm/context pair to a compound context
>> + * @ctx: the existing compound context
>> + * @ctxlen: size of the old context, including terminating nul byte
>> + * @lsm: new lsm name, nul terminated
>> + * @new: new context, possibly nul terminated
>> + * @newlen: maximum size of @new
>> + *
>> + * replace @ctx with a new compound context, appending @newlsm and @new
>> + * to @ctx. On exit the new data replaces the old, which is freed.
>> + * @ctxlen is set to the new size, which includes a trailing nul byte.
>> + *
>> + * Returns 0 on success, -ENOMEM if no memory is available.
>> + */
>> +static int append_ctx(char **ctx, int *ctxlen, const char *lsm, char 
>> *new,
>> +              int newlen)
>> +{
>> +    char *final;
>> +    int llen;
>> +
>> +    llen = strlen(lsm) + 1;
>> +    newlen = strnlen(new, newlen) + 1;
>> +
>> +    final = kzalloc(*ctxlen + llen + newlen, GFP_KERNEL);
>> +    if (final == NULL)
>> +        return -ENOMEM;
>> +    if (*ctxlen)
>> +        memcpy(final, *ctx, *ctxlen);
>> +    memcpy(final + *ctxlen, lsm, llen);
>> +    memcpy(final + *ctxlen + llen, new, newlen);
>> +    kfree(*ctx);
>> +    *ctx = final;
>> +    *ctxlen = *ctxlen + llen + newlen;
>> +    return 0;
>> +}
> 
> You should likely take some precautions against integer overflows in the 
> above code?
> 
>> +
>>   /*
>>    * Hook list operation macros.
>>    *
>> @@ -2164,8 +2200,8 @@ int security_setprocattr(const char *lsm, const 
>> char *name, void *value,
>>       hlist_for_each_entry(hp, &security_hook_heads.setprocattr, list) {
>>           if (lsm != NULL && strcmp(lsm, hp->lsmid->lsm))
>>               continue;
>> -        if (lsm == NULL && *display != LSMBLOB_INVALID &&
>> -            *display != hp->lsmid->slot)
>> +        if (lsm == NULL && display != NULL &&
>> +            *display != LSMBLOB_INVALID && *display != hp->lsmid->slot)
>>               continue;
>>           return hp->hook.setprocattr(name, value, size);
>>       }
> 
> Is this a bug fix that should be folded into the earlier patch that 
> introduced it?
> 
>> @@ -2196,7 +2232,7 @@ int security_secid_to_secctx(struct lsmblob 
>> *blob, struct lsmcontext *cp,
>>        */
>>       if (display == LSMBLOB_DISPLAY)
>>           display = lsm_task_display(current);
>> -    else if (display == LSMBLOB_FIRST)
>> +    else if (display == 0)
>>           display = LSMBLOB_INVALID;
>>       else if (display < 0) {
>>           WARN_ONCE(true,
> 
> Why is it necessary to re-map display 0 in this manner? Previously if 
> display 0 was specified, it would require it to match the lsmid->slot 
> value.  Won't it match anyway?
> 
>> @@ -2246,6 +2282,15 @@ void security_release_secctx(struct lsmcontext 
>> *cp)
>>       struct security_hook_list *hp;
>>       bool found = false;
>> +    if (cp->slot == LSMBLOB_INVALID)
>> +        return;
>> +
>> +    if (cp->slot == LSMBLOB_COMPOUND) {
>> +        kfree(cp->context);
>> +        found = true;
>> +        goto clear_out;
>> +    }
>> +
> 
> If you re-order your pr_warn() below with your memset() to address the 
> earlier comment, you'll end up trying to print the freed memory.  Not a 
> problem if you just drop the pr_warn() altogether.