[RFC] namei: prevent sgid-hardlinks for unmapped gids

[Dirk Steinmetz <public@rsjtdrjgfuzkfg.com>]

In order to hardlink to a sgid-executable, it is sufficient to be the
file's owner. When hardlinking within an unprivileged user namespace, the
users of that namespace could thus use hardlinks to pin setgid binaries
owned by themselves (or any mapped uid, with CAP_FOWNER) and a gid outside
of the namespace. This is a possible security risk.

This change prevents hardlinking of sgid-executables within user
namespaces, if the file is not owned by a mapped gid.

Signed-off-by: Dirk Steinmetz <public@rsjtdrjgfuzkfg.com>
---

MISSING: Documentation/sysctl/fs.txt not updated, as this patch is
intended for discussion.

If there are no further misunderstandings on my side, this patch is what
Serge and I agree on (modulo my not-that-much-linux-kernel-experience
codestyle, feel free to suggest improvements!).

The new condition for sgid-executables is equivalent to
> inode_owner_or_capable(inode) && kgid_has_mapping(ns, inode->i_gid)
which, as recommended by Serge, does not change the behaviour for the init
namespace. It fixes the problem of pinning parent namespace's gids.

However, I think the "same" security issue is also valid within any
namespace, for regular users pinning other gids within the same namespace.
I already presented an example for that in a previous mail:
- A file has the setgid and user/group executable bits set, and is owned
  by user:group.
- The user 'user' is not in the group 'group', and does not have any
  capabilities.
- The user 'user' hardlinks the file. The permission check will succeed,
  as the user is the owner of the file.
- The file is replaced with a newer version (for example fixing a security
  issue)
- Now user can still use the hardlink-pinned version to execute the file
  as 'user:group' (and for example exploit the security issue).

To prevent that, the condition would need to be changed to something like
inode_group_or_capable, resembling inode_owner_or_capable, but checking
that the caller is in the group the inode belongs to or has some
capability (for consistency with former behaviour, CAP_FOWNER? for
consistency with the documentation, CAP_FSETID?). However, this would
change userland behaviour outside of userns. Thus my main question:
Is the scenario above bad enough to change userland behaviour?

I'd apprechiate your comments.

- Dirk


Diffstat:
 fs/namei.c | 47 ++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 38 insertions(+), 9 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index 29fc6a6..9c6c2e2 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -913,18 +913,19 @@ static inline int may_follow_link(struct nameidata *nd)
 }
 
 /**
- * safe_hardlink_source - Check for safe hardlink conditions
+ * safe_hardlink_source_uid - Check for safe hardlink conditions not dependent
+ * on the inode's group. These conditions may be overridden by inode ownership
+ * or CAP_FOWNER with respect to the inode's uid
  * @inode: the source inode to hardlink from
  *
  * Return false if at least one of the following conditions:
  *    - inode is not a regular file
  *    - inode is setuid
- *    - inode is setgid and group-exec
  *    - access failure for read and write
  *
  * Otherwise returns true.
  */
-static bool safe_hardlink_source(struct inode *inode)
+static bool safe_hardlink_source_uid(struct inode *inode)
 {
 	umode_t mode = inode->i_mode;
 
@@ -936,10 +937,6 @@ static bool safe_hardlink_source(struct inode *inode)
 	if (mode & S_ISUID)
 		return false;
 
-	/* Executable setgid files should not get pinned to the filesystem. */
-	if ((mode & (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP))
-		return false;
-
 	/* Hardlinking to unreadable or unwritable sources is dangerous. */
 	if (inode_permission(inode, MAY_READ | MAY_WRITE))
 		return false;
@@ -948,30 +945,62 @@ static bool safe_hardlink_source(struct inode *inode)
 }
 
 /**
+ * safe_hardlink_source_gid - Check for safe hardlink conditions dependent
+ * on the inode's group. These conditions may be overridden by inode ownership
+ * or CAP_FOWNER with respect to the inode's gid
+ * @inode: the source inode to hardlink from
+ *
+ * Return false if inode is setgid and group-exec
+ *
+ * Otherwise returns true.
+ */
+static bool safe_hardlink_source_gid(struct inode *inode)
+{
+	umode_t mode = inode->i_mode;
+
+	/* Executable setgid files should not get pinned to the filesystem. */
+	if ((mode & (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP))
+		return false;
+
+	return true;
+}
+
+/**
  * may_linkat - Check permissions for creating a hardlink
  * @link: the source to hardlink from
  *
  * Block hardlink when all of:
  *  - sysctl_protected_hardlinks enabled
  *  - fsuid does not match inode
- *  - hardlink source is unsafe (see safe_hardlink_source() above)
+ *  - hardlink source is unsafe (see safe_hardlink_source_*() above)
  *  - not CAP_FOWNER in a namespace with the inode owner uid mapped
+ *    (and inode gid mapped, if hardlink conditions depending on the inode's
+ *    group are not satisfied)
  *
  * Returns 0 if successful, -ve on error.
  */
 static int may_linkat(struct path *link)
 {
 	struct inode *inode;
+	struct user_namespace *ns;
+	bool owner;
+	bool safe_uid;
+	bool safe_gid;
 
 	if (!sysctl_protected_hardlinks)
 		return 0;
 
 	inode = link->dentry->d_inode;
+	ns = current_user_ns();
 
 	/* Source inode owner (or CAP_FOWNER) can hardlink all they like,
 	 * otherwise, it must be a safe source.
 	 */
-	if (inode_owner_or_capable(inode) || safe_hardlink_source(inode))
+	owner = inode_owner_or_capable(inode);
+	safe_uid = safe_hardlink_source_uid(inode) || owner;
+	safe_gid = safe_hardlink_source_gid(inode) ||
+			(owner && kgid_has_mapping(ns, inode->i_gid));
+	if (safe_uid && safe_gid)
 		return 0;
 
 	audit_log_link_denied("linkat", link);
-- 
2.1.4