[v14,22/23] LSM: Add /proc attr entry for full LSM context
diff mbox series

Message ID 20200124002306.3552-23-casey@schaufler-ca.com
State New
Headers show
Series
  • LSM: Module stacking for AppArmor
Related show

Commit Message

Casey Schaufler Jan. 24, 2020, 12:23 a.m. UTC
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
---
 Documentation/security/lsm.rst | 14 ++++++++
 fs/proc/base.c                 |  1 +
 security/security.c            | 63 ++++++++++++++++++++++++++++++++++
 3 files changed, 78 insertions(+)

Comments

Stephen Smalley Jan. 24, 2020, 2:42 p.m. UTC | #1
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.

> ---
>   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;
>
Stephen Smalley Jan. 24, 2020, 4:20 p.m. UTC | #2
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;
>>
>
Casey Schaufler Jan. 24, 2020, 7:28 p.m. UTC | #3
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;
>>>
>>
>
Stephen Smalley Jan. 24, 2020, 8:16 p.m. UTC | #4
On 1/24/20 2:28 PM, Casey Schaufler wrote:
> 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.

Well, you can either introduce a new common underlying hook for use by 
/proc/pid/attr/context and SO_PEERCONTEXT to produce the string that is 
to be returned to userspace (in order to guarantee consistency in format 
and allowing them to be directly compared, which I think is what the 
dbus maintainers wanted), or you can modify every security module to 
provide that guarantee in its existing getprocattr and getpeersec* hook 
functions (SELinux already provides this guarantee; Smack and AppArmor 
produce slightly different results with respect to \0 and/or \n), or you 
can have the framework trim the values it gets from the security modules 
before composing them.  But you need to do one of those things before 
this interface gets merged upstream.

Aside from the trailing newline and \0 issues, AppArmor also has a 
whitespace-separated (mode) field that may or may not be present in the 
contexts it presently returns, ala "/usr/sbin/cupsd (enforce)".  Not 
sure what they want for the new interfaces.
Simon McVittie Jan. 27, 2020, 8:05 p.m. UTC | #5
On Fri, 24 Jan 2020 at 15:16:36 -0500, Stephen Smalley wrote:
> Aside from the trailing newline and \0 issues, AppArmor also has a
> whitespace-separated (mode) field that may or may not be present in the
> contexts it presently returns, ala "/usr/sbin/cupsd (enforce)".

My understanding from last time I worked with AppArmor is that this
is genuinely part of the context, and whether it is present or absent
does not vary according to the kernel API used to access contexts.
AppArmor-specific higher-level APIs parse it into a label and an optional
mode, but LSM-agnostic user-space APIs (like the one in dbus) pass the
whole string through as-is.

