Bug 196567 - CIFS does not set sclass before d_instantiate causes SELinux failures
CIFS does not set sclass before d_instantiate causes SELinux failures
Status: CLOSED UPSTREAM
Product: Fedora
Classification: Fedora
Component: kernel (Show other bugs)
5
All Linux
medium Severity medium
: ---
: ---
Assigned To: Eric Paris
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2006-06-24 18:02 EDT by David-Alexandre Davidson
Modified: 2007-11-30 17:11 EST (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-10-11 15:47:04 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)

  None (edit)
Description David-Alexandre Davidson 2006-06-24 18:02:00 EDT
Description of problem:
When running httpd with php scripts in cgi mode and accessing object on a cifs
share, Selinux report directories on that mount as target security class
(tclass) "file", causing actions such as search and rmdir impossible on that
directory. On such occasion the audit.log report an HEX value in the denial
description instead of the mapped name, because selinux cannot map "search" to
any existing action available for a file. example from audit.log:

type=AVC msg=audit(1151178769.339:656): avc:  denied  { 0x200000 } for  pid=2834
comm="rmdir" name="sample_dir" dev=cifs ino=9600
scontext=root:system_r:httpd_suexec_t:s0 tcontext=system_u:object_r:cifs_t:s0
tclass=file
type=SYSCALL msg=audit(1151178769.339:656): arch=40000003 syscall=40 success=no
exit=-13 a0=8b58c14 a1=81a4a60 a2=8b58c14 a3=bf8bbdd8 items=1 pid=2834 auid=0
uid=20000 gid=20000 euid=20000 suid=20000 fsuid=20000 egid=20000 sgid=20000
fsgid=20000 tty=(none) comm="rmdir" exe="/usr/bin/php"
subj=root:system_r:httpd_suexec_t:s0

