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

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

[Jeff Vander Stoep]

---- motivation ----
Ioctls provide many of the operations necessary for device control. The typical
driver supports a device specific set of operations accessible by the ioctl
system call and specified by the command argument. SELinux provides per
operation access control to many system operations e.g. chown, kill, setuid,
ipc_lock, etc. Ioclts on the other hand are granted on a per file descriptor
basis using the ioctl permission, meaning that the set of operations provided
by the driver are granted on an all-or-nothing basis. In some cases this may be
acceptable, but often the same driver provides a large and diverse set of
operations such as benign and necessary functionality as well as dangerous
capabilities, or access to system information that should be restricted.

Example 1 - restrict access to privacy sensitive information:
Applications with the socket permission require the ioctl permission to perform
control operations. However access to this ioctl also provides the application
with access to information such as the MAC address, Wifi ESSID, layer two
encryption information etc that can be used to uniquely identify the user and
compromise privacy.

Example 2 - reduce kernel attack surface by eliminating access to unnecessary
operations:
Of the 34 available operations provided by a graphics driver, only 11 are used
on the device under test. Media drivers are frequently under development and are
notorious for containing bugs. By limiting application access to the subset of
commands needed, many bugs can be made unreachable without sacrificing
functionality. Real world example, we recently discovered three bugs within a
driver that would be unreachable by restricting access to unused ioctl commands.

---- Design constraints ----
Policy: Support existing policy, targeted whitelisting. The ioctls commands used
on a system may not be completely known to a sysadmin/policywriter. It is not
reasonable to enforce that all needed commands be known in order to use this
feature in a targeted manner. Existing policy using the ioctl permission will
continue to work as-is. Policy targeting a specific source/target/class will
only be enforced on that particular source/target/class. E.g. continue to allow
init to access all ioctls provided by a driver, but only allow the browser to
access a subset.

Performance: Many ioctl calls are performance sensitive, and some ioctls have a
large command set (e.g. sockets support hundreds of commands). Execution time on
a filtered ioctl should be similar to execution time on an unfiltered one and
not related to the number of commands being filtered. Performance numbers will
be included in a separate document.

Command space: The ioctl command is a 32 bit number comprised of four fields,
number - sequence number of the command. 8 bits
type - magic number assigned to the driver. 8 bits
size - size of the user data involved. typically 14 bits (arch dependent)
direction - The direction of data transfer. typically 2 bits (arch dependent)
The command is uniquely identified by the type and number, size and direction
are considered to be arguments and are not considered for SELinux whitelisting.

---- Policy format ----
allow <source> <target>:<class> { 0x8910-0x8926 0x892A-0x8935 }
auditallow <source> <target>:<class> 0x892A

---- Architecture ----
policy: Ioctl operations include allow, auditallow and dontaudit permissions.

binary policy/avtab: Like AVC permissions, ioctl operations permissions are
represented by set/cleared bits. Each ioctl operation avtab entry represents a
single ioctl command type (using “type” as defined above in the “command space”
section) and includes permissions for the 256 operations within that type. This
architecture was designed after observing the sets of operations used by each
source/target/class across several devices. Most significantly, a
source/target/class normally uses just one ioctl type and only rarely uses more
than one type. For example in a prototyping device, 90 source/target/class
sets use just 1 type, and 2 source/target/class sets use 2 types, no
source/target/class used more than 2 types. Including permissions for all 256
bits creates a deterministic lookup time within a type because the command being
checked is mapped directly to a permission. Additionally, very complex policies
within a type map to the same size as simple policies e.g. ioctl commands
often switch between get and set operations where a policy writer could
conceivably want to give an application the ability to access all the get
operations and not set operations i.e. a policy allowing every other ioctl
number within a type. Optimizing for complexity within types was done to 1) make
the solution general 2) make the solution fast for both complex simple policies
and 3) ioctls that have a small operation set (many use just one) very likely
need all operations allowed and thus would not use this feature but would
continue to rely on the all-or-nothing ioctl permission for that particular
ioctl.

AVC: A pointer to an operations structure has been added to the avc_node such
that general avc_lookup also returns the allowed ioctl operations. The structure
flags the ioctl types that are allowed and only looks up the permissions within
that type from the avtab when they are first used. After the initial lookup of a
type it is added to a list of types that are allowed for the source/target/class
being retrieved. As mentioned earlier, this list is typically one item long, but
can be 2, 3, etc, if necessary. The list lookup is linear with the length of
the list so it might be a good idea to enforce a restriction on the number of
types allowed in the policy compiler (if there exists a good example of a
source/target/class that uses more than a handful of types, none of the
prototyping devices tested would benefit from this).

---- example policy ----
allow system_server ion_device:chr_file { 0x4901 0x4905-0x4906 };
allow keystore tee_device:chr_file { 0x9707-0x9708 0x970a 0x970f };
allow netmgrd netmgrd:udp_socket { 0x8913 0x8933 0x89f2-0x89f3 0x89f5
	0x89f8-0x89f9 };
allow surfaceflinger graphics_device:chr_file { 0x4600 0x4602 0x4611
0x6d87-0x6d89 0x6da0 0x6da2 0x6da4 0x6da6 };
allow untrusted_app gpu_device:chr_file { 0x902 0x907 0x913 0x915 0x933-0x935
	0x938-0x939 0x93b 0x93d };
allow shell owntty_device:chr_file { 0x5401 0x5403 0x540f 0x5413 };
allow system_server alarm_device:chr_file { 0x6101 0x6122 0x6132 0x6134 };
allow mediaserver camera_device:chr_file { 0x5600 0x5602 0x5604-0x5605 0x5608
0x561c 0x564a 0x564d 0x5659-0x565b 0x5660 0x7c00-0x7c01 };
allow domain binder_device:chr_file { 0x6201 0x6205 0x6207-0x6209 };
allow system_server input_device:chr_file { 0x4501-0x4503 0x4506-0x4509 0x4518
0x451b 0x4521-0x4523 0x4525 0x4531 0x4535 0x456f-0x4570 0x4575-0x4576
	0x4579-0x457a 0x4591 0x45a0 };
allow mediaserver audio_device:chr_file { 0x4101 0x4103 0x4111 0x4113 0x4123
	0x4140 0x4150 0x5501 0x5510-0x5513 0x610f 0x6167 0x6169 0x6171 0x6173
	0x6175 0x617b-0x617c };
allow camera camera_device:chr_file { 0x5659-0x565b 0x56c0-0x56c8 0x56de 0x56e0
	0x7c00-0x7c01 };
allow platform_app gpu_device:chr_file { 0x902 0x907 0x913-0x915 0x917 0x921
	0x933-0x935 0x938-0x939 0x93b 0x93d };
allow surfaceflinger gpu_device:chr_file { 0x902 0x907 0x913 0x915 0x921
	0x933-0x935 0x938-0x939 0x93b 0x93d };
allow sensors sensors:socket { 0xc302 0xc304 };
allow system_server gpu_device:chr_file { 0x902 0x907 0x913-0x915 0x917 0x921
	0x933-0x935 0x938-0x939 0x93b 0x93d };
allow domain ashmem_device:chr_file { 0x7701 0x7703-0x7705 };
allow bootanim gpu_device:chr_file { 0x902 0x907 0x913-0x915 0x917 0x921
	0x933-0x935 0x938-0x939 0x93b 0x93d };

Jeff Vander Stoep (2):
  security: lsm_audit: add ioctl specific auditing
  SELinux: per-command whitelisting of ioctls

 include/linux/lsm_audit.h           |   7 +
 security/lsm_audit.c                |  15 ++
 security/selinux/avc.c              | 428 ++++++++++++++++++++++++++++++++++--
 security/selinux/hooks.c            |  40 +++-
 security/selinux/include/avc.h      |   5 +
 security/selinux/include/security.h |  34 ++-
 security/selinux/ss/avtab.c         |  91 ++++++--
 security/selinux/ss/avtab.h         |  25 ++-
 security/selinux/ss/conditional.c   |  32 ++-
 security/selinux/ss/conditional.h   |   6 +-
 security/selinux/ss/policydb.c      |   5 +
 security/selinux/ss/services.c      | 202 +++++++++++++++--
 security/selinux/ss/services.h      |   6 +
 13 files changed, 832 insertions(+), 64 deletions(-)

-- 
2.2.0.rc0.207.ga3a616c

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

[Jeffrey Vander Stoep]

