Message-ID: <CAGXu5j+Es+12b=0j28a149nVahJUFktMbft3Bx0CBLJ7J2ZJUA@mail.gmail.com>
Date:	Wed, 7 Dec 2011 10:23:28 -0800
From:	Kees Cook <keescook@...omium.org>
To:	Ingo Molnar <mingo@...e.hu>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Al Viro <viro@...iv.linux.org.uk>,
	linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org,
	linux-doc@...r.kernel.org, Randy Dunlap <rdunlap@...otime.net>,
	Rik van Riel <riel@...hat.com>,
	Federica Teodori <federica.teodori@...glemail.com>,
	Lucian Adrian Grijincu <lucian.grijincu@...il.com>,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	Eric Paris <eparis@...hat.com>,
	Dan Rosenberg <drosenberg@...curity.com>,
	kernel-hardening@...ts.openwall.com,
	Linus Torvalds <torvalds@...ux-foundation.org>
Subject: Re: [PATCH v2011.1] fs: symlink restrictions on sticky directories

On Tue, Dec 6, 2011 at 11:30 PM, Ingo Molnar <mingo@...e.hu> wrote:
> * Kees Cook <keescook@...omium.org> wrote:
>
>> Past objections and rebuttals could be summarized as:
>> ...
>>  - Might break unknown applications that use this feature.
>>    - Applications that break because of the change are easy to spot and
>>      fix. Applications that are vulnerable to symlink ToCToU by not having
>>      the change aren't. Additionally, no applications have yet been found
>>      that rely on this behavior.
>
> Are there *known* applications that break?

Not that I've seen and none have been mentioned in any of the previous
discussions I've been able to find. (From above: "Additionally, no
applications have yet been found that rely on this behavior.")

>>  - This should live in the core VFS.
>>    - This should live in an LSM. (https://lkml.org/lkml/2010/5/31/135)
>>  - This should live in an LSM.
>>    - This should live in the core VFS. (https://lkml.org/lkml/2010/8/2/188)
>
> Ugh - and people continue to get exploited from a preventable,
> fixable and already fixed VFS design flaw.

Right.

>> +config PROTECTED_STICKY_SYMLINKS
>> +     bool "Protect symlink following in sticky world-writable directories"
>> +     help
>> +       A long-standing class of security issues is the symlink-based
>> +       time-of-check-time-of-use race, most commonly seen in
>> +       world-writable directories like /tmp. The common method of
>> +       exploitation of this flaw is to cross privilege boundaries
>> +       when following a given symlink (i.e. a root process follows
>> +       a malicious symlink belonging to another user).
>> +
>> +       Enabling this solves the problem by permitting symlinks to only
>> +       be followed when outside a sticky world-writable directory,
>> +       or when the uid of the symlink and follower match, or when
>> +       the directory and symlink owners match.
>
> Unless there are known apps that regress, shouldnt this be
> default y? Even if we were forced to not actually release such a
> final kernel, doing it in -rc1 would flush weird apps out of the
> woodwork.

I would totally agree. I was trying to be conservative here, but would
much rather this default to "y".

>> +int protected_sticky_symlinks =
>
> __read_mostly, most emphatically.

Ah yes. I will fix this.

>> +static inline int
>> +may_follow_link(struct dentry *dentry, struct nameidata *nameidata)
>> +{
>> +     int error = 0;
>> +     const struct inode *parent;
>> +     const struct inode *inode;
>> +     const struct cred *cred;
>> +
>> +     if (!protected_sticky_symlinks)
>> +             return 0;
>> +
>> +     /* Allowed if owner and follower match. */
>> +     cred = current_cred();
>> +     inode = dentry->d_inode;
>> +     if (cred->fsuid == inode->i_uid)
>> +             return 0;
>> +
>> +     /* Check parent directory mode and owner. */
>> +     spin_lock(&dentry->d_lock);
>> +     parent = dentry->d_parent->d_inode;
>> +     if ((parent->i_mode & (S_ISVTX|S_IWOTH)) == (S_ISVTX|S_IWOTH) &&
>> +         parent->i_uid != inode->i_uid) {
>> +             error = -EACCES;
>> +     }
>> +     spin_unlock(&dentry->d_lock);
>
> Taking the global /tmp and /lib, /usr/lib dentry spinlocks here
> every time we follow a symlink is a bit sad: there are a lot of
> high-profile symlinks in a Linux installation in those places,
> followed by non-owners.
>
> Nor is it really a realistic race that happens often: neither
> /tmp nor the library directories are being renamed or their
> permissions changed all that often for this parent lock taking
> to matter in practice.
>
> Couldnt we somehow export this information outside the dentry
> lock, allowing safe lockless access to it?

I'm open to whatever is needed, though I think this change would be
outside the scope of this patch's intent.

>> +     if (error) {
>> +             char name[sizeof(current->comm)];
>> +             printk_ratelimited(KERN_NOTICE "non-matching-uid symlink "
>> +                     "following attempted in sticky world-writable "
>> +                     "directory by %s (fsuid %d)\n",
>> +                     get_task_comm(name, current), cred->fsuid);
>
> I don't think the get_task_comm() extra layer is really
> necessary here - this is called in the *current* task's context
> - and it's not like that tasks are changing their own names all
> that often while they are busy executing VFS code, right? The
> race with some other task writing to /proc/PID/comm is
> uninteresting.

I would rather not have any kind of race here. It's only during the
failure condition, so we don't need speed.

> The more important piece of forensic information to log would be
> the file name that got attempted and perhaps the directory name
> as well. If there's an unknown hole in an unknown privileged app
> then this warning alone does not give the admin any clues where
> to look - the name of the expoiting task is probably obfuscated
> anyway, it's the identity of the attack vector that matters -
> and that name can generally not be controlled and obfuscated by
> the attacker.

Agreed. I will work on extracting this information and presenting it sanely.

> If those issues are resolved:
>
>  Reviewed-by: Ingo Molnar <mingo@...e.hu>

Thanks for looking it over!

-Kees

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

Powered by blists - more mailing lists