[PATCH] fs: switch order of CAP_DAC_OVERRIDE and CAP_DAC_READ_SEARCH checks

generic_permission() presently checks CAP_DAC_OVERRIDE prior to
CAP_DAC_READ_SEARCH.  This can cause misleading audit messages when
using a LSM such as SELinux or AppArmor, since CAP_DAC_OVERRIDE
may not be required for the operation.  Flip the order of the
tests so that CAP_DAC_OVERRIDE is only checked when required for
the operation.

Signed-off-by: Stephen Smalley <redacted>
---
 fs/namei.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index d41fab7..482414a 100644
--- a/fs/namei.c
+++ b/fs/namei.c

@@ -340,22 +340,14 @@ int generic_permission(struct inode *inode, int mask)
 
 	if (S_ISDIR(inode->i_mode)) {
 		/* DACs are overridable for directories */
-		if (capable_wrt_inode_uidgid(inode, CAP_DAC_OVERRIDE))
-			return 0;
 		if (!(mask & MAY_WRITE))
 			if (capable_wrt_inode_uidgid(inode,
 						     CAP_DAC_READ_SEARCH))
 				return 0;
-		return -EACCES;
-	}
-	/*
-	 * Read/write DACs are always overridable.
-	 * Executable DACs are overridable when there is
-	 * at least one exec bit set.
-	 */
-	if (!(mask & MAY_EXEC) || (inode->i_mode & S_IXUGO))
 		if (capable_wrt_inode_uidgid(inode, CAP_DAC_OVERRIDE))
 			return 0;
+		return -EACCES;
+	}
 
 	/*
 	 * Searching includes executable on directories, else just read.

@@ -364,6 +356,14 @@ int generic_permission(struct inode *inode, int mask)
 	if (mask == MAY_READ)
 		if (capable_wrt_inode_uidgid(inode, CAP_DAC_READ_SEARCH))
 			return 0;
+	/*
+	 * Read/write DACs are always overridable.
+	 * Executable DACs are overridable when there is
+	 * at least one exec bit set.
+	 */
+	if (!(mask & MAY_EXEC) || (inode->i_mode & S_IXUGO))
+		if (capable_wrt_inode_uidgid(inode, CAP_DAC_OVERRIDE))
+			return 0;
 
 	return -EACCES;
 }

-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Fri, Mar 10, 2017 at 12:14 PM, Stephen Smalley [off-list ref] wrote:

generic_permission() presently checks CAP_DAC_OVERRIDE prior to
CAP_DAC_READ_SEARCH.  This can cause misleading audit messages when
using a LSM such as SELinux or AppArmor, since CAP_DAC_OVERRIDE
may not be required for the operation.  Flip the order of the
tests so that CAP_DAC_OVERRIDE is only checked when required for
the operation.

Signed-off-by: Stephen Smalley <redacted>
---
 fs/namei.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

This is the second posting of this patch and so far no comment ... if
I don't see any negative responses by next week I'll go ahead and
merge this into the selinux/next tree.

-- 
paul moore
www.paul-moore.com
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 03/10/2017 11:54 AM, Paul Moore wrote:

On Fri, Mar 10, 2017 at 12:14 PM, Stephen Smalley [off-list ref] wrote:

quoted

generic_permission() presently checks CAP_DAC_OVERRIDE prior to
CAP_DAC_READ_SEARCH.  This can cause misleading audit messages when
using a LSM such as SELinux or AppArmor, since CAP_DAC_OVERRIDE
may not be required for the operation.  Flip the order of the
tests so that CAP_DAC_OVERRIDE is only checked when required for
the operation.

Signed-off-by: Stephen Smalley <redacted>
---
 fs/namei.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

This is the second posting of this patch and so far no comment ... if
I don't see any negative responses by next week I'll go ahead and
merge this into the selinux/next tree.

sounds good to me, the patch looks good you can have my acked-by for how
this affects apparmor, or hrmm should that be a reviewed-by for the vfs
end

Acked-by: John Johansen <john.johansen@canonical.com>

quoted

diff --git a/fs/namei.c b/fs/namei.c
index d41fab7..482414a 100644
--- a/fs/namei.c
+++ b/fs/namei.c

@@ -340,22 +340,14 @@ int generic_permission(struct inode *inode, int mask)

        if (S_ISDIR(inode->i_mode)) {
                /* DACs are overridable for directories */
-               if (capable_wrt_inode_uidgid(inode, CAP_DAC_OVERRIDE))
-                       return 0;
                if (!(mask & MAY_WRITE))
                        if (capable_wrt_inode_uidgid(inode,
                                                     CAP_DAC_READ_SEARCH))
                                return 0;
-               return -EACCES;
-       }
-       /*
-        * Read/write DACs are always overridable.
-        * Executable DACs are overridable when there is
-        * at least one exec bit set.
-        */
-       if (!(mask & MAY_EXEC) || (inode->i_mode & S_IXUGO))
                if (capable_wrt_inode_uidgid(inode, CAP_DAC_OVERRIDE))
                        return 0;
