[PATCH] SELinux: work around filesystems which call d_instantiate before setting inode mode

[PATCH] SELinux: work around filesystems which call d_instantiate before setting inode mode

[Eric Paris]

One filesystem in particular, CIFS, is known to call d_instantiate
before setting the mode for for some operations.  It will create a
dentry for any children when doing a directory search and thereby is
calling d_instantiate.   But it does so before setting the inode mode
for the child inodes.  Thus the selinux sclass in the corresponding
inode security struct is set incorrectly to always believe these inodes
are regular files.  Then when operations are performed on these inodes
at a later point in time SELinux will deny operations which may be
allowed for the correct class but not for files or SELinux may check for
access permissions to do operations which do not even pertain to the
'file' class.  An example would be the user may attempt to remove a
subdirectory which would need SELinux permissions to rmdir what it
believes is a 'file.'  rmdir'ing a regular file doesn't make sense and
is obviously not properly defined.  What this patch does is to
recalculate the sclass for each inode on each permission check.  Thus if
a filesystem decided to later (after the d_instantiate) set the mode
bits (as CIFS does) we will make the correct security checks.  We also
output a warning message letting the user know that they have a
filesystem which doing operations in a questionable order.
(Questionable because after calling d_instantiate the new inode may be
available to other threads through the dentry cache with the mode set
improperly)

I believe that the CIFS people were contacted twice trying to get this
changed but we want SELinux users to be able to work now and we want to
know if any other filesystem uses this same ordering or choose to
implement it in the future.

Signed-off-by: Eric Paris <eparis@redhat.com>
Acked-by:  Stephen Smalley <sds@tycho.nsa.gov>
Acked-by: James Morris <jmorris@redhat.com>

 security/selinux/hooks.c       |   27 +++++++++++++++++++++++++++
 security/selinux/include/avc.h |    2 ++
 security/selinux/avc.c         |   14 +++++++++++++-
 3 files changed, 42 insertions(+), 1 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 5d1b8c7..5527aec 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -1066,6 +1066,25 @@ static int task_has_system(struct task_s
 			    SECCLASS_SYSTEM, perms, NULL);
 }
 
+/*
+ * Update the sclass of an inode.  This shouldn't ever do anything unless a FS
+ * actually called d_instantiate before it set the i_mode.
+ */
+static inline void inode_update_sclass(struct inode *inode)
+{
+	struct inode_security_struct *isec = inode->i_security;
+	if (isec->sclass == SECCLASS_FILE)
+	{
+		isec->sclass = inode_mode_to_security_class(inode->i_mode);
+		if (unlikely(isec->sclass != SECCLASS_FILE) && printk_ratelimit())
+			printk(KERN_WARNING "SELinux: Inode on a %s filesystem "
+				"with sclass=file but should have been sclass="
+				"%s, fixing up this issue\n",
+				inode->i_sb->s_type->name,
+				avc_class_to_string(isec->sclass));
+	}
+}
+
 /* Check whether a task has a particular permission to an inode.
    The 'adp' parameter is optional and allows other audit
    data to be passed (e.g. the dentry). */
