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@xxxxxxxxxxxxxxxx>
>>> Cc: linux-api@xxxxxxxxxxxxxxx
>>
>> 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;
>>>
>>
>
