selinux: add targeted whitelisting of ioctl commands.

---- 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

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.

Quoted 149 lines · 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

---- 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.

Quoted 11 lines · 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.

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

---- 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

---- 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 ...

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.

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 ...

Quoted 5 lines · 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,

Quoted 1 line

kill, setuid, ipc_lock, etc. Ioclts on the other hand are granted on a

per

Quoted 2 lines

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.

...

Quoted 2 lines

---- Design constraints ----
Policy: Support existing policy, targeted whitelisting. The ioctls

commands

Quoted 1 line

used on a system may not be completely known to a sysadmin/policywriter.

It

Quoted 1 line

is not reasonable to enforce that all needed commands be known in order

to

Quoted 10 lines

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.

Quoted 8 lines

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

Quoted 6 lines

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.

Quoted 5 lines

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

binary policy/avtab: Like AVC permissions, ioctl operations permissions

are

Quoted 1 line

represented by set/cleared bits. Each ioctl operation avtab entry

represents

Quoted 1 line

a single ioctl command type (using “type” as defined above in the

“command

Quoted 1 line

space” section) and includes permissions for the 256 operations within

that

Quoted 1 line

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 ;)

Quoted 2 lines

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 ...

Quoted 1 line

The structure flags the ioctl types that are allowed and only looks up

the

Quoted 1 line

permissions within that type from the avtab when they are first used.

After

Quoted 2 lines

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

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 ...

Quoted 5 lines · 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,

Quoted 1 line

kill, setuid, ipc_lock, etc. Ioclts on the other hand are granted on a

per

Quoted 2 lines

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.

...

Quoted 2 lines

---- Design constraints ----
Policy: Support existing policy, targeted whitelisting. The ioctls

commands

Quoted 1 line

used on a system may not be completely known to a sysadmin/policywriter.

It

Quoted 1 line

is not reasonable to enforce that all needed commands be known in order

to

Quoted 10 lines

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.

Quoted 8 lines

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

Quoted 6 lines

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.

Quoted 5 lines

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

binary policy/avtab: Like AVC permissions, ioctl operations permissions

are

Quoted 1 line

represented by set/cleared bits. Each ioctl operation avtab entry

represents

Quoted 1 line

a single ioctl command type (using “type” as defined above in the

“command

Quoted 1 line

space” section) and includes permissions for the 256 operations within

that

Quoted 1 line

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 ;)

Quoted 2 lines

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 ...

Quoted 1 line

The structure flags the ioctl types that are allowed and only looks up

the

Quoted 1 line

permissions within that type from the avtab when they are first used.

After

Quoted 2 lines

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
[email protected]
To unsubscribe, send email to [email protected]. To get help, send an email containing "help" to [email protected].

Quoted 12 lines

Quoted 3 lines

---- 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.

Quoted 6 lines

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?

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.

Quoted 6 lines · 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.

Great, thank you.

Quoted 6 lines

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.

Quoted 6 lines

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.

Quoted 19 lines · Paul Moore

Quoted 7 lines · 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.

Great, thank you.

Quoted 8 lines

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

Quoted 6 lines

Quoted 4 lines

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.

Quoted 23 lines · Joshua Brindle

Quoted 22 lines · Paul Moore

Quoted 6 lines · 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.

Great, thank you.

Quoted 4 lines

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.

Quoted 8 lines

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.

Quoted 1 line

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.

Quoted 2 lines

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.

Quoted 4 lines

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.

Quoted 33 lines · Paul Moore

Quoted 25 lines · Joshua Brindle

Quoted 24 lines · Paul Moore

Quoted 7 lines · 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.

Great, thank you.

Quoted 12 lines

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.

Quoted 4 lines

Quoted 2 lines

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

m4 is never the answer....

Quoted 7 lines

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.

Quoted 31 lines · Joshua Brindle

Quoted 30 lines · Paul Moore

Quoted 18 lines · Joshua Brindle

Quoted 17 lines · Paul Moore

Quoted 6 lines · 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.

Great, thank you.

Quoted 8 lines

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

Quoted 7 lines

Quoted 6 lines

Quoted 4 lines

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. ;)

Quoted 8 lines

Quoted 5 lines

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.

Quoted 5 lines · Paul Moore

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.

Quoted 6 lines · Joshua Brindle

Quoted 5 lines · Paul Moore

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.

