[PATCH] vfs: Tighten up linkat(..., AT_EMPTY_PATH)

[PATCH] vfs: Tighten up linkat(..., AT_EMPTY_PATH)

[Andy Lutomirski]

There have long been two ways to ask the kernel to create a new
hardlink to the inode represented by an fd: linkat(..., AT_EMPTY_PATH)
and AT_SYMLINK_FOLLOW on /proc/self/fd/N.  The latter has no
particular security restrictions, but the former required privilege
until:

    commit bb2314b47996491bbc5add73633905c3120b6268
    Author: Andy Lutomirski <luto@amacapital.net>
    Date:   Thu Aug 1 21:44:31 2013 -0700

        fs: Allow unprivileged linkat(..., AT_EMPTY_PATH) aka flink

Spender pointed out that there could be code that passes an fd to an
untrusted, chrooted process, and allowing that process to flink the
fd could be dangerous.  (I'm not aware of any actual known attack.)

So let's be careful for now: only allow linkat(..., AT_EMPTY_PATH)
if the target is I_LINKABLE.  I_LINKABLE is only set for inodes
created by O_TMPFILE without O_EXCL, which is an explicit request
for flinkability.

Signed-off-by: Andy Lutomirski <luto@amacapital.net>
---
 fs/namei.c | 36 ++++++++++++++++++++++++++++++++----
 1 file changed, 32 insertions(+), 4 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index 89a612e..44ffd3d 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -3652,6 +3652,32 @@ int vfs_link(struct dentry *old_dentry, struct inode *dir, struct dentry *new_de
 }
 
 /*
+ * flink is dangerous.  For now, only allow it when specifically requested
+ * or when the caller is privileged.
+ *
+ * NB: This is not currently enforced for AT_SYMLINK_FOLLOW on procfs.
+ *     Fixing that would be intrusive and could break something.
+ */
+static bool may_flink(const struct path *path)
+{
+	bool ret;
+	struct inode *inode = path->dentry->d_inode;
+
+	/*
+	 * This is racy: I_LINKABLE could be cleared between this check
+	 * and the actual link operation.  This is odd but not a security
+	 * problem: the caller could get the same effect by flinking once
+	 * and then using normal link(2) to create a second link.
+	 */
+
+	spin_lock(&inode->i_lock);
+	ret = !!(inode->i_state & I_LINKABLE);
+	spin_unlock(&inode->i_lock);
+
+	return ret || capable(CAP_DAC_READ_SEARCH);
+}
+
+/*
  * Hardlinks are often used in delicate situations.  We avoid
  * security-related surprises by not following symlinks on the
  * newname.  --KAB
@@ -3670,10 +3696,7 @@ SYSCALL_DEFINE5(linkat, int, olddfd, const char __user *, oldname,
 
 	if ((flags & ~(AT_SYMLINK_FOLLOW | AT_EMPTY_PATH)) != 0)
 		return -EINVAL;
-	/*
-	 * Using empty names is equivalent to using AT_SYMLINK_FOLLOW
-	 * on /proc/self/fd/<fd>.
-	 */
+
 	if (flags & AT_EMPTY_PATH)
 		how = LOOKUP_EMPTY;
 
@@ -3684,6 +3707,11 @@ retry:
 	if (error)
 		return error;
 
+	if ((flags & AT_EMPTY_PATH) && !may_flink(&old_path)) {
+		error = -EPERM;
+		goto out;
+	}
+
 	new_dentry = user_path_create(newdfd, newname, &new_path,
 					(how & LOOKUP_REVAL));
 	error = PTR_ERR(new_dentry);
-- 
1.8.3.1

Re: [PATCH] vfs: Tighten up linkat(..., AT_EMPTY_PATH)

[Oleg Nesterov]

Can't really comment the patch, just a nit:

On 08/21, Andy Lutomirski wrote:
>
> +static bool may_flink(const struct path *path)
> +{
> +	bool ret;
> +	struct inode *inode = path->dentry->d_inode;
> +
> +	/*
> +	 * This is racy: I_LINKABLE could be cleared between this check
> +	 * and the actual link operation.

OK,

> +	spin_lock(&inode->i_lock);
> +	ret = !!(inode->i_state & I_LINKABLE);
> +	spin_unlock(&inode->i_lock);

so why do we need to take a lock ?

Oleg.

Re: [PATCH] vfs: Tighten up linkat(..., AT_EMPTY_PATH)

[Andy Lutomirski]

On Wed, Aug 21, 2013 at 11:20 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> Can't really comment the patch, just a nit:
>
> On 08/21, Andy Lutomirski wrote:
>>
>> +static bool may_flink(const struct path *path)
>> +{
>> +     bool ret;
>> +     struct inode *inode = path->dentry->d_inode;
>> +
>> +     /*
>> +      * This is racy: I_LINKABLE could be cleared between this check
>> +      * and the actual link operation.
>
> OK,
>
>> +     spin_lock(&inode->i_lock);
>> +     ret = !!(inode->i_state & I_LINKABLE);
>> +     spin_unlock(&inode->i_lock);
>
> so why do we need to take a lock ?
>

We probably don't.  But other accesses to this field take that lock,
so it seemed safer.

(In principle, someone could take the lock, write I_LINKABLE, clear
it, and unlock, and we'd get confused if we didn't take the lock
ourselves.)

> Oleg.
>



-- 
Andy Lutomirski
AMA Capital Management, LLC

Re: [PATCH] vfs: Tighten up linkat(..., AT_EMPTY_PATH)

[Oleg Nesterov]

On 08/21, Andy Lutomirski wrote:
>
> On Wed, Aug 21, 2013 at 11:20 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> > Can't really comment the patch, just a nit:
> >
> > On 08/21, Andy Lutomirski wrote:
> >>
> >> +static bool may_flink(const struct path *path)
> >> +{
> >> +     bool ret;
> >> +     struct inode *inode = path->dentry->d_inode;
> >> +
> >> +     /*
> >> +      * This is racy: I_LINKABLE could be cleared between this check
> >> +      * and the actual link operation.
> >
> > OK,
> >
> >> +     spin_lock(&inode->i_lock);
> >> +     ret = !!(inode->i_state & I_LINKABLE);
> >> +     spin_unlock(&inode->i_lock);
> >
> > so why do we need to take a lock ?
> >
>
> We probably don't.  But other accesses to this field take that lock,

Not if you only need to check ->i_lock,

> (In principle, someone could take the lock, write I_LINKABLE, clear
> it, and unlock, and we'd get confused if we didn't take the lock
> ourselves.)

Or it can be cleared right after we drop i_lock, I do not think there
is any difference.

But OK, I won't argue.

Oleg.