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

[Casey Schaufler <casey@schaufler-ca.com>]

On 1/24/2020 8:20 AM, Stephen Smalley wrote:
> 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.

I don't believe that a new hook is necessary, and that introducing one
just to deal with a '\n' would be pedantic. We really have two rational
options. AppArmor could drop the '\n' from their "context". Or, we can
simply document that the /proc/pid/attr/context interface will trim any
trailing whitespace. I understand that this would be a break from the
notion that the LSM infrastructure does not constrain what a module uses
for its own data. On the other hand, we have been saying that "context"s
are strings, and ignoring trailing whitespace is usual behavior for
strings.



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