Can the VFS layer rely on i_uid

Can the VFS layer rely on i_uid

[Andy Whitcroft]

We have been testing with some of the newly proposed security checks from
the Yama security module.  Some of the pending checks for that module
use inode ownership and permissions as factors in these checks, checks
essentially done at the VFS level.  The issue I am seeing is that not all
filesystems populate i_uid or i_mode (completely).  Overlayfs for example
does not populate i_uid in its inodes.  This leads these Yama checks to
fail unexpectedly.

When an inode does not provide a permissions() op generic permissions
checks are used and these assume that i_mode, i_uid, and i_gid are
all populated, using them directly to perform these checks.  When a
permissions() op is provided obviously this code is not used.  What is not
so clear to me is whether there is still an assumption or requirement that
these are populated in this case.  My gut feeling is that if you have a
permissions() op then there is no obligation to use these fields at all,
indeed it seems entirely reasonable that your permission model not map
sensibly onto such permissions.

I am hoping that someone in the know could clarify what you may rely on
when at the VFS layer, or point me at the documenation if I have missed it.

Thanks.

-apw

Re: Can the VFS layer rely on i_uid

[Al Viro]

On Mon, Mar 05, 2012 at 12:12:39PM +0000, Andy Whitcroft wrote:

> When an inode does not provide a permissions() op generic permissions
> checks are used and these assume that i_mode, i_uid, and i_gid are
> all populated, using them directly to perform these checks.  When a
> permissions() op is provided obviously this code is not used.  What is not
> so clear to me is whether there is still an assumption or requirement that
> these are populated in this case.  My gut feeling is that if you have a
> permissions() op then there is no obligation to use these fields at all,
> indeed it seems entirely reasonable that your permission model not map
> sensibly onto such permissions.

Not quite...  Ideally, yes, we'd want to have ->i_uid used only by fs-specific
code and helpers used by that fs (including those that are implicit defaults).
And BTW, you proposal to have non-trivial differences in behaviour of code
based on whether ->permission() (and ->getattr()) is NULL is an atrocity -
this kind of layering violations is wrong and brittle, so please abstain from
that.  In practice we have enough places where uid/gid is used directly
to make setting them practically a requirement - places like /proc/<pid>/
can get away with not doing that, but only because shitloads of syscalls
are not allowed on those anyway, permissions or no permissions.  In anything
general-purpose you really need to set it.

Re: Can the VFS layer rely on i_uid

[Andy Whitcroft]

On Mon, Mar 5, 2012 at 1:26 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Mon, Mar 05, 2012 at 12:12:39PM +0000, Andy Whitcroft wrote:
>
>> When an inode does not provide a permissions() op generic permissions
>> checks are used and these assume that i_mode, i_uid, and i_gid are
>> all populated, using them directly to perform these checks.  When a
>> permissions() op is provided obviously this code is not used.  What is not
>> so clear to me is whether there is still an assumption or requirement that
>> these are populated in this case.  My gut feeling is that if you have a
>> permissions() op then there is no obligation to use these fields at all,
>> indeed it seems entirely reasonable that your permission model not map
>> sensibly onto such permissions.
>
> Not quite...  Ideally, yes, we'd want to have ->i_uid used only by fs-specific
> code and helpers used by that fs (including those that are implicit defaults).
> And BTW, you proposal to have non-trivial differences in behaviour of code
> based on whether ->permission() (and ->getattr()) is NULL is an atrocity -
> this kind of layering violations is wrong and brittle, so please abstain from
> that.  In practice we have enough places where uid/gid is used directly
> to make setting them practically a requirement - places like /proc/<pid>/
> can get away with not doing that, but only because shitloads of syscalls
> are not allowed on those anyway, permissions or no permissions.  In anything
> general-purpose you really need to set it.

I was not trying to suggest that code could look at whether
->permission() was NULL from above and use that to rely on the values
*shudder*.  I was mearly expressing the feeling that it seemed that
there was a distinct possibily that those fields were really only
there for the ops and below to use.  From your description here it
seems we should be able to assume these fields are populated, which
makes these Yama checks valid and so I need to look to have them
populated.

