Re: [PATCH v4] Smack: limited capability for changing process label

["Rafał Krypa" <r.krypa@samsung.com>]

On 2015-10-15 10:04, Casey Schaufler wrote:
> On 10/15/2015 12:48 AM, Rafał Krypa wrote:
>> On 2015-10-14 17:54, Rafal Krypa wrote:
>>> From: Zbigniew Jasinski <z.jasinski@samsung.com>
>>>
>>> This feature introduces new kernel interface:
>>>
>>> - <smack_fs>/relabel-self - for setting transition labels list
>>>
>>> This list is used to control smack label transition mechanism.
>>> List is set by, and per process. Process can transit to new label only if
>>> label is on the list. Only process with CAP_MAC_ADMIN capability can add
>>> labels to this list. With this list, process can change it's label without
>>> CAP_MAC_ADMIN but only once. After label changing, list is unset.
>>>
>>> Changes in v2:
>>> * use list_for_each_entry instead of _rcu during label write
>>> * added missing description in security/Smack.txt
>>>
>>> Changes in v3:
>>> * squashed into one commit
>>>
>>> Changes in v4:
>>> * switch from global list to per-task list
>>> * since the per-task list is accessed only by the task itself
>>>   there is no need to use synchronization mechanisms on it
>>>
>>> Signed-off-by: Zbigniew Jasinski <z.jasinski@samsung.com>
>>> Signed-off-by: Rafal Krypa <r.krypa@samsung.com>
>>> ---
>>>  Documentation/security/Smack.txt |  14 ++++
>>>  security/smack/smack.h           |   3 +-
>>>  security/smack/smack_access.c    |   6 +-
>>>  security/smack/smack_lsm.c       |  73 ++++++++++++++++-
>>>  security/smack/smackfs.c         | 167 ++++++++++++++++++++++++++++++++++++---
>>>  5 files changed, 246 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/Documentation/security/Smack.txt b/Documentation/security/Smack.txt
>>> index 5e6d07f..d9ace08 100644
>>> --- a/Documentation/security/Smack.txt
>>> +++ b/Documentation/security/Smack.txt
>>> @@ -255,6 +255,20 @@ unconfined
>>>  	the access permitted if it wouldn't be otherwise. Note that this
>>>  	is dangerous and can ruin the proper labeling of your system.
>>>  	It should never be used in production.
>>> +relabel-self
>>> +	This interface contains a list of labels to which the process can
>>> +	transition to, by writing to /proc/self/attr/current.
>>> +	Normally a process can change its own label to any legal value, but only
>>> +	if it has CAP_MAC_ADMIN. This interface allows a process without
>>> +	CAP_MAC_ADMIN to relabel itself to one of labels from predefined list.
>>> +	A process without CAP_MAC_ADMIN can change its label only once. When it
>>> +	does, this list will be cleared.
>>> +
>>> +	The format accepted on write is:
>>> +		"%s"
>>> +	for adding label, and:
>>> +		"-%s"
>>> +	for removing label from list.
>> I have one concern here, let me make some self-criticism.
>> The interface described here for relabel-self is convenient and suiting actual needs of user space parts that are going to use it.
>> But it is inconsistent with other existing interfaces in smackfs. Recently I submitted a patch (merged into v4.2) that extended onlycap to allow multiple labels in it.
>> The smackfs interface for onlycap always takes the full list of labels that replaces the list that was previously set.
>> Now relabel-self is also going to contain a list of labels. But smackfs interface gets one label at a time and performs add/remove operations.
>>
>> Are you OK. with such inconsistency?
>>
> A foolish consistency is the hobgoblin of little minds.
>
> More directly, I am fine with it. Some of your previous work
> made removing labels from lists practical where it had not been
> before. I would rather have an inconsistent interface set
> than one that is consistently bad.

I am sorry forcausing confusion. I have reassessed the user space requirements and it seems that with per-task list, it will be always written in full. The ability for adding or removing labels was nice, but only useful when the list was global.
So please let me update the patch one more time, with relabel-self having the same interface as onlycap.