Re: [PATCH v14 22/23] LSM: Add /proc attr entry for full LSM context

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

On 1/24/20 9:42 AM, Stephen Smalley wrote:
> On 1/23/20 7:23 PM, Casey Schaufler wrote:
>> Add an entry /proc/.../attr/context which displays the full
>> process security "context" in compound format:'
>>          lsm1\0value\0lsm2\0value\0...
>> This entry is not writable.
>>
>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
>> Cc: linux-api@vger.kernel.org
> 
> As previously discussed, there are issues with AppArmor's implementation 
> of getprocattr() particularly around the trailing newline that 
> dbus-daemon and perhaps others would like to see go away in any new 
> interface.  Hence, I don't think we should implement this new API using 
> the existing getprocattr() hook lest it also be locked into the current 
> behavior forever.

Also, it would be good if whatever hook is introduced to support 
/proc/pid/attr/context could also be leveraged by the SO_PEERCONTEXT 
implementation in the future so that we are guaranteed a consistent 
result between the two interfaces, unlike the current situation for 
/proc/self/attr/current versus SO_PEERSEC.

> 
>> ---
>>   Documentation/security/lsm.rst | 14 ++++++++
>>   fs/proc/base.c                 |  1 +
>>   security/security.c            | 63 ++++++++++++++++++++++++++++++++++
>>   3 files changed, 78 insertions(+)
>>
>> diff --git a/Documentation/security/lsm.rst 
>> b/Documentation/security/lsm.rst
>> index aadf47c808c0..a4979060f5d3 100644
>> --- a/Documentation/security/lsm.rst
>> +++ b/Documentation/security/lsm.rst
>> @@ -199,3 +199,17 @@ capability-related fields:
>>   -  ``fs/nfsd/auth.c``::c:func:`nfsd_setuser()`
>>   -  ``fs/proc/array.c``::c:func:`task_cap()`
>> +
>> +LSM External Interfaces
>> +=======================
>> +
>> +The LSM infrastructure does not generally provide external interfaces.
>> +The individual security modules provide what external interfaces they
>> +require. The infrastructure does provide an interface for the special
>> +case where multiple security modules provide a process context. This
>> +is provided in compound context format.
>> +
>> +-  `lsm1\0value\0lsm2\0value\0`
>> +
>> +The special file ``/proc/pid/attr/context`` provides the security
>> +context of the identified process.
>> diff --git a/fs/proc/base.c b/fs/proc/base.c
>> index 950c200cb9ad..d13c2cf50e4b 100644
>> --- a/fs/proc/base.c
>> +++ b/fs/proc/base.c
>> @@ -2653,6 +2653,7 @@ static const struct pid_entry attr_dir_stuff[] = {
>>       ATTR(NULL, "keycreate",        0666),
>>       ATTR(NULL, "sockcreate",    0666),
>>       ATTR(NULL, "display",        0666),
>> +    ATTR(NULL, "context",        0666),
>>   #ifdef CONFIG_SECURITY_SMACK
>>       DIR("smack",            0555,
>>           proc_smack_attr_dir_inode_ops, proc_smack_attr_dir_ops),
>> diff --git a/security/security.c b/security/security.c
>> index 6a77c8b2ffbc..fdd0c85df89e 100644
>> --- a/security/security.c
>> +++ b/security/security.c
>> @@ -722,6 +722,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;
>> +}
>> +
>>   /*
>>    * Hook list operation macros.
>>    *
>> @@ -2041,6 +2077,10 @@ int security_getprocattr(struct task_struct *p, 
>> const char *lsm, char *name,
>>                   char **value)
>>   {
>>       struct security_hook_list *hp;
>> +    char *final = NULL;
>> +    char *cp;
>> +    int rc = 0;
>> +    int finallen = 0;
>>       int display = lsm_task_display(current);
>>       int slot = 0;
>> @@ -2068,6 +2108,29 @@ int security_getprocattr(struct task_struct *p, 
>> const char *lsm, char *name,
>>           return -ENOMEM;
>>       }
>> +    if (!strcmp(name, "context")) {
>> +        hlist_for_each_entry(hp, &security_hook_heads.getprocattr,
>> +                     list) {
>> +            rc = hp->hook.getprocattr(p, "current", &cp);
>> +            if (rc == -EINVAL || rc == -ENOPROTOOPT)
>> +                continue;
>> +            if (rc < 0) {
>> +                kfree(final);
>> +                return rc;
>> +            }
>> +            rc = append_ctx(&final, &finallen, hp->lsmid->lsm,
>> +                    cp, rc);
>> +            if (rc < 0) {
>> +                kfree(final);
>> +                return rc;
>> +            }
>> +        }
>> +        if (final == NULL)
>> +            return -EINVAL;
>> +        *value = final;
>> +        return finallen;
>> +    }
>> +
>>       hlist_for_each_entry(hp, &security_hook_heads.getprocattr, list) {
>>           if (lsm != NULL && strcmp(lsm, hp->lsmid->lsm))
>>               continue;
>>
>