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

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

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.