Re: [RFC PATCH] security, capability: pass object information to security_capable

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

On 7/12/2019 1:21 PM, Stephen Smalley wrote:
> On 7/12/19 3:54 PM, Casey Schaufler wrote:
>> On 7/12/2019 11:25 AM, Stephen Smalley wrote:
>>> On 7/12/19 1:58 PM, Casey Schaufler wrote:
>>>> On 7/12/2019 10:34 AM, Nicholas Franck wrote:
>>>>> At present security_capable does not pass any object information
>>>>> and therefore can neither audit the particular object nor take it
>>>>> into account. Augment the security_capable interface to support
>>>>> passing supplementary data. Use this facility initially to convey
>>>>> the inode for capability checks relevant to inodes. This only
>>>>> addresses capable_wrt_inode_uidgid calls; other capability checks
>>>>> relevant to inodes will be addressed in subsequent changes. In the
>>>>> future, this will be further extended to pass object information for
>>>>> other capability checks such as the target task for CAP_KILL.
>>>>
>>>> This seems wrong to me. The capability system has nothing to do
>>>> with objects. Passing object information through security_capable()
>>>> may be convenient, but isn't relevant to the purpose of the interface.
>>>> It appears that there are very few places where the object information
>>>> is actually useful.
>>>
>>> A fair number of capabilities are checked upon some attempted object access (often right after comparing UIDs or other per-object state), and the particular object can be very helpful in both audit and in access control.  More below.
>>
>> I'm not disagreeing with that. What I'm saying is that the capability
>> check interface is not the right place to pass that information. The
>> capability check has no use for the object information. I would much
>> rather see a security_pass_object_data() hook that gets called after
>> (or before) the security_capable() hook in the places where you want
>> the extra information.
>
> I don't see how that would work or be advantageous.  Within the capable hook, the security module(s) decide whether or not to allow the capability, and generate any relevant LSM audit records.  It is precisely at those two points (deciding and auditing) that we need the information; both occur within the existing capable hooks.  Calling a separate hook before the capable call and e.g. saving the information in the task security structure for later consumption during the capable call offers only overhead, no benefit.  Calling a separate hook after the capable call is too late to be of use - the decision and auditing are already done.  And both hooks would be invoked from precisely the same function at the same point.  If the information wasn't already readily available at the point of the hook call, it might be a different matter, but that isn't the case here.

If there's a problem with the audit system you should be
looking at fixing that rather than tacking information that's
useless for capabilities onto it's interfaces.

>>>>> In SELinux this new information is leveraged here to include the inode
>>>>> in the audit message. In the future, it could also be used to perform
>>>>> a per inode capability checks.
>>>>
>>>> I suggest that you want a mechanism for adding the inode information
>>>> to the audit record instead.
>>>
>>> That is part of what we want, but not the entire picture.  But with respect to audit, the problem today is that one sees SELinux dac_read_search, dac_override, etc denials with no indication as to the particular file, which is unfortunate both from a security auditing perspective and from a policy development perspective.
>>
>> I can see how that is a problem.
>>
>>> The only option today to gain that information is by enabling system call audit and setting at least one audit filter so that the audit framework will collect that information and include it in the audit records that are emitted upon syscall exit after any such AVC denial.  Unfortunately, that is all disabled by default on most systems due to its associated performance overhead, and one doesn't even have the option of enabling it on some systems, e.g. Android devices.  And even when one can enable syscall audit, one must correlate the syscall audit record to the associated AVC record to identify the object rather than having the information directly included in the same record.
>>
>> None of which gives any rationale for adding the information
>> to the capability check. Sure, it's in the right place, but there
>> is no object interaction with the capability call.
>
> We introducing such an interaction - that's the point.

And I say that you're breaking the layering.

>>>> What would a "per inode" capability check be? Capability checks are
>>>> process checks, not object checks.
>>>
>>> Ideally it would be possible to scope dac_override and other capabilities to specific objects rather than having to allow it for all or none.
>>
>> That would require a major overhaul of the capability scheme,
>> and you're going to get arguments from people like me about
>> whether or not that would be ideal. Besides, isn't that what
>> SELinux is all about, providing that sort of privilege granularity?
>
> It only requires passing the information to the security modules at the point of the hook call, and then SELinux or other security modules can implement it themselves without any other changes to the kernel.  We aren't changing the way the base capabilities logic works.

If you're not changing how capabilities work you
shouldn't be adding parameters to its interface.

> We can't provide that degree of granularity without the additional information.  Let's say domain A needs DAC override for files of type B and for nothing else.  To support this requirement, SELinux policy has to include at least:
> 1) allow A self:capability dac_override;
> 2) allow A B:file { read write };
>
> Let's say that A also reads from files of type C and writes to file of type D.  So SELinux policy also has to allow:
> 3) allow A C:file read;
> 4) allow A D:file write;
>
> There are files within type C and within type D that are under different DAC ownerships or modes.  Only some of these files should be accessible to A.  But because dac_override is global and not scoped to specific sets of files, the combination of these permissions now allows A to override DAC on files of type C and of type D; thus it can read all files of type C and write to all files of type D even though it has no legitimate need to do so.

Capabilities do not implement a fine grained policy.
This has been lamented ad nauseam regarding CAP_SYS_ADMIN.
It is also irrelevant to the issue here. 

>>> Just because a process needs to override DAC on one file or one set of files is not a reason to allow it to do so on every file it can access at all.
>>
>> That's an argument for privilege bracketing within the process.
>> Not something I recommend (the oft referenced sendmail problems
>> being but one example) but the only way to do it properly without
>> delving into path based controls.
>
> As you note, historically privilege bracketing hasn't worked so well, and fundamentally it puts the trust burden on userspace for something that could be enforced at the system level quite easily and in a race-free manner.

I'm not arguing against that goal, I'm saying that you're
going about it the wrong way.


>>> If we want to apply least privilege, then this is a desirable facility.
>>
>> The capability mechanism is object agnostic by design.
>
> Some might argue that's a flawed design.

Might? Show me someone who's looked at capabilities who
doesn't think the design is flawed! The entire POSIX 1003.1e/2c
group knew it was flawed, which is the primary reason it never
got past DRAFT status. If capabilities had been perfect no one
would ever have been tempted to add domain enforcement.

But there it is. That is not an excuse to muddle it up
with pass-through parameters in an effort to wiggle around
the issues in other facilities.

>>> I understand that doesn't mesh with Smack's mental modelbut it would probably be useful to both SELinux and AppArmor, among others.
>>
>> I'm perfectly happy to have the information transmitted.
>> I think a separate interface for doing so is appropriate.
>
> As above, I don't see any way to do that that isn't just adding overhead.

I'll see if I can squeeze some time into alternatives, but
my brain is already wrestling with audit issues of my own.