Re: [PATCH 0/2] selinux: add targeted whitelisting of ioctl commands.

[Joshua Brindle <brindle@quarksecurity.com>]

Stephen Smalley wrote:
> On 05/21/2015 10:57 AM, Joshua Brindle wrote:
>> William Roberts wrote:
>>> On May 21, 2015 7:53 AM, "Joshua Brindle"<brindle@quarksecurity.com>
>>> wrote:
>>>> William Roberts wrote:
>>>>> On Wed, May 20, 2015 at 9:17 PM, Jeffrey Vander Stoep<jeffv@google.com>
>>>>> wrote:
>>>>>
>>>>>> Thanks for all the feedback and suggestions. Agreed that raw numerical
>>>>>> values are confusing. I will fix up the commit message to set a better
>>>>>> precedent for intended use. I included them more to illustrate what is
>>>>>> happening under the hood. I like the idea of a qualifier for clarity.
>>>>>> The qualifier seems necessary for the suggested non-ioctl-specific
>>>>>> approach.
>>>>>>
>>>>>> Individual ioctl labels are only marginally better than raw numbers.
>>>>>> E.g. { TCSETSF TIOCGWINSZ TCGETA TCSETA TCSETAW TCSETAF TCSBRK TCXONC
>>>>>> TIOCMBIS }. More helpful...but not much.
>>>>>>
>>>>>> My plan was to group commonly used ioctl sets into macros.
>>>>>>
>>>>>> e.g. common_socket_ioc, priv_socket_ioc, tty_ioc, gpu_ioc, etc
>>>>>>
>>>>> This is exactly what I had in mind, just use m4
>>>>>
>>>> Even outside of my analysis use case I think this is not a good idea.
>>>>
>>>> Consider the Android base policy defined gpu_op as a set of ioctls, and
>>> there were related rules for gpu users defined there. Then a device
>>> variant
>>> policy has additional gpu_op ioctl's. How does it add them? Does it
>>> have to
>>> re-add all of the related rules with the new ioctls?
>>>
>>> I could define a new macro my_device_GPU_ioctls and include the base
>>> macro
>>> in its expansion.
>> And repeat every related rule (and keep them in sync as the base policy
>> develops). How much easier is it just to add your ioctl to an ioctl
>> attribute and be done?
>
> Technically not required for that purpose, e.g. I can do this today:
> $ cat device/lge/hammerhead/sepolicy/global_macros
> define(`r_file_perms', `{ ' r_file_perms `write }')
> and have it automatically add write everywhere we allow r_file_perms
> even in the core policy, because the build process unions on a
> file-by-file basis.
>
> Effectively your attribute would only be useful as inline documentation;
> it would have no binding to the actual semantic meaning of the check.
> If the policy writer allows it as gpu_ioc in policy but the driver is
> actually something other than a gpu driver or chooses to interpret a
> particular command value included in that attribute in some other way,
> then it might have an entirely different effect.  So it might be helpful
> but not sufficient for analysis.

Unless the attributes were specific to the type in question:

ioctl gpu_device gpu_ioc { 0x8910-0x8926 0x892A-0x8935 };

allow surfaceflinger gpu_device : chr_file gpu_ioc;

This would 1) enforce the policy authors intention, 2) force policy 
authors to be specific with what ioctls mean what with which devices and 
3) allow more meaningful analysis.