[PATCH 1/3] vfs: re-introduce MAY_CHDIR

[PATCH 1/3] vfs: re-introduce MAY_CHDIR

[Eric Paris]

Currently MAY_ACCESS means that filesystems must check the permissions
right then and not rely on cached results or the results of future
operations on the object.  This can be because of a call to sys_access() or
because of a call to chdir() which needs to check search without relying on
any future operations inside that dir.  I plan to use MAY_ACCESS for other
purposes in the security system, so I split the MAY_ACCESS and the
MAY_CHDIR cases.

Signed-off-by: Eric Paris <eparis-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---

 fs/fuse/dir.c      |    2 +-
 fs/nfs/dir.c       |    2 +-
 fs/open.c          |    6 +++---
 include/linux/fs.h |    1 +
 4 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
index 4787ae6..7c8c55b 100644
--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -1016,7 +1016,7 @@ static int fuse_permission(struct inode *inode, int mask)
 		   exist.  So if permissions are revoked this won't be
 		   noticed immediately, only after the attribute
 		   timeout has expired */
-	} else if (mask & MAY_ACCESS) {
+	} else if (mask & (MAY_ACCESS | MAY_CHDIR)) {
 		err = fuse_access(inode, mask);
 	} else if ((mask & MAY_EXEC) && S_ISREG(inode->i_mode)) {
 		if (!(inode->i_mode & S_IXUGO)) {
diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index be46f26..4c7d8fc 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -1927,7 +1927,7 @@ int nfs_permission(struct inode *inode, int mask)
 	if ((mask & (MAY_READ | MAY_WRITE | MAY_EXEC)) == 0)
 		goto out;
 	/* Is this sys_access() ? */
-	if (mask & MAY_ACCESS)
+	if (mask & (MAY_ACCESS | MAY_CHDIR))
 		goto force_lookup;
 
 	switch (inode->i_mode & S_IFMT) {
diff --git a/fs/open.c b/fs/open.c
index b93eac3..d01e116 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -534,7 +534,7 @@ SYSCALL_DEFINE1(chdir, const char __user *, filename)
 	if (error)
 		goto out;
 
-	error = inode_permission(path.dentry->d_inode, MAY_EXEC | MAY_ACCESS);
+	error = inode_permission(path.dentry->d_inode, MAY_EXEC | MAY_CHDIR);
 	if (error)
 		goto dput_and_out;
 
@@ -563,7 +563,7 @@ SYSCALL_DEFINE1(fchdir, unsigned int, fd)
 	if (!S_ISDIR(inode->i_mode))
 		goto out_putf;
 
-	error = inode_permission(inode, MAY_EXEC | MAY_ACCESS);
+	error = inode_permission(inode, MAY_EXEC | MAY_CHDIR);
 	if (!error)
 		set_fs_pwd(current->fs, &file->f_path);
 out_putf:
@@ -581,7 +581,7 @@ SYSCALL_DEFINE1(chroot, const char __user *, filename)
 	if (error)
 		goto out;
 
-	error = inode_permission(path.dentry->d_inode, MAY_EXEC | MAY_ACCESS);
+	error = inode_permission(path.dentry->d_inode, MAY_EXEC | MAY_CHDIR);
 	if (error)
 		goto dput_and_out;
 
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 14d8597..188d3e4 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -53,6 +53,7 @@ struct inodes_stat_t {
 #define MAY_APPEND 8
 #define MAY_ACCESS 16
 #define MAY_OPEN 32
+#define MAY_CHDIR 64
 
 /*
  * flags in file.f_mode.  Note that FMODE_READ and FMODE_WRITE must correspond

[PATCH 2/3] security: make LSMs explicitly mask off permissions

[Eric Paris]

SELinux needs to pass the MAY_ACCESS flag so it can handle auditting
correctly.  Presently the masking of MAY_* flags is done in the VFS.  In
order to allow LSMs to decide what flags they care about and what flags
they don't just pass them all and the each LSM mask off what they don't
need.  This patch should contain no functional changes to either the VFS or
any LSM.

Signed-off-by: Eric Paris <eparis-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---

 fs/namei.c                 |    3 +--
 security/selinux/hooks.c   |    2 ++
 security/smack/smack_lsm.c |    2 ++
 3 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index f068192..3b0f583 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -282,8 +282,7 @@ int inode_permission(struct inode *inode, int mask)
 	if (retval)
 		return retval;
 
-	return security_inode_permission(inode,
-			mask & (MAY_READ|MAY_WRITE|MAY_EXEC|MAY_APPEND));
+	return security_inode_permission(inode, mask);
 }
 
 /**
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 740a71f..344ba62 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -2700,6 +2700,8 @@ static int selinux_inode_permission(struct inode *inode, int mask)
 {
 	const struct cred *cred = current_cred();
 
+	mask &= (MAY_READ|MAY_WRITE|MAY_EXEC|MAY_APPEND);
+
 	if (!mask) {
 		/* No permission to check.  Existence test. */
 		return 0;
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index f1b6846..df467f4 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -599,6 +599,8 @@ static int smack_inode_rename(struct inode *old_inode,
 static int smack_inode_permission(struct inode *inode, int mask)
 {
 	struct smk_audit_info ad;
+
+	mask &= (MAY_READ|MAY_WRITE|MAY_EXEC|MAY_APPEND);
 	/*
 	 * No permission to check. Existence test. Yup, it's there.
 	 */

[PATCH 3/3] SELinux: special dontaudit for access checks

[Eric Paris]

Currently there are a number of applications (nautilus being the main one) which
calls access() on files in order to determine how they should be displayed.  It
is normal and expected that nautilus will want to see if files are executable
or if they are really read/write-able.  access() should return the real
permission.  SELinux policy checks are done in access() and can result in lots
of AVC denials as policy denies RWX on files which DAC allows.  Currently
SELinux must dontaudit actual attempts to read/write/execute a file in
order to silence these messages (and not flood the logs.)  But dontaudit rules
like that can hide real attacks.  This patch addes a new common file
permission audit_access.  This permission is special in that it is meaningless
and should never show up in an allow rule.  Instead the only place this
permission has meaning is in a dontaudit rule like so:

dontaudit nautilus_t sbin_t:file audit_access

With such a rule if nautilus just checks access() we will still get denied and
thus userspace will still get the correct answer but we will not log the denial.
If nautilus attempted to actually perform one of the forbidden actions
(rather than just querying access(2) about it) we would still log a denial.
This type of dontaudit rule should be used sparingly, as it could be a
method for an attacker to probe the system permissions without detection.

Signed-off-by: Eric Paris <eparis-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---

 security/selinux/hooks.c            |   46 ++++++++++++++++++++++++++++++-----
 security/selinux/include/classmap.h |    2 +-
 2 files changed, 40 insertions(+), 8 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 344ba62..34e9d1b 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -2696,19 +2696,51 @@ static int selinux_inode_follow_link(struct dentry *dentry, struct nameidata *na
 	return dentry_has_perm(cred, NULL, dentry, FILE__READ);
 }
 
-static int selinux_inode_permission(struct inode *inode, int mask)
+static int selinux_inode_permission(struct inode *inode, int in_mask)
 {
 	const struct cred *cred = current_cred();
+	struct inode_security_struct *isec;
+	struct common_audit_data ad;
+	struct av_decision avd;
+	u32 sid, perms;
+	int rc, mask;
 
-	mask &= (MAY_READ|MAY_WRITE|MAY_EXEC|MAY_APPEND);
+	mask = in_mask & (MAY_READ|MAY_WRITE|MAY_EXEC|MAY_APPEND);
 
-	if (!mask) {
-		/* No permission to check.  Existence test. */
+	/* No permission to check.  Existence test. */
+	if (!mask)
+		return 0;
+
+	validate_creds(cred);
+
+	if (unlikely(IS_PRIVATE(inode)))
 		return 0;
-	}
 
-	return inode_has_perm(cred, inode,
-			      file_mask_to_av(inode->i_mode, mask), NULL);
+	sid = cred_sid(cred);
+	isec = inode->i_security;
+
+	COMMON_AUDIT_DATA_INIT(&ad, FS);
+	ad.u.fs.inode = inode;
+
+	perms = file_mask_to_av(inode->i_mode, mask);
+
+	rc = avc_has_perm_noaudit(sid, isec->sid, isec->sclass, perms, 0, &avd);
+	/*
+	 * We want to audit if this call was not from access(2).
+	 * We also want to audit if the call was from access(2)
+	 * but the magic FILE__AUDIT_ACCESS permission was in the auditdeny
+	 * vector.
+	 *
+	 * aka there is a not dontaudit rule for file__audit_access.  This
+	 * might make more sense as a test inside avc_audit, but then we would
+	 * have to push the MAY_ACCESS flag down to avc_audit and I think we
+	 * already have enough stuff down there.
+	 */
+	if (!(in_mask & MAY_ACCESS) ||
+	    (avd.auditdeny & FILE__AUDIT_ACCESS))
+		avc_audit(sid, isec->sid, isec->sclass, perms, &avd, rc, &ad);
+
+	return rc;
 }
 
 static int selinux_inode_setattr(struct dentry *dentry, struct iattr *iattr)
diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
index 8b32e95..d64603e 100644
--- a/security/selinux/include/classmap.h
+++ b/security/selinux/include/classmap.h
@@ -2,7 +2,7 @@
     "getattr", "setattr", "lock", "relabelfrom", "relabelto", "append"
 
 #define COMMON_FILE_PERMS COMMON_FILE_SOCK_PERMS, "unlink", "link", \
-    "rename", "execute", "swapon", "quotaon", "mounton"
+    "rename", "execute", "swapon", "quotaon", "mounton", "audit_access"
 
 #define COMMON_SOCK_PERMS COMMON_FILE_SOCK_PERMS, "bind", "connect", \
     "listen", "accept", "getopt", "setopt", "shutdown", "recvfrom",  \

[PATCH 1/3] vfs: re-introduce MAY_CHDIR

[Eric Paris]

Currently MAY_ACCESS means that filesystems must check the permissions
right then and not rely on cached results or the results of future
operations on the object.  This can be because of a call to sys_access() or
because of a call to chdir() which needs to check search without relying on
any future operations inside that dir.  I plan to use MAY_ACCESS for other
purposes in the security system, so I split the MAY_ACCESS and the
MAY_CHDIR cases.

Signed-off-by: Eric Paris <eparis@redhat.com>
---

 fs/fuse/dir.c      |    2 +-
 fs/nfs/dir.c       |    2 +-
 fs/open.c          |    6 +++---
 include/linux/fs.h |    1 +
 4 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
index 4787ae6..7c8c55b 100644
--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -1016,7 +1016,7 @@ static int fuse_permission(struct inode *inode, int mask)
 		   exist.  So if permissions are revoked this won't be
 		   noticed immediately, only after the attribute
 		   timeout has expired */
-	} else if (mask & MAY_ACCESS) {
+	} else if (mask & (MAY_ACCESS | MAY_CHDIR)) {
 		err = fuse_access(inode, mask);
 	} else if ((mask & MAY_EXEC) && S_ISREG(inode->i_mode)) {
 		if (!(inode->i_mode & S_IXUGO)) {
diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index be46f26..4c7d8fc 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -1927,7 +1927,7 @@ int nfs_permission(struct inode *inode, int mask)
 	if ((mask & (MAY_READ | MAY_WRITE | MAY_EXEC)) == 0)
 		goto out;
 	/* Is this sys_access() ? */
-	if (mask & MAY_ACCESS)
+	if (mask & (MAY_ACCESS | MAY_CHDIR))
 		goto force_lookup;
 
 	switch (inode->i_mode & S_IFMT) {
diff --git a/fs/open.c b/fs/open.c
index b93eac3..d01e116 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -534,7 +534,7 @@ SYSCALL_DEFINE1(chdir, const char __user *, filename)
 	if (error)
 		goto out;
 
-	error = inode_permission(path.dentry->d_inode, MAY_EXEC | MAY_ACCESS);
+	error = inode_permission(path.dentry->d_inode, MAY_EXEC | MAY_CHDIR);
 	if (error)
 		goto dput_and_out;
 
@@ -563,7 +563,7 @@ SYSCALL_DEFINE1(fchdir, unsigned int, fd)
 	if (!S_ISDIR(inode->i_mode))
 		goto out_putf;
 
-	error = inode_permission(inode, MAY_EXEC | MAY_ACCESS);
+	error = inode_permission(inode, MAY_EXEC | MAY_CHDIR);
 	if (!error)
 		set_fs_pwd(current->fs, &file->f_path);
 out_putf:
@@ -581,7 +581,7 @@ SYSCALL_DEFINE1(chroot, const char __user *, filename)
 	if (error)
 		goto out;
 
-	error = inode_permission(path.dentry->d_inode, MAY_EXEC | MAY_ACCESS);
+	error = inode_permission(path.dentry->d_inode, MAY_EXEC | MAY_CHDIR);
 	if (error)
 		goto dput_and_out;
 
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 14d8597..188d3e4 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -53,6 +53,7 @@ struct inodes_stat_t {
 #define MAY_APPEND 8
 #define MAY_ACCESS 16
 #define MAY_OPEN 32
+#define MAY_CHDIR 64
 
 /*
  * flags in file.f_mode.  Note that FMODE_READ and FMODE_WRITE must correspond

[PATCH 3/3] SELinux: special dontaudit for access checks

[Eric Paris]

Currently there are a number of applications (nautilus being the main one) which
calls access() on files in order to determine how they should be displayed.  It
is normal and expected that nautilus will want to see if files are executable
or if they are really read/write-able.  access() should return the real
permission.  SELinux policy checks are done in access() and can result in lots
of AVC denials as policy denies RWX on files which DAC allows.  Currently
SELinux must dontaudit actual attempts to read/write/execute a file in
order to silence these messages (and not flood the logs.)  But dontaudit rules
like that can hide real attacks.  This patch addes a new common file
permission audit_access.  This permission is special in that it is meaningless
and should never show up in an allow rule.  Instead the only place this
permission has meaning is in a dontaudit rule like so:

dontaudit nautilus_t sbin_t:file audit_access

With such a rule if nautilus just checks access() we will still get denied and
thus userspace will still get the correct answer but we will not log the denial.
If nautilus attempted to actually perform one of the forbidden actions
(rather than just querying access(2) about it) we would still log a denial.
This type of dontaudit rule should be used sparingly, as it could be a
method for an attacker to probe the system permissions without detection.

Signed-off-by: Eric Paris <eparis@redhat.com>
---

 security/selinux/hooks.c            |   46 ++++++++++++++++++++++++++++++-----
 security/selinux/include/classmap.h |    2 +-
 2 files changed, 40 insertions(+), 8 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 344ba62..34e9d1b 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -2696,19 +2696,51 @@ static int selinux_inode_follow_link(struct dentry *dentry, struct nameidata *na
 	return dentry_has_perm(cred, NULL, dentry, FILE__READ);
 }
 
-static int selinux_inode_permission(struct inode *inode, int mask)
+static int selinux_inode_permission(struct inode *inode, int in_mask)
 {
 	const struct cred *cred = current_cred();
+	struct inode_security_struct *isec;
+	struct common_audit_data ad;
+	struct av_decision avd;
+	u32 sid, perms;
+	int rc, mask;
 
-	mask &= (MAY_READ|MAY_WRITE|MAY_EXEC|MAY_APPEND);
+	mask = in_mask & (MAY_READ|MAY_WRITE|MAY_EXEC|MAY_APPEND);
 
-	if (!mask) {
-		/* No permission to check.  Existence test. */
+	/* No permission to check.  Existence test. */
+	if (!mask)
+		return 0;
+
+	validate_creds(cred);
+
+	if (unlikely(IS_PRIVATE(inode)))
 		return 0;
-	}
 
-	return inode_has_perm(cred, inode,
-			      file_mask_to_av(inode->i_mode, mask), NULL);
+	sid = cred_sid(cred);
+	isec = inode->i_security;
+
+	COMMON_AUDIT_DATA_INIT(&ad, FS);
+	ad.u.fs.inode = inode;
+
+	perms = file_mask_to_av(inode->i_mode, mask);
+
+	rc = avc_has_perm_noaudit(sid, isec->sid, isec->sclass, perms, 0, &avd);
+	/*
+	 * We want to audit if this call was not from access(2).
+	 * We also want to audit if the call was from access(2)
+	 * but the magic FILE__AUDIT_ACCESS permission was in the auditdeny
+	 * vector.
+	 *
+	 * aka there is a not dontaudit rule for file__audit_access.  This
+	 * might make more sense as a test inside avc_audit, but then we would
+	 * have to push the MAY_ACCESS flag down to avc_audit and I think we
+	 * already have enough stuff down there.
+	 */
+	if (!(in_mask & MAY_ACCESS) ||
+	    (avd.auditdeny & FILE__AUDIT_ACCESS))
+		avc_audit(sid, isec->sid, isec->sclass, perms, &avd, rc, &ad);
+
+	return rc;
 }
 
 static int selinux_inode_setattr(struct dentry *dentry, struct iattr *iattr)
diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
index 8b32e95..d64603e 100644
--- a/security/selinux/include/classmap.h
+++ b/security/selinux/include/classmap.h
@@ -2,7 +2,7 @@
     "getattr", "setattr", "lock", "relabelfrom", "relabelto", "append"
 
 #define COMMON_FILE_PERMS COMMON_FILE_SOCK_PERMS, "unlink", "link", \
-    "rename", "execute", "swapon", "quotaon", "mounton"
+    "rename", "execute", "swapon", "quotaon", "mounton", "audit_access"
 
 #define COMMON_SOCK_PERMS COMMON_FILE_SOCK_PERMS, "bind", "connect", \
     "listen", "accept", "getopt", "setopt", "shutdown", "recvfrom",  \

Re: [PATCH 3/3] SELinux: special dontaudit for access checks

[Stephen Smalley]

On Fri, 2010-04-09 at 18:16 -0400, Eric Paris wrote:
> Currently there are a number of applications (nautilus being the main one) which
> calls access() on files in order to determine how they should be displayed.  It
> is normal and expected that nautilus will want to see if files are executable
> or if they are really read/write-able.  access() should return the real
> permission.  SELinux policy checks are done in access() and can result in lots
> of AVC denials as policy denies RWX on files which DAC allows.  Currently
> SELinux must dontaudit actual attempts to read/write/execute a file in
> order to silence these messages (and not flood the logs.)  But dontaudit rules
> like that can hide real attacks.  This patch addes a new common file
> permission audit_access.  This permission is special in that it is meaningless
> and should never show up in an allow rule.  Instead the only place this
> permission has meaning is in a dontaudit rule like so:
> 
> dontaudit nautilus_t sbin_t:file audit_access
> 
> With such a rule if nautilus just checks access() we will still get denied and
> thus userspace will still get the correct answer but we will not log the denial.
> If nautilus attempted to actually perform one of the forbidden actions
> (rather than just querying access(2) about it) we would still log a denial.
> This type of dontaudit rule should be used sparingly, as it could be a
> method for an attacker to probe the system permissions without detection.

So let's think about how this will likely play out in practice.
If you add this check, what rules will Dan add to the standard policy?
nautilus doesn't run in a separate domain nor is it likely to do so
(otherwise you have to clone all of the user's permissions to it).  So
we'll likely end up with something like:
	dontaudit userdomain file_type:file audit_access;

Right?

> Signed-off-by: Eric Paris <eparis@redhat.com>
> ---
> 
>  security/selinux/hooks.c            |   46 ++++++++++++++++++++++++++++++-----
>  security/selinux/include/classmap.h |    2 +-
>  2 files changed, 40 insertions(+), 8 deletions(-)
> 
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 344ba62..34e9d1b 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -2696,19 +2696,51 @@ static int selinux_inode_follow_link(struct dentry *dentry, struct nameidata *na
>  	return dentry_has_perm(cred, NULL, dentry, FILE__READ);
>  }
>  
> -static int selinux_inode_permission(struct inode *inode, int mask)
> +static int selinux_inode_permission(struct inode *inode, int in_mask)
>  {
>  	const struct cred *cred = current_cred();
> +	struct inode_security_struct *isec;
> +	struct common_audit_data ad;
> +	struct av_decision avd;
> +	u32 sid, perms;
> +	int rc, mask;
>  
> -	mask &= (MAY_READ|MAY_WRITE|MAY_EXEC|MAY_APPEND);
> +	mask = in_mask & (MAY_READ|MAY_WRITE|MAY_EXEC|MAY_APPEND);
>  
> -	if (!mask) {
> -		/* No permission to check.  Existence test. */
> +	/* No permission to check.  Existence test. */
> +	if (!mask)
> +		return 0;
> +
> +	validate_creds(cred);
> +
> +	if (unlikely(IS_PRIVATE(inode)))
>  		return 0;

This is handled by security_inode_permission().  The check inside
inode_has_perm() stems from other code paths.

> -	}
>  
> -	return inode_has_perm(cred, inode,
> -			      file_mask_to_av(inode->i_mode, mask), NULL);
> +	sid = cred_sid(cred);
> +	isec = inode->i_security;
> +
> +	COMMON_AUDIT_DATA_INIT(&ad, FS);
> +	ad.u.fs.inode = inode;
> +
> +	perms = file_mask_to_av(inode->i_mode, mask);
> +
> +	rc = avc_has_perm_noaudit(sid, isec->sid, isec->sclass, perms, 0, &avd);
> +	/*
> +	 * We want to audit if this call was not from access(2).
> +	 * We also want to audit if the call was from access(2)
> +	 * but the magic FILE__AUDIT_ACCESS permission was in the auditdeny
> +	 * vector.
> +	 *
> +	 * aka there is a not dontaudit rule for file__audit_access.  This
> +	 * might make more sense as a test inside avc_audit, but then we would
> +	 * have to push the MAY_ACCESS flag down to avc_audit and I think we
> +	 * already have enough stuff down there.
> +	 */

Why can't we just push it down through inode_has_perm -> avc_has_perm ->
avc_audit() via a field in common_audit_data?

> +	if (!(in_mask & MAY_ACCESS) ||
> +	    (avd.auditdeny & FILE__AUDIT_ACCESS))
> +		avc_audit(sid, isec->sid, isec->sclass, perms, &avd, rc, &ad);
> +
> +	return rc;
>  }
>  


-- 
Stephen Smalley
National Security Agency

Re: [PATCH 3/3] SELinux: special dontaudit for access checks

[Stephen Smalley]

On Tue, 2010-04-27 at 09:47 -0400, Stephen Smalley wrote:
> On Fri, 2010-04-09 at 18:16 -0400, Eric Paris wrote:
> > Currently there are a number of applications (nautilus being the main one) which
> > calls access() on files in order to determine how they should be displayed.  It
> > is normal and expected that nautilus will want to see if files are executable
> > or if they are really read/write-able.  access() should return the real
> > permission.  SELinux policy checks are done in access() and can result in lots
> > of AVC denials as policy denies RWX on files which DAC allows.  Currently
> > SELinux must dontaudit actual attempts to read/write/execute a file in
> > order to silence these messages (and not flood the logs.)  But dontaudit rules
> > like that can hide real attacks.  This patch addes a new common file
> > permission audit_access.  This permission is special in that it is meaningless
> > and should never show up in an allow rule.  Instead the only place this
> > permission has meaning is in a dontaudit rule like so:
> > 
> > dontaudit nautilus_t sbin_t:file audit_access
> > 
> > With such a rule if nautilus just checks access() we will still get denied and
> > thus userspace will still get the correct answer but we will not log the denial.
> > If nautilus attempted to actually perform one of the forbidden actions
> > (rather than just querying access(2) about it) we would still log a denial.
> > This type of dontaudit rule should be used sparingly, as it could be a
> > method for an attacker to probe the system permissions without detection.
> 
> So let's think about how this will likely play out in practice.
> If you add this check, what rules will Dan add to the standard policy?
> nautilus doesn't run in a separate domain nor is it likely to do so
> (otherwise you have to clone all of the user's permissions to it).  So
> we'll likely end up with something like:
> 	dontaudit userdomain file_type:file audit_access;
> 
> Right?
> 
> > Signed-off-by: Eric Paris <eparis@redhat.com>
> > ---
> > 
> >  security/selinux/hooks.c            |   46 ++++++++++++++++++++++++++++++-----
> >  security/selinux/include/classmap.h |    2 +-
> >  2 files changed, 40 insertions(+), 8 deletions(-)
> > 
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index 344ba62..34e9d1b 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -2696,19 +2696,51 @@ static int selinux_inode_follow_link(struct dentry *dentry, struct nameidata *na
> >  	return dentry_has_perm(cred, NULL, dentry, FILE__READ);
> >  }
> >  
> > -static int selinux_inode_permission(struct inode *inode, int mask)
> > +static int selinux_inode_permission(struct inode *inode, int in_mask)
> >  {
> >  	const struct cred *cred = current_cred();
> > +	struct inode_security_struct *isec;
> > +	struct common_audit_data ad;
> > +	struct av_decision avd;
> > +	u32 sid, perms;
> > +	int rc, mask;
> >  
> > -	mask &= (MAY_READ|MAY_WRITE|MAY_EXEC|MAY_APPEND);
> > +	mask = in_mask & (MAY_READ|MAY_WRITE|MAY_EXEC|MAY_APPEND);
> >  
> > -	if (!mask) {
> > -		/* No permission to check.  Existence test. */
> > +	/* No permission to check.  Existence test. */
> > +	if (!mask)
> > +		return 0;
> > +
> > +	validate_creds(cred);
> > +
> > +	if (unlikely(IS_PRIVATE(inode)))
> >  		return 0;
> 
> This is handled by security_inode_permission().  The check inside
> inode_has_perm() stems from other code paths.
> 
> > -	}
> >  
> > -	return inode_has_perm(cred, inode,
> > -			      file_mask_to_av(inode->i_mode, mask), NULL);
> > +	sid = cred_sid(cred);
> > +	isec = inode->i_security;
> > +
> > +	COMMON_AUDIT_DATA_INIT(&ad, FS);
> > +	ad.u.fs.inode = inode;
> > +
> > +	perms = file_mask_to_av(inode->i_mode, mask);
> > +
> > +	rc = avc_has_perm_noaudit(sid, isec->sid, isec->sclass, perms, 0, &avd);
> > +	/*
> > +	 * We want to audit if this call was not from access(2).
> > +	 * We also want to audit if the call was from access(2)
> > +	 * but the magic FILE__AUDIT_ACCESS permission was in the auditdeny
> > +	 * vector.
> > +	 *
> > +	 * aka there is a not dontaudit rule for file__audit_access.  This
> > +	 * might make more sense as a test inside avc_audit, but then we would
> > +	 * have to push the MAY_ACCESS flag down to avc_audit and I think we
> > +	 * already have enough stuff down there.
> > +	 */
> 
> Why can't we just push it down through inode_has_perm -> avc_has_perm ->
> avc_audit() via a field in common_audit_data?

e.g.
static int selinux_inode_permission(struct inode *inode, int mask)
{
        const struct cred *cred = current_cred();
	struct common_audit_data ad;
	int access;

	access = mask & MAY_ACCESS;
	mask &= (MAY_READ|MAY_WRITE|MAY_EXEC|MAY_APPEND);

        if (!mask) {
                /* No permission to check.  Existence test. */
                return 0;
        }

	COMMON_AUDIT_DATA_INIT(&ad, FS);
	ad.u.fs.inode = inode;

	if (access)
		ad.selinux_audit_data.auditdeny = FILE__AUDIT_ACCESS;

        return inode_has_perm(cred, inode,
                              file_mask_to_av(inode->i_mode, mask), &ad);
}

And then test a->selinux_audit_data.auditdeny inside of avc_audit() and
use it if set.
	
-- 
Stephen Smalley
National Security Agency

Re: [PATCH 3/3] SELinux: special dontaudit for access checks

[Eric Paris]

On Tue, 2010-04-27 at 09:47 -0400, Stephen Smalley wrote:
> On Fri, 2010-04-09 at 18:16 -0400, Eric Paris wrote:
> > Currently there are a number of applications (nautilus being the main one) which
> > calls access() on files in order to determine how they should be displayed.  It
> > is normal and expected that nautilus will want to see if files are executable
> > or if they are really read/write-able.  access() should return the real
> > permission.  SELinux policy checks are done in access() and can result in lots
> > of AVC denials as policy denies RWX on files which DAC allows.  Currently
> > SELinux must dontaudit actual attempts to read/write/execute a file in
> > order to silence these messages (and not flood the logs.)  But dontaudit rules
> > like that can hide real attacks.  This patch addes a new common file
> > permission audit_access.  This permission is special in that it is meaningless
> > and should never show up in an allow rule.  Instead the only place this
> > permission has meaning is in a dontaudit rule like so:
> > 
> > dontaudit nautilus_t sbin_t:file audit_access
> > 
> > With such a rule if nautilus just checks access() we will still get denied and
> > thus userspace will still get the correct answer but we will not log the denial.
> > If nautilus attempted to actually perform one of the forbidden actions
> > (rather than just querying access(2) about it) we would still log a denial.
> > This type of dontaudit rule should be used sparingly, as it could be a
> > method for an attacker to probe the system permissions without detection.
> 
> So let's think about how this will likely play out in practice.
> If you add this check, what rules will Dan add to the standard policy?
> nautilus doesn't run in a separate domain nor is it likely to do so
> (otherwise you have to clone all of the user's permissions to it).  So
> we'll likely end up with something like:
> 	dontaudit userdomain file_type:file audit_access;

Yes, although I think it should likely be kept out of some of them
namely:
(*_mono_t, *_java_t, *_execmem_t)


> > +	rc = avc_has_perm_noaudit(sid, isec->sid, isec->sclass, perms, 0, &avd);
> > +	/*
> > +	 * We want to audit if this call was not from access(2).
> > +	 * We also want to audit if the call was from access(2)
> > +	 * but the magic FILE__AUDIT_ACCESS permission was in the auditdeny
> > +	 * vector.
> > +	 *
> > +	 * aka there is a not dontaudit rule for file__audit_access.  This
> > +	 * might make more sense as a test inside avc_audit, but then we would
> > +	 * have to push the MAY_ACCESS flag down to avc_audit and I think we
> > +	 * already have enough stuff down there.
> > +	 */
> 
> Why can't we just push it down through inode_has_perm -> avc_has_perm ->
> avc_audit() via a field in common_audit_data?

Ok, I like that idea.  I'll probably put it in a flag in the SELinux
private union in the common_audit_data but I'll figure that out when I
look at exactly what we already have where.

I'm only going to resend #3.  1 and 2 seem to make people happy as they
stand.

-Eric

Re: [PATCH 3/3] SELinux: special dontaudit for access checks

[James Morris]

On Tue, 27 Apr 2010, Eric Paris wrote:

> I'm only going to resend #3.  1 and 2 seem to make people happy as they
> stand.

I don't see any acks or even discussion of patch #1 from the vfs/fs folk.



- James
-- 
James Morris
<jmorris@namei.org>

Re: [PATCH 3/3] SELinux: special dontaudit for access checks

[Daniel J Walsh]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 04/27/2010 09:47 AM, Stephen Smalley wrote:
> On Fri, 2010-04-09 at 18:16 -0400, Eric Paris wrote:
>> Currently there are a number of applications (nautilus being the main one) which
>> calls access() on files in order to determine how they should be displayed.  It
>> is normal and expected that nautilus will want to see if files are executable
>> or if they are really read/write-able.  access() should return the real
>> permission.  SELinux policy checks are done in access() and can result in lots
>> of AVC denials as policy denies RWX on files which DAC allows.  Currently
>> SELinux must dontaudit actual attempts to read/write/execute a file in
>> order to silence these messages (and not flood the logs.)  But dontaudit rules
>> like that can hide real attacks.  This patch addes a new common file
>> permission audit_access.  This permission is special in that it is meaningless
>> and should never show up in an allow rule.  Instead the only place this
>> permission has meaning is in a dontaudit rule like so:
>>
>> dontaudit nautilus_t sbin_t:file audit_access
>>
>> With such a rule if nautilus just checks access() we will still get denied and
>> thus userspace will still get the correct answer but we will not log the denial.
>> If nautilus attempted to actually perform one of the forbidden actions
>> (rather than just querying access(2) about it) we would still log a denial.
>> This type of dontaudit rule should be used sparingly, as it could be a
>> method for an attacker to probe the system permissions without detection.
> 
> So let's think about how this will likely play out in practice.
> If you add this check, what rules will Dan add to the standard policy?
> nautilus doesn't run in a separate domain nor is it likely to do so
> (otherwise you have to clone all of the user's permissions to it).  So
> we'll likely end up with something like:
> 	dontaudit userdomain file_type:file audit_access;
> 
> Right?
> 
>> Signed-off-by: Eric Paris <eparis@redhat.com>
>> ---
>>
>>  security/selinux/hooks.c            |   46 ++++++++++++++++++++++++++++++-----
>>  security/selinux/include/classmap.h |    2 +-
>>  2 files changed, 40 insertions(+), 8 deletions(-)
>>
>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>> index 344ba62..34e9d1b 100644
>> --- a/security/selinux/hooks.c
>> +++ b/security/selinux/hooks.c
>> @@ -2696,19 +2696,51 @@ static int selinux_inode_follow_link(struct dentry *dentry, struct nameidata *na
>>  	return dentry_has_perm(cred, NULL, dentry, FILE__READ);
>>  }
>>  
>> -static int selinux_inode_permission(struct inode *inode, int mask)
>> +static int selinux_inode_permission(struct inode *inode, int in_mask)
>>  {
>>  	const struct cred *cred = current_cred();
>> +	struct inode_security_struct *isec;
>> +	struct common_audit_data ad;
>> +	struct av_decision avd;
>> +	u32 sid, perms;
>> +	int rc, mask;
>>  
>> -	mask &= (MAY_READ|MAY_WRITE|MAY_EXEC|MAY_APPEND);
>> +	mask = in_mask & (MAY_READ|MAY_WRITE|MAY_EXEC|MAY_APPEND);
>>  
>> -	if (!mask) {
>> -		/* No permission to check.  Existence test. */
>> +	/* No permission to check.  Existence test. */
>> +	if (!mask)
>> +		return 0;
>> +
>> +	validate_creds(cred);
>> +
>> +	if (unlikely(IS_PRIVATE(inode)))
>>  		return 0;
> 
> This is handled by security_inode_permission().  The check inside
> inode_has_perm() stems from other code paths.
> 
>> -	}
>>  
>> -	return inode_has_perm(cred, inode,
>> -			      file_mask_to_av(inode->i_mode, mask), NULL);
>> +	sid = cred_sid(cred);
>> +	isec = inode->i_security;
>> +
>> +	COMMON_AUDIT_DATA_INIT(&ad, FS);
>> +	ad.u.fs.inode = inode;
>> +
>> +	perms = file_mask_to_av(inode->i_mode, mask);
>> +
>> +	rc = avc_has_perm_noaudit(sid, isec->sid, isec->sclass, perms, 0, &avd);
>> +	/*
>> +	 * We want to audit if this call was not from access(2).
>> +	 * We also want to audit if the call was from access(2)
>> +	 * but the magic FILE__AUDIT_ACCESS permission was in the auditdeny
>> +	 * vector.
>> +	 *
>> +	 * aka there is a not dontaudit rule for file__audit_access.  This
>> +	 * might make more sense as a test inside avc_audit, but then we would
>> +	 * have to push the MAY_ACCESS flag down to avc_audit and I think we
>> +	 * already have enough stuff down there.
>> +	 */
> 
> Why can't we just push it down through inode_has_perm -> avc_has_perm ->
> avc_audit() via a field in common_audit_data?
> 
>> +	if (!(in_mask & MAY_ACCESS) ||
>> +	    (avd.auditdeny & FILE__AUDIT_ACCESS))
>> +		avc_audit(sid, isec->sid, isec->sclass, perms, &avd, rc, &ad);
>> +
>> +	return rc;
>>  }
>>  
> 
> 
Yes.  Although I can probably start to remove some dontaudit stuff also.


Like every app that uses kernberos has a dontaudit rule

sesearch --dontaudit -t krb5_conf_t -p write -c file | wc
    266    1874   14081


Might be able to remove the pam code that checks write on /etc/shadow also.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.14 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAkvW+RwACgkQrlYvE4MpobNPfACgpUdEf1Wt/FzxmDmqetqv7Csl
4zYAoJeRlIj2Cml4duUFA9hwQy9HAp3g
=BhbJ
-----END PGP SIGNATURE-----

Re: [PATCH 3/3] SELinux: special dontaudit for access checks

[Daniel J Walsh]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 04/27/2010 09:47 AM, Stephen Smalley wrote:
> On Fri, 2010-04-09 at 18:16 -0400, Eric Paris wrote:
>> Currently there are a number of applications (nautilus being the main one) which
>> calls access() on files in order to determine how they should be displayed.  It
>> is normal and expected that nautilus will want to see if files are executable
>> or if they are really read/write-able.  access() should return the real
>> permission.  SELinux policy checks are done in access() and can result in lots
>> of AVC denials as policy denies RWX on files which DAC allows.  Currently
>> SELinux must dontaudit actual attempts to read/write/execute a file in
>> order to silence these messages (and not flood the logs.)  But dontaudit rules
>> like that can hide real attacks.  This patch addes a new common file
>> permission audit_access.  This permission is special in that it is meaningless
>> and should never show up in an allow rule.  Instead the only place this
>> permission has meaning is in a dontaudit rule like so:
>>
>> dontaudit nautilus_t sbin_t:file audit_access
>>
>> With such a rule if nautilus just checks access() we will still get denied and
>> thus userspace will still get the correct answer but we will not log the denial.
>> If nautilus attempted to actually perform one of the forbidden actions
>> (rather than just querying access(2) about it) we would still log a denial.
>> This type of dontaudit rule should be used sparingly, as it could be a
>> method for an attacker to probe the system permissions without detection.
> 
> So let's think about how this will likely play out in practice.
> If you add this check, what rules will Dan add to the standard policy?
> nautilus doesn't run in a separate domain nor is it likely to do so
> (otherwise you have to clone all of the user's permissions to it).  So
> we'll likely end up with something like:
> 	dontaudit userdomain file_type:file audit_access;
> 
> Right?
> 
>> Signed-off-by: Eric Paris <eparis@redhat.com>
>> ---
>>
>>  security/selinux/hooks.c            |   46 ++++++++++++++++++++++++++++++-----
>>  security/selinux/include/classmap.h |    2 +-
>>  2 files changed, 40 insertions(+), 8 deletions(-)
>>
>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>> index 344ba62..34e9d1b 100644
>> --- a/security/selinux/hooks.c
>> +++ b/security/selinux/hooks.c
>> @@ -2696,19 +2696,51 @@ static int selinux_inode_follow_link(struct dentry *dentry, struct nameidata *na
>>  	return dentry_has_perm(cred, NULL, dentry, FILE__READ);
>>  }
>>  
>> -static int selinux_inode_permission(struct inode *inode, int mask)
>> +static int selinux_inode_permission(struct inode *inode, int in_mask)
>>  {
>>  	const struct cred *cred = current_cred();
>> +	struct inode_security_struct *isec;
>> +	struct common_audit_data ad;
>> +	struct av_decision avd;
>> +	u32 sid, perms;
>> +	int rc, mask;
>>  
>> -	mask &= (MAY_READ|MAY_WRITE|MAY_EXEC|MAY_APPEND);
>> +	mask = in_mask & (MAY_READ|MAY_WRITE|MAY_EXEC|MAY_APPEND);
>>  
>> -	if (!mask) {
>> -		/* No permission to check.  Existence test. */
>> +	/* No permission to check.  Existence test. */
>> +	if (!mask)
>> +		return 0;
>> +
>> +	validate_creds(cred);
>> +
>> +	if (unlikely(IS_PRIVATE(inode)))
>>  		return 0;
> 
> This is handled by security_inode_permission().  The check inside
> inode_has_perm() stems from other code paths.
> 
>> -	}
>>  
>> -	return inode_has_perm(cred, inode,
>> -			      file_mask_to_av(inode->i_mode, mask), NULL);
>> +	sid = cred_sid(cred);
>> +	isec = inode->i_security;
>> +
>> +	COMMON_AUDIT_DATA_INIT(&ad, FS);
>> +	ad.u.fs.inode = inode;
>> +
>> +	perms = file_mask_to_av(inode->i_mode, mask);
>> +
>> +	rc = avc_has_perm_noaudit(sid, isec->sid, isec->sclass, perms, 0, &avd);
>> +	/*
>> +	 * We want to audit if this call was not from access(2).
>> +	 * We also want to audit if the call was from access(2)
>> +	 * but the magic FILE__AUDIT_ACCESS permission was in the auditdeny
>> +	 * vector.
>> +	 *
>> +	 * aka there is a not dontaudit rule for file__audit_access.  This
>> +	 * might make more sense as a test inside avc_audit, but then we would
>> +	 * have to push the MAY_ACCESS flag down to avc_audit and I think we
>> +	 * already have enough stuff down there.
>> +	 */
> 
> Why can't we just push it down through inode_has_perm -> avc_has_perm ->
> avc_audit() via a field in common_audit_data?
> 
>> +	if (!(in_mask & MAY_ACCESS) ||
>> +	    (avd.auditdeny & FILE__AUDIT_ACCESS))
>> +		avc_audit(sid, isec->sid, isec->sclass, perms, &avd, rc, &ad);
>> +
>> +	return rc;
>>  }
>>  
> 
> 
Now if we had a policy lanquage that said staff_nautilus_t == (staff_t +
audit_access)

Meaning all interface/transitions and rules apply to both domains and
self rules allow interaction between the two, we could allow this access
to only nautilus.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.14 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAkvW+tcACgkQrlYvE4MpobN/nQCgigZZv8oW6HGzqT4YPxGK5tWj
OaMAn1yG+hhS4qUDBbo3YeNgNG0YUpfB
=dl0O
-----END PGP SIGNATURE-----