Re: [PATCH] SELinux: requesting no permissions in avc_has_perm_noaudit is a BUG()

[Eric Paris <eparis@redhat.com>]

On Wed, 2008-03-12 at 13:18 -0400, Stephen Smalley wrote:
> On Wed, 2008-03-12 at 11:31 -0400, Eric Paris wrote:
> > On Wed, 2008-03-12 at 09:26 +1100, James Morris wrote:
> > > On Wed, 12 Mar 2008, James Morris wrote:
> > > 
> > > > Applied to 
> > > > git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/selinux-2.6.git#for-akpm
> > > 
> > > Just saw this during boot.
> > > 
> > > [    8.238863] ------------[ cut here ]------------
> > > [    8.239762] kernel BUG at security/selinux/avc.c:874!
> > 
> > > [    8.239762]  [<ffffffff802a7e65>] ? link_path_walk+0xbd/0xcd
> > > [    8.239762]  [<ffffffff8031ab34>] avc_has_perm+0x2e/0x5e
> > > [    8.239762]  [<ffffffff8031b9ca>] inode_has_perm+0x6b/0x7a
> > > [    8.239762]  [<ffffffff8031f242>] selinux_dentry_open+0x6a/0x73
> > > [    8.239762]  [<ffffffff8031742e>] security_dentry_open+0x11/0x13
> > > [    8.239762]  [<ffffffff8029c69a>] __dentry_open+0xce/0x1d3
> > > [    8.239762]  [<ffffffff8029c838>] nameidata_to_filp+0x2e/0x40
> > > [    8.239762]  [<ffffffff8029c880>] do_filp_open+0x36/0x46
> > > [    8.239762]  [<ffffffff804a9da3>] ? _spin_unlock+0x26/0x2a
> > > [    8.239762]  [<ffffffff8029c5b1>] ? get_unused_fd_flags+0x113/0x121
> > > [    8.239762]  [<ffffffff8029c8e1>] do_sys_open+0x51/0xd2
> > > [    8.239762]  [<ffffffff8029c98b>] sys_open+0x1b/0x1d
> > > [    8.239762]  [<ffffffff8020bf7b>] system_call_after_swapgs+0x7b/0x80
> > 
> > Admittedly I don't understand all the code, but James, can you let me
> > know if this solves the problem?  I don't see this issue on my machine
> > and I don't know what opening a file with 'special' means.
> > 
> > /*
> >  * Note that while the flag value (low two bits) for sys_open means:
> >  *      00 - read-only
> >  *      01 - write-only
> >  *      10 - read-write
> >  *      11 - special
> >  * it is changed into
> >  *      00 - no permissions needed
> >  *      01 - read-permission
> >  *      10 - write-permission
> >  *      11 - read-write
> >  * for the internal routines (ie open_namei()/follow_link() etc). 00 is
> >  * used by symlinks.
> >  */
> > 
> > I noticed that the conversion of flags to f_mode in __dentry_open()
> > handles things a bit differently than do_filp_open().  This makes the
> > __dentry_open() handling like do_filp_open() so that 'special' gets
> > turned into RW instead of no permissions.
> > 
> > Time to go figure out what the heck 'special' means....
> > 
> > -Eric
> > 
> > ---
> > 
> > diff --git a/fs/open.c b/fs/open.c
> > index 5419853..04a8efa 100644
> > --- a/fs/open.c
> > +++ b/fs/open.c
> > @@ -736,10 +736,15 @@ static struct file *__dentry_open(struct dentry *dentry, struct vfsmount *mnt,
> >  {
> >  	struct inode *inode;
> >  	int error;
> > +	mode_t f_mode;
> > +
> > +	if ((flags+1) & O_ACCMODE)
> > +		f_mode = (flags+1) & O_ACCMODE;
> > +	else
> > +		f_mode = flags & O_ACCMODE;
> >  
> >  	f->f_flags = flags;
> > -	f->f_mode = ((flags+1) & O_ACCMODE) | FMODE_LSEEK |
> > -				FMODE_PREAD | FMODE_PWRITE;
> > +	f->f_mode = f_mode | FMODE_LSEEK | FMODE_PREAD | FMODE_PWRITE;
> >  	inode = dentry->d_inode;
> >  	if (f->f_mode & FMODE_WRITE) {
> >  		error = get_write_access(inode);
> 
> Alternatively, if this is a valid state and is used internally by the
> kernel, then possibly selinux_dentry_open() should just return 0
> immediately if ((file->f_mode & (FMODE_READ | FMODE_WRITE) == 0), much
> as selinux_inode_permission() returns 0 if the mask is 0 (existence
> test).  Not sure though why we haven't encountered this before since we
> should have been getting denials even before Eric's patch.

We probably did start getting denials when we added the
security_dentry_open() call.  But it appears from my looking that this
is always a buggy program.  (I filed a bug against mdadm, number 437145)

Different paths, different flags:

sys_open
 do_sys_open
  do_filp_open
   open_namei
    vfs_permission
     permission
      security_inode_permission
       selinux_inode_permission

do_filp_open() has:
        if ((namei_flags+1) & O_ACCMODE)
                namei_flags++;
so flags of 11 get mapped to 11, which means read and write.


sys_open
 do_sys_open
  do_filp_open
   nameidata_to_filp
    __dentry_open
     security_dentry_open

dentry_open() has:
       f->f_mode = ((flags+1) & O_ACCMODE) | FMODE_LSEEK |
                               FMODE_PREAD | FMODE_PWRITE;
so flags of 11 get mapped to 00, which means no permission.

Before security_dentry_open I think we would have been fine.  Since then
I think buggy programs probably got a denial.  Maybe noone noticed with
mdadm since here is the code in question:

                close(0);
                open("/dev/null", 3);
                dup2(0,1);
                dup2(0,2);
                setsid();

Good error checking there....

I can't find anywhere in kernel that uses x & O_ACCMODE == O_ACCMODE;

And my latest patch seems to be booting find and not bugging the
security_dentry_open() call path.....

-Eric


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.