linux-kernel - Re: [PATCH 1/2] fs: add link restrictions

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20120630091422.GE14083@ZenIV.linux.org.uk>
Date:	Sat, 30 Jun 2012 10:14:23 +0100
From:	Al Viro <viro@...IV.linux.org.uk>
To:	Kees Cook <keescook@...omium.org>
Cc:	linux-kernel@...r.kernel.org,
	Andrew Morton <akpm@...ux-foundation.org>,
	linux-fsdevel@...r.kernel.org, Eric Paris <eparis@...hat.com>,
	Matthew Wilcox <matthew@....cx>,
	Doug Ledford <dledford@...hat.com>,
	Joe Korty <joe.korty@...r.com>,
	"Eric W. Biederman" <ebiederm@...ssion.com>,
	Ingo Molnar <mingo@...e.hu>,
	David Howells <dhowells@...hat.com>,
	James Morris <james.l.morris@...cle.com>,
	linux-doc@...r.kernel.org,
	Dan Rosenberg <drosenberg@...curity.com>,
	kernel-hardening@...ts.openwall.com
Subject: Re: [PATCH 1/2] fs: add link restrictions

On Mon, Jun 25, 2012 at 02:05:26PM -0700, Kees Cook wrote:

> +config PROTECTED_LINKS
> +	bool "Evaluate vulnerable link conditions"
> +	default y

Remember Linus' rants about 'default y' in general?

> +#ifdef CONFIG_PROTECTED_LINKS
> +int sysctl_protected_symlinks __read_mostly =
> +	CONFIG_PROTECTED_SYMLINKS_SYSCTL;
> +int sysctl_protected_hardlinks __read_mostly =
> +	CONFIG_PROTECTED_HARDLINKS_SYSCTL;
> +
> +/**
> + * may_follow_link - Check symlink following for unsafe situations
> + * @link: The path of the symlink
> + *
> + * In the case of the sysctl_protected_symlinks sysctl being enabled,
> + * CAP_DAC_OVERRIDE needs to be specifically ignored if the symlink is
> + * in a sticky world-writable directory. This is to protect privileged
> + * processes from failing races against path names that may change out
> + * from under them by way of other users creating malicious symlinks.
> + * It will permit symlinks to be followed only when outside a sticky
> + * world-writable directory, or when the uid of the symlink and follower
> + * match, or when the directory owner matches the symlink's owner.
> + *
> + * Returns 0 if following the symlink is allowed, -ve on error.
> + */
> +static inline int may_follow_link(struct path *link)
> +{
> +	int error = 0;
> +	const struct inode *parent;
> +	const struct inode *inode;
> +	const struct cred *cred;
> +	struct dentry *dentry;
> +
> +	if (!sysctl_protected_symlinks)
> +		return 0;

Um.  What this says to me is "this function should be outside of ifdef, with
#else of that ifdef defining sysctl_protected_symlinks to 0".

> +	/* Check parent directory mode and owner. */

I suspect that we ought to simply pass that parent directory as argument - caller
*does* have a reference to it, so we don't need to mess with ->d_lock, etc.

> +	mode_t mode = inode->i_mode;

umode_t, please.

> +static int may_linkat(struct path *link)
> +{
> +	const struct cred *cred;
> +	struct inode *inode;
> +
> +	if (!sysctl_protected_hardlinks)
> +		return 0;

Ditto about taking it outside of ifdef.

> +			err = may_follow_link(&link);
> +			if (unlikely(err))
> +				break;

No.  This is definitely wrong - you are leaking dentries and vfsmount here.
> +		error = may_follow_link(&link);
> +		if (unlikely(error))
> +			break;

Ditto.
--
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/