[-- Attachment #1: Type: text/plain, Size: 9721 bytes --]

Approximately 150000 ioctls are executed during the ~40 seconds of
boot time. Adding policy such that all of these ioctls are run through
the ioctl whitelisting code has no noticeable impact on boot time. See
attached graph of boot time comparison.

Measuring the impact of an individual call is a bit showed a 20-30 ns
increase in the ioctl call time on a very fast ioctl (SIOCGIFHWADDR is
basically a 5 byte memcpy). Benchmarking was done on a Nexus 9, 64-bit
NVIDIA Tegra K1 Dual Denver @ 2.3GHz. See attached graph of individual
ioctl call time comparison.

On Thu, Apr 9, 2015 at 2:48 PM, Jeff Vander Stoep <jeffv@google.com> wrote:
> ---- motivation ----
> Ioctls provide many of the operations necessary for device control. The typical
> driver supports a device specific set of operations accessible by the ioctl
> system call and specified by the command argument. SELinux provides per
> operation access control to many system operations e.g. chown, kill, setuid,
> ipc_lock, etc. Ioclts on the other hand are granted on a per file descriptor
> basis using the ioctl permission, meaning that the set of operations provided
> by the driver are granted on an all-or-nothing basis. In some cases this may be
> acceptable, but often the same driver provides a large and diverse set of
> operations such as benign and necessary functionality as well as dangerous
> capabilities, or access to system information that should be restricted.
>
> Example 1 - restrict access to privacy sensitive information:
> Applications with the socket permission require the ioctl permission to perform
> control operations. However access to this ioctl also provides the application
> with access to information such as the MAC address, Wifi ESSID, layer two
> encryption information etc that can be used to uniquely identify the user and
> compromise privacy.
>
> Example 2 - reduce kernel attack surface by eliminating access to unnecessary
> operations:
> Of the 34 available operations provided by a graphics driver, only 11 are used
> on the device under test. Media drivers are frequently under development and are
> notorious for containing bugs. By limiting application access to the subset of
> commands needed, many bugs can be made unreachable without sacrificing
> functionality. Real world example, we recently discovered three bugs within a
> driver that would be unreachable by restricting access to unused ioctl commands.
>
> ---- Design constraints ----
> Policy: Support existing policy, targeted whitelisting. The ioctls commands used
> on a system may not be completely known to a sysadmin/policywriter. It is not
> reasonable to enforce that all needed commands be known in order to use this
> feature in a targeted manner. Existing policy using the ioctl permission will
> continue to work as-is. Policy targeting a specific source/target/class will
> only be enforced on that particular source/target/class. E.g. continue to allow
> init to access all ioctls provided by a driver, but only allow the browser to
> access a subset.
>
> Performance: Many ioctl calls are performance sensitive, and some ioctls have a
> large command set (e.g. sockets support hundreds of commands). Execution time on
> a filtered ioctl should be similar to execution time on an unfiltered one and
> not related to the number of commands being filtered. Performance numbers will
> be included in a separate document.
>
> Command space: The ioctl command is a 32 bit number comprised of four fields,
> number - sequence number of the command. 8 bits
> type - magic number assigned to the driver. 8 bits
> size - size of the user data involved. typically 14 bits (arch dependent)
> direction - The direction of data transfer. typically 2 bits (arch dependent)
> The command is uniquely identified by the type and number, size and direction
> are considered to be arguments and are not considered for SELinux whitelisting.
>
> ---- Policy format ----
> allow <source> <target>:<class> { 0x8910-0x8926 0x892A-0x8935 }
> auditallow <source> <target>:<class> 0x892A
>
> ---- Architecture ----
> policy: Ioctl operations include allow, auditallow and dontaudit permissions.
>
> binary policy/avtab: Like AVC permissions, ioctl operations permissions are
> represented by set/cleared bits. Each ioctl operation avtab entry represents a
> single ioctl command type (using “type” as defined above in the “command space”
> section) and includes permissions for the 256 operations within that type. This
> architecture was designed after observing the sets of operations used by each
> source/target/class across several devices. Most significantly, a
> source/target/class normally uses just one ioctl type and only rarely uses more
> than one type. For example in a prototyping device, 90 source/target/class
> sets use just 1 type, and 2 source/target/class sets use 2 types, no
> source/target/class used more than 2 types. Including permissions for all 256
> bits creates a deterministic lookup time within a type because the command being
> checked is mapped directly to a permission. Additionally, very complex policies
> within a type map to the same size as simple policies e.g. ioctl commands
> often switch between get and set operations where a policy writer could
> conceivably want to give an application the ability to access all the get
> operations and not set operations i.e. a policy allowing every other ioctl
> number within a type. Optimizing for complexity within types was done to 1) make
> the solution general 2) make the solution fast for both complex simple policies
> and 3) ioctls that have a small operation set (many use just one) very likely
> need all operations allowed and thus would not use this feature but would
> continue to rely on the all-or-nothing ioctl permission for that particular
> ioctl.
>
> AVC: A pointer to an operations structure has been added to the avc_node such
> that general avc_lookup also returns the allowed ioctl operations. The structure
> flags the ioctl types that are allowed and only looks up the permissions within
> that type from the avtab when they are first used. After the initial lookup of a
> type it is added to a list of types that are allowed for the source/target/class
> being retrieved. As mentioned earlier, this list is typically one item long, but
> can be 2, 3, etc, if necessary. The list lookup is linear with the length of
> the list so it might be a good idea to enforce a restriction on the number of
> types allowed in the policy compiler (if there exists a good example of a
> source/target/class that uses more than a handful of types, none of the
> prototyping devices tested would benefit from this).
>
> ---- example policy ----
> allow system_server ion_device:chr_file { 0x4901 0x4905-0x4906 };
> allow keystore tee_device:chr_file { 0x9707-0x9708 0x970a 0x970f };
> allow netmgrd netmgrd:udp_socket { 0x8913 0x8933 0x89f2-0x89f3 0x89f5
>         0x89f8-0x89f9 };
> allow surfaceflinger graphics_device:chr_file { 0x4600 0x4602 0x4611
> 0x6d87-0x6d89 0x6da0 0x6da2 0x6da4 0x6da6 };
> allow untrusted_app gpu_device:chr_file { 0x902 0x907 0x913 0x915 0x933-0x935
>         0x938-0x939 0x93b 0x93d };
> allow shell owntty_device:chr_file { 0x5401 0x5403 0x540f 0x5413 };
> allow system_server alarm_device:chr_file { 0x6101 0x6122 0x6132 0x6134 };
> allow mediaserver camera_device:chr_file { 0x5600 0x5602 0x5604-0x5605 0x5608
> 0x561c 0x564a 0x564d 0x5659-0x565b 0x5660 0x7c00-0x7c01 };
> allow domain binder_device:chr_file { 0x6201 0x6205 0x6207-0x6209 };
> allow system_server input_device:chr_file { 0x4501-0x4503 0x4506-0x4509 0x4518
> 0x451b 0x4521-0x4523 0x4525 0x4531 0x4535 0x456f-0x4570 0x4575-0x4576
>         0x4579-0x457a 0x4591 0x45a0 };
> allow mediaserver audio_device:chr_file { 0x4101 0x4103 0x4111 0x4113 0x4123
>         0x4140 0x4150 0x5501 0x5510-0x5513 0x610f 0x6167 0x6169 0x6171 0x6173
>         0x6175 0x617b-0x617c };
> allow camera camera_device:chr_file { 0x5659-0x565b 0x56c0-0x56c8 0x56de 0x56e0
>         0x7c00-0x7c01 };
> allow platform_app gpu_device:chr_file { 0x902 0x907 0x913-0x915 0x917 0x921
>         0x933-0x935 0x938-0x939 0x93b 0x93d };
> allow surfaceflinger gpu_device:chr_file { 0x902 0x907 0x913 0x915 0x921
>         0x933-0x935 0x938-0x939 0x93b 0x93d };
> allow sensors sensors:socket { 0xc302 0xc304 };
> allow system_server gpu_device:chr_file { 0x902 0x907 0x913-0x915 0x917 0x921
>         0x933-0x935 0x938-0x939 0x93b 0x93d };
> allow domain ashmem_device:chr_file { 0x7701 0x7703-0x7705 };
> allow bootanim gpu_device:chr_file { 0x902 0x907 0x913-0x915 0x917 0x921
>         0x933-0x935 0x938-0x939 0x93b 0x93d };
>
> Jeff Vander Stoep (2):
>   security: lsm_audit: add ioctl specific auditing
>   SELinux: per-command whitelisting of ioctls
>
>  include/linux/lsm_audit.h           |   7 +
>  security/lsm_audit.c                |  15 ++
>  security/selinux/avc.c              | 428 ++++++++++++++++++++++++++++++++++--
>  security/selinux/hooks.c            |  40 +++-
>  security/selinux/include/avc.h      |   5 +
>  security/selinux/include/security.h |  34 ++-
>  security/selinux/ss/avtab.c         |  91 ++++++--
>  security/selinux/ss/avtab.h         |  25 ++-
>  security/selinux/ss/conditional.c   |  32 ++-
>  security/selinux/ss/conditional.h   |   6 +-
>  security/selinux/ss/policydb.c      |   5 +
>  security/selinux/ss/services.c      | 202 +++++++++++++++--
>  security/selinux/ss/services.h      |   6 +
>  13 files changed, 832 insertions(+), 64 deletions(-)
>
> --
> 2.2.0.rc0.207.ga3a616c
>

[-- Attachment #2: individualIoctlCalltime.png --]
[-- Type: image/png, Size: 34885 bytes --]

[-- Attachment #3: BoottimeComparison.png --]
[-- Type: image/png, Size: 39319 bytes --]

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

[Jeffrey Vander Stoep]

Supporting policy compiler patch is now up on the SELinux mailing list.

http://marc.info/?l=selinux&m=142973617003169&w=2

Note this is for monolithic kernel policies. Support for modules and
CIL still needs to be added.



On Thu, Apr 9, 2015 at 4:03 PM, Jeffrey Vander Stoep <jeffv@google.com> wrote:
> Approximately 150000 ioctls are executed during the ~40 seconds of
> boot time. Adding policy such that all of these ioctls are run through
> the ioctl whitelisting code has no noticeable impact on boot time. See
> attached graph of boot time comparison.
>
> Measuring the impact of an individual call is a bit showed a 20-30 ns
> increase in the ioctl call time on a very fast ioctl (SIOCGIFHWADDR is
> basically a 5 byte memcpy). Benchmarking was done on a Nexus 9, 64-bit
> NVIDIA Tegra K1 Dual Denver @ 2.3GHz. See attached graph of individual
> ioctl call time comparison.
>
> On Thu, Apr 9, 2015 at 2:48 PM, Jeff Vander Stoep <jeffv@google.com> wrote:
>> ---- motivation ----
>> Ioctls provide many of the operations necessary for device control. The typical
>> driver supports a device specific set of operations accessible by the ioctl
>> system call and specified by the command argument. SELinux provides per
>> operation access control to many system operations e.g. chown, kill, setuid,
>> ipc_lock, etc. Ioclts on the other hand are granted on a per file descriptor
>> basis using the ioctl permission, meaning that the set of operations provided
>> by the driver are granted on an all-or-nothing basis. In some cases this may be
>> acceptable, but often the same driver provides a large and diverse set of
>> operations such as benign and necessary functionality as well as dangerous
>> capabilities, or access to system information that should be restricted.
>>
>> Example 1 - restrict access to privacy sensitive information:
>> Applications with the socket permission require the ioctl permission to perform
>> control operations. However access to this ioctl also provides the application
>> with access to information such as the MAC address, Wifi ESSID, layer two
>> encryption information etc that can be used to uniquely identify the user and
>> compromise privacy.
>>
>> Example 2 - reduce kernel attack surface by eliminating access to unnecessary
>> operations:
>> Of the 34 available operations provided by a graphics driver, only 11 are used
>> on the device under test. Media drivers are frequently under development and are
>> notorious for containing bugs. By limiting application access to the subset of
>> commands needed, many bugs can be made unreachable without sacrificing
>> functionality. Real world example, we recently discovered three bugs within a
>> driver that would be unreachable by restricting access to unused ioctl commands.
>>
>> ---- Design constraints ----
>> Policy: Support existing policy, targeted whitelisting. The ioctls commands used
>> on a system may not be completely known to a sysadmin/policywriter. It is not
>> reasonable to enforce that all needed commands be known in order to use this
>> feature in a targeted manner. Existing policy using the ioctl permission will
>> continue to work as-is. Policy targeting a specific source/target/class will
>> only be enforced on that particular source/target/class. E.g. continue to allow
>> init to access all ioctls provided by a driver, but only allow the browser to
>> access a subset.
>>
>> Performance: Many ioctl calls are performance sensitive, and some ioctls have a
>> large command set (e.g. sockets support hundreds of commands). Execution time on
>> a filtered ioctl should be similar to execution time on an unfiltered one and
>> not related to the number of commands being filtered. Performance numbers will
>> be included in a separate document.
>>
>> Command space: The ioctl command is a 32 bit number comprised of four fields,
>> number - sequence number of the command. 8 bits
>> type - magic number assigned to the driver. 8 bits
>> size - size of the user data involved. typically 14 bits (arch dependent)
>> direction - The direction of data transfer. typically 2 bits (arch dependent)
>> The command is uniquely identified by the type and number, size and direction
>> are considered to be arguments and are not considered for SELinux whitelisting.
>>
>> ---- Policy format ----
>> allow <source> <target>:<class> { 0x8910-0x8926 0x892A-0x8935 }
>> auditallow <source> <target>:<class> 0x892A
>>
>> ---- Architecture ----
>> policy: Ioctl operations include allow, auditallow and dontaudit permissions.
>>
>> binary policy/avtab: Like AVC permissions, ioctl operations permissions are
>> represented by set/cleared bits. Each ioctl operation avtab entry represents a
>> single ioctl command type (using “type” as defined above in the “command space”
>> section) and includes permissions for the 256 operations within that type. This
>> architecture was designed after observing the sets of operations used by each
>> source/target/class across several devices. Most significantly, a
>> source/target/class normally uses just one ioctl type and only rarely uses more
>> than one type. For example in a prototyping device, 90 source/target/class
>> sets use just 1 type, and 2 source/target/class sets use 2 types, no
>> source/target/class used more than 2 types. Including permissions for all 256
>> bits creates a deterministic lookup time within a type because the command being
>> checked is mapped directly to a permission. Additionally, very complex policies
>> within a type map to the same size as simple policies e.g. ioctl commands
>> often switch between get and set operations where a policy writer could
>> conceivably want to give an application the ability to access all the get
>> operations and not set operations i.e. a policy allowing every other ioctl
>> number within a type. Optimizing for complexity within types was done to 1) make
>> the solution general 2) make the solution fast for both complex simple policies
>> and 3) ioctls that have a small operation set (many use just one) very likely
>> need all operations allowed and thus would not use this feature but would
>> continue to rely on the all-or-nothing ioctl permission for that particular
>> ioctl.
>>
>> AVC: A pointer to an operations structure has been added to the avc_node such
>> that general avc_lookup also returns the allowed ioctl operations. The structure
>> flags the ioctl types that are allowed and only looks up the permissions within
>> that type from the avtab when they are first used. After the initial lookup of a
>> type it is added to a list of types that are allowed for the source/target/class
>> being retrieved. As mentioned earlier, this list is typically one item long, but
>> can be 2, 3, etc, if necessary. The list lookup is linear with the length of
>> the list so it might be a good idea to enforce a restriction on the number of
>> types allowed in the policy compiler (if there exists a good example of a
>> source/target/class that uses more than a handful of types, none of the
>> prototyping devices tested would benefit from this).
>>
>> ---- example policy ----
>> allow system_server ion_device:chr_file { 0x4901 0x4905-0x4906 };
>> allow keystore tee_device:chr_file { 0x9707-0x9708 0x970a 0x970f };
>> allow netmgrd netmgrd:udp_socket { 0x8913 0x8933 0x89f2-0x89f3 0x89f5
>>         0x89f8-0x89f9 };
>> allow surfaceflinger graphics_device:chr_file { 0x4600 0x4602 0x4611
>> 0x6d87-0x6d89 0x6da0 0x6da2 0x6da4 0x6da6 };
>> allow untrusted_app gpu_device:chr_file { 0x902 0x907 0x913 0x915 0x933-0x935
>>         0x938-0x939 0x93b 0x93d };
>> allow shell owntty_device:chr_file { 0x5401 0x5403 0x540f 0x5413 };
>> allow system_server alarm_device:chr_file { 0x6101 0x6122 0x6132 0x6134 };
>> allow mediaserver camera_device:chr_file { 0x5600 0x5602 0x5604-0x5605 0x5608
>> 0x561c 0x564a 0x564d 0x5659-0x565b 0x5660 0x7c00-0x7c01 };
>> allow domain binder_device:chr_file { 0x6201 0x6205 0x6207-0x6209 };
>> allow system_server input_device:chr_file { 0x4501-0x4503 0x4506-0x4509 0x4518
>> 0x451b 0x4521-0x4523 0x4525 0x4531 0x4535 0x456f-0x4570 0x4575-0x4576
>>         0x4579-0x457a 0x4591 0x45a0 };
>> allow mediaserver audio_device:chr_file { 0x4101 0x4103 0x4111 0x4113 0x4123
>>         0x4140 0x4150 0x5501 0x5510-0x5513 0x610f 0x6167 0x6169 0x6171 0x6173
>>         0x6175 0x617b-0x617c };
>> allow camera camera_device:chr_file { 0x5659-0x565b 0x56c0-0x56c8 0x56de 0x56e0
>>         0x7c00-0x7c01 };
>> allow platform_app gpu_device:chr_file { 0x902 0x907 0x913-0x915 0x917 0x921
>>         0x933-0x935 0x938-0x939 0x93b 0x93d };
>> allow surfaceflinger gpu_device:chr_file { 0x902 0x907 0x913 0x915 0x921
>>         0x933-0x935 0x938-0x939 0x93b 0x93d };
>> allow sensors sensors:socket { 0xc302 0xc304 };
>> allow system_server gpu_device:chr_file { 0x902 0x907 0x913-0x915 0x917 0x921
>>         0x933-0x935 0x938-0x939 0x93b 0x93d };
>> allow domain ashmem_device:chr_file { 0x7701 0x7703-0x7705 };
>> allow bootanim gpu_device:chr_file { 0x902 0x907 0x913-0x915 0x917 0x921
>>         0x933-0x935 0x938-0x939 0x93b 0x93d };
>>
>> Jeff Vander Stoep (2):
>>   security: lsm_audit: add ioctl specific auditing
>>   SELinux: per-command whitelisting of ioctls
>>
>>  include/linux/lsm_audit.h           |   7 +
>>  security/lsm_audit.c                |  15 ++
>>  security/selinux/avc.c              | 428 ++++++++++++++++++++++++++++++++++--
>>  security/selinux/hooks.c            |  40 +++-
>>  security/selinux/include/avc.h      |   5 +
>>  security/selinux/include/security.h |  34 ++-
>>  security/selinux/ss/avtab.c         |  91 ++++++--
>>  security/selinux/ss/avtab.h         |  25 ++-
>>  security/selinux/ss/conditional.c   |  32 ++-
>>  security/selinux/ss/conditional.h   |   6 +-
>>  security/selinux/ss/policydb.c      |   5 +
>>  security/selinux/ss/services.c      | 202 +++++++++++++++--
>>  security/selinux/ss/services.h      |   6 +
>>  13 files changed, 832 insertions(+), 64 deletions(-)
>>
>> --
>> 2.2.0.rc0.207.ga3a616c
>>

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

