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

From: Stephen Smalley <sds@tycho.nsa.gov>
To: Casey Schaufler <casey@schaufler-ca.com>,
	casey.schaufler@intel.com, jmorris@namei.org,
	linux-security-module@vger.kernel.org, selinux@vger.kernel.org
Cc: keescook@chromium.org, john.johansen@canonical.com,
	penguin-kernel@i-love.sakura.ne.jp, paul@paul-moore.com
Subject: Re: [PATCH v12 23/25] NET: Add SO_PEERCONTEXT for multiple LSMs
Date: Wed, 18 Dec 2019 15:50:45 -0500	[thread overview]
Message-ID: <a522de22-ba62-a24d-24f7-b69418e7ec0b@tycho.nsa.gov> (raw)
In-Reply-To: <e7aa3b6f-cee1-6277-21dd-77a4db9bbc2b@tycho.nsa.gov>

On 12/18/19 2:12 PM, Stephen Smalley wrote:
> 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@schaufler-ca.com>
>>> cc: netdev@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).

Ok, this seems to be a lack of support in AppArmor for saving the peer 
info for unix/local domain sockets, so not your bug.  Doesn't implement 
the necessary hooks.

> 
>>
>>> ---
>>
>>> 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.
> 