lkml.org 
[lkml]   [2012]   [Jun]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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.


    \
     
     \ /
      Last update: 2012-06-30 12:01    [W:0.024 / U:31.140 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site