[Paul Moore]

On Thu, Apr 9, 2015 at 5:48 PM, Jeff Vander Stoep <jeffv@google.com> wrote:
> ---- motivation ----
> Ioctls provide many of the operations necessary for device control. The typical
> driver supports a device specific set of operations accessible by the ioctl
> system call and specified by the command argument. SELinux provides per
> operation access control to many system operations e.g. chown, kill, setuid,
> ipc_lock, etc. Ioclts on the other hand are granted on a per file descriptor
> basis using the ioctl permission, meaning that the set of operations provided
> by the driver are granted on an all-or-nothing basis. In some cases this may be
> acceptable, but often the same driver provides a large and diverse set of
> operations such as benign and necessary functionality as well as dangerous
> capabilities, or access to system information that should be restricted.

I haven't had a chance to review your patches yet, but thank you for
posting them and responding to the early feedback.  It may take me a
few weeks to review these patches, but they are in my review queue.

-Paul

-- 
paul moore
www.paul-moore.com

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

[Jeffrey Vander Stoep]

Thanks Paul. I wasn't anticipating much feedback until the userspace
changes were submitted. Just got those in on Wednesday. Let me know if
there is anything else I can provide.

On Thu, Apr 23, 2015 at 3:28 PM, Paul Moore <paul@paul-moore.com> wrote:
> On Thu, Apr 9, 2015 at 5:48 PM, Jeff Vander Stoep <jeffv@google.com> wrote:
>> ---- motivation ----
>> Ioctls provide many of the operations necessary for device control. The typical
>> driver supports a device specific set of operations accessible by the ioctl
>> system call and specified by the command argument. SELinux provides per
>> operation access control to many system operations e.g. chown, kill, setuid,
>> ipc_lock, etc. Ioclts on the other hand are granted on a per file descriptor
>> basis using the ioctl permission, meaning that the set of operations provided
>> by the driver are granted on an all-or-nothing basis. In some cases this may be
>> acceptable, but often the same driver provides a large and diverse set of
>> operations such as benign and necessary functionality as well as dangerous
>> capabilities, or access to system information that should be restricted.
>
> I haven't had a chance to review your patches yet, but thank you for
> posting them and responding to the early feedback.  It may take me a
> few weeks to review these patches, but they are in my review queue.
>
> -Paul
>
> --
> paul moore
> www.paul-moore.com

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

[Paul Moore]

First off, my apologies for such a long delay in providing feedback.  The 
delay wasn't due to any fault of yours, rather just a backlog on my todo list.

Comments below ...

On Thursday, April 09, 2015 02:48:50 PM Jeff Vander Stoep wrote:
> ---- motivation ----
> Ioctls provide many of the operations necessary for device control. The
> typical driver supports a device specific set of operations accessible by
> the ioctl system call and specified by the command argument. SELinux
> provides per operation access control to many system operations e.g. chown,
> kill, setuid, ipc_lock, etc. Ioclts on the other hand are granted on a per
> file descriptor basis using the ioctl permission, meaning that the set of
> operations provided by the driver are granted on an all-or-nothing basis.

...

> ---- Design constraints ----
> Policy: Support existing policy, targeted whitelisting. The ioctls commands
> used on a system may not be completely known to a sysadmin/policywriter. It
> is not reasonable to enforce that all needed commands be known in order to
> use this feature in a targeted manner. Existing policy using the ioctl
> permission will continue to work as-is. Policy targeting a specific
> source/target/class will only be enforced on that particular
> source/target/class. E.g. continue to allow init to access all ioctls
> provided by a driver, but only allow the browser to access a subset.
> 
> Performance: Many ioctl calls are performance sensitive, and some ioctls
> have a large command set (e.g. sockets support hundreds of commands).
> Execution time on a filtered ioctl should be similar to execution time on
> an unfiltered one and not related to the number of commands being filtered.
> Performance numbers will be included in a separate document.
> 
> Command space: The ioctl command is a 32 bit number comprised of four
> fields, number - sequence number of the command. 8 bits
> type - magic number assigned to the driver. 8 bits
> size - size of the user data involved. typically 14 bits (arch dependent)
> direction - The direction of data transfer. typically 2 bits (arch
> dependent) The command is uniquely identified by the type and number, size
> and direction are considered to be arguments and are not considered for
> SELinux whitelisting.
> 
> ---- Policy format ----
> allow <source> <target>:<class> { 0x8910-0x8926 0x892A-0x8935 }
> auditallow <source> <target>:<class> 0x892A

I agree with only specifying the lower 16 bits (command,type) when specifying 
the individual ioctls, I even like the '-' shortcut, but I'm a little 
concerned about specifying a number directly in the permission field without 
any sort of qualifier.  Specifically I'm worried that it hurts the readability 
of the policy and could pose problems with future work.

I'd be much happier if we could add some sort of syntax which would qualify 
the numbers as ioctls, for example:

  allow <source> <target>:<class> { ioctl(0x8910-0x8926) ioctl(0x892A) }

The above is just an example, if you have a better idea for the syntax I'm 
open to suggestions.

> ---- Architecture ----
> policy: Ioctl operations include allow, auditallow and dontaudit
> permissions.
> 
> binary policy/avtab: Like AVC permissions, ioctl operations permissions are
> represented by set/cleared bits. Each ioctl operation avtab entry represents
> a single ioctl command type (using “type” as defined above in the “command
> space” section) and includes permissions for the 256 operations within that
> type ...

I like the work you did expanding the policy/AVC, especially since it is 
rather generic.  Sure, there are some ioctl specific quirks, e.g. the 
type/number handling, but in general, the "operations" concept could be used 
for any number of things where the existing 32 choices are too limiting.  The 
netfilter command handling is one such example.

While what you've submitted here is just fine for ioctl handling, I'd like to 
see us tweak things a bit, and move around some of the ioctl specific quirks, 
so that we can leverage some of the underlying avtab and avc code for future 
expansion (if/when we get there).

The unfortunate side effect of doing your job too well is that I ask you to do 
more ;)

> AVC: A pointer to an operations structure has been added to the avc_node
> such that general avc_lookup also returns the allowed ioctl operations.

More on this when I send my comments on the patches themselves, but I found 
the term "operations" a bit ... misleading?  Overly generic?  Odd considering 
my desire to make this more generic, I know.

How about "extops" or something similar?  I would much prefer struct/type 
names and function names that help indicate that it relates to the extended 
permissions/operations and not the original 32 permissions; "operations" 
doesn't in my mind, but "extops" or something similar helps.  Once again, if 
you have a better idea I'm open to suggestions.

Nitpicky, I know ...

> The structure flags the ioctl types that are allowed and only looks up the
> permissions within that type from the avtab when they are first used. After
> the initial lookup of a type it is added to a list of types that are
> allowed for the source/target/class being retrieved.

More on this in the individual patch comments.  I have no argument with your 
logic from an ioctl perspective, I'd just like to see some slight changes so 
we can recycle your work later.

-- 
paul moore
www.paul-moore.com

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

[Jeffrey Vander Stoep]