(In practice it seems to be present if and only if the context is
something other than "unconfined", although I don't know offhand whether
that's an API guarantee.)

    smcv
Casey Schaufler Jan. 27, 2020, 10:49 p.m. UTC | #6
On 1/24/2020 12:16 PM, Stephen Smalley wrote:
> On 1/24/20 2:28 PM, Casey Schaufler wrote:
>> 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.
>
> Well, you can either introduce a new common underlying hook for use by /proc/pid/attr/context and SO_PEERCONTEXT to produce the string that is to be returned to userspace (in order to guarantee consistency in format and allowing them to be directly compared, which I think is what the dbus maintainers wanted),

The getprocattr() hooks are already required to provide a nul terminated string.

The behavior of /proc/self/attr/current with regard to trailing whitespace or
the terminating nul is left to the security module.

The behavior of /proc/self/attr/context is new, and as such it can be defined
to be reasonable and consistent.

> or you can modify every security module to provide that guarantee in its existing getprocattr and getpeersec* hook functions (SELinux already provides this guarantee; Smack and AppArmor produce slightly different results with respect to \0 and/or \n), or you can have the framework trim the values it gets from the security modules before composing them.

Changing "every security module" turns out to be trivial, really.
The security_getprocattr() interface doesn't change, the getprocattr
hooks get a bool argument indicating if newlines are allowed, the
AppArmor code changes to check the bool and security_getprocattr()
sets to bool specially for the "context" case.

The getpeersec() interface use for SO_PEERCONTEXT isn't any more difficult.
The security modules are free to include a terminating nul or not in
the existing use case, but a bool can tell them what the behavior must
be when we get to that interface.

It's not like there are all that many security modules to bring in line
at this point. Establishing sane rules of behavior is overdue. With the
current set of new modules sniffing at the door it really is time to
tighten these up.

>   But you need to do one of those things before this interface gets merged upstream.
>
> Aside from the trailing newline and \0 issues, AppArmor also has a whitespace-separated (mode) field that may or may not be present in the contexts it presently returns, ala "/usr/sbin/cupsd (enforce)".  Not sure what they want for the new interfaces.

Using '\0' as a field terminator addresses that issue. That's a
primary motivator.
Casey Schaufler Jan. 31, 2020, 10:10 p.m. UTC | #7
On 1/24/2020 12:16 PM, Stephen Smalley wrote:
> On 1/24/20 2:28 PM, Casey Schaufler wrote:
>> 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.
>
> Well, you can either introduce a new common underlying hook for use by /proc/pid/attr/context and SO_PEERCONTEXT to produce the string that is to be returned to userspace (in order to guarantee consistency in format and allowing them to be directly compared, which I think is what the dbus maintainers wanted), or you can modify every security module to provide that guarantee in its existing getprocattr and getpeersec* hook functions (SELinux already provides this guarantee; Smack and AppArmor produce slightly different results with respect to \0 and/or \n), or you can have the framework trim the values it gets from the security modules before composing them.  But you need to do one of those things before this interface gets merged upstream.
>
> Aside from the trailing newline and \0 issues, AppArmor also has a whitespace-separated (mode) field that may or may not be present in the contexts it presently returns, ala "/usr/sbin/cupsd (enforce)".  Not sure what they want for the new interfaces.
>
From c4085435215653b7c4d07a35a9df308120441d79 Mon Sep 17 00:00:00 2001
From: Casey Schaufler <casey@schaufler-ca.com>
Date: Fri, 31 Jan 2020 13:57:23 -0800
Subject: [PATCH v14] LSM: Move "context" format enforcement into security
 modules

Document in lsm_hooks.h what is expected of a security module that
supplies the "context" attribute.  Add handling of the "context"
attribute to SELinux, Smack and AppArmor security modules. The
AppArmor implementation provides a different string for "context"
than it does for other attributes to conform with the "context"
format.

Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
---
 include/linux/lsm_hooks.h            |  6 ++++++
 security/apparmor/include/procattr.h |  2 +-
 security/apparmor/lsm.c              |  8 ++++++--
 security/apparmor/procattr.c         | 11 +++++++----
 security/security.c                  |  2 +-
 security/selinux/hooks.c             |  2 +-
 security/smack/smack_lsm.c           |  2 +-
 7 files changed, 23 insertions(+), 10 deletions(-)

diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 2bf82e1cf347..61977a33f2c3 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -1321,6 +1321,12 @@
  *	@pages contains the number of pages.
  *	Return 0 if permission is granted.
  *
+ * @getprocattr:
+ *	Provide the named process attribute for display in special files in
+ *	the /proc/.../attr directory.  Attribute naming and the data displayed
+ *	is at the discretion of the security modules.  The exception is the
+ *	"context" attribute, which will contain the security context of the
+ *	task as a nul terminated text string without trailing whitespace.
  * @ismaclabel:
  *	Check if the extended attribute specified by @name
  *	represents a MAC label. Returns 1 if name is a MAC
diff --git a/security/apparmor/include/procattr.h b/security/apparmor/include/procattr.h
index 31689437e0e1..03dbfdb2f2c0 100644
--- a/security/apparmor/include/procattr.h
+++ b/security/apparmor/include/procattr.h
@@ -11,7 +11,7 @@
 #ifndef __AA_PROCATTR_H
 #define __AA_PROCATTR_H
 
-int aa_getprocattr(struct aa_label *label, char **string);
+int aa_getprocattr(struct aa_label *label, char **string, bool newline);
 int aa_setprocattr_changehat(char *args, size_t size, int flags);
 
 #endif /* __AA_PROCATTR_H */
diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
index 37d003568e82..07729c28275e 100644
--- a/security/apparmor/lsm.c
+++ b/security/apparmor/lsm.c
@@ -593,6 +593,7 @@ static int apparmor_getprocattr(struct task_struct *task, char *name,
 	const struct cred *cred = get_task_cred(task);
 	struct aa_task_ctx *ctx = task_ctx(current);
 	struct aa_label *label = NULL;
+	bool newline = true;
 
 	if (strcmp(name, "current") == 0)
 		label = aa_get_newest_label(cred_label(cred));
@@ -600,11 +601,14 @@ static int apparmor_getprocattr(struct task_struct *task, char *name,
 		label = aa_get_newest_label(ctx->previous);
 	else if (strcmp(name, "exec") == 0 && ctx->onexec)
 		label = aa_get_newest_label(ctx->onexec);
-	else
+	else if (strcmp(name, "context") == 0) {
+		label = aa_get_newest_label(cred_label(cred));
+		newline = false;
+	} else
 		error = -EINVAL;
 
 	if (label)
-		error = aa_getprocattr(label, value);
+		error = aa_getprocattr(label, value, newline);
 
 	aa_put_label(label);
 	put_cred(cred);
diff --git a/security/apparmor/procattr.c b/security/apparmor/procattr.c
index c929bf4a3df1..1098bca3d0e4 100644
--- a/security/apparmor/procattr.c
+++ b/security/apparmor/procattr.c
@@ -20,6 +20,7 @@
  * aa_getprocattr - Return the profile information for @profile
  * @profile: the profile to print profile info about  (NOT NULL)
  * @string: Returns - string containing the profile info (NOT NULL)
+ * @newline: Should a newline be added to @string.
  *
  * Returns: length of @string on success else error on failure
  *
@@ -30,7 +31,7 @@
  *
  * Returns: size of string placed in @string else error code on failure
  */
-int aa_getprocattr(struct aa_label *label, char **string)
+int aa_getprocattr(struct aa_label *label, char **string, bool newline)
 {
 	struct aa_ns *ns = labels_ns(label);
 	struct aa_ns *current_ns = aa_get_current_ns();
@@ -60,11 +61,13 @@ int aa_getprocattr(struct aa_label *label, char **string)
 		return len;
 	}
 
-	(*string)[len] = '\n';
-	(*string)[len + 1] = 0;
+	if (newline) {
+		(*string)[len] = '\n';
+		(*string)[++len] = 0;
+	}
 
 	aa_put_ns(current_ns);
-	return len + 1;
+	return len;
 }
 
 /**
diff --git a/security/security.c b/security/security.c
index fdd0c85df89e..5a4d90256fd7 100644
--- a/security/security.c
+++ b/security/security.c
@@ -2111,7 +2111,7 @@ int security_getprocattr(struct task_struct *p, const char *lsm, char *name,
 	if (!strcmp(name, "context")) {
 		hlist_for_each_entry(hp, &security_hook_heads.getprocattr,
 				     list) {
-			rc = hp->hook.getprocattr(p, "current", &cp);
+			rc = hp->hook.getprocattr(p, "context", &cp);
 			if (rc == -EINVAL || rc == -ENOPROTOOPT)
 				continue;
 			if (rc < 0) {
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index cd4743331800..1f53a0c66a46 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -6281,7 +6281,7 @@ static int selinux_getprocattr(struct task_struct *p,
 			goto bad;
 	}
 
-	if (!strcmp(name, "current"))
+	if (!strcmp(name, "current") || !strcmp(name, "context"))
 		sid = __tsec->sid;
 	else if (!strcmp(name, "prev"))
 		sid = __tsec->osid;
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 9ce67e03ac49..834b6e886e7b 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -3487,7 +3487,7 @@ static int smack_getprocattr(struct task_struct *p, char *name, char **value)
 	char *cp;
 	int slen;
 
-	if (strcmp(name, "current") != 0)
+	if (strcmp(name, "current") != 0 && strcmp(name, "context") != 0)
 		return -EINVAL;
 
 	cp = kstrdup(skp->smk_known, GFP_KERNEL);
Stephen Smalley Feb. 3, 2020, 6:54 p.m. UTC | #8
On 1/31/20 5:10 PM, Casey Schaufler wrote:
>  From c4085435215653b7c4d07a35a9df308120441d79 Mon Sep 17 00:00:00 2001
> From: Casey Schaufler <casey@schaufler-ca.com>
> Date: Fri, 31 Jan 2020 13:57:23 -0800
> Subject: [PATCH v14] LSM: Move "context" format enforcement into security
>   modules
> 
> Document in lsm_hooks.h what is expected of a security module that
> supplies the "context" attribute.  Add handling of the "context"
> attribute to SELinux, Smack and AppArmor security modules. The
> AppArmor implementation provides a different string for "context"
> than it does for other attributes to conform with the "context"
> format.
> 
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> ---
>   include/linux/lsm_hooks.h            |  6 ++++++
>   security/apparmor/include/procattr.h |  2 +-
>   security/apparmor/lsm.c              |  8 ++++++--
>   security/apparmor/procattr.c         | 11 +++++++----
>   security/security.c                  |  2 +-
>   security/selinux/hooks.c             |  2 +-
>   security/smack/smack_lsm.c           |  2 +-
>   7 files changed, 23 insertions(+), 10 deletions(-)
> 
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 2bf82e1cf347..61977a33f2c3 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -1321,6 +1321,12 @@
>    *	@pages contains the number of pages.
>    *	Return 0 if permission is granted.
>    *
> + * @getprocattr:
> + *	Provide the named process attribute for display in special files in
> + *	the /proc/.../attr directory.  Attribute naming and the data displayed
> + *	is at the discretion of the security modules.  The exception is the
> + *	"context" attribute, which will contain the security context of the
> + *	task as a nul terminated text string without trailing whitespace.

I'd suggest something like the following instead:
* @getprocattr
*   Get the value of process attribute @name for task @p into a buffer
*   allocated by the security module and returned via @value.  The
*   caller will free the returned buffer via kfree.  The set of
*   attribute names is fixed by proc but the format of @value is up
*   to the security module authors except for the "context" attribute,
*   whose value is required to be a NUL-terminated printable ASCII
*   string without trailing whitespace.
*   @p the task whose attribute is being fetched
*   @name the name of the process attribute being fetched
*   @value set to point to the buffer containing the attribute value
*   Return the length of @value including the NUL on success, or -errno 
on error.

The printable ASCII bit is based on what the dbus maintainer requested 
in previous discussions.  The question of whether the terminating NUL 
should be included in the returned length was otherwise left ambiguous 
and inconsistent in your patch among the different security modules; if 
you prefer not including it in the length returned by the security 
modules, you'll need to adjust SELinux at least to not do so for "context".
Stephen Smalley Feb. 3, 2020, 7:37 p.m. UTC | #9
On 2/3/20 1:54 PM, Stephen Smalley wrote:
> I'd suggest something like the following instead:
> * @getprocattr
> *   Get the value of process attribute @name for task @p into a buffer
> *   allocated by the security module and returned via @value.  The
> *   caller will free the returned buffer via kfree.  The set of
> *   attribute names is fixed by proc but the format of @value is up
> *   to the security module authors except for the "context" attribute,
> *   whose value is required to be a NUL-terminated printable ASCII
> *   string without trailing whitespace.
> *   @p the task whose attribute is being fetched
> *   @name the name of the process attribute being fetched
> *   @value set to point to the buffer containing the attribute value
> *   Return the length of @value including the NUL on success, or -errno 
> on error.
> 
> The printable ASCII bit is based on what the dbus maintainer requested 
> in previous discussions.  The question of whether the terminating NUL 
> should be included in the returned length was otherwise left ambiguous 
> and inconsistent in your patch among the different security modules; if 
> you prefer not including it in the length returned by the security 
> modules, you'll need to adjust SELinux at least to not do so for "context".

BTW, I think the above guarantees with the exception of no trailing 
whitespace and whether the NUL byte is included or excluded from length 
are in reality also required for "current" (and SO_PEERSEC) or existing 
userspace will break.
John Johansen Feb. 3, 2020, 8:54 p.m. UTC | #10
On 1/27/20 12:05 PM, Simon McVittie wrote:
> On Fri, 24 Jan 2020 at 15:16:36 -0500, Stephen Smalley wrote:
>> Aside from the trailing newline and \0 issues, AppArmor also has a
>> whitespace-separated (mode) field that may or may not be present in the
>> contexts it presently returns, ala "/usr/sbin/cupsd (enforce)".
> 
> My understanding from last time I worked with AppArmor is that this
> is genuinely part of the context, and whether it is present or absent
> does not vary according to the kernel API used to access contexts.
> AppArmor-specific higher-level APIs parse it into a label and an optional
> mode, but LSM-agnostic user-space APIs (like the one in dbus) pass the
> whole string through as-is.
> 
> (In practice it seems to be present if and only if the context is
> something other than "unconfined", although I don't know offhand whether
> that's an API guarantee.)
> 

Correct, currently it is always included unless the context is unconfined.
There is no guarantee that I am aware of beyond that is what the code
did in the past and so as to not break things we continue to do exactly
that.

The mode certainly does not need to be included in a newer interface.
John Johansen Feb. 3, 2020, 8:59 p.m. UTC | #11
On 1/24/20 11:28 AM, Casey Schaufler wrote:
> 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.
> 
> 

AppArmor can drop the trailing '\n' it is not required. I would say it
could be dropped from /proc/pid/attr/current except there may be some
third party code that requires it.
John Johansen Feb. 3, 2020, 9:02 p.m. UTC | #12
On 1/24/20 12:16 PM, Stephen Smalley wrote:
> On 1/24/20 2:28 PM, Casey Schaufler wrote:
>> 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.
> 
> Well, you can either introduce a new common underlying hook for use by /proc/pid/attr/context and SO_PEERCONTEXT to produce the string that is to be returned to userspace (in order to guarantee consistency in format and allowing them to be directly compared, which I think is what the dbus maintainers wanted), or you can modify every security module to provide that guarantee in its existing getprocattr and getpeersec* hook functions (SELinux already provides this guarantee; Smack and AppArmor produce slightly different results with respect to \0 and/or \n), or you can have the framework trim the values it gets from the security modules before composing them.  But you need to do one of those things before this interface gets merged upstream.
> 
> Aside from the trailing newline and \0 issues, AppArmor also has a whitespace-separated (mode) field that may or may not be present in the contexts it presently returns, ala "/usr/sbin/cupsd (enforce)".  Not sure what they want for the new interfaces.
> 

It is not needed for the new interface. And if I could go back and remove it from the old interface I would.
Casey Schaufler Feb. 3, 2020, 9:39 p.m. UTC | #13
On 2/3/2020 11:37 AM, Stephen Smalley wrote:
> On 2/3/20 1:54 PM, Stephen Smalley wrote:
>> I'd suggest something like the following instead:
>> * @getprocattr
>> *   Get the value of process attribute @name for task @p into a buffer
>> *   allocated by the security module and returned via @value.  The
>> *   caller will free the returned buffer via kfree.  The set of
>> *   attribute names is fixed by proc but the format of @value is up
>> *   to the security module authors except for the "context" attribute,
>> *   whose value is required to be a NUL-terminated printable ASCII
>> *   string without trailing whitespace.
>> *   @p the task whose attribute is being fetched
>> *   @name the name of the process attribute being fetched
>> *   @value set to point to the buffer containing the attribute value
>> *   Return the length of @value including the NUL on success, or -errno on error.

I'm fine with either description, so I'll adopt yours.

>>
>> The printable ASCII bit is based on what the dbus maintainer requested in previous discussions.  The question of whether the terminating NUL should be included in the returned length was otherwise left ambiguous and inconsistent in your patch among the different security modules; if you prefer not including it in the length returned by the security modules, you'll need to adjust SELinux at least to not do so for "context".

append_ctx() is supposed to take the possibility that the module specific context string
may or may not include a trailing '\0' into account. Alas, I see a problem in the current
version for the "no trailing '\0'" case. On the other hand, with your proposed description
the trailing '\0' is required, so it's a matter of verifying that all modules providing
a "context" provide one that includes a trailing '\0' and return strlen()+1.

>
> BTW, I think the above guarantees with the exception of no trailing whitespace and whether the NUL byte is included or excluded from length are in reality also required for "current" (and SO_PEERSEC) or existing userspace will break.

The behavior of interfaces (e.g. "current", "exec") that are module defined
is only of concern with respect to to "display" behavior. If a security module
wants to provide a variable size binary blob in "current" I would object on
principle, but policy as I understand it has long been that if the authors want
to do that, it's their call. The "context" has a defined format, and it would
be up to the authors to come up with a printable ASCII representation of the
binary blob. If they care. They're not required to provide a "context".
Casey Schaufler Feb. 3, 2020, 9:43 p.m. UTC | #14
On 2/3/2020 1:02 PM, John Johansen wrote:
> On 1/24/20 12:16 PM, Stephen Smalley wrote:
>> ...
>>
>> Aside from the trailing newline and \0 issues, AppArmor also has a whitespace-separated (mode) field that may or may not be present in the contexts it presently returns, ala "/usr/sbin/cupsd (enforce)".  Not sure what they want for the new interfaces.
>>
>
> It is not needed for the new interface. And if I could go back and remove it from the old interface I would.

So, what would the "context" for this case be? "/usr/sbin/cupsd" or "enforce"?
John Johansen Feb. 3, 2020, 10:49 p.m. UTC | #15
On 2/3/20 1:43 PM, Casey Schaufler wrote:
> On 2/3/2020 1:02 PM, John Johansen wrote:
>> On 1/24/20 12:16 PM, Stephen Smalley wrote:
>>> ...
>>>
>>> Aside from the trailing newline and \0 issues, AppArmor also has a whitespace-separated (mode) field that may or may not be present in the contexts it presently returns, ala "/usr/sbin/cupsd (enforce)".  Not sure what they want for the new interfaces.
>>>
>>
>> It is not needed for the new interface. And if I could go back and remove it from the old interface I would.
> 
> So, what would the "context" for this case be? "/usr/sbin/cupsd" or "enforce"?
> 

"/usr/sbin/cupsd"

"enforce" is the profile mode which can be looked up separately using "/usr/sbin/cupsd" if it is really needed.
Stephen Smalley Feb. 4, 2020, 1:37 p.m. UTC | #16
On 2/3/20 4:39 PM, Casey Schaufler wrote:
> On 2/3/2020 11:37 AM, Stephen Smalley wrote:
>> BTW, I think the above guarantees with the exception of no trailing whitespace and whether the NUL byte is included or excluded from length are in reality also required for "current" (and SO_PEERSEC) or existing userspace will break.
> 
> The behavior of interfaces (e.g. "current", "exec") that are module defined
> is only of concern with respect to to "display" behavior. If a security module
> wants to provide a variable size binary blob in "current" I would object on
> principle, but policy as I understand it has long been that if the authors want
> to do that, it's their call.

Doing so would break existing userspace (not just LSM-specific 
userspace), so I think it would be a deal breaker even for new security 
modules to move away from those guarantees for "current" at least. 
procps-ng (and I think procps before it) directly reads 
/proc/pid/attr/current and truncates at the first unprintable character. 
  systemd's sd-bus reads /proc/pid/attr/current directly and treats \n, 
\r, or \0 byte as terminators and truncated on first occurrence.  A 
variety of userspace code uses the value obtained from 
/proc/pid/attr/current and/or SO_PEERSEC as something that it can pass 
to printf-like functions using a %s specifier for inclusion in logs and 
audit messages.

> The "context" has a defined format, and it would
> be up to the authors to come up with a printable ASCII representation of the
> binary blob. If they care. They're not required to provide a "context".
Casey Schaufler Feb. 4, 2020, 5:14 p.m. UTC | #17
On 2/4/2020 5:37 AM, Stephen Smalley wrote:
> On 2/3/20 4:39 PM, Casey Schaufler wrote:
>> On 2/3/2020 11:37 AM, Stephen Smalley wrote:
>>> BTW, I think the above guarantees with the exception of no trailing whitespace and whether the NUL byte is included or excluded from length are in reality also required for "current" (and SO_PEERSEC) or existing userspace will break.
>>
>> The behavior of interfaces (e.g. "current", "exec") that are module defined
>> is only of concern with respect to to "display" behavior. If a security module
>> wants to provide a variable size binary blob in "current" I would object on
>> principle, but policy as I understand it has long been that if the authors want
>> to do that, it's their call.
>
> Doing so would break existing userspace (not just LSM-specific userspace), so I think it would be a deal breaker even for new security modules to move away from those guarantees for "current" at least. procps-ng (and I think procps before it) directly reads /proc/pid/attr/current and truncates at the first unprintable character. 

An user-space that makes invalid assumptions about an interface
can't implicitly define the behavior of that interface. You can't
declare that "current" is defined to be a string just because a
developer looked at how one security module uses it and coded the
application accordingly. You can't declare that "current" will
always be a SELinux context. That horse left the barn in 2007,
and there are still people writing code assuming that is what
they're getting.

If the sub-system maintainer, James Morris, is willing to state that
"current" must have a particular format that would be different.

> systemd's sd-bus reads /proc/pid/attr/current directly and treats \n, \r, or \0 byte as terminators and truncated on first occurrence.  A variety of userspace code uses the value obtained from /proc/pid/attr/current and/or SO_PEERSEC as something that it can pass to printf-like functions using a %s specifier for inclusion in logs and audit messages.

Yup. And so far no security module has been foolish enough to export
a binary blob in a /proc/.../attr interface. That doesn't mean that
the interface is defined as a string. It's certainly the convention,
but nowhere is it documented as a requirement.

That's why I'm putting in the effort to define the format for "context"
and SO_PEERCONTEXT. Interfaces at the LSM level need to be defined so as
to allow the underlying security modules to provide the information they
want in a way that is unambiguous and application non-hostile. The help
I've gotten from you and the rest of the reviewers over the life of this
effort has been extremely helpful, if not always easy to swallow.

>
>> The "context" has a defined format, and it would
>> be up to the authors to come up with a printable ASCII representation of the
>> binary blob. If they care. They're not required to provide a "context".
>
>
Simon McVittie Feb. 10, 2020, 11:56 a.m. UTC | #18
On Mon, 03 Feb 2020 at 13:54:45 -0500, Stephen Smalley wrote:
> The printable ASCII bit is based on what the dbus maintainer requested in
> previous discussions.

I thought in previous discussions, we had come to the conclusion that
I can't assume it's 7-bit ASCII. (If I *can* assume that for this new
API, that's even better.)

To be clear, when I say ASCII I mean a sequence of bytes != '\0' with
their high bit unset (x & 0x7f == x) and the obvious mapping to/from
Unicode (bytes '\1' to '\x7f' represent codepoints U+0001 to U+007F). Is
that the same thing you mean?

I thought the conclusion we had come to in previous conversations was
that the LSM context is what GLib calls a "bytestring", the same as
filenames and environment variables - an opaque sequence of bytes != '\0',
with no further guarantees, and no specified encoding or mapping to/from
Unicode (most likely some superset of ASCII like UTF-8 or Latin-1,
but nobody knows which one, and they could equally well be some binary
encoding with no Unicode meaning, as long as it avoids '\0').

If I can safely assume that a new kernel <-> user-space API is constrained
to UTF-8 or a UTF-8 subset like ASCII, then I can provide more friendly
APIs for user-space features built over it. If that isn't possible, the
next best thing is a "bytestring" like filenames, environment variables,
and most kernel <-> user-space strings in general.

    smcv
Stephen Smalley Feb. 10, 2020, 1:25 p.m. UTC | #19
On 2/10/20 6:56 AM, Simon McVittie wrote:
> On Mon, 03 Feb 2020 at 13:54:45 -0500, Stephen Smalley wrote:
>> The printable ASCII bit is based on what the dbus maintainer requested in
>> previous discussions.
> 
> I thought in previous discussions, we had come to the conclusion that
> I can't assume it's 7-bit ASCII. (If I *can* assume that for this new
> API, that's even better.)
> 
> To be clear, when I say ASCII I mean a sequence of bytes != '\0' with
> their high bit unset (x & 0x7f == x) and the obvious mapping to/from
> Unicode (bytes '\1' to '\x7f' represent codepoints U+0001 to U+007F). Is
> that the same thing you mean?

I mean the subset of 7-bit ASCII that satisfies isprint() using the "C" 
locale.  That is already true for SELinux with the existing interfaces. 
I can't necessarily speak for the others.

> I thought the conclusion we had come to in previous conversations was
> that the LSM context is what GLib calls a "bytestring", the same as
> filenames and environment variables - an opaque sequence of bytes != '\0',
> with no further guarantees, and no specified encoding or mapping to/from
> Unicode (most likely some superset of ASCII like UTF-8 or Latin-1,
> but nobody knows which one, and they could equally well be some binary
> encoding with no Unicode meaning, as long as it avoids '\0').
> 
> If I can safely assume that a new kernel <-> user-space API is constrained
> to UTF-8 or a UTF-8 subset like ASCII, then I can provide more friendly
> APIs for user-space features built over it. If that isn't possible, the
> next best thing is a "bytestring" like filenames, environment variables,
> and most kernel <-> user-space strings in general.
> 
>      smcv
>
Stephen Smalley Feb. 10, 2020, 2:55 p.m. UTC | #20
On 2/10/20 8:25 AM, Stephen Smalley wrote:
> On 2/10/20 6:56 AM, Simon McVittie wrote:
>> On Mon, 03 Feb 2020 at 13:54:45 -0500, Stephen Smalley wrote:
>>> The printable ASCII bit is based on what the dbus maintainer 
>>> requested in
>>> previous discussions.
>>
>> I thought in previous discussions, we had come to the conclusion that
>> I can't assume it's 7-bit ASCII. (If I *can* assume that for this new
>> API, that's even better.)
>>
>> To be clear, when I say ASCII I mean a sequence of bytes != '\0' with
>> their high bit unset (x & 0x7f == x) and the obvious mapping to/from
>> Unicode (bytes '\1' to '\x7f' represent codepoints U+0001 to U+007F). Is
>> that the same thing you mean?
> 
> I mean the subset of 7-bit ASCII that satisfies isprint() using the "C" 
> locale.  That is already true for SELinux with the existing interfaces. 
> I can't necessarily speak for the others.

Looks like Smack labels are similarly restricted, per 
Documentation/admin-guide/LSM/Smack.rst.  So I guess the only one that 
is perhaps unclear is AppArmor, since its labels are typically derived 
from pathnames?  Can an AppArmor label returned via its getprocattr() 
hook be any legal pathname?

>> I thought the conclusion we had come to in previous conversations was
>> that the LSM context is what GLib calls a "bytestring", the same as
>> filenames and environment variables - an opaque sequence of bytes != 
>> '\0',
>> with no further guarantees, and no specified encoding or mapping to/from
>> Unicode (most likely some superset of ASCII like UTF-8 or Latin-1,
>> but nobody knows which one, and they could equally well be some binary
>> encoding with no Unicode meaning, as long as it avoids '\0').
>>
>> If I can safely assume that a new kernel <-> user-space API is 
>> constrained
>> to UTF-8 or a UTF-8 subset like ASCII, then I can provide more friendly
>> APIs for user-space features built over it. If that isn't possible, the
>> next best thing is a "bytestring" like filenames, environment variables,
>> and most kernel <-> user-space strings in general.
>>
>>      smcv
>>
>
Casey Schaufler Feb. 10, 2020, 6:32 p.m. UTC | #21
On 2/10/2020 6:55 AM, Stephen Smalley wrote:
> On 2/10/20 8:25 AM, Stephen Smalley wrote:
>> On 2/10/20 6:56 AM, Simon McVittie wrote:
>>> On Mon, 03 Feb 2020 at 13:54:45 -0500, Stephen Smalley wrote:
>>>> The printable ASCII bit is based on what the dbus maintainer requested in
>>>> previous discussions.
>>>
>>> I thought in previous discussions, we had come to the conclusion that
>>> I can't assume it's 7-bit ASCII. (If I *can* assume that for this new
>>> API, that's even better.)
>>>
>>> To be clear, when I say ASCII I mean a sequence of bytes != '\0' with
>>> their high bit unset (x & 0x7f == x) and the obvious mapping to/from
>>> Unicode (bytes '\1' to '\x7f' represent codepoints U+0001 to U+007F). Is
>>> that the same thing you mean?
>>
>> I mean the subset of 7-bit ASCII that satisfies isprint() using the "C" locale.  That is already true for SELinux with the existing interfaces. I can't necessarily speak for the others.
>
> Looks like Smack labels are similarly restricted, per Documentation/admin-guide/LSM/Smack.rst.  So I guess the only one that is perhaps unclear is AppArmor, since its labels are typically derived from pathnames?  Can an AppArmor label returned via its getprocattr() hook be any legal pathname?

Because attr/context (and later, SO_PEERCONTEXT) are new interfaces
there is no need to exactly duplicate what is in attr/current (later
SO_PEERSEC). I already plan to omit the "mode" component of the
AppArmor data in the AppArmor hook, as was discussed earlier. I would
prefer ASCII, but if AppArmor needs bytestrings, that's what we'll
have to do.

>
>>> I thought the conclusion we had come to in previous conversations was
>>> that the LSM context is what GLib calls a "bytestring", the same as
>>> filenames and environment variables - an opaque sequence of bytes != '\0',
>>> with no further guarantees, and no specified encoding or mapping to/from
>>> Unicode (most likely some superset of ASCII like UTF-8 or Latin-1,
>>> but nobody knows which one, and they coould equally well be some binary
>>> encoding with no Unicode meaning, as long as it avoids '\0').
>>>
>>> If I can safely assume that a new kernel <-> user-space API is constrained
>>> to UTF-8 or a UTF-8 subset like ASCII, then I can provide more friendly
>>> APIs for user-space features built over it. If that isn't possible, the
>>> next best thing is a "bytestring" like filenames, environment variables,
>>> and most kernel <-> user-space strings in general.
>>>
>>>      smcv
>>>
>>
>
John Johansen Feb. 10, 2020, 6:56 p.m. UTC | #22
On 2/10/20 6:55 AM, Stephen Smalley wrote:
> On 2/10/20 8:25 AM, Stephen Smalley wrote:
>> On 2/10/20 6:56 AM, Simon McVittie wrote:
>>> On Mon, 03 Feb 2020 at 13:54:45 -0500, Stephen Smalley wrote:
>>>> The printable ASCII bit is based on what the dbus maintainer requested in
>>>> previous discussions.
>>>
>>> I thought in previous discussions, we had come to the conclusion that
>>> I can't assume it's 7-bit ASCII. (If I *can* assume that for this new
>>> API, that's even better.)
>>>
>>> To be clear, when I say ASCII I mean a sequence of bytes != '\0' with
>>> their high bit unset (x & 0x7f == x) and the obvious mapping to/from
>>> Unicode (bytes '\1' to '\x7f' represent codepoints U+0001 to U+007F). Is
>>> that the same thing you mean?
>>
>> I mean the subset of 7-bit ASCII that satisfies isprint() using the "C" locale.  That is already true for SELinux with the existing interfaces. I can't necessarily speak for the others.
> 
> Looks like Smack labels are similarly restricted, per Documentation/admin-guide/LSM/Smack.rst.  So I guess the only one that is perhaps unclear is AppArmor, since its labels are typically derived from pathnames?  Can an AppArmor label returned via its getprocattr() hook be any legal pathname?
> 

yes sadly, as much as I would like to depracate/remove it the pathname based profile names are a byte string. AppArmor supports a more restricted profile name and I have been trying to get people to move to it for 12 years with only limited success.

>>> I thought the conclusion we had come to in previous conversations was
>>> that the LSM context is what GLib calls a "bytestring", the same as
>>> filenames and environment variables - an opaque sequence of bytes != '\0',
>>> with no further guarantees, and no specified encoding or mapping to/from
>>> Unicode (most likely some superset of ASCII like UTF-8 or Latin-1,
>>> but nobody knows which one, and they could equally well be some binary
>>> encoding with no Unicode meaning, as long as it avoids '\0').
>>>
>>> If I can safely assume that a new kernel <-> user-space API is constrained
>>> to UTF-8 or a UTF-8 subset like ASCII, then I can provide more friendly
>>> APIs for user-space features built over it. If that isn't possible, the
>>> next best thing is a "bytestring" like filenames, environment variables,
>>> and most kernel <-> user-space strings in general.
>>>
>>>      smcv
>>>
>>
>
John Johansen Feb. 10, 2020, 7 p.m. UTC | #23
On 2/10/20 10:32 AM, Casey Schaufler wrote:
> On 2/10/2020 6:55 AM, Stephen Smalley wrote:
>> On 2/10/20 8:25 AM, Stephen Smalley wrote:
>>> On 2/10/20 6:56 AM, Simon McVittie wrote:
>>>> On Mon, 03 Feb 2020 at 13:54:45 -0500, Stephen Smalley wrote:
>>>>> The printable ASCII bit is based on what the dbus maintainer requested in
>>>>> previous discussions.
>>>>
>>>> I thought in previous discussions, we had come to the conclusion that
>>>> I can't assume it's 7-bit ASCII. (If I *can* assume that for this new
>>>> API, that's even better.)
>>>>
>>>> To be clear, when I say ASCII I mean a sequence of bytes != '\0' with
>>>> their high bit unset (x & 0x7f == x) and the obvious mapping to/from
>>>> Unicode (bytes '\1' to '\x7f' represent codepoints U+0001 to U+007F). Is
>>>> that the same thing you mean?
>>>
>>> I mean the subset of 7-bit ASCII that satisfies isprint() using the "C" locale.  That is already true for SELinux with the existing interfaces. I can't necessarily speak for the others.
>>
>> Looks like Smack labels are similarly restricted, per Documentation/admin-guide/LSM/Smack.rst.  So I guess the only one that is perhaps unclear is AppArmor, since its labels are typically derived from pathnames?  Can an AppArmor label returned via its getprocattr() hook be any legal pathname?
> 
> Because attr/context (and later, SO_PEERCONTEXT) are new interfaces
> there is no need to exactly duplicate what is in attr/current (later
> SO_PEERSEC). I already plan to omit the "mode" component of the
> AppArmor data in the AppArmor hook, as was discussed earlier. I would
> prefer ASCII, but if AppArmor needs bytestrings, that's what we'll
> have to do.
> 

sadly, to not break userspace its a byte string because that is what the path based profile names are. AppArmor does support a more limited non path based profile name but I can't guarantee that is what userspace is using in policy.


>>
>>>> I thought the conclusion we had come to in previous conversations was
>>>> that the LSM context is what GLib calls a "bytestring", the same as
>>>> filenames and environment variables - an opaque sequence of bytes != '\0',
>>>> with no further guarantees, and no specified encoding or mapping to/from
>>>> Unicode (most likely some superset of ASCII like UTF-8 or Latin-1,
>>>> but nobody knows which one, and they coould equally well be some binary
>>>> encoding with no Unicode meaning, as long as it avoids '\0').
>>>>
>>>> If I can safely assume that a new kernel <-> user-space API is constrained
>>>> to UTF-8 or a UTF-8 subset like ASCII, then I can provide more friendly
>>>> APIs for user-space features built over it. If that isn't possible, the
>>>> next best thing is a "bytestring" like filenames, environment variables,
>>>> and most kernel <-> user-space strings in general.
>>>>
>>>>       smcv
>>>>
>>>
>>
>
Stephen Smalley Feb. 11, 2020, 3:59 p.m. UTC | #24
On 2/10/20 2:00 PM, John Johansen wrote:
> On 2/10/20 10:32 AM, Casey Schaufler wrote:
>> Because attr/context (and later, SO_PEERCONTEXT) are new interfaces
>> there is no need to exactly duplicate what is in attr/current (later
>> SO_PEERSEC). I already plan to omit the "mode" component of the
>> AppArmor data in the AppArmor hook, as was discussed earlier. I would
>> prefer ASCII, but if AppArmor needs bytestrings, that's what we'll
>> have to do.
>>
> 
> sadly, to not break userspace its a byte string because that is what the 
> path based profile names are. AppArmor does support a more limited non 
> path based profile name but I can't guarantee that is what userspace is 
> using in policy.

Ok, so /proc/pid/attr/context and SO_PEERCONTEXT have to be defined as 
returning bytestrings.

So far we've talked about having AppArmor drop the trailing newline, be 
consistent with regard to whether the terminating NUL is included or 
excluded, and drop the mode field from what it returns for 
/proc/pid/attr/context and SO_PEERCONTEXT versus the current 
/proc/pid/attr/current and SO_PEERSEC.  Is that correct?

How do we envision a liblsm processing the value returned from 
/proc/pid/attr/context or SO_PEEERCONTEXT?  It can certainly split it up 
per LSM.  But can it then take the apparmor portion of the context and 
pass that to existing libapparmor interfaces without breakage?  Or will 
the changes to the format described above create issues there?
John Johansen Feb. 11, 2020, 5:58 p.m. UTC | #25
On 2/11/20 7:59 AM, Stephen Smalley wrote:
> On 2/10/20 2:00 PM, John Johansen wrote:
>> On 2/10/20 10:32 AM, Casey Schaufler wrote:
>>> Because attr/context (and later, SO_PEERCONTEXT) are new interfaces
>>> there is no need to exactly duplicate what is in attr/current (later
>>> SO_PEERSEC). I already plan to omit the "mode" component of the
>>> AppArmor data in the AppArmor hook, as was discussed earlier. I would
>>> prefer ASCII, but if AppArmor needs bytestrings, that's what we'll
>>> have to do.
>>>
>>
>> sadly, to not break userspace its a byte string because that is what the path based profile names are. AppArmor does support a more limited non path based profile name but I can't guarantee that is what userspace is using in policy.
> 
> Ok, so /proc/pid/attr/context and SO_PEERCONTEXT have to be defined as returning bytestrings.
> 
> So far we've talked about having AppArmor drop the trailing newline, be consistent with regard to whether the terminating NUL is included or excluded, and drop the mode field from what it returns for /proc/pid/attr/context and SO_PEERCONTEXT versus the current /proc/pid/attr/current and SO_PEERSEC.  Is that correct?
> 
> How do we envision a liblsm processing the value returned from /proc/pid/attr/context or SO_PEEERCONTEXT?  It can certainly split it up per LSM.  But can it then take the apparmor portion of the context and pass that to existing libapparmor interfaces without breakage?  Or will the changes to the format described above create issues there?
> 
>
libapparmor can handle the changes. It does not require the mode string, currently anything unconfined does not include it, and we have already done experiments with dropping it in other cases. The trailing '\n' is handled conditionally so only if its there; this is well tested as the currently out of upstream af_unix socket mediation that is used by dbus does not include a trailing '\n' on the SO_PEERSEC.
Casey Schaufler Feb. 11, 2020, 6:35 p.m. UTC | #26
On 2/11/2020 9:58 AM, John Johansen wrote:
> On 2/11/20 7:59 AM, Stephen Smalley wrote:
>> On 2/10/20 2:00 PM, John Johansen wrote:
>>> On 2/10/20 10:32 AM, Casey Schaufler wrote:
>>>> Because attr/context (and later, SO_PEERCONTEXT) are new interfaces
>>>> there is no need to exactly duplicate what is in attr/current (later
>>>> SO_PEERSEC). I already plan to omit the "mode" component of the
>>>> AppArmor data in the AppArmor hook, as was discussed earlier. I would
>>>> prefer ASCII, but if AppArmor needs bytestrings, that's what we'll
>>>> have to do.
>>>>
>>>
>>> sadly, to not break userspace its a byte string because that is what the path based profile names are. AppArmor does support a more limited non path based profile name but I can't guarantee that is what userspace is using in policy.
>>
>> Ok, so /proc/pid/attr/context and SO_PEERCONTEXT have to be defined as returning bytestrings.
>>
>> So far we've talked about having AppArmor drop the trailing newline, be consistent with regard to whether the terminating NUL is included or excluded, and drop the mode field from what it returns for /proc/pid/attr/context and SO_PEERCONTEXT versus the current /proc/pid/attr/current and SO_PEERSEC.  Is that correct?
>>
>> How do we envision a liblsm processing the value returned from /proc/pid/attr/context or SO_PEEERCONTEXT?  

There hasn't been any serious thought put into liblsm. To date
there hasn't been an LSM level interface to worry about except
for /sys/kernel/security/lsm. My notions of what a liblsm ought
provide would seem archaic to most of the people here. I can make
proposals if there's a notion that liblsm makes sense.


>> It can certainly split it up per LSM.  But can it then take the apparmor portion of the context and pass that to existing libapparmor interfaces without breakage?  Or will the changes to the format described above create issues there?
>>
>>
> libapparmor can handle the changes. It does not require the mode string, currently anything unconfined does not include it, and we have already done experiments with dropping it in other cases. The trailing '\n' is handled conditionally so only if its there; this is well tested as the currently out of upstream af_unix socket mediation that is used by dbus does not include a trailing '\n' on the SO_PEERSEC.

So it doesn't seem that there would be significant difficulties
switching from "current" to "context". It won't be transparent,
but we're providing "display" to address that.
John Johansen Feb. 11, 2020, 7:11 p.m. UTC | #27
On 2/11/20 10:35 AM, Casey Schaufler wrote:
> On 2/11/2020 9:58 AM, John Johansen wrote:
>> On 2/11/20 7:59 AM, Stephen Smalley wrote:
>>> On 2/10/20 2:00 PM, John Johansen wrote:
>>>> On 2/10/20 10:32 AM, Casey Schaufler wrote:
>>>>> Because attr/context (and later, SO_PEERCONTEXT) are new interfaces
>>>>> there is no need to exactly duplicate what is in attr/current (later
>>>>> SO_PEERSEC). I already plan to omit the "mode" component of the
>>>>> AppArmor data in the AppArmor hook, as was discussed earlier. I would
>>>>> prefer ASCII, but if AppArmor needs bytestrings, that's what we'll
>>>>> have to do.
>>>>>
>>>>
>>>> sadly, to not break userspace its a byte string because that is what the path based profile names are. AppArmor does support a more limited non path based profile name but I can't guarantee that is what userspace is using in policy.
>>>
>>> Ok, so /proc/pid/attr/context and SO_PEERCONTEXT have to be defined as returning bytestrings.
>>>
>>> So far we've talked about having AppArmor drop the trailing newline, be consistent with regard to whether the terminating NUL is included or excluded, and drop the mode field from what it returns for /proc/pid/attr/context and SO_PEERCONTEXT versus the current /proc/pid/attr/current and SO_PEERSEC.  Is that correct?
>>>
>>> How do we envision a liblsm processing the value returned from /proc/pid/attr/context or SO_PEEERCONTEXT?
> 
> There hasn't been any serious thought put into liblsm. To date
> there hasn't been an LSM level interface to worry about except
> for /sys/kernel/security/lsm. My notions of what a liblsm ought
> provide would seem archaic to most of the people here. I can make
> proposals if there's a notion that liblsm makes sense.
> 
> 
>>> It can certainly split it up per LSM.  But can it then take the apparmor portion of the context and pass that to existing libapparmor interfaces without breakage?  Or will the changes to the format described above create issues there?
>>>
>>>
>> libapparmor can handle the changes. It does not require the mode string, currently anything unconfined does not include it, and we have already done experiments with dropping it in other cases. The trailing '\n' is handled conditionally so only if its there; this is well tested as the currently out of upstream af_unix socket mediation that is used by dbus does not include a trailing '\n' on the SO_PEERSEC.
> 
> So it doesn't seem that there would be significant difficulties
> switching from "current" to "context". It won't be transparent,
> but we're providing "display" to address that.
> 
> 
right

Patch
diff mbox series

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;