+               return -EACCES;
+       }

        /*
         * Searching includes executable on directories, else just read.

@@ -364,6 +356,14 @@ int generic_permission(struct inode *inode, int mask)
        if (mask == MAY_READ)
                if (capable_wrt_inode_uidgid(inode, CAP_DAC_READ_SEARCH))
                        return 0;
+       /*
+        * Read/write DACs are always overridable.
+        * Executable DACs are overridable when there is
+        * at least one exec bit set.
+        */
+       if (!(mask & MAY_EXEC) || (inode->i_mode & S_IXUGO))
+               if (capable_wrt_inode_uidgid(inode, CAP_DAC_OVERRIDE))
+                       return 0;

        return -EACCES;
 }
--

2.7.4

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Quoting Stephen Smalley (sds at tycho.nsa.gov):

generic_permission() presently checks CAP_DAC_OVERRIDE prior to
CAP_DAC_READ_SEARCH.  This can cause misleading audit messages when
using a LSM such as SELinux or AppArmor, since CAP_DAC_OVERRIDE
may not be required for the operation.  Flip the order of the
tests so that CAP_DAC_OVERRIDE is only checked when required for
the operation.

Signed-off-by: Stephen Smalley <redacted>

Lol, not sure if that patch has arranged itself to be as confusing
as possible (for a simple end result) or if it's in my head :), but
I had to read it like 3 times, despite it appearing trivial in the
end.

Reviewed-by: Serge Hallyn <serge@hallyn.com>

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Fri, 10 Mar 2017, Stephen Smalley wrote:

generic_permission() presently checks CAP_DAC_OVERRIDE prior to
CAP_DAC_READ_SEARCH.  This can cause misleading audit messages when
using a LSM such as SELinux or AppArmor, since CAP_DAC_OVERRIDE
may not be required for the operation.  Flip the order of the
tests so that CAP_DAC_OVERRIDE is only checked when required for
the operation.

Signed-off-by: Stephen Smalley <redacted>

Acked-by: James Morris <redacted>


-- 
James Morris
[off-list ref]

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Fri, Mar 10, 2017 at 2:54 PM, Paul Moore [off-list ref] wrote:

On Fri, Mar 10, 2017 at 12:14 PM, Stephen Smalley [off-list ref] wrote:

quoted

generic_permission() presently checks CAP_DAC_OVERRIDE prior to
CAP_DAC_READ_SEARCH.  This can cause misleading audit messages when
using a LSM such as SELinux or AppArmor, since CAP_DAC_OVERRIDE
may not be required for the operation.  Flip the order of the
tests so that CAP_DAC_OVERRIDE is only checked when required for
the operation.

Signed-off-by: Stephen Smalley <redacted>
---
 fs/namei.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

This is the second posting of this patch and so far no comment ... if
I don't see any negative responses by next week I'll go ahead and
merge this into the selinux/next tree.

No objections, but plenty of ACKs and Reviewed-bys so I just merged
this into the selinux/next tree.

Thanks all.

quoted

diff --git a/fs/namei.c b/fs/namei.c
index d41fab7..482414a 100644
--- a/fs/namei.c
+++ b/fs/namei.c

@@ -340,22 +340,14 @@ int generic_permission(struct inode *inode, int mask)

        if (S_ISDIR(inode->i_mode)) {
                /* DACs are overridable for directories */
-               if (capable_wrt_inode_uidgid(inode, CAP_DAC_OVERRIDE))
-                       return 0;
                if (!(mask & MAY_WRITE))
                        if (capable_wrt_inode_uidgid(inode,
                                                     CAP_DAC_READ_SEARCH))
                                return 0;
-               return -EACCES;
-       }
-       /*
-        * Read/write DACs are always overridable.
-        * Executable DACs are overridable when there is
-        * at least one exec bit set.
-        */
-       if (!(mask & MAY_EXEC) || (inode->i_mode & S_IXUGO))
                if (capable_wrt_inode_uidgid(inode, CAP_DAC_OVERRIDE))
                        return 0;
+               return -EACCES;
+       }

        /*
         * Searching includes executable on directories, else just read.

@@ -364,6 +356,14 @@ int generic_permission(struct inode *inode, int mask)
        if (mask == MAY_READ)
                if (capable_wrt_inode_uidgid(inode, CAP_DAC_READ_SEARCH))
                        return 0;
+       /*
+        * Read/write DACs are always overridable.
+        * Executable DACs are overridable when there is
+        * at least one exec bit set.
+        */
+       if (!(mask & MAY_EXEC) || (inode->i_mode & S_IXUGO))
+               if (capable_wrt_inode_uidgid(inode, CAP_DAC_OVERRIDE))
+                       return 0;

        return -EACCES;
 }
--

2.7.4

--
paul moore
www.paul-moore.com

-- 
paul moore
www.paul-moore.com
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html