[-- Attachment #1: Type: text/plain, Size: 5973 bytes --]

Thank you Paul.

I'll put together some updated patches.


On Wed, May 20, 2015 at 1:04 PM, Paul Moore <paul@paul-moore.com> wrote:

> First off, my apologies for such a long delay in providing feedback.  The
> delay wasn't due to any fault of yours, rather just a backlog on my todo
> list.
>
> Comments below ...
>
> On Thursday, April 09, 2015 02:48:50 PM Jeff Vander Stoep wrote:
> > ---- motivation ----
> > Ioctls provide many of the operations necessary for device control. The
> > typical driver supports a device specific set of operations accessible by
> > the ioctl system call and specified by the command argument. SELinux
> > provides per operation access control to many system operations e.g.
> chown,
> > kill, setuid, ipc_lock, etc. Ioclts on the other hand are granted on a
> per
> > file descriptor basis using the ioctl permission, meaning that the set of
> > operations provided by the driver are granted on an all-or-nothing basis.
>
> ...
>
> > ---- Design constraints ----
> > Policy: Support existing policy, targeted whitelisting. The ioctls
> commands
> > used on a system may not be completely known to a sysadmin/policywriter.
> It
> > is not reasonable to enforce that all needed commands be known in order
> to
> > use this feature in a targeted manner. Existing policy using the ioctl
> > permission will continue to work as-is. Policy targeting a specific
> > source/target/class will only be enforced on that particular
> > source/target/class. E.g. continue to allow init to access all ioctls
> > provided by a driver, but only allow the browser to access a subset.
> >
> > Performance: Many ioctl calls are performance sensitive, and some ioctls
> > have a large command set (e.g. sockets support hundreds of commands).
> > Execution time on a filtered ioctl should be similar to execution time on
> > an unfiltered one and not related to the number of commands being
> filtered.
> > Performance numbers will be included in a separate document.
> >
> > Command space: The ioctl command is a 32 bit number comprised of four
> > fields, number - sequence number of the command. 8 bits
> > type - magic number assigned to the driver. 8 bits
> > size - size of the user data involved. typically 14 bits (arch dependent)
> > direction - The direction of data transfer. typically 2 bits (arch
> > dependent) The command is uniquely identified by the type and number,
> size
> > and direction are considered to be arguments and are not considered for
> > SELinux whitelisting.
> >
> > ---- Policy format ----
> > allow <source> <target>:<class> { 0x8910-0x8926 0x892A-0x8935 }
> > auditallow <source> <target>:<class> 0x892A
>
> I agree with only specifying the lower 16 bits (command,type) when
> specifying
> the individual ioctls, I even like the '-' shortcut, but I'm a little
> concerned about specifying a number directly in the permission field
> without
> any sort of qualifier.  Specifically I'm worried that it hurts the
> readability
> of the policy and could pose problems with future work.
>
> I'd be much happier if we could add some sort of syntax which would qualify
> the numbers as ioctls, for example:
>
>   allow <source> <target>:<class> { ioctl(0x8910-0x8926) ioctl(0x892A) }
>
> The above is just an example, if you have a better idea for the syntax I'm
> open to suggestions.
>
> > ---- Architecture ----
> > policy: Ioctl operations include allow, auditallow and dontaudit
> > permissions.
> >
> > binary policy/avtab: Like AVC permissions, ioctl operations permissions
> are
> > represented by set/cleared bits. Each ioctl operation avtab entry
> represents
> > a single ioctl command type (using “type” as defined above in the
> “command
> > space” section) and includes permissions for the 256 operations within
> that
> > type ...
>
> I like the work you did expanding the policy/AVC, especially since it is
> rather generic.  Sure, there are some ioctl specific quirks, e.g. the
> type/number handling, but in general, the "operations" concept could be
> used
> for any number of things where the existing 32 choices are too limiting.
> The
> netfilter command handling is one such example.
>
> While what you've submitted here is just fine for ioctl handling, I'd like
> to
> see us tweak things a bit, and move around some of the ioctl specific
> quirks,
> so that we can leverage some of the underlying avtab and avc code for
> future
> expansion (if/when we get there).
>
> The unfortunate side effect of doing your job too well is that I ask you
> to do
> more ;)
>
> > AVC: A pointer to an operations structure has been added to the avc_node
> > such that general avc_lookup also returns the allowed ioctl operations.
>
> More on this when I send my comments on the patches themselves, but I found
> the term "operations" a bit ... misleading?  Overly generic?  Odd
> considering
> my desire to make this more generic, I know.
>
> How about "extops" or something similar?  I would much prefer struct/type
> names and function names that help indicate that it relates to the extended
> permissions/operations and not the original 32 permissions; "operations"
> doesn't in my mind, but "extops" or something similar helps.  Once again,
> if
> you have a better idea I'm open to suggestions.
>
> Nitpicky, I know ...
>
> > The structure flags the ioctl types that are allowed and only looks up
> the
> > permissions within that type from the avtab when they are first used.
> After
> > the initial lookup of a type it is added to a list of types that are
> > allowed for the source/target/class being retrieved.
>
> More on this in the individual patch comments.  I have no argument with
> your
> logic from an ioctl perspective, I'd just like to see some slight changes
> so
> we can recycle your work later.
>
> --
> paul moore
> www.paul-moore.com
>
>

