[PATCH] fs: hardlink creation restriction cleanup

[PATCH] fs: hardlink creation restriction cleanup

[Kees Cook]

Clean-up of hardlink restriction logic, as suggested by Andrew Morton.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 fs/namei.c |   59 +++++++++++++++++++++++++++++++++++++++++------------------
 1 files changed, 41 insertions(+), 18 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index 8ed4e00..a4a21a5 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -693,46 +693,69 @@ static inline int may_follow_link(struct path *link)
 }
 
 /**
+ * safe_hardlink_source - Check for safe hardlink conditions
+ * @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)
+{
+	mode_t mode = inode->i_mode;
+
+	/* Special files should not get pinned to the filesystem. */
+	if (!S_ISREG(mode))
+		return false;
+	/* Setuid files should not get pinned to the filesystem. */
+	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;
+
+	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
- *  - at least one of:
- *    - inode is not a regular file
- *    - inode is setuid
- *    - inode is setgid and group-exec
- *    - access failure for read and write
+ *  - hardlink source is unsafe (see safe_hardlink_source() above)
  *  - not CAP_FOWNER
  *
  * Returns 0 if successful, -ve on error.
  */
 static int may_linkat(struct path *link)
 {
-	int error = 0;
 	const struct cred *cred;
 	struct inode *inode;
-	int mode;
 
 	if (!sysctl_protected_hardlinks)
 		return 0;
 
 	cred = current_cred();
 	inode = link->dentry->d_inode;
-	mode = inode->i_mode;
 
-	if (cred->fsuid != inode->i_uid &&
-	    (!S_ISREG(mode) || (mode & S_ISUID) ||
-	     ((mode & (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP)) ||
-	     (inode_permission(inode, MAY_READ | MAY_WRITE))) &&
-	    !capable(CAP_FOWNER))
-		error = -EPERM;
-
-	if (error)
-		audit_log_link_denied("linkat", link);
+	/* Source inode owner (or CAP_FOWNER) can hardlink all they like,
+	 * otherwise, it must be a safe source.
+	 */
+	if (cred->fsuid == inode->i_uid || safe_hardlink_source(inode) ||
+	    capable(CAP_FOWNER))
+		return 0;
 
-	return error;
+	audit_log_link_denied("linkat", link);
+	return -EPERM;
 }
 #else
 static inline int may_follow_link(struct path *link)
-- 
1.7.0.4

Re: [PATCH] fs: hardlink creation restriction cleanup

[Ingo Molnar]

* Kees Cook <keescook@chromium.org> wrote:

> Clean-up of hardlink restriction logic, as suggested by Andrew Morton.
> 
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  fs/namei.c |   59 +++++++++++++++++++++++++++++++++++++++++------------------
>  1 files changed, 41 insertions(+), 18 deletions(-)
> 
> diff --git a/fs/namei.c b/fs/namei.c
> index 8ed4e00..a4a21a5 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -693,46 +693,69 @@ static inline int may_follow_link(struct path *link)
>  }
>  
>  /**
> + * safe_hardlink_source - Check for safe hardlink conditions
> + * @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)
> +{
> +	mode_t mode = inode->i_mode;
> +
> +	/* Special files should not get pinned to the filesystem. */
> +	if (!S_ISREG(mode))
> +		return false;
> +	/* Setuid files should not get pinned to the filesystem. */
> +	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;
> +
> +	return true;

A really minor nitpick, could we use this form please:

static bool safe_hardlink_source(struct inode *inode)
{
	mode_t mode = inode->i_mode;

	/* Special files should not get pinned to the filesystem. */
	if (!S_ISREG(mode))
		return false;

	/* Setuid files should not get pinned to the filesystem. */
	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;

	return true;
}

Those separate blocks of comments and conditions stand out much 
nicer this way, making it way easier on the eyes - to my eyes at 
least ;-)

Thanks,

	Ingo

Re: [PATCH] fs: hardlink creation restriction cleanup

[Kees Cook]

On Wed, Feb 22, 2012 at 2:23 AM, Ingo Molnar <mingo@elte.hu> wrote:
>
> * Kees Cook <keescook@chromium.org> wrote:
>
>> Clean-up of hardlink restriction logic, as suggested by Andrew Morton.
>>
>> Signed-off-by: Kees Cook <keescook@chromium.org>
>> ---
>>  fs/namei.c |   59 +++++++++++++++++++++++++++++++++++++++++------------------
>>  1 files changed, 41 insertions(+), 18 deletions(-)
>>
>> diff --git a/fs/namei.c b/fs/namei.c
>> index 8ed4e00..a4a21a5 100644
>> --- a/fs/namei.c
>> +++ b/fs/namei.c
>> @@ -693,46 +693,69 @@ static inline int may_follow_link(struct path *link)
>>  }
>>
>>  /**
>> + * safe_hardlink_source - Check for safe hardlink conditions
>> + * @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)
>> +{
>> +     mode_t mode = inode->i_mode;
>> +
>> +     /* Special files should not get pinned to the filesystem. */
>> +     if (!S_ISREG(mode))
>> +             return false;
>> +     /* Setuid files should not get pinned to the filesystem. */
>> +     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;
>> +
>> +     return true;
>
> A really minor nitpick, could we use this form please:
>
> static bool safe_hardlink_source(struct inode *inode)
> {
>        mode_t mode = inode->i_mode;
>
>        /* Special files should not get pinned to the filesystem. */
>        if (!S_ISREG(mode))
>                return false;
>
>        /* Setuid files should not get pinned to the filesystem. */
>        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;
>
>        return true;
> }
>
> Those separate blocks of comments and conditions stand out much
> nicer this way, making it way easier on the eyes - to my eyes at
> least ;-)

Heh, sure. I've sent v2 now. :)

-Kees

-- 
Kees Cook
ChromeOS Security
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html