Quoted 6 lines

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.

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.

Quoted 11 lines · 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.

Quoted 4 lines

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

Quoted 6 lines

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.

Quoted 20 lines · William Roberts

Quoted 15 lines · 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

Quoted 2 lines

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

In the ioctl-attibute case the variant policy would just add its specific

Then we just need to preserve the symbols for looking at policies and

Quoted 11 lines

Quoted 8 lines

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.

Quoted 23 lines · Joshua Brindle

Quoted 14 lines · William Roberts

Quoted 11 lines · 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.

Quoted 6 lines

Quoted 4 lines

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.

Quoted 1 line

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.

Quoted 2 lines

In the ioctl-attibute case the variant policy would just add its specific

ioctls to gpu_op and everything would be taken care of.

Quoted 1 line

Then we just need to preserve the symbols for looking at policies and

we'll all be happy, right?

Quoted 8 lines

Quoted 8 lines

Quoted 6 lines

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.

Quoted 32 lines · William Roberts

Quoted 17 lines · Joshua Brindle

Quoted 16 lines · William Roberts

Quoted 13 lines · 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.

Quoted 7 lines

Quoted 4 lines

Quoted 2 lines

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?

Quoted 3 lines

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?

Quoted 33 lines · Joshua Brindle

Quoted 32 lines · William Roberts

Quoted 17 lines · Joshua Brindle

Quoted 16 lines · William Roberts

Quoted 13 lines · 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.

Quoted 7 lines

Quoted 4 lines

Quoted 2 lines

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?

Quoted 6 lines

Quoted 3 lines

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.

Quoted 60 lines · Stephen Smalley

Quoted 42 lines · Joshua Brindle

Quoted 40 lines · William Roberts

Quoted 23 lines · Joshua Brindle

Quoted 21 lines · William Roberts

Quoted 16 lines · 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.

Quoted 7 lines

Quoted 4 lines

Quoted 2 lines

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?

Quoted 7 lines

Quoted 3 lines

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.

Quoted 7 lines

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
[email protected]
To unsubscribe, send email to [email protected]. To get help, send an email containing "help" to [email protected].

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!

William Roberts wrote:
<snip>

Quoted 16 lines

Quoted 16 lines

Quoted 3 lines

---- 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:

Quoted 7 lines

Quoted 4 lines

   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?

On Wed, May 20, 2015 at 1:04 PM, Paul Moore <[email protected] <mailto:[email protected]>> 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?

Quoted 84 lines · William Roberts

On Wed, May 20, 2015 at 1:04 PM, Paul Moore <[email protected] <mailto:[email protected]>> 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) }

Quoted 2 lines

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.

_______________________________________________
Selinux mailing list
[email protected]
To unsubscribe, send email to [email protected].
To get help, send an email containing "help" to [email protected].

Quoted 99 lines · Stephen Smalley

Quoted 98 lines · William Roberts

On Wed, May 20, 2015 at 1:04 PM, Paul Moore <[email protected] <mailto:[email protected]>> 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) }

Quoted 7 lines

Quoted 2 lines

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))

Quoted 111 lines · James Carter

Quoted 99 lines · Stephen Smalley

Quoted 98 lines · William Roberts

On Wed, May 20, 2015 at 1:04 PM, Paul Moore <[email protected] <mailto:[email protected]>> 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) }

Quoted 7 lines

Quoted 2 lines

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?

Quoted 7 lines

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.

Quoted 11 lines · Stephen Smalley

Quoted 9 lines · James Carter

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))

Quoted 11 lines

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))

Quoted 24 lines · Stephen Smalley

Quoted 22 lines · Stephen Smalley

Quoted 9 lines · James Carter

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.

Quoted 4 lines

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.

Quoted 29 lines · Paul Moore

Quoted 27 lines · Stephen Smalley

Quoted 25 lines · Stephen Smalley

Quoted 10 lines · James Carter

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.

Quoted 7 lines

Quoted 4 lines

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?

Quoted 30 lines · Paul Moore

Quoted 28 lines · Stephen Smalley

Quoted 25 lines · Stephen Smalley

Quoted 11 lines · James Carter

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.

Quoted 8 lines

Quoted 4 lines

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?

Quoted 22 lines · Stephen Smalley

Quoted 21 lines · Stephen Smalley

Quoted 9 lines · James Carter

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.

Quoted 4 lines

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.