When accessing the cifs share with selinux disabled, or from an unconfined space
no problem occurs. In the sample above I run the cgi script with suexec, but the
 problem is the same when running scripts in the httpd_t context (I really don't
believe httpd is involved anyway, it should be the same with other confined
spaces. But it's where observed the bug in the first place)

Version-Release number of selected component (if applicable):
Fedora core 5, with kernel 2.6.17-1.2139 (same behavior as previous 2 releases)
httpd 2.2.0-5.1.2
libselinux 1.30.3-3
samba 3.0.22-1
selinux-policy-targeted 2.2.43-4

How reproducible:
Always

Steps to Reproduce:
1. install httpd samba samba-client selinux-targeted-policy php
2. set :  "unix extensions = no"    in /etc/samba/smb.conf
3. create a share locally and put some content in it (random files / dirs)
4. Add a user to use in accessing the share
5. map the share locally (for testing it is useful to set uid/gid of the mount
to apache's one) I used autofs with the following options to mount in
/home/testmount :
testmount
-fstype=cifs,username=shareuser,password=sharepass,uid=48,gid=48,dir_mode=0770,file_mode=0660
://localhost/testmount
6. create a virtualhost or set apache to run php as cgi (result could be the
same with mod_php)
7. create a test script  "<?php rmdir('/home/testmount/sample_dir'); ?>
8. run that script with a web browser

(9. Add all selinux rules that could be needed (for cifs_t, httpd_t, etc) and be
sure unix permissions are sufficient on all files and folder. be sure that
sample_dir is an empty directory)

Actual results:
Directory is not removed and selinux give the error denied { 0x200000 } on
sample_dir and incorectly identify it as a file instead of a folder

Expected results:
Directory should be deleted with no denial

Additional info:
0x200000 is related to rmdir.  Using a script like "<?php shell_exec('du
/home/testmount'); ?>",  An error of 0x100000 is reported on some folder
(search)  which also are incorectly identified as files.
Comment 1 David-Alexandre Davidson 2006-06-24 21:03:35 EDT
An easier way to test this issue using the same cifs share setup:
enable the strict policy, (I set a permissive state)  and as root do:
rmdir /home/testmount/sample_dir

observe in audit.log :
type=AVC msg=audit(1151197545.712:144): avc:  denied  { 0x200000 } for  pid=2608
comm="rmdir" name="sample_dir" dev=cifs ino=8889
scontext=root:staff_r:staff_t:s0-s0:c0.c255 tcontext=system_u:object_r:cifs_t:s0
tclass=file
Comment 2 Stephen Smalley 2006-06-28 11:31:09 EDT
SELinux sets up the inode security state upon d_instantiate, so it expects the
inode mode to be set (so that it can set the inode security class).  cifs
appears to call d_instantiate before fully setting up the inode state.  Either
cifs needs to change to set up the inode first, or selinux needs to revalidate
the inode security class on every hook call/permission check.
Comment 3 Stephen Smalley 2006-06-28 11:48:32 EDT
This bug is identical to bug 163493, which unfortunately was closed without
resolution.
I raised that bug with the upstream cifs maintainer at the time, and he
acknowledged it, but I don't think anything happened to address it in the cifs code.
Comment 4 Stephen Smalley 2006-06-28 13:25:41 EDT
Re-raised issue with upstream cifs maintainer.
cifs_filldir and cifs_mkdir seem to both call d_instantiate before setting up
the inode state.  I think that this is a general bug in cifs, as it would mean
that the inode might be visible to other threads in an inconsistent state.
Comment 5 Stephen Smalley 2006-07-28 13:36:28 EDT
I think we need to work around this issue in the SELinux code, both for cifs and
any other filesystems that may fail to set the inode mode properly before
calling d_instantiate.

We could recompute the isec->sclass via
inode_mode_to_security_class(inode->i_mode) in inode_has_perm, as long as it is
still SECCLASS_FILE.  If it has already been set to any other class, we
shouldn't touch it again, because it has already been set to a more specific
value, and in particular, we don't want to disturb it if it has been set to a
socket class by the socket hooks.
Comment 6 Eric Paris 2006-08-04 12:25:12 EDT
Would it be reasonable to print a warning if we actually make a change in
inode_has_perm to sclass?  Yes this is fixing the problem with CIFS since they
don't seem to be interested in fixing it, but it may also paper over problems in
other filesystems that may creep up and may be willing to fix it.
Comment 7 Stephen Smalley 2006-08-04 12:33:17 EDT
Possibly, but make sure you call printk_ratelimit(); otherwise, you could flood
the system if the fs lazily sets the inode mode for all of its inodes...
Comment 8 Eric Paris 2006-08-08 13:22:11 EDT
Taking bug as I'm sure Dan doesn't mind one less on his list.
Comment 9 Eric Paris 2006-08-09 16:53:51 EDT
I found 4 hooks that seem to use isec->sclass and we may wish to have it rechecked.

inode_has_perm
may_link
may_rename
selinux_inode_setxattr

At first I wondered if inode_has_perm covered all 4 cases, but I don't see that
this is true.  I'm going to check again tomorrow to see if there is one place I
can put this to be sure it covers all 4 hooks.  The code I plan to add
(untested, not even compiled) is below.

        if (isec->sclass == SECCLASS_FILE)
        {
                isec->sclass = inode_mode_to_security_class(inode->i_mode);
                /*
                 * can be caused by fs calling d_instantiate before setting i_mode.
                 */
                if (unlikely(isec->sclass != SECCLASS_FILE) && printk_ratelimit())
                        printk(KERN_INFO "Inode found with sclass set improperly."
                                "  Please contact the filesystem maintainer\n");
        }
Comment 10 Stephen Smalley 2006-08-10 09:59:42 EDT
I'm not sure it is truly necessary to have complete coverage here, as the
d_instantiate/d_splice_alias hook is supposed to provide us with complete
coverage in the first place, and the inode_has_perm one is just to catch these
erroneous cases which manifest as search attempts on directories that are
mis-classified as files.  If you are going to do it in multiple places, use a
static inline helper.

The message isn't useful as it stands because it doesn't provide any identifying
information, e.g. the fs type, the device, the inode number, etc.  
Comment 11 Eric Paris 2006-08-14 13:39:36 EDT
I think i'm going to go for complete coverage unless there is a strong
objection.  inode_has_perm does take care of the search requests.  And actually
it also took care of using mv (which I hoped would hit may_rename) and chcon
(which I hoped would hit selinux_inode_setxattr) but I'm still able to hit
0x200000 with rmdir (which I'm reasonably sure is from running down the may_link
path.

I'll make the error message pretty and useful, use an inline helper and post
something soon.
Comment 12 Eric Paris 2006-09-25 11:09:33 EDT
A CIFS fix was included in 2.6.18.  If we still want to include my code to make
checks, assurances, and reporting is still up in the air.
Comment 13 Eric Paris 2006-10-11 15:47:04 EDT
Since there are no known problems we are going to pass on the redundant check of
the type on every access.  Thus this bug is solved in 2.6.18 and later.

Note You need to log in before you can comment on or make changes to this bug.