@@ -1081,6 +1100,8 @@ static int inode_has_perm(struct task_st
 	tsec = tsk->security;
 	isec = inode->i_security;
 
+	inode_update_sclass(inode);
+
 	if (!adp) {
 		adp = &ad;
 		AVC_AUDIT_DATA_INIT(&ad, FS);
@@ -1220,6 +1241,8 @@ static int may_link(struct inode *dir,
 	dsec = dir->i_security;
 	isec = dentry->d_inode->i_security;
 
+	inode_update_sclass(dentry->d_inode);
+
 	AVC_AUDIT_DATA_INIT(&ad, FS);
 	ad.u.fs.dentry = dentry;
 
@@ -1266,6 +1289,8 @@ static inline int may_rename(struct inod
 	old_is_dir = S_ISDIR(old_dentry->d_inode->i_mode);
 	new_dsec = new_dir->i_security;
 
+	inode_update_sclass(old_dir);
+
 	AVC_AUDIT_DATA_INIT(&ad, FS);
 
 	ad.u.fs.dentry = old_dentry;
@@ -2260,6 +2285,8 @@ static int selinux_inode_setxattr(struct
 	if ((current->fsuid != inode->i_uid) && !capable(CAP_FOWNER))
 		return -EPERM;
 
+	inode_update_sclass(inode);
+
 	AVC_AUDIT_DATA_INIT(&ad,FS);
 	ad.u.fs.dentry = dentry;
 
diff --git a/security/selinux/include/avc.h b/security/selinux/include/avc.h
index 960ef18..043d479 100644
--- a/security/selinux/include/avc.h
+++ b/security/selinux/include/avc.h
@@ -125,6 +125,8 @@ int avc_add_callback(int (*callback)(u32
 		     u32 events, u32 ssid, u32 tsid,
 		     u16 tclass, u32 perms);
 
+const char *avc_class_to_string(u16 tclass);
+
 /* Exported to selinuxfs */
 int avc_get_hash_stats(char *page);
 extern unsigned int avc_cache_threshold;
diff --git a/security/selinux/avc.c b/security/selinux/avc.c
index a300702..88bba69 100644
--- a/security/selinux/avc.c
+++ b/security/selinux/avc.c
@@ -218,7 +218,7 @@ static void avc_dump_query(struct audit_
 		audit_log_format(ab, " tcontext=%s", scontext);
 		kfree(scontext);
 	}
-	audit_log_format(ab, " tclass=%s", class_to_string[tclass]);
+	audit_log_format(ab, " tclass=%s", avc_class_to_string(tclass));
 }
 
 /**
@@ -913,3 +913,15 @@ int avc_has_perm(u32 ssid, u32 tsid, u16
 	avc_audit(ssid, tsid, tclass, requested, &avd, rc, auditdata);
 	return rc;
 }
+
+/**
+ * avc_class_to_string - return a human readable string given an object class.
+ * @tclass: the target class we wish to translate
+ *
+ * Simply take the target object class passed to us and return the human
+ * readable string associated with that class
+ */
+const char *avc_class_to_string(u16 tclass)
+{
+	return class_to_string[tclass];
+}

Re: [PATCH] SELinux: work around filesystems which call d_instantiate before setting inode mode

[Benjamin LaHaise]

On Tue, Aug 29, 2006 at 04:08:25PM -0400, Eric Paris wrote:
> diff --git a/security/selinux/include/avc.h b/security/selinux/include/avc.h
> index 960ef18..043d479 100644
> --- a/security/selinux/include/avc.h
> +++ b/security/selinux/include/avc.h
> @@ -125,6 +125,8 @@ int avc_add_callback(int (*callback)(u32
>  		     u32 events, u32 ssid, u32 tsid,
>  		     u16 tclass, u32 perms);
>  
> +const char *avc_class_to_string(u16 tclass);
> +
>  /* Exported to selinuxfs */
>  int avc_get_hash_stats(char *page);
>  extern unsigned int avc_cache_threshold;
> diff --git a/security/selinux/avc.c b/security/selinux/avc.c
> index a300702..88bba69 100644
> --- a/security/selinux/avc.c
> +++ b/security/selinux/avc.c
> @@ -218,7 +218,7 @@ static void avc_dump_query(struct audit_
>  		audit_log_format(ab, " tcontext=%s", scontext);
>  		kfree(scontext);
>  	}
> -	audit_log_format(ab, " tclass=%s", class_to_string[tclass]);
> +	audit_log_format(ab, " tclass=%s", avc_class_to_string(tclass));
>  }
>  
>  /**
> @@ -913,3 +913,15 @@ int avc_has_perm(u32 ssid, u32 tsid, u16
>  	avc_audit(ssid, tsid, tclass, requested, &avd, rc, auditdata);
>  	return rc;
>  }
> +
> +/**
> + * avc_class_to_string - return a human readable string given an object class.
> + * @tclass: the target class we wish to translate
> + *
> + * Simply take the target object class passed to us and return the human
> + * readable string associated with that class
> + */
> +const char *avc_class_to_string(u16 tclass)
> +{
> +	return class_to_string[tclass];
> +}

This portion of the patch has absolutely nothing to do with the core 
changes, and should be separate.  It is also introducing bloat, as the 
array index is very easy to calculate.

		-ben
-- 
"Time is of no importance, Mr. President, only life is important."
Don't Email: <dont@kvack.org>.

Re: [PATCH] SELinux: work around filesystems which call d_instantiate before setting inode mode

[Eric Paris]

On Tue, 2006-08-29 at 16:13 -0400, Benjamin LaHaise wrote:
> On Tue, Aug 29, 2006 at 04:08:25PM -0400, Eric Paris wrote:
> > diff --git a/security/selinux/include/avc.h b/security/selinux/include/avc.h
> > index 960ef18..043d479 100644
> > --- a/security/selinux/include/avc.h
> > +++ b/security/selinux/include/avc.h
> > @@ -125,6 +125,8 @@ int avc_add_callback(int (*callback)(u32
> >  		     u32 events, u32 ssid, u32 tsid,
> >  		     u16 tclass, u32 perms);
> >  
> > +const char *avc_class_to_string(u16 tclass);
> > +
> >  /* Exported to selinuxfs */
> >  int avc_get_hash_stats(char *page);
> >  extern unsigned int avc_cache_threshold;
> > diff --git a/security/selinux/avc.c b/security/selinux/avc.c
> > index a300702..88bba69 100644
> > --- a/security/selinux/avc.c
> > +++ b/security/selinux/avc.c
> > @@ -218,7 +218,7 @@ static void avc_dump_query(struct audit_
> >  		audit_log_format(ab, " tcontext=%s", scontext);
> >  		kfree(scontext);
> >  	}
> > -	audit_log_format(ab, " tclass=%s", class_to_string[tclass]);
> > +	audit_log_format(ab, " tclass=%s", avc_class_to_string(tclass));
> >  }
> >  
> >  /**
> > @@ -913,3 +913,15 @@ int avc_has_perm(u32 ssid, u32 tsid, u16
> >  	avc_audit(ssid, tsid, tclass, requested, &avd, rc, auditdata);
> >  	return rc;
> >  }
> > +
> > +/**
> > + * avc_class_to_string - return a human readable string given an object class.
> > + * @tclass: the target class we wish to translate
> > + *
> > + * Simply take the target object class passed to us and return the human
> > + * readable string associated with that class
> > + */
> > +const char *avc_class_to_string(u16 tclass)
> > +{
> > +	return class_to_string[tclass];
> > +}
> 
> This portion of the patch has absolutely nothing to do with the core 
> changes, and should be separate.  It is also introducing bloat, as the 
> array index is very easy to calculate.
> 
> 		-ben

We need some method to get access to the class_to_string array (defined
inside avc.c) from hooks.c  In my core changes this is needed to be able
to translate the numeric tclass into a human readable string in the new
warning message in hooks.c  I agree this would be needless if the only
user of avc_class_to_string was avc_dump_query but the whole point of
adding this new accessor was the new inode_update_sclass user in
hooks.c.

Were you suggesting that rather than use an accessor function I instead
make the array extern and use it directly?  I don't forsee this array
syntax changing any time in the future but I'd still rather have some
logical interface that is most likely just going to get compiled out.
This way if things do change we don't have to hunt down all the direct
users of the array across different code.

-Eric

Re: [PATCH] SELinux: work around filesystems which call d_instantiate before setting inode mode

[Steve French]

[-- Attachment #1: Type: text/plain, Size: 6418 bytes --]

Eric,
Does this patch do what you need?

It rearranges the cifs call to d_instantiate until after the inode is 
filled in in fs/cifs/readdir.c
which IIRC was the only place which did the reverse order from what you 
expected (at
least the only place in cifs).   I will try it tomorrow but I don't know 
SE Linux
scenarios to try that would prove whether it works.



>-
>
>Eric Paris <eparis@parisplace.org> wrote on 08/29/2006 03:08:25 PM:
>  
>
>>One filesystem in particular, CIFS, is known to call d_instantiate
>>before setting the mode for for some operations.  It will create a
>>dentry for any children when doing a directory search and thereby is
>>calling d_instantiate.   But it does so before setting the inode mode
>>for the child inodes.  Thus the selinux sclass in the corresponding
>>inode security struct is set incorrectly to always believe these inodes
>>are regular files.  Then when operations are performed on these inodes
>>at a later point in time SELinux will deny operations which may be
>>allowed for the correct class but not for files or SELinux may check for
>>access permissions to do operations which do not even pertain to the
>>'file' class.  An example would be the user may attempt to remove a
>>subdirectory which would need SELinux permissions to rmdir what it
>>believes is a 'file.'  rmdir'ing a regular file doesn't make sense and
>>is obviously not properly defined.  What this patch does is to
>>recalculate the sclass for each inode on each permission check.  Thus if
>>a filesystem decided to later (after the d_instantiate) set the mode
>>bits (as CIFS does) we will make the correct security checks.  We also
>>output a warning message letting the user know that they have a
>>filesystem which doing operations in a questionable order.
>>(Questionable because after calling d_instantiate the new inode may be
>>available to other threads through the dentry cache with the mode set
>>improperly)
>>
>>I believe that the CIFS people were contacted twice trying to get this
>>changed but we want SELinux users to be able to work now and we want to
>>know if any other filesystem uses this same ordering or choose to
>>implement it in the future.
>>
>>Signed-off-by: Eric Paris <eparis@redhat.com>
>>Acked-by:  Stephen Smalley <sds@tycho.nsa.gov>
>>Acked-by: James Morris <jmorris@redhat.com>
>>
>> security/selinux/hooks.c       |   27 +++++++++++++++++++++++++++
>> security/selinux/include/avc.h |    2 ++
>> security/selinux/avc.c         |   14 +++++++++++++-
>> 3 files changed, 42 insertions(+), 1 deletions(-)
>>
>>diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>>index 5d1b8c7..5527aec 100644
>>--- a/security/selinux/hooks.c
>>+++ b/security/selinux/hooks.c
>>@@ -1066,6 +1066,25 @@ static int task_has_system(struct task_s
>>              SECCLASS_SYSTEM, perms, NULL);
>> }
>>
>>+/*
>>+ * Update the sclass of an inode.  This shouldn't ever do anything 
>>unless a FS
>>+ * actually called d_instantiate before it set the i_mode.
>>+ */
>>+static inline void inode_update_sclass(struct inode *inode)
>>+{
>>+   struct inode_security_struct *isec = inode->i_security;
>>+   if (isec->sclass == SECCLASS_FILE)
>>+   {
>>+      isec->sclass = inode_mode_to_security_class(inode->i_mode);
>>+      if (unlikely(isec->sclass != SECCLASS_FILE) && 
>>    
>>
>printk_ratelimit())
>  
>
>>+         printk(KERN_WARNING "SELinux: Inode on a %s filesystem "
>>+            "with sclass=file but should have been sclass="
>>+            "%s, fixing up this issue\n",
>>+            inode->i_sb->s_type->name,
>>+            avc_class_to_string(isec->sclass));
>>+   }
>>+}
>>+
>> /* Check whether a task has a particular permission to an inode.
>>    The 'adp' parameter is optional and allows other audit
>>    data to be passed (e.g. the dentry). */
>>@@ -1081,6 +1100,8 @@ static int inode_has_perm(struct task_st
>>    tsec = tsk->security;
>>    isec = inode->i_security;
>>
>>+   inode_update_sclass(inode);
>>+
>>    if (!adp) {
>>       adp = &ad;
>>       AVC_AUDIT_DATA_INIT(&ad, FS);
>>@@ -1220,6 +1241,8 @@ static int may_link(struct inode *dir,
>>    dsec = dir->i_security;
>>    isec = dentry->d_inode->i_security;
>>
>>+   inode_update_sclass(dentry->d_inode);
>>+
>>    AVC_AUDIT_DATA_INIT(&ad, FS);
>>    ad.u.fs.dentry = dentry;
>>
>>@@ -1266,6 +1289,8 @@ static inline int may_rename(struct inod
>>    old_is_dir = S_ISDIR(old_dentry->d_inode->i_mode);
>>    new_dsec = new_dir->i_security;
>>
>>+   inode_update_sclass(old_dir);
>>+
>>    AVC_AUDIT_DATA_INIT(&ad, FS);
>>
>>    ad.u.fs.dentry = old_dentry;
>>@@ -2260,6 +2285,8 @@ static int selinux_inode_setxattr(struct
>>    if ((current->fsuid != inode->i_uid) && !capable(CAP_FOWNER))
>>       return -EPERM;
>>
>>+   inode_update_sclass(inode);
>>+
>>    AVC_AUDIT_DATA_INIT(&ad,FS);
>>    ad.u.fs.dentry = dentry;
>>
>>diff --git a/security/selinux/include/avc.h 
>>    
>>
>b/security/selinux/include/avc.h
>  
>
>>index 960ef18..043d479 100644
>>--- a/security/selinux/include/avc.h
>>+++ b/security/selinux/include/avc.h
>>@@ -125,6 +125,8 @@ int avc_add_callback(int (*callback)(u32
>>            u32 events, u32 ssid, u32 tsid,
>>            u16 tclass, u32 perms);
>>
>>+const char *avc_class_to_string(u16 tclass);
>>+
>> /* Exported to selinuxfs */
>> int avc_get_hash_stats(char *page);
>> extern unsigned int avc_cache_threshold;
>>diff --git a/security/selinux/avc.c b/security/selinux/avc.c
>>index a300702..88bba69 100644
>>--- a/security/selinux/avc.c
>>+++ b/security/selinux/avc.c
>>@@ -218,7 +218,7 @@ static void avc_dump_query(struct audit_
>>       audit_log_format(ab, " tcontext=%s", scontext);
>>       kfree(scontext);
>>    }
>>-   audit_log_format(ab, " tclass=%s", class_to_string[tclass]);
>>+   audit_log_format(ab, " tclass=%s", avc_class_to_string(tclass));
>> }
>>
>> /**
>>@@ -913,3 +913,15 @@ int avc_has_perm(u32 ssid, u32 tsid, u16
>>    avc_audit(ssid, tsid, tclass, requested, &avd, rc, auditdata);
>>    return rc;
>> }
>>+
>>+/**
>>+ * avc_class_to_string - return a human readable string given an 
>>object class.
>>+ * @tclass: the target class we wish to translate
>>+ *
>>+ * Simply take the target object class passed to us and return the 
>>    
>>
>human
>  
>
>>+ * readable string associated with that class
>>+ */
>>+const char *avc_class_to_string(u16 tclass)
>>+{
>>+   return class_to_string[tclass];
>>+}
>>    
>>
>
>  
>


[-- Attachment #2: cifs-readdir-dentry-instantiate-ordering-change.diff --]
[-- Type: text/plain, Size: 1006 bytes --]

diff --git a/fs/cifs/readdir.c b/fs/cifs/readdir.c
index 105761e..9aeb58a 100644
--- a/fs/cifs/readdir.c
+++ b/fs/cifs/readdir.c
@@ -82,7 +82,6 @@ static int construct_dentry(struct qstr 
 			if(*ptmp_inode == NULL)
 				return rc;
 			rc = 1;
-			d_instantiate(tmp_dentry, *ptmp_inode);
 		}
 	} else {
 		tmp_dentry = d_alloc(file->f_dentry, qstring);
@@ -99,9 +98,7 @@ static int construct_dentry(struct qstr 
 			tmp_dentry->d_op = &cifs_dentry_ops;
 		if(*ptmp_inode == NULL)
 			return rc;
-		rc = 1;
-		d_instantiate(tmp_dentry, *ptmp_inode);
-		d_rehash(tmp_dentry);
+		rc = 2;
 	}
 
 	tmp_dentry->d_time = jiffies;
@@ -870,6 +867,12 @@ static int cifs_filldir(char *pfindEntry
 				pfindEntry, &obj_type, rc);
 	else
 		fill_in_inode(tmp_inode, 1 /* NT */, pfindEntry, &obj_type, rc);
+
+	if(rc) /* new inode - needs to be tied to dentry */ {
+		d_instantiate(tmp_dentry, tmp_inode);
+		if(rc == 2)
+			d_rehash(tmp_dentry);
+	}
 	
 	
 	rc = filldir(direntry,qstring.name,qstring.len,file->f_pos,

Re: [PATCH] SELinux: work around filesystems which call d_instantiate before setting inode mode

[Eric Paris]

On Tue, 2006-08-29 at 23:01 -0500, Steve French wrote:
> Eric,
> Does this patch do what you need?
> 
> It rearranges the cifs call to d_instantiate until after the inode is 
> filled in in fs/cifs/readdir.c
> which IIRC was the only place which did the reverse order from what you 
> expected (at
> least the only place in cifs).   I will try it tomorrow but I don't know 
> SE Linux
> scenarios to try that would prove whether it works.

This patch does seem to solve the issue with CIFS that we were
experiencing and I would love to see it submitted.

I would also like to have my original patch included as it will help to
flush out any other cases of this ordering in the future.

-Eric