[-- Attachment #2: Type: text/html, Size: 7034 bytes --]

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

[William Roberts]

[-- Attachment #1: Type: text/plain, Size: 6330 bytes --]

On Wed, May 20, 2015 at 1:04 PM, Paul Moore <paul@paul-moore.com> wrote:

> First off, my apologies for such a long delay in providing feedback.  The
> delay wasn't due to any fault of yours, rather just a backlog on my todo
> list.
>
> Comments below ...
>
> On Thursday, April 09, 2015 02:48:50 PM Jeff Vander Stoep wrote:
> > ---- motivation ----
> > Ioctls provide many of the operations necessary for device control. The
> > typical driver supports a device specific set of operations accessible by
> > the ioctl system call and specified by the command argument. SELinux
> > provides per operation access control to many system operations e.g.
> chown,
> > kill, setuid, ipc_lock, etc. Ioclts on the other hand are granted on a
> per
> > file descriptor basis using the ioctl permission, meaning that the set of
> > operations provided by the driver are granted on an all-or-nothing basis.
>
> ...
>
> > ---- Design constraints ----
> > Policy: Support existing policy, targeted whitelisting. The ioctls
> commands
> > used on a system may not be completely known to a sysadmin/policywriter.
> It
> > is not reasonable to enforce that all needed commands be known in order
> to
> > use this feature in a targeted manner. Existing policy using the ioctl
> > permission will continue to work as-is. Policy targeting a specific
> > source/target/class will only be enforced on that particular
> > source/target/class. E.g. continue to allow init to access all ioctls
> > provided by a driver, but only allow the browser to access a subset.
> >
> > Performance: Many ioctl calls are performance sensitive, and some ioctls
> > have a large command set (e.g. sockets support hundreds of commands).
> > Execution time on a filtered ioctl should be similar to execution time on
> > an unfiltered one and not related to the number of commands being
> filtered.
> > Performance numbers will be included in a separate document.
> >
> > Command space: The ioctl command is a 32 bit number comprised of four
> > fields, number - sequence number of the command. 8 bits
> > type - magic number assigned to the driver. 8 bits
> > size - size of the user data involved. typically 14 bits (arch dependent)
> > direction - The direction of data transfer. typically 2 bits (arch
> > dependent) The command is uniquely identified by the type and number,
> size
> > and direction are considered to be arguments and are not considered for
> > SELinux whitelisting.
> >
> > ---- Policy format ----
> > allow <source> <target>:<class> { 0x8910-0x8926 0x892A-0x8935 }
> > auditallow <source> <target>:<class> 0x892A
>
> I agree with only specifying the lower 16 bits (command,type) when
> specifying
> the individual ioctls, I even like the '-' shortcut, but I'm a little
> concerned about specifying a number directly in the permission field
> without
> any sort of qualifier.  Specifically I'm worried that it hurts the
> readability
> of the policy and could pose problems with future work.
>
> I'd be much happier if we could add some sort of syntax which would qualify
> the numbers as ioctls, for example:
>
>   allow <source> <target>:<class> { ioctl(0x8910-0x8926) ioctl(0x892A) }
>
>
If you want additional syntax couldn't we move that burden to m4 rather
then making it a part of the compiler core?


> The above is just an example, if you have a better idea for the syntax I'm
> open to suggestions.
>
> > ---- Architecture ----
> > policy: Ioctl operations include allow, auditallow and dontaudit
> > permissions.
> >
> > binary policy/avtab: Like AVC permissions, ioctl operations permissions
> are
> > represented by set/cleared bits. Each ioctl operation avtab entry
> represents
> > a single ioctl command type (using “type” as defined above in the
> “command
> > space” section) and includes permissions for the 256 operations within
> that
> > type ...
>
> I like the work you did expanding the policy/AVC, especially since it is
> rather generic.  Sure, there are some ioctl specific quirks, e.g. the
> type/number handling, but in general, the "operations" concept could be
> used
> for any number of things where the existing 32 choices are too limiting.
> The
> netfilter command handling is one such example.
>
> While what you've submitted here is just fine for ioctl handling, I'd like
> to
> see us tweak things a bit, and move around some of the ioctl specific
> quirks,
> so that we can leverage some of the underlying avtab and avc code for
> future
> expansion (if/when we get there).
>
> The unfortunate side effect of doing your job too well is that I ask you
> to do
> more ;)
>
> > AVC: A pointer to an operations structure has been added to the avc_node
> > such that general avc_lookup also returns the allowed ioctl operations.
>
> More on this when I send my comments on the patches themselves, but I found
> the term "operations" a bit ... misleading?  Overly generic?  Odd
> considering
> my desire to make this more generic, I know.
>
> How about "extops" or something similar?  I would much prefer struct/type
> names and function names that help indicate that it relates to the extended
> permissions/operations and not the original 32 permissions; "operations"
> doesn't in my mind, but "extops" or something similar helps.  Once again,
> if
> you have a better idea I'm open to suggestions.
>
> Nitpicky, I know ...
>
> > The structure flags the ioctl types that are allowed and only looks up
> the
> > permissions within that type from the avtab when they are first used.
> After
> > the initial lookup of a type it is added to a list of types that are
> > allowed for the source/target/class being retrieved.
>
> More on this in the individual patch comments.  I have no argument with
> your
> logic from an ioctl perspective, I'd just like to see some slight changes
> so
> we can recycle your work later.
>
> --
> paul moore
> www.paul-moore.com
>
>
> _______________________________________________
> Selinux mailing list
> Selinux@tycho.nsa.gov
> To unsubscribe, send email to Selinux-leave@tycho.nsa.gov.
> To get help, send an email containing "help" to
> Selinux-request@tycho.nsa.gov.




-- 
Respectfully,

William C Roberts

[-- Attachment #2: Type: text/html, Size: 7597 bytes --]

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

[Joshua Brindle]

William Roberts wrote:
<snip>
>>> ---- Policy format ----
>>> allow<source>  <target>:<class>  { 0x8910-0x8926 0x892A-0x8935 }
>>> auditallow<source>  <target>:<class>  0x892A
>> I agree with only specifying the lower 16 bits (command,type) when
>> specifying
>> the individual ioctls, I even like the '-' shortcut, but I'm a little
>> concerned about specifying a number directly in the permission field
>> without
>> any sort of qualifier.  Specifically I'm worried that it hurts the
>> readability
>> of the policy and could pose problems with future work.
>>
>> I'd be much happier if we could add some sort of syntax which would qualify
>> the numbers as ioctls, for example:
>>
>>    allow<source>  <target>:<class>  { ioctl(0x8910-0x8926) ioctl(0x892A) }
>>
>>
> If you want additional syntax couldn't we move that burden to m4 rather
> then making it a part of the compiler core?
>

I haven't looked at the patches but the parser isn't going to be any 
more or less complex either way, so whatever looks better and is easier 
to generate is likely better.

These raw values really bother me, though. They make policy impossible 
to interpret if you do not have an intimate understanding of the 
drivers. Are these really permissions? It seems closer to Xen's labeling 
of memory addresses and pci devices.

For reference Xen policy labels various objects:
pirqcon 33 system_u:object_r:nicP_t
iomemcon 0xfebe0-0xfebff system_u:object_r:nicP_t
iomemcon 0xfebd9 system_u:object_r:nicP_t
ioportcon 0xecc0-0xecdf system_u:object_r:nicP_t
pcidevicecon 0xc800 system_u:object_r:nicP_t

And uses standard TE rules to allow access.

Why not label ioctls and have an ioctl object class?

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

[Jeffrey Vander Stoep]

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

After monitoring ioctl use across five different devices I think this
is a good approach as just 10-20 macros would be adequate for a
targeted policy and would provide a clearer explanation of the
permissions given.

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

[Paul Moore]

On Thu, May 21, 2015 at 12:17 AM, 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.

Great, thank you.

> 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
>
> After monitoring ioctl use across five different devices I think this
> is a good approach as just 10-20 macros would be adequate for a
> targeted policy and would provide a clearer explanation of the
> permissions given.

Agreed.  We can use m4 to provide both the ioctl names and sets if needed.

-- 
paul moore
www.paul-moore.com

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

[Stephen Smalley]

On 05/20/2015 08:39 PM, William Roberts wrote:
> 
> 
> On Wed, May 20, 2015 at 1:04 PM, Paul Moore <paul@paul-moore.com
> <mailto:paul@paul-moore.com>> wrote:
> 
>     First off, my apologies for such a long delay in providing
>     feedback.  The
>     delay wasn't due to any fault of yours, rather just a backlog on my
>     todo list.
> 
>     Comments below ...
> 
>     On Thursday, April 09, 2015 02:48:50 PM Jeff Vander Stoep wrote:
>     > ---- motivation ----
>     > Ioctls provide many of the operations necessary for device
>     control. The
>     > typical driver supports a device specific set of operations
>     accessible by
>     > the ioctl system call and specified by the command argument. SELinux
>     > provides per operation access control to many system operations
>     e.g. chown,
>     > kill, setuid, ipc_lock, etc. Ioclts on the other hand are granted
>     on a per
>     > file descriptor basis using the ioctl permission, meaning that the
>     set of
>     > operations provided by the driver are granted on an all-or-nothing
>     basis.
> 
>     ...
> 
>     > ---- Design constraints ----
>     > Policy: Support existing policy, targeted whitelisting. The ioctls
>     commands
>     > used on a system may not be completely known to a
>     sysadmin/policywriter. It
>     > is not reasonable to enforce that all needed commands be known in
>     order to
>     > use this feature in a targeted manner. Existing policy using the ioctl
>     > permission will continue to work as-is. Policy targeting a specific
>     > source/target/class will only be enforced on that particular
>     > source/target/class. E.g. continue to allow init to access all ioctls
>     > provided by a driver, but only allow the browser to access a subset.
>     >
>     > Performance: Many ioctl calls are performance sensitive, and some
>     ioctls
>     > have a large command set (e.g. sockets support hundreds of commands).
>     > Execution time on a filtered ioctl should be similar to execution
>     time on
>     > an unfiltered one and not related to the number of commands being
>     filtered.
>     > Performance numbers will be included in a separate document.
>     >
>     > Command space: The ioctl command is a 32 bit number comprised of four
>     > fields, number - sequence number of the command. 8 bits
>     > type - magic number assigned to the driver. 8 bits
>     > size - size of the user data involved. typically 14 bits (arch
>     dependent)
>     > direction - The direction of data transfer. typically 2 bits (arch
>     > dependent) The command is uniquely identified by the type and
>     number, size
>     > and direction are considered to be arguments and are not
>     considered for
>     > SELinux whitelisting.
>     >
>     > ---- Policy format ----
>     > allow <source> <target>:<class> { 0x8910-0x8926 0x892A-0x8935 }
>     > auditallow <source> <target>:<class> 0x892A
> 
>     I agree with only specifying the lower 16 bits (command,type) when
>     specifying
>     the individual ioctls, I even like the '-' shortcut, but I'm a little
>     concerned about specifying a number directly in the permission field
>     without
>     any sort of qualifier.  Specifically I'm worried that it hurts the
>     readability
>     of the policy and could pose problems with future work.
> 
>     I'd be much happier if we could add some sort of syntax which would
>     qualify
>     the numbers as ioctls, for example:
> 
>       allow <source> <target>:<class> { ioctl(0x8910-0x8926) ioctl(0x892A) }
> 
> 
> If you want additional syntax couldn't we move that burden to m4 rather
> then making it a part of the compiler core?

It has to be handled by checkpolicy and encoded in the kernel binary
policy as an additional field if we want to support other uses of these
operation structures for purposes other than just ioctl.

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

[Stephen Smalley]

On 05/20/2015 10:08 PM, Joshua Brindle wrote:
> William Roberts wrote:
> <snip>
>>>> ---- Policy format ----
>>>> allow<source>  <target>:<class>  { 0x8910-0x8926 0x892A-0x8935 }
>>>> auditallow<source>  <target>:<class>  0x892A
>>> I agree with only specifying the lower 16 bits (command,type) when
>>> specifying
>>> the individual ioctls, I even like the '-' shortcut, but I'm a little
>>> concerned about specifying a number directly in the permission field
>>> without
>>> any sort of qualifier.  Specifically I'm worried that it hurts the
>>> readability
>>> of the policy and could pose problems with future work.
>>>
>>> I'd be much happier if we could add some sort of syntax which would
>>> qualify
>>> the numbers as ioctls, for example:
>>>
>>>    allow<source>  <target>:<class>  { ioctl(0x8910-0x8926)
>>> ioctl(0x892A) }
>>>
>>>
>> If you want additional syntax couldn't we move that burden to m4 rather
>> then making it a part of the compiler core?
>>
> 
> I haven't looked at the patches but the parser isn't going to be any
> more or less complex either way, so whatever looks better and is easier
> to generate is likely better.
> 
> These raw values really bother me, though. They make policy impossible
> to interpret if you do not have an intimate understanding of the
> drivers. Are these really permissions? It seems closer to Xen's labeling
> of memory addresses and pci devices.
> 
> For reference Xen policy labels various objects:
> pirqcon 33 system_u:object_r:nicP_t
> iomemcon 0xfebe0-0xfebff system_u:object_r:nicP_t
> iomemcon 0xfebd9 system_u:object_r:nicP_t
> ioportcon 0xecc0-0xecdf system_u:object_r:nicP_t
> pcidevicecon 0xc800 system_u:object_r:nicP_t
> 
> And uses standard TE rules to allow access.
> 
> Why not label ioctls and have an ioctl object class?

An early version of the patches tried that, but I advised against it.
ioctl commands are not objects/resources; they are actions and therefore
map to something like permissions.  Further, the object in these checks
is the device node security context and class, which reflects the
underlying driver, e.g. the GPU driver, the binder driver, the block
driver for the /system partition, etc.  IIRC, the early version of the
patch had two checks, the normal ioctl check against the device node
security context and a new check against the security context derived
from the ioctl command value, but the pairwise checking doesn't
necessarily give you the same control or expressiveness.

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

[Joshua Brindle]

Paul Moore wrote:
> On Thu, May 21, 2015 at 12:17 AM, 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.
>
> Great, thank you.
>
>> 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
>>
>> After monitoring ioctl use across five different devices I think this
>> is a good approach as just 10-20 macros would be adequate for a
>> targeted policy and would provide a clearer explanation of the
>> permissions given.
>
> Agreed.  We can use m4 to provide both the ioctl names and sets if needed.

m4 is never the answer....

An attribute-like symbol that is maintained in the binary would make 
these visible in the loaded policy and therefore more 
understandable/analyzable.

It would also allow you to easily add them in specific devices in a more 
readable way. For example, if the Android base policy allowed gpu_ioc to 
surfaceflinger all a device specific variant would need to do is add its 
ioctls to the attribute.

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

[James Carter]

On 05/21/2015 08:34 AM, Stephen Smalley wrote:
> On 05/20/2015 08:39 PM, William Roberts wrote:
>>
>>
>> On Wed, May 20, 2015 at 1:04 PM, Paul Moore <paul@paul-moore.com
>> <mailto:paul@paul-moore.com>> wrote:
>>
>>      First off, my apologies for such a long delay in providing
>>      feedback.  The
>>      delay wasn't due to any fault of yours, rather just a backlog on my
>>      todo list.
>>
>>      Comments below ...
>>
>>      On Thursday, April 09, 2015 02:48:50 PM Jeff Vander Stoep wrote:
>>      > ---- motivation ----
>>      > Ioctls provide many of the operations necessary for device
>>      control. The
>>      > typical driver supports a device specific set of operations
>>      accessible by
>>      > the ioctl system call and specified by the command argument. SELinux
>>      > provides per operation access control to many system operations
>>      e.g. chown,
>>      > kill, setuid, ipc_lock, etc. Ioclts on the other hand are granted
>>      on a per
>>      > file descriptor basis using the ioctl permission, meaning that the
>>      set of
>>      > operations provided by the driver are granted on an all-or-nothing
>>      basis.
>>
>>      ...
>>
>>      > ---- Design constraints ----
>>      > Policy: Support existing policy, targeted whitelisting. The ioctls
>>      commands
>>      > used on a system may not be completely known to a
>>      sysadmin/policywriter. It
>>      > is not reasonable to enforce that all needed commands be known in
>>      order to
>>      > use this feature in a targeted manner. Existing policy using the ioctl
>>      > permission will continue to work as-is. Policy targeting a specific
>>      > source/target/class will only be enforced on that particular
>>      > source/target/class. E.g. continue to allow init to access all ioctls
>>      > provided by a driver, but only allow the browser to access a subset.
>>      >
>>      > Performance: Many ioctl calls are performance sensitive, and some
>>      ioctls
>>      > have a large command set (e.g. sockets support hundreds of commands).
>>      > Execution time on a filtered ioctl should be similar to execution
>>      time on
>>      > an unfiltered one and not related to the number of commands being
>>      filtered.
>>      > Performance numbers will be included in a separate document.
>>      >
>>      > Command space: The ioctl command is a 32 bit number comprised of four
>>      > fields, number - sequence number of the command. 8 bits
>>      > type - magic number assigned to the driver. 8 bits
>>      > size - size of the user data involved. typically 14 bits (arch
>>      dependent)
>>      > direction - The direction of data transfer. typically 2 bits (arch
>>      > dependent) The command is uniquely identified by the type and
>>      number, size
>>      > and direction are considered to be arguments and are not
>>      considered for
>>      > SELinux whitelisting.
>>      >
>>      > ---- Policy format ----
>>      > allow <source> <target>:<class> { 0x8910-0x8926 0x892A-0x8935 }
>>      > auditallow <source> <target>:<class> 0x892A
>>
>>      I agree with only specifying the lower 16 bits (command,type) when
>>      specifying
>>      the individual ioctls, I even like the '-' shortcut, but I'm a little
>>      concerned about specifying a number directly in the permission field
>>      without
>>      any sort of qualifier.  Specifically I'm worried that it hurts the
>>      readability
>>      of the policy and could pose problems with future work.
>>
>>      I'd be much happier if we could add some sort of syntax which would
>>      qualify
>>      the numbers as ioctls, for example:
>>
>>        allow <source> <target>:<class> { ioctl(0x8910-0x8926) ioctl(0x892A) }
>>
>>
>> If you want additional syntax couldn't we move that burden to m4 rather
>> then making it a part of the compiler core?
>
> It has to be handled by checkpolicy and encoded in the kernel binary
> policy as an additional field if we want to support other uses of these
> operation structures for purposes other than just ioctl.
>

Do you have something else in mind that might use the operation structures?


In CIL we will probably make the syntax something like this:

((allowop <source> <target> <class> <ioctl expression>)

So the above rule might look like:

(allowop <source> <target> <class> ((range 0x8910 0x8926) 0x892A))

>
> _______________________________________________
> Selinux mailing list
> Selinux@tycho.nsa.gov
> To unsubscribe, send email to Selinux-leave@tycho.nsa.gov.
> To get help, send an email containing "help" to Selinux-request@tycho.nsa.gov.
>
>


-- 
James Carter <jwcart2@tycho.nsa.gov>
National Security Agency

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

[Stephen Smalley]

On 05/21/2015 09:43 AM, James Carter wrote:
> On 05/21/2015 08:34 AM, Stephen Smalley wrote:
>> On 05/20/2015 08:39 PM, William Roberts wrote:
>>>
>>>
>>> On Wed, May 20, 2015 at 1:04 PM, Paul Moore <paul@paul-moore.com
>>> <mailto:paul@paul-moore.com>> wrote:
>>>
>>>      First off, my apologies for such a long delay in providing
>>>      feedback.  The
>>>      delay wasn't due to any fault of yours, rather just a backlog on my
>>>      todo list.
>>>
>>>      Comments below ...
>>>
>>>      On Thursday, April 09, 2015 02:48:50 PM Jeff Vander Stoep wrote:
>>>      > ---- motivation ----
>>>      > Ioctls provide many of the operations necessary for device
>>>      control. The
>>>      > typical driver supports a device specific set of operations
>>>      accessible by
>>>      > the ioctl system call and specified by the command argument.
>>> SELinux
>>>      > provides per operation access control to many system operations
>>>      e.g. chown,
>>>      > kill, setuid, ipc_lock, etc. Ioclts on the other hand are granted
>>>      on a per
>>>      > file descriptor basis using the ioctl permission, meaning that
>>> the
>>>      set of
>>>      > operations provided by the driver are granted on an
>>> all-or-nothing
>>>      basis.
>>>
>>>      ...
>>>
>>>      > ---- Design constraints ----
>>>      > Policy: Support existing policy, targeted whitelisting. The
>>> ioctls
>>>      commands
>>>      > used on a system may not be completely known to a
>>>      sysadmin/policywriter. It
>>>      > is not reasonable to enforce that all needed commands be known in
>>>      order to
>>>      > use this feature in a targeted manner. Existing policy using
>>> the ioctl
>>>      > permission will continue to work as-is. Policy targeting a
>>> specific
>>>      > source/target/class will only be enforced on that particular
>>>      > source/target/class. E.g. continue to allow init to access all
>>> ioctls
>>>      > provided by a driver, but only allow the browser to access a
>>> subset.
>>>      >
>>>      > Performance: Many ioctl calls are performance sensitive, and some
>>>      ioctls
>>>      > have a large command set (e.g. sockets support hundreds of
>>> commands).
>>>      > Execution time on a filtered ioctl should be similar to execution
>>>      time on
>>>      > an unfiltered one and not related to the number of commands being
>>>      filtered.
>>>      > Performance numbers will be included in a separate document.
>>>      >
>>>      > Command space: The ioctl command is a 32 bit number comprised
>>> of four
>>>      > fields, number - sequence number of the command. 8 bits
>>>      > type - magic number assigned to the driver. 8 bits
>>>      > size - size of the user data involved. typically 14 bits (arch
>>>      dependent)
>>>      > direction - The direction of data transfer. typically 2 bits
>>> (arch
>>>      > dependent) The command is uniquely identified by the type and
>>>      number, size
>>>      > and direction are considered to be arguments and are not
>>>      considered for
>>>      > SELinux whitelisting.
>>>      >
>>>      > ---- Policy format ----
>>>      > allow <source> <target>:<class> { 0x8910-0x8926 0x892A-0x8935 }
>>>      > auditallow <source> <target>:<class> 0x892A
>>>
>>>      I agree with only specifying the lower 16 bits (command,type) when
>>>      specifying
>>>      the individual ioctls, I even like the '-' shortcut, but I'm a
>>> little
>>>      concerned about specifying a number directly in the permission
>>> field
>>>      without
>>>      any sort of qualifier.  Specifically I'm worried that it hurts the
>>>      readability
>>>      of the policy and could pose problems with future work.
>>>
>>>      I'd be much happier if we could add some sort of syntax which would
>>>      qualify
>>>      the numbers as ioctls, for example:
>>>
>>>        allow <source> <target>:<class> { ioctl(0x8910-0x8926)
>>> ioctl(0x892A) }
>>>
>>>
>>> If you want additional syntax couldn't we move that burden to m4 rather
>>> then making it a part of the compiler core?
>>
>> It has to be handled by checkpolicy and encoded in the kernel binary
>> policy as an additional field if we want to support other uses of these
>> operation structures for purposes other than just ioctl.
>>
> 
> Do you have something else in mind that might use the operation structures?
> 
> 
> In CIL we will probably make the syntax something like this:
> 
> ((allowop <source> <target> <class> <ioctl expression>)
> 
> So the above rule might look like:
> 
> (allowop <source> <target> <class> ((range 0x8910 0x8926) 0x892A))

Paul asked to generalize it so it can be reused for other purposes.  One
example would be netlink message types.  Today, there is a hardcoded
table in SELinux (security/selinux/nlmsgtab.c) that maps specific
netlink message type values to nlmsg_read (i.e. reads public kernel
state), nlmsg_readpriv (i.e. reads potentially sensitive kernel state),
nlmsg_write (i.e. modifies kernel state), and a few other audit-specific
values so we can distinguish e.g. who can read vs write routing table
entries or audit configuration, as the normal socket read/write checks
merely control the ability to send/recv on the socket, which is always
required for any user of it.  That has limited flexibility and isn't
generally well maintained for newer netlink protocols.

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

[Stephen Smalley]

On 05/21/2015 09:50 AM, Stephen Smalley wrote:
> On 05/21/2015 09:43 AM, James Carter wrote:
>> On 05/21/2015 08:34 AM, Stephen Smalley wrote:
>>> On 05/20/2015 08:39 PM, William Roberts wrote:
>>>>
>>>>
>>>> On Wed, May 20, 2015 at 1:04 PM, Paul Moore <paul@paul-moore.com
>>>> <mailto:paul@paul-moore.com>> wrote:
>>>>
>>>>      First off, my apologies for such a long delay in providing
>>>>      feedback.  The
>>>>      delay wasn't due to any fault of yours, rather just a backlog on my
>>>>      todo list.
>>>>
>>>>      Comments below ...
>>>>
>>>>      On Thursday, April 09, 2015 02:48:50 PM Jeff Vander Stoep wrote:
>>>>      > ---- motivation ----
>>>>      > Ioctls provide many of the operations necessary for device
>>>>      control. The
>>>>      > typical driver supports a device specific set of operations
>>>>      accessible by
>>>>      > the ioctl system call and specified by the command argument.
>>>> SELinux
>>>>      > provides per operation access control to many system operations
>>>>      e.g. chown,
>>>>      > kill, setuid, ipc_lock, etc. Ioclts on the other hand are granted
>>>>      on a per
>>>>      > file descriptor basis using the ioctl permission, meaning that
>>>> the
>>>>      set of
>>>>      > operations provided by the driver are granted on an
>>>> all-or-nothing
>>>>      basis.
>>>>
>>>>      ...
>>>>
>>>>      > ---- Design constraints ----
>>>>      > Policy: Support existing policy, targeted whitelisting. The
>>>> ioctls
>>>>      commands
>>>>      > used on a system may not be completely known to a
>>>>      sysadmin/policywriter. It
>>>>      > is not reasonable to enforce that all needed commands be known in
>>>>      order to
>>>>      > use this feature in a targeted manner. Existing policy using
>>>> the ioctl
>>>>      > permission will continue to work as-is. Policy targeting a
>>>> specific
>>>>      > source/target/class will only be enforced on that particular
>>>>      > source/target/class. E.g. continue to allow init to access all
>>>> ioctls
>>>>      > provided by a driver, but only allow the browser to access a
>>>> subset.
>>>>      >
>>>>      > Performance: Many ioctl calls are performance sensitive, and some
>>>>      ioctls
>>>>      > have a large command set (e.g. sockets support hundreds of
>>>> commands).
>>>>      > Execution time on a filtered ioctl should be similar to execution
>>>>      time on
>>>>      > an unfiltered one and not related to the number of commands being
>>>>      filtered.
>>>>      > Performance numbers will be included in a separate document.
>>>>      >
>>>>      > Command space: The ioctl command is a 32 bit number comprised
>>>> of four
>>>>      > fields, number - sequence number of the command. 8 bits
>>>>      > type - magic number assigned to the driver. 8 bits
>>>>      > size - size of the user data involved. typically 14 bits (arch
>>>>      dependent)
>>>>      > direction - The direction of data transfer. typically 2 bits
>>>> (arch
>>>>      > dependent) The command is uniquely identified by the type and
>>>>      number, size
>>>>      > and direction are considered to be arguments and are not
>>>>      considered for
>>>>      > SELinux whitelisting.
>>>>      >
>>>>      > ---- Policy format ----
>>>>      > allow <source> <target>:<class> { 0x8910-0x8926 0x892A-0x8935 }
>>>>      > auditallow <source> <target>:<class> 0x892A
>>>>
>>>>      I agree with only specifying the lower 16 bits (command,type) when
>>>>      specifying
>>>>      the individual ioctls, I even like the '-' shortcut, but I'm a
>>>> little
>>>>      concerned about specifying a number directly in the permission
>>>> field
>>>>      without
>>>>      any sort of qualifier.  Specifically I'm worried that it hurts the
>>>>      readability
>>>>      of the policy and could pose problems with future work.
>>>>
>>>>      I'd be much happier if we could add some sort of syntax which would
>>>>      qualify
>>>>      the numbers as ioctls, for example:
>>>>
>>>>        allow <source> <target>:<class> { ioctl(0x8910-0x8926)
>>>> ioctl(0x892A) }
>>>>
>>>>
>>>> If you want additional syntax couldn't we move that burden to m4 rather
>>>> then making it a part of the compiler core?
>>>
>>> It has to be handled by checkpolicy and encoded in the kernel binary
>>> policy as an additional field if we want to support other uses of these
>>> operation structures for purposes other than just ioctl.
>>>
>>
>> Do you have something else in mind that might use the operation structures?
>>
>>
>> In CIL we will probably make the syntax something like this:
>>
>> ((allowop <source> <target> <class> <ioctl expression>)
>>
>> So the above rule might look like:
>>
>> (allowop <source> <target> <class> ((range 0x8910 0x8926) 0x892A))
> 
> Paul asked to generalize it so it can be reused for other purposes.  One
> example would be netlink message types.  Today, there is a hardcoded
> table in SELinux (security/selinux/nlmsgtab.c) that maps specific
> netlink message type values to nlmsg_read (i.e. reads public kernel
> state), nlmsg_readpriv (i.e. reads potentially sensitive kernel state),
> nlmsg_write (i.e. modifies kernel state), and a few other audit-specific
> values so we can distinguish e.g. who can read vs write routing table
> entries or audit configuration, as the normal socket read/write checks
> merely control the ability to send/recv on the socket, which is always
> required for any user of it.  That has limited flexibility and isn't
> generally well maintained for newer netlink protocols.

So the proposal would be to just add an additional field specifying how
to interpret/apply the operation list, e.g.
(allowop <source> <target> <class> <ioctl|netlink|...> ((range 0x8910
0x8926) 0x892A))

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

[Paul Moore]

On Thu, May 21, 2015 at 9:54 AM, Stephen Smalley <sds@tycho.nsa.gov> wrote:
> On 05/21/2015 09:50 AM, Stephen Smalley wrote:
>> On 05/21/2015 09:43 AM, James Carter wrote:
>>> Do you have something else in mind that might use the operation structures?
>>>
>>> In CIL we will probably make the syntax something like this:
>>>
>>> ((allowop <source> <target> <class> <ioctl expression>)
>>>
>>> So the above rule might look like:
>>>
>>> (allowop <source> <target> <class> ((range 0x8910 0x8926) 0x892A))
>>
>> Paul asked to generalize it so it can be reused for other purposes.  One
>> example would be netlink message types.  Today, there is a hardcoded
>> table in SELinux (security/selinux/nlmsgtab.c) that maps specific
>> netlink message type values to nlmsg_read (i.e. reads public kernel
>> state), nlmsg_readpriv (i.e. reads potentially sensitive kernel state),
>> nlmsg_write (i.e. modifies kernel state), and a few other audit-specific
>> values so we can distinguish e.g. who can read vs write routing table
>> entries or audit configuration, as the normal socket read/write checks
>> merely control the ability to send/recv on the socket, which is always
>> required for any user of it.  That has limited flexibility and isn't
>> generally well maintained for newer netlink protocols.
>
> So the proposal would be to just add an additional field specifying how
> to interpret/apply the operation list, e.g.
> (allowop <source> <target> <class> <ioctl|netlink|...> ((range 0x8910
> 0x8926) 0x892A))

Looks to me like a good solution for CIL.

-- 
paul moore
www.paul-moore.com

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

[James Carter]

On 05/21/2015 10:04 AM, Paul Moore wrote:
> On Thu, May 21, 2015 at 9:54 AM, Stephen Smalley <sds@tycho.nsa.gov> wrote:
>> On 05/21/2015 09:50 AM, Stephen Smalley wrote:
>>> On 05/21/2015 09:43 AM, James Carter wrote:
>>>> Do you have something else in mind that might use the operation structures?
>>>>
>>>> In CIL we will probably make the syntax something like this:
>>>>
>>>> ((allowop <source> <target> <class> <ioctl expression>)
>>>>
>>>> So the above rule might look like:
>>>>
>>>> (allowop <source> <target> <class> ((range 0x8910 0x8926) 0x892A))
>>>
>>> Paul asked to generalize it so it can be reused for other purposes.  One
>>> example would be netlink message types.  Today, there is a hardcoded
>>> table in SELinux (security/selinux/nlmsgtab.c) that maps specific
>>> netlink message type values to nlmsg_read (i.e. reads public kernel
>>> state), nlmsg_readpriv (i.e. reads potentially sensitive kernel state),
>>> nlmsg_write (i.e. modifies kernel state), and a few other audit-specific
>>> values so we can distinguish e.g. who can read vs write routing table
>>> entries or audit configuration, as the normal socket read/write checks
>>> merely control the ability to send/recv on the socket, which is always
>>> required for any user of it.  That has limited flexibility and isn't
>>> generally well maintained for newer netlink protocols.
>>
>> So the proposal would be to just add an additional field specifying how
>> to interpret/apply the operation list, e.g.
>> (allowop <source> <target> <class> <ioctl|netlink|...> ((range 0x8910
>> 0x8926) 0x892A))
>
> Looks to me like a good solution for CIL.
>

I agree.

So why not have a syntax like the following for checkpolicy?

allowop <source> <target> <class> <ioctl|netlink|...> { 0x8910-0x8926 0x892A };

Woudln't that be clearer?


-- 
James Carter <jwcart2@tycho.nsa.gov>
National Security Agency

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

[Paul Moore]

On Thu, May 21, 2015 at 9:35 AM, Joshua Brindle
<brindle@quarksecurity.com> wrote:
> Paul Moore wrote:
>> On Thu, May 21, 2015 at 12:17 AM, Jeffrey Vander Stoep 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.
>>
>> Great, thank you.
>>
>>> 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
>>>
>>> After monitoring ioctl use across five different devices I think this
>>> is a good approach as just 10-20 macros would be adequate for a
>>> targeted policy and would provide a clearer explanation of the
>>> permissions given.
>>
>> Agreed.  We can use m4 to provide both the ioctl names and sets if needed.
>
> m4 is never the answer....

Except for the policy interfaces, permission sets, etc. ;)

See Stephen's comments on this, specifically the idea that ioctls are
not objects.  Further, the existing permission set shorthand is a very
good precedence for this approach towards ioctl number handling; I see
no reason *not* to use m4.

> An attribute-like symbol that is maintained in the binary would make these
> visible in the loaded policy and therefore more understandable/analyzable.

I think the proposed approach is easily understood and/or analyzable.

> It would also allow you to easily add them in specific devices in a more
> readable way. For example, if the Android base policy allowed gpu_ioc to
> surfaceflinger all a device specific variant would need to do is add its
> ioctls to the attribute.

Once again, I think the proposed approach ticks these boxes as well.

-- 
paul moore
www.paul-moore.com

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

[Stephen Smalley]

On 05/21/2015 10:10 AM, James Carter wrote:
> On 05/21/2015 10:04 AM, Paul Moore wrote:
>> On Thu, May 21, 2015 at 9:54 AM, Stephen Smalley <sds@tycho.nsa.gov>
>> wrote:
>>> On 05/21/2015 09:50 AM, Stephen Smalley wrote:
>>>> On 05/21/2015 09:43 AM, James Carter wrote:
>>>>> Do you have something else in mind that might use the operation
>>>>> structures?
>>>>>
>>>>> In CIL we will probably make the syntax something like this:
>>>>>
>>>>> ((allowop <source> <target> <class> <ioctl expression>)
>>>>>
>>>>> So the above rule might look like:
>>>>>
>>>>> (allowop <source> <target> <class> ((range 0x8910 0x8926) 0x892A))
>>>>
>>>> Paul asked to generalize it so it can be reused for other purposes. 
>>>> One
>>>> example would be netlink message types.  Today, there is a hardcoded
>>>> table in SELinux (security/selinux/nlmsgtab.c) that maps specific
>>>> netlink message type values to nlmsg_read (i.e. reads public kernel
>>>> state), nlmsg_readpriv (i.e. reads potentially sensitive kernel state),
>>>> nlmsg_write (i.e. modifies kernel state), and a few other
>>>> audit-specific
>>>> values so we can distinguish e.g. who can read vs write routing table
>>>> entries or audit configuration, as the normal socket read/write checks
>>>> merely control the ability to send/recv on the socket, which is always
>>>> required for any user of it.  That has limited flexibility and isn't
>>>> generally well maintained for newer netlink protocols.
>>>
>>> So the proposal would be to just add an additional field specifying how
>>> to interpret/apply the operation list, e.g.
>>> (allowop <source> <target> <class> <ioctl|netlink|...> ((range 0x8910
>>> 0x8926) 0x892A))
>>
>> Looks to me like a good solution for CIL.
>>
> 
> I agree.
> 
> So why not have a syntax like the following for checkpolicy?
> 
> allowop <source> <target> <class> <ioctl|netlink|...> { 0x8910-0x8926
> 0x892A };
> 
> Woudln't that be clearer?

Possibly.  cc'd Jeff since he is the one writing the patch.  Not sure
why he got dropped from your replies; maybe the list is truncating the
cc list...

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

[Paul Moore]

On Thu, May 21, 2015 at 10:10 AM, James Carter <jwcart2@tycho.nsa.gov> wrote:
> On 05/21/2015 10:04 AM, Paul Moore wrote:
>> On Thu, May 21, 2015 at 9:54 AM, Stephen Smalley <sds@tycho.nsa.gov>
>> wrote:
>>> On 05/21/2015 09:50 AM, Stephen Smalley wrote:
>>>> On 05/21/2015 09:43 AM, James Carter wrote:
>>>>>
>>>>> Do you have something else in mind that might use the operation
>>>>> structures?
>>>>>
>>>>> In CIL we will probably make the syntax something like this:
>>>>>
>>>>> ((allowop <source> <target> <class> <ioctl expression>)
>>>>>
>>>>> So the above rule might look like:
>>>>>
>>>>> (allowop <source> <target> <class> ((range 0x8910 0x8926) 0x892A))
>>>>
>>>>
>>>> Paul asked to generalize it so it can be reused for other purposes.  One
>>>> example would be netlink message types.  Today, there is a hardcoded
>>>> table in SELinux (security/selinux/nlmsgtab.c) that maps specific
>>>> netlink message type values to nlmsg_read (i.e. reads public kernel
>>>> state), nlmsg_readpriv (i.e. reads potentially sensitive kernel state),
>>>> nlmsg_write (i.e. modifies kernel state), and a few other audit-specific
>>>> values so we can distinguish e.g. who can read vs write routing table
>>>> entries or audit configuration, as the normal socket read/write checks
>>>> merely control the ability to send/recv on the socket, which is always
>>>> required for any user of it.  That has limited flexibility and isn't
>>>> generally well maintained for newer netlink protocols.
>>>
>>>
>>> So the proposal would be to just add an additional field specifying how
>>> to interpret/apply the operation list, e.g.
>>> (allowop <source> <target> <class> <ioctl|netlink|...> ((range 0x8910
>>> 0x8926) 0x892A))
>>
>>
>> Looks to me like a good solution for CIL.
>>
>
> I agree.
>
> So why not have a syntax like the following for checkpolicy?
>
> allowop <source> <target> <class> <ioctl|netlink|...> { 0x8910-0x8926 0x892A
> };
>
> Woudln't that be clearer?

I have no strong preference here, I'll leave that to the folks working
on the policy toolchain.

-- 
paul moore
www.paul-moore.com

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

[Joshua Brindle]

Paul Moore wrote:
> On Thu, May 21, 2015 at 9:54 AM, Stephen Smalley<sds@tycho.nsa.gov>  wrote:
>> On 05/21/2015 09:50 AM, Stephen Smalley wrote:
>>> On 05/21/2015 09:43 AM, James Carter wrote:
>>>> Do you have something else in mind that might use the operation structures?
>>>>
>>>> In CIL we will probably make the syntax something like this:
>>>>
>>>> ((allowop<source>  <target>  <class>  <ioctl expression>)
>>>>
>>>> So the above rule might look like:
>>>>
>>>> (allowop<source>  <target>  <class>  ((range 0x8910 0x8926) 0x892A))
>>> Paul asked to generalize it so it can be reused for other purposes.  One
>>> example would be netlink message types.  Today, there is a hardcoded
>>> table in SELinux (security/selinux/nlmsgtab.c) that maps specific
>>> netlink message type values to nlmsg_read (i.e. reads public kernel
>>> state), nlmsg_readpriv (i.e. reads potentially sensitive kernel state),
>>> nlmsg_write (i.e. modifies kernel state), and a few other audit-specific
>>> values so we can distinguish e.g. who can read vs write routing table
>>> entries or audit configuration, as the normal socket read/write checks
>>> merely control the ability to send/recv on the socket, which is always
>>> required for any user of it.  That has limited flexibility and isn't
>>> generally well maintained for newer netlink protocols.
>> So the proposal would be to just add an additional field specifying how
>> to interpret/apply the operation list, e.g.
>> (allowop<source>  <target>  <class>  <ioctl|netlink|...>  ((range 0x8910
>> 0x8926) 0x892A))
>
> Looks to me like a good solution for CIL.
>

Something that ends up in the kernel binary would be very helpful for 
policy analysis.

Something like:

ioctl foo_op { 0x8910-0x8926 0x892A };

allow domain device : chr_file foo_op;

or ioctl(foo_op), in case there are conflicting symbol names.

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

[Joshua Brindle]

Paul Moore wrote:
> On Thu, May 21, 2015 at 9:35 AM, Joshua Brindle
> <brindle@quarksecurity.com>  wrote:
>> Paul Moore wrote:
>>> On Thu, May 21, 2015 at 12:17 AM, Jeffrey Vander Stoep 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.
>>> Great, thank you.
>>>
>>>> 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
>>>>
>>>> After monitoring ioctl use across five different devices I think this
>>>> is a good approach as just 10-20 macros would be adequate for a
>>>> targeted policy and would provide a clearer explanation of the
>>>> permissions given.
>>> Agreed.  We can use m4 to provide both the ioctl names and sets if needed.
>> m4 is never the answer....
>
> Except for the policy interfaces, permission sets, etc. ;)
>
> See Stephen's comments on this, specifically the idea that ioctls are
> not objects.  Further, the existing permission set shorthand is a very
> good precedence for this approach towards ioctl number handling; I see
> no reason *not* to use m4.
>

The reason *not* to use m4 is because we want some sort of meaningful 
identifiers preserved in the kernel policy for analysis. I know that 
isn't your use case but it is some of ours.

>> An attribute-like symbol that is maintained in the binary would make these
>> visible in the loaded policy and therefore more understandable/analyzable.
>
> I think the proposed approach is easily understood and/or analyzable.
>
>> It would also allow you to easily add them in specific devices in a more
>> readable way. For example, if the Android base policy allowed gpu_ioc to
>> surfaceflinger all a device specific variant would need to do is add its
>> ioctls to the attribute.
>
> Once again, I think the proposed approach ticks these boxes as well.
>

Not in the *kernel binary*

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

[Paul Moore]

On Thu, May 21, 2015 at 10:16 AM, Joshua Brindle
<brindle@quarksecurity.com> wrote:
> Paul Moore wrote:
>> On Thu, May 21, 2015 at 9:35 AM, Joshua Brindle
>> <brindle@quarksecurity.com>  wrote:
>>> Paul Moore wrote:
>>>> On Thu, May 21, 2015 at 12:17 AM, Jeffrey Vander Stoep 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.
>>>>
>>>> Great, thank you.
>>>>
>>>>> 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
>>>>>
>>>>> After monitoring ioctl use across five different devices I think this
>>>>> is a good approach as just 10-20 macros would be adequate for a
>>>>> targeted policy and would provide a clearer explanation of the
>>>>> permissions given.
>>>>
>>>> Agreed.  We can use m4 to provide both the ioctl names and sets if
>>>> needed.
>>>
>>> m4 is never the answer....
>>
>>
>> Except for the policy interfaces, permission sets, etc. ;)
>>
>> See Stephen's comments on this, specifically the idea that ioctls are
>> not objects.  Further, the existing permission set shorthand is a very
>> good precedence for this approach towards ioctl number handling; I see
>> no reason *not* to use m4.
>>
>
> The reason *not* to use m4 is because we want some sort of meaningful
> identifiers preserved in the kernel policy for analysis. I know that isn't
> your use case but it is some of ours.

You've got the ioctl numbers in the binary policy, which are the same
numbers used in the policy representation, which are also the same
numbers used by applications actually making use of the ioctl()
syscall.  Other than the fact that these things are numbers and not a
more conventional label string, I don't understand the problem.

-- 
paul moore
www.paul-moore.com

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

[Joshua Brindle]

Paul Moore wrote:
> On Thu, May 21, 2015 at 10:16 AM, Joshua Brindle
> <brindle@quarksecurity.com>  wrote:
>> Paul Moore wrote:
>>> On Thu, May 21, 2015 at 9:35 AM, Joshua Brindle
>>> <brindle@quarksecurity.com>   wrote:
>>>> Paul Moore wrote:
>>>>> On Thu, May 21, 2015 at 12:17 AM, Jeffrey Vander Stoep 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.
>>>>> Great, thank you.
>>>>>
>>>>>> 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
>>>>>>
>>>>>> After monitoring ioctl use across five different devices I think this
>>>>>> is a good approach as just 10-20 macros would be adequate for a
>>>>>> targeted policy and would provide a clearer explanation of the
>>>>>> permissions given.
>>>>> Agreed.  We can use m4 to provide both the ioctl names and sets if
>>>>> needed.
>>>> m4 is never the answer....
>>>
>>> Except for the policy interfaces, permission sets, etc. ;)
>>>
>>> See Stephen's comments on this, specifically the idea that ioctls are
>>> not objects.  Further, the existing permission set shorthand is a very
>>> good precedence for this approach towards ioctl number handling; I see
>>> no reason *not* to use m4.
>>>
>> The reason *not* to use m4 is because we want some sort of meaningful
>> identifiers preserved in the kernel policy for analysis. I know that isn't
>> your use case but it is some of ours.
>
> You've got the ioctl numbers in the binary policy, which are the same
> numbers used in the policy representation, which are also the same
> numbers used by applications actually making use of the ioctl()
> syscall.  Other than the fact that these things are numbers and not a
> more conventional label string, I don't understand the problem.
>

That is precisely the problem. I'd like any extra symbolic information 
(e.g., calling this range of ioctls gpu_op) preserved and not thrown 
away by m4.

Some of us work on binary only systems where we don't get to see the 
source code of the applications or the policy.

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

[Paul Moore]

On Thu, May 21, 2015 at 10:23 AM, Joshua Brindle
<brindle@quarksecurity.com> wrote:
> Paul Moore wrote:
>> You've got the ioctl numbers in the binary policy, which are the same
>> numbers used in the policy representation, which are also the same
>> numbers used by applications actually making use of the ioctl()
>> syscall.  Other than the fact that these things are numbers and not a
>> more conventional label string, I don't understand the problem.
>
> That is precisely the problem. I'd like any extra symbolic information
> (e.g., calling this range of ioctls gpu_op) preserved and not thrown away by
> m4.
>
> Some of us work on binary only systems where we don't get to see the source
> code of the applications or the policy.

Sorry, I keep forgetting you are special.  Thanks for the multiple reminders.

I suggest talking with James and the CIL folks to arrive at some
solution that works well with CIL, whatever that might be.  As for the
checkpolicy based policy, I think m4 is just fine if folks want to use
it; there is no requirement that m4 must be used for this.

Further, if we do need to introduce some attribute like construct for
these operations/ranges/permissions/whatever-we-call-them, I'm okay
adding that in a future patchset, I see no reason to hold up this
initial work for that.

-- 
paul moore
www.paul-moore.com

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

[Joshua Brindle]

Paul Moore wrote:
> On Thu, May 21, 2015 at 10:23 AM, Joshua Brindle
> <brindle@quarksecurity.com>  wrote:
>> Paul Moore wrote:
>>> You've got the ioctl numbers in the binary policy, which are the same
>>> numbers used in the policy representation, which are also the same
>>> numbers used by applications actually making use of the ioctl()
>>> syscall.  Other than the fact that these things are numbers and not a
>>> more conventional label string, I don't understand the problem.
>> That is precisely the problem. I'd like any extra symbolic information
>> (e.g., calling this range of ioctls gpu_op) preserved and not thrown away by
>> m4.
>>
>> Some of us work on binary only systems where we don't get to see the source
>> code of the applications or the policy.
>
> Sorry, I keep forgetting you are special.  Thanks for the multiple reminders.
>
> I suggest talking with James and the CIL folks to arrive at some
> solution that works well with CIL, whatever that might be.  As for the
> checkpolicy based policy, I think m4 is just fine if folks want to use
> it; there is no requirement that m4 must be used for this.
>
> Further, if we do need to introduce some attribute like construct for
> these operations/ranges/permissions/whatever-we-call-them, I'm okay
> adding that in a future patchset, I see no reason to hold up this
> initial work for that.

It seems like some adjustments are being made anyway. I wish I'd brought 
this up when the patches were first posted so it is definitely my fault 
if it doesn't happen.

I doubt you'd want to be on the hook to analyze a policy that just dumps 
a wall of hex values instead of descriptive symbols though. This is the 
exact reason that attribute names are now preserved in the kernel 
binary, despite the kernel not needing them.

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

[William Roberts]

[-- Attachment #1: Type: text/plain, Size: 1092 bytes --]

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


>
> After monitoring ioctl use across five different devices I think this
> is a good approach as just 10-20 macros would be adequate for a
> targeted policy and would provide a clearer explanation of the
> permissions given.
>



-- 
Respectfully,

William C Roberts

[-- Attachment #2: Type: text/html, Size: 1677 bytes --]

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

[Joshua Brindle]

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?

In the ioctl-attibute case the variant policy would just add its 
specific ioctls to gpu_op and everything would be taken care of.

Then we just need to preserve the symbols for looking at policies and 
we'll all be happy, right?

>
>> After monitoring ioctl use across five different devices I think this
>> is a good approach as just 10-20 macros would be adequate for a
>> targeted policy and would provide a clearer explanation of the
>> permissions given.
>>
>
>
>

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

[William Roberts]

[-- Attachment #1: Type: text/plain, Size: 1913 bytes --]

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.

>
> In the ioctl-attibute case the variant policy would just add its specific
ioctls to gpu_op and everything would be taken care of.
>
> Then we just need to preserve the symbols for looking at policies and
we'll all be happy, right?
>
>
>>
>>> After monitoring ioctl use across five different devices I think this
>>> is a good approach as just 10-20 macros would be adequate for a
>>> targeted policy and would provide a clearer explanation of the
>>> permissions given.
>>>
>>
>>
>>
>

[-- Attachment #2: Type: text/html, Size: 2592 bytes --]

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

[Joshua Brindle]

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?

>
>> In the ioctl-attibute case the variant policy would just add its specific
> ioctls to gpu_op and everything would be taken care of.
>> Then we just need to preserve the symbols for looking at policies and
> we'll all be happy, right?
>>
>>>> After monitoring ioctl use across five different devices I think this
>>>> is a good approach as just 10-20 macros would be adequate for a
>>>> targeted policy and would provide a clearer explanation of the
>>>> permissions given.
>>>>
>>>
>>>
>

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

[Stephen Smalley]

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.

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

[Joshua Brindle]

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.

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

[Jeffrey Vander Stoep]

Here is my personal todo list based on this conversation.

- change example policy in commit message to demonstrate intended use.
No raw ioctl values.
- Look into making logic more general, less ioctl specific
- Look at making the code clearer. I.e. address Paul's comments on
lack of clarity in struct/variable naming.

In the spirit of keeping this commit concise and as basic as possible
(it's already 800 LOC!) I will not address suggestions to propagate
additional policy information such as ioctl names and groups into the
kernel binary. I agree that would be useful, but I will leave as
future work.

Regarding comments on policy syntax, those will be addressed in a
separate non-kernel commit to the selinux project.

Thanks again for all the feedback!

On Thu, May 21, 2015 at 8:27 AM, Joshua Brindle
<brindle@quarksecurity.com> wrote:
> 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.
>
> _______________________________________________
> Selinux mailing list
> Selinux@tycho.nsa.gov
> To unsubscribe, send email to Selinux-leave@tycho.nsa.gov.
> To get help, send an email containing "help" to
> Selinux-request@tycho.nsa.gov.

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

[Paul Moore]

On Thu, May 21, 2015 at 2:05 PM, Jeffrey Vander Stoep <jeffv@google.com> wrote:
> Here is my personal todo list based on this conversation.
>
> - change example policy in commit message to demonstrate intended use.
> No raw ioctl values.
> - Look into making logic more general, less ioctl specific
> - Look at making the code clearer. I.e. address Paul's comments on
> lack of clarity in struct/variable naming.

Thanks.  That all sounds reasonable to me.

> In the spirit of keeping this commit concise and as basic as possible
> (it's already 800 LOC!) I will not address suggestions to propagate
> additional policy information such as ioctl names and groups into the
> kernel binary. I agree that would be useful, but I will leave as
> future work.

Agreed.

Also, if you want, you could probably split up patch 2/2 if you wanted
into a few more patches.  While the golden rule is that you can't
break anything with a single patch, e.g. it must still compile/boot,
it is perfectly fine to add non-functional code midway through a
patchset so long as everything is working and enabled by the time you
reach the end of the patchset.

> Regarding comments on policy syntax, those will be addressed in a
> separate non-kernel commit to the selinux project.
>
> Thanks again for all the feedback!

Thanks for the patches!