Re: [PATCH 2/2] proc,security: move restriction on writing /proc/pid/attr nodes to proc

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

On 12/19/2016 9:09 AM, Stephen Smalley wrote:
> On Mon, 2016-12-19 at 08:32 -0800, Casey Schaufler wrote:
>> On 12/19/2016 7:52 AM, Stephen Smalley wrote:
>>> On Mon, 2016-12-19 at 16:41 +0100, José Bollo wrote:
>>>> Le lundi 19 décembre 2016 à 09:33 -0500, Stephen Smalley a écrit
>>>> :
>>>>> On Mon, 2016-12-19 at 10:44 +0100, José Bollo wrote:
>>>>>> Le vendredi 16 décembre 2016 à 12:41 -0500, Stephen Smalley a
>>>>>> écrit :
>>>>>>>
>>>>>>> Processes can only alter their own security attributes via
>>>>>>> /proc/pid/attr nodes.  This is presently enforced by each
>>>>>>> individual
>>>>>>> security module and is also imposed by the Linux
>>>>>>> credentials
>>>>>>> implementation, which only allows a task to alter its own
>>>>>>> credentials.
>>>>>>> Move the check enforcing this restriction from the
>>>>>>> individual
>>>>>>> security modules to proc_pid_attr_write() before calling
>>>>>>> the
>>>>>>> security
>>>>>>> hook,
>>>>>>> and drop the unnecessary task argument to the security hook
>>>>>>> since
>>>>>>> it
>>>>>>> can
>>>>>>> only ever be the current task.
>>>>>> Hi Stephen,
>>>>>>
>>>>>> The module PTAGS that I made relies on the ability to write
>>>>>> files
>>>>>> in
>>>>>> /proc/pid/attr/...
>>>>>>
>>>>>> Why did I used this files? Thaere are serious reasons:
>>>>>>  - follows the life cycle of the process
>>>>>>  - implements pid namespace
>>>>>>
>>>>>> There is absolutely no way to get these features easyly
>>>>>> outside
>>>>>> that
>>>>>> implementation context of /proc/pid/attr/xxx.
>>>>>>
>>>>>> For these reasons I vote against that change.
>>>>> It is already prohibited by the credentials framework;
>>>> If it were REALLY prohibited, your patch shouldn't exist ;)
>>> You can't do it safely (and your current implementation is unsafe).
>>>  Please, read Documentation/security/credentials.txt in full, and
>>> look
>>> at the setprocattr implementations in the upstream security modules
>>> for
>>> reference.
>> I just reviewed the referenced documentation (thanks David)
>> and it does clearly identify why, from the viewpoint of the
>> credential implementation, modifying another task's credentials
>> is unsafe. Both Smack and SELinux view modification of task
>> credentials as unsafe from a security viewpoint, so neither of
>> these modules would force the credential implementation to
>> allow the behavior.
>>
>> This, unfortunately, leaves Jose in a bit of a bind. It looks
>> to me like there are options, so let's not just tell him to
>> go away. The options I see include:
>>
>>  - Fix the credential code so modifying another task is safe
>> 	I don't advocate this approach because the existing
>> 	implementation is only reasonable because of the
>> 	restriction.
>>  - Revert shared credentials
>> 	I confess to not being a fan of shared credentials,
>> 	for reason of complexity. I would not expect a
>> 	proposal to go back to embedding credentials in
>> 	their containing structures to be considered seriously.
>> 	Nonetheless, it would solve the problem.
>>  - Add a security blob for the task
>> 	It is unfortunate that when shared credentials were
>> 	introduced the task blob was not renamed a cred blob.
>> 	It would be simple and consistent to add a security
>> 	blob to the task in addition to the blob in the cred.
>> 	I would add a t_security field to the task structure
>> 	that associates security data with the task. This
>> 	blob would be used for data that does not meet intent
>> 	of a credential, which I believe may be the case for
>> 	Jose's PTAGS.
>>
>> I know which I would prefer, but as I expect someone to shout
>> that none can possibly be acceptable I think I'll stop here and
>> let the discussion go a while.
>>
>> ("Light fuse. Stand back. :))
> Why not:
>
> - Reconsider the approach used in PTAGS in light of the security
> implications of allowing one task to change another task's security
> attributes, and rework PTAGS to use a more secure approach.

Not every approach to security relies on being unable to change
another process' attributes. I know of people who believe that
SELinux would be more secure if an application launcher could set
the context on another process. While I do not agree with these
people I understand the logic they use and why it does not apply
to SELinux or Smack.

Consider a module that does statistical analysis on program behavior
and tags (using PTAGS or something similar) processes that are
behaving in suspect ways. You probably don't want the math running
in the kernel. The kernel decides to start denying the suspect
process access once some tagging threshold is exceeded. Sure, you
might be able to come up with some mechanism to get the process to
tag itself, but it's kind of silly to make it hard when it doesn't
have to be.

If you can suggest a mechanism that achieves Jose's goals (which
may not match your goals) that does not require allowing a process
to change another's security attributes I'm sure we'd all be
interested.