Thanks.

-apw
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Can the VFS layer rely on i_uid

[Szeredi Miklos]

On Mon, Mar 5, 2012 at 2:48 PM, Andy Whitcroft <apw@canonical.com> wrote:
> I was not trying to suggest that code could look at whether
> ->permission() was NULL from above and use that to rely on the values
> *shudder*.  I was mearly expressing the feeling that it seemed that
> there was a distinct possibily that those fields were really only
> there for the ops and below to use.  From your description here it
> seems we should be able to assume these fields are populated, which
> makes these Yama checks valid and so I need to look to have them
> populated.

I'd argue that overlayfs is also special (anything that opens a file
will only be able to access the underlying inode) and Yama should do a
vfs_getattr() to access the file attributes.

Thanks,
Miklos
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Can the VFS layer rely on i_uid

[Tetsuo Handa]

Szeredi Miklos wrote:
> I'd argue that overlayfs is also special (anything that opens a file
> will only be able to access the underlying inode) and Yama should do a
> vfs_getattr() to access the file attributes.

TOMOYO and AppArmor are using i_uid field.

693 void tomoyo_get_attributes(struct tomoyo_obj_info *obj)
694 {
695         u8 i;
696         struct dentry *dentry = NULL;
697 
698         for (i = 0; i < TOMOYO_MAX_PATH_STAT; i++) {
699                 struct inode *inode;
700                 switch (i) {
701                 case TOMOYO_PATH1:
702                         dentry = obj->path1.dentry;
703                         if (!dentry)
704                                 continue;
705                         break;
706                 case TOMOYO_PATH2:
707                         dentry = obj->path2.dentry;
708                         if (!dentry)
709                                 continue;
710                         break;
711                 default:
712                         if (!dentry)
713                                 continue;
714                         dentry = dget_parent(dentry);
715                         break;
716                 }
717                 inode = dentry->d_inode;
718                 if (inode) {
719                         struct tomoyo_mini_stat *stat = &obj->stat[i];
720                         stat->uid  = inode->i_uid;
721                         stat->gid  = inode->i_gid;
722                         stat->ino  = inode->i_ino;
723                         stat->mode = inode->i_mode;
724                         stat->dev  = inode->i_sb->s_dev;
725                         stat->rdev = inode->i_rdev;
726                         obj->stat_valid[i] = true;
727                 }
728                 if (i & 1) /* i == TOMOYO_PATH1_PARENT ||
729                               i == TOMOYO_PATH2_PARENT */
730                         dput(dentry);
731         }
732 }

376 static int apparmor_dentry_open(struct file *file, const struct cred *cred)
377 {
378         struct aa_file_cxt *fcxt = file->f_security;
379         struct aa_profile *profile;
380         int error = 0;
381 
382         if (!mediated_filesystem(file->f_path.dentry->d_inode))
383                 return 0;
384 
385         /* If in exec, permission is handled by bprm hooks.
386          * Cache permissions granted by the previous exec check, with
387          * implicit read and executable mmap which are required to
388          * actually execute the image.
389          */
390         if (current->in_execve) {
391                 fcxt->allow = MAY_EXEC | MAY_READ | AA_EXEC_MMAP;
392                 return 0;
393         }
394 
395         profile = aa_cred_profile(cred);
396         if (!unconfined(profile)) {
397                 struct inode *inode = file->f_path.dentry->d_inode;
398                 struct path_cond cond = { inode->i_uid, inode->i_mode };
399 
400                 error = aa_path_perm(OP_OPEN, profile, &file->f_path, 0,
401                                      aa_map_file_to_perms(file), &cond);
402                 /* todo cache full allowed permissions set and state */
403                 fcxt->allow = aa_map_file_to_perms(file);
404         }
405 
406         return error;
407 }

So, TOMOYO and AppArmor need to carefully call (I mean, not to trigger
unlimited recursive LSM calls) vfs_getattr() if the inode's magic number says
it is an overlayfs inode?

I'd like to browse the code. Is the current code
http://git.kernel.org/?p=linux/kernel/git/mszeredi/vfs.git #overlayfs.v12 ?