On Wednesday, April 29, 2015 12:56:12 PM Jeff Vander Stoep wrote:
> Extend the generic ioctl permission check with support for per-command
> filtering. Source/target/class sets including the ioctl permission may
> additionally include a set of commands. Example:
>
> allow <source> <target>:<class> { 0x8910-0x8926 0x892A-0x8935 }
> auditallow <source> <target>:<class> 0x892A
>
> When ioctl commands are omitted only the permissions are checked. This
> feature is intended to provide finer granularity for the ioctl
> permission which may be too imprecise in some circumstances. For
> example, the same driver may use ioctls to provide important and
> benign functionality such as driver version or socket type as well as
> dangerous capabilities such as debugging features, read/write/execute
> to physical memory or access to sensitive data. Per-command filtering
> provides a mechanism to reduce the attack surface of the kernel, and
> limit applications to the subset of commands required.
>
> The format of the policy binary has been modified to include ioctl
> commands, and the policy version number has been incremented to
> POLICYDB_VERSION_IOCTL_OPERATIONS=30 to account for the format change.
>
> Signed-off-by: Jeff Vander Stoep <jeffv@xxxxxxxxxx>
If you haven't already, read my comments on patch 0/2; additional comments
below ...
> diff --git a/security/selinux/avc.c b/security/selinux/avc.c
> index 3c17dda..47918fa 100644
> --- a/security/selinux/avc.c
> +++ b/security/selinux/avc.c
> @@ -22,6 +22,7 @@
> #include <linux/init.h>
> #include <linux/skbuff.h>
> #include <linux/percpu.h>
> +#include <linux/list.h>
> #include <net/sock.h>
> #include <linux/un.h>
> #include <net/af_unix.h>
> @@ -48,6 +49,7 @@ struct avc_entry {
> u32 tsid;
> u16 tclass;
> struct av_decision avd;
> + struct avc_operation_node *ops_node;
As I said earlier, I would like to make this avc extension more general. In
avc_entry that probably will mean nothing, but I wanted to mention this soon
in this patch's comments.
> };
>
> struct avc_node {
> @@ -64,6 +66,16 @@ struct avc_cache {
> u32 latest_notif; /* latest revocation notification */
> };
>
> +struct avc_operation_decision_node {
> + struct operation_decision od;
> + struct list_head od_list;
> +};
Making this more generic may mean adding an extra field here to specify the
type of extension, e.g. ioctl commands.
> +struct avc_operation_node {
> + struct operation ops;
> + struct list_head od_head; /* list of operation_decision_node */
> +};
As mentioned earlier, I think "operation" needs a name change; I tend to like
"extop" better, e.g. "avc_extop_decision_node" and "avc_extop_node". Feel
free to suggest others.
The "operation" struct is named poorly as well; even if we stick with
"operation" elsewhere we really need to name this one better, it's way too
generic.
> @@ -367,6 +655,7 @@ static int avc_latest_notif_update(int seqno, int
> is_insert) * @tsid: target security identifier
> * @tclass: target security class
> * @avd: resulting av decision
> + * @ops: resulting operation decisions
Parameter in the comment block doesn't match the actual function definition.
> +/*
> + * ioctl commands are comprised of four fields, direction, size, type, and
> + * number. The avc operation logic filters based on two of them:
> + *
> + * type: or code, typically unique to each driver
> + * number: or function
> + *
> + * For example, 0x89 is a socket type, and number 0x27 is the get hardware
> + * address function.
> + */
> +int avc_has_operation(u32 ssid, u32 tsid, u16 tclass, u32 requested,
> + u16 cmd, struct common_audit_data *ad)
> +{
> + struct avc_node *node;
> + struct av_decision avd;
> + u32 denied;
> + struct operation_decision *od = NULL;
> + struct operation_decision od_local;
> + struct operation_perm allowed;
> + struct operation_perm auditallow;
> + struct operation_perm dontaudit;
> + struct avc_operation_node local_ops_node;
> + struct avc_operation_node *ops_node;
> + u8 type = cmd >> 8;
> + int rc = 0, rc2;
> +
> + ops_node = &local_ops_node;
> + BUG_ON(!requested);
> +
> + rcu_read_lock();
> +
> + node = avc_lookup(ssid, tsid, tclass);
> + if (unlikely(!node)) {
> + node = avc_compute_av(ssid, tsid, tclass, &avd, ops_node);
> + } else {
> + memcpy(&avd, &node->ae.avd, sizeof(avd));
> + ops_node = node->ae.ops_node;
> + }
> + /* if operations are not defined, only consider av_decision */
> + if (!ops_node || !ops_node->ops.len)
> + goto decision;
> +
> + od_local.allowed = &allowed;
> + od_local.auditallow = &auditallow;
> + od_local.dontaudit = &dontaudit;
> +
> + /* lookup operation decision */
> + od = avc_operation_lookup(type, ops_node);
> + if (unlikely(!od)) {
> + /* Compute operation decision if type is flagged */
> + if (!security_operation_test(ops_node->ops.type, type)) {
> + avd.allowed &= ~requested;
> + goto decision;
> + }
> + rcu_read_unlock();
> + security_compute_operation(ssid, tsid, tclass, type, &od_local);
> + rcu_read_lock();
> + avc_update_node(AVC_CALLBACK_ADD_OPERATION, requested, cmd,
> + ssid, tsid, tclass, avd.seqno, &od_local, 0);
> + } else {
> + avc_quick_copy_operation_decision(cmd, &od_local, od);
> + }
> + od = &od_local;
> +
> + if (!avc_operation_has_perm(od, cmd, OPERATION_ALLOWED))
> + avd.allowed &= ~requested;
> +
> +decision:
> + denied = requested & ~(avd.allowed);
> + if (unlikely(denied))
> + rc = avc_denied(ssid, tsid, tclass, requested, cmd,
> + AVC_OPERATION_CMD, &avd);
> +
> + rcu_read_unlock();
> +
> + rc2 = avc_operation_audit(ssid, tsid, tclass, requested,
> + &avd, od, cmd, rc, ad);
> + if (rc2)
> + return rc2;
> + return rc;
> +}
The above function is a good example of what this patchset being very close to
a general avc expansion with some minor tweaks. With the exception of the
ioctl cmd/type encoding/handling it really is generic function; splitting this
single function into a generic version and an ioctl specific wrapper would be
a good thing.
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 4d1a541..0fe64bb 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -3239,6 +3239,44 @@ static void selinux_file_free_security(struct file
> *file) file_free_security(file);
> }
>
> +/*
> + * Check whether a task has the ioctl permission and cmd
> + * operation to an inode.
> + */
> +int ioctl_has_perm(const struct cred *cred, struct file *file,
> + u32 requested, u16 cmd)
> +{
> + struct common_audit_data ad;
> + struct file_security_struct *fsec = file->f_security;
> + struct inode *inode = file_inode(file);
> + struct inode_security_struct *isec = inode->i_security;
> + struct lsm_ioctlop_audit ioctl;
> + u32 ssid = cred_sid(cred);
> + int rc;
> +
> + ad.type = LSM_AUDIT_DATA_IOCTL_OP;
> + ad.u.op = &ioctl;
> + ad.u.op->cmd = cmd;
> + ad.u.op->path = file->f_path;
> +
> + if (ssid != fsec->sid) {
> + rc = avc_has_perm(ssid, fsec->sid,
> + SECCLASS_FD,
> + FD__USE,
> + &ad);
> + if (rc)
> + goto out;
> + }
> +
> + if (unlikely(IS_PRIVATE(inode)))
> + return 0;
> +
> + rc = avc_has_operation(ssid, isec->sid, isec->sclass,
> + requested, cmd, &ad);
> +out:
> + return rc;
> +}
If we generalize things, I imagine we would replace the "avc_has_operation"
call with something ioctl specific, or maybe an ioctl parameter ... let's see
where the discussion/investigation takes us.
> diff --git a/security/selinux/include/security.h
> b/security/selinux/include/security.h index d1e0b23..9f4f7cb 100644
> --- a/security/selinux/include/security.h
> +++ b/security/selinux/include/security.h
> @@ -35,13 +35,14 @@
> #define POLICYDB_VERSION_NEW_OBJECT_DEFAULTS 27
> #define POLICYDB_VERSION_DEFAULT_TYPE 28
> #define POLICYDB_VERSION_CONSTRAINT_NAMES 29
> +#define POLICYDB_VERSION_IOCTL_OPERATIONS 30
>
> /* Range of policy versions we understand*/
> #define POLICYDB_VERSION_MIN POLICYDB_VERSION_BASE
> #ifdef CONFIG_SECURITY_SELINUX_POLICYDB_VERSION_MAX
> #define
> POLICYDB_VERSION_MAX CONFIG_SECURITY_SELINUX_POLICYDB_VERSION_MAX_VALUE
> #else
> -#define POLICYDB_VERSION_MAX POLICYDB_VERSION_CONSTRAINT_NAMES
> +#define POLICYDB_VERSION_MAX POLICYDB_VERSION_IOCTL_OPERATIONS
> #endif
>
> /* Mask for just the mount related flags */
> @@ -108,11 +109,40 @@ struct av_decision {
> u32 flags;
> };
>
> +#define security_operation_set(perms, x) (perms[x >> 5] |= 1 << (x & 0x1f))
> +#define security_operation_test(perms, x) (1 & (perms[x >> 5] >> (x &
> 0x1f)))
See the other comments about "operation" vs. "extop" or similar.
> +
> +struct operation_perm {
> + u32 perms[8];
> +};
It might be nice to put "8" in a #define somewhere. Ideally it would be nice
to make this variable, but likely not worth the effort at this point in time;
256 bits is fine for ioctls and it makes the code a bit more tidy.
> +struct operation_decision {
> + u8 type;
> + u8 specified;
> + struct operation_perm *allowed;
> + struct operation_perm *auditallow;
> + struct operation_perm *dontaudit;
> +};
Beyond the "operation" name discussion, we *need* to find another name for the
"type" field; it simply carries too many meanings and has special meaning
within the context of SELinux.
It would also be nice to have some comments here explaining the fields.
> +#define OPERATION_ALLOWED 1
> +#define OPERATION_AUDITALLOW 2
> +#define OPERATION_DONTAUDIT 4
> +#define OPERATION_ALL (OPERATION_ALLOWED | OPERATION_AUDITALLOW |\
> + OPERATION_DONTAUDIT)
> +struct operation {
> + u16 len; /* length of operation decision chain */
> + u32 type[8]; /* 256 types */
> +};
Name. Also, I expect this struct will change if we generalize things.
> @@ -429,11 +456,15 @@ int avtab_read_item(struct avtab *a, void *fp, struct
> policydb *pol, printk(KERN_ERR "SELinux: avtab: entry has both access
> vectors and types\n"); return -EINVAL;
> }
> + if (val & AVTAB_OP) {
> + printk(KERN_ERR "SELinux: avtab: entry has operations\n");
> + return -EINVAL;
> + }
Another "operations" vs. "extop" or similar. If we generalize, it would also
be nice to know what kind of extended operations, e.g. ioctl commands.
Further, beyond the extension type (ioctl), I think it would be nice to
include a size value in the binary policy. With the current ioctl code it
would be 8/256, but we might want to make this variable in the future and it
would be nice not to have to bump the policy format again.
--
paul moore
www.paul-moore.com
_______________________________________________
Selinux mailing list
Selinux@xxxxxxxxxxxxx
To unsubscribe, send email to Selinux-leave@xxxxxxxxxxxxx.
To get help, send an email containing "help" to Selinux-request@xxxxxxxxxxxxx.
