Hi Kees,
On Wed, Jul 25, 2012 at 17:29 -0700, Kees Cook wrote:
> +/**
> + * safe_hardlink_source - Check for safe hardlink conditions
> + * @inode: the source inode to hardlink from
> + *
> + * Return false if at least one of the following conditions:
> + * - inode is not a regular file
> + * - inode is setuid
> + * - inode is setgid and group-exec
> + * - access failure for read and write
> + *
> + * Otherwise returns true.
> + */
> +static bool safe_hardlink_source(struct inode *inode)
> +{
> + umode_t mode = inode->i_mode;
> +
> + /* Special files should not get pinned to the filesystem. */
> + if (!S_ISREG(mode))
> + return false;
> +
> + /* Setuid files should not get pinned to the filesystem. */
> + if (mode & S_ISUID)
> + return false;
We don't want to make hardlinks of SUID files, but we still allow to create
hardlinks to SUID'ish cap'ed files. Probably check whether the inode is
setcap'ed?
Probably we can enhance this further and allow LSMs to define whether this
particular file is special in LSM's point of view (IOW, it can be able to move
a process to another security domain which is served by LSM).
> +
> + /* Executable setgid files should not get pinned to the filesystem. */
> + if ((mode & (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP))
> + return false;
> +
> + /* Hardlinking to unreadable or unwritable sources is dangerous. */
> + if (inode_permission(inode, MAY_READ | MAY_WRITE))
> + return false;
> +
> + return true;
> +}
Thanks,
--
Vasily
On Wed, Aug 8, 2012 at 5:19 AM, Vasily Kulikov <[email protected]> wrote:
> Hi Kees,
>
> On Wed, Jul 25, 2012 at 17:29 -0700, Kees Cook wrote:
>> +/**
>> + * safe_hardlink_source - Check for safe hardlink conditions
>> + * @inode: the source inode to hardlink from
>> + *
>> + * Return false if at least one of the following conditions:
>> + * - inode is not a regular file
>> + * - inode is setuid
>> + * - inode is setgid and group-exec
>> + * - access failure for read and write
>> + *
>> + * Otherwise returns true.
>> + */
>> +static bool safe_hardlink_source(struct inode *inode)
>> +{
>> + umode_t mode = inode->i_mode;
>> +
>> + /* Special files should not get pinned to the filesystem. */
>> + if (!S_ISREG(mode))
>> + return false;
>> +
>> + /* Setuid files should not get pinned to the filesystem. */
>> + if (mode & S_ISUID)
>> + return false;
>
> We don't want to make hardlinks of SUID files, but we still allow to create
> hardlinks to SUID'ish cap'ed files. Probably check whether the inode is
> setcap'ed?
Excellent idea. It doesn't look like there is anything "simple" to do
this already. It'd be close to get_file_caps() but without the bprm.
Maybe just get_vfs_caps_from_disk() and a walk of the caps? What would
you recommend?
> Probably we can enhance this further and allow LSMs to define whether this
> particular file is special in LSM's point of view (IOW, it can be able to move
> a process to another security domain which is served by LSM).
Yeah. Perhaps implementing the needed check above with a new security
check and have commoncaps do the vfs fetch with LSMs able to override?
-Kees
--
Kees Cook
Chrome OS Security
On Sat, Aug 11, 2012 at 23:34 -0700, Kees Cook wrote:
> On Wed, Aug 8, 2012 at 5:19 AM, Vasily Kulikov <[email protected]> wrote:
> > Hi Kees,
> >
> > On Wed, Jul 25, 2012 at 17:29 -0700, Kees Cook wrote:
> >> +/**
> >> + * safe_hardlink_source - Check for safe hardlink conditions
> >> + * @inode: the source inode to hardlink from
> >> + *
> >> + * Return false if at least one of the following conditions:
> >> + * - inode is not a regular file
> >> + * - inode is setuid
> >> + * - inode is setgid and group-exec
> >> + * - access failure for read and write
> >> + *
> >> + * Otherwise returns true.
> >> + */
> >> +static bool safe_hardlink_source(struct inode *inode)
> >> +{
> >> + umode_t mode = inode->i_mode;
> >> +
> >> + /* Special files should not get pinned to the filesystem. */
> >> + if (!S_ISREG(mode))
> >> + return false;
> >> +
> >> + /* Setuid files should not get pinned to the filesystem. */
> >> + if (mode & S_ISUID)
> >> + return false;
> >
> > We don't want to make hardlinks of SUID files, but we still allow to create
> > hardlinks to SUID'ish cap'ed files. Probably check whether the inode is
> > setcap'ed?
>
> Excellent idea. It doesn't look like there is anything "simple" to do
> this already. It'd be close to get_file_caps() but without the bprm.
> Maybe just get_vfs_caps_from_disk() and a walk of the caps? What would
> you recommend?
Yes, I think get_vfs_caps_from_disk() plus identifying whether any permitted
or inheritable capability is set or effective bit is set. IOW, if there is
_anything_ related to cababilities in the file, it is protected.
> > Probably we can enhance this further and allow LSMs to define whether this
> > particular file is special in LSM's point of view (IOW, it can be able to move
> > a process to another security domain which is served by LSM).
>
> Yeah. Perhaps implementing the needed check above with a new security
> check and have commoncaps do the vfs fetch with LSMs able to override?
Yep. Then uid and gid checks from the above code should move to commoncaps
function as default candidates for "privileged files" checks.
Thanks,
--
Vasily Kulikov
http://www.openwall.com - bringing security into open computing environments