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-doc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
