Re: [PATCH v3 2/4] xfs: initialize inode security on tmpfile creation

Re: [PATCH v3 2/4] xfs: initialize inode security on tmpfile creation

[Christoph Hellwig]

On Tue, Apr 15, 2014 at 12:18:24PM -0400, Brian Foster wrote:
> +	error = xfs_init_security(inode, dir, &dentry->d_name);
> +	if (unlikely(error)) {
> +		iput(inode);
> +		return -error;
> +	}
> +
>  	d_tmpfile(dentry, inode);
>  

I'd really love to hear from the LSM people who they plan to deal with
O_TMPFILE inodes.    But given that this seems to fix a real life bug
let's go with it for now.

Re: [PATCH v3 2/4] xfs: initialize inode security on tmpfile creation

[Stephen Smalley]

On 04/15/2014 01:50 PM, Christoph Hellwig wrote:
> On Tue, Apr 15, 2014 at 12:18:24PM -0400, Brian Foster wrote:
>> +	error = xfs_init_security(inode, dir, &dentry->d_name);
>> +	if (unlikely(error)) {
>> +		iput(inode);
>> +		return -error;
>> +	}
>> +
>>  	d_tmpfile(dentry, inode);
>>  
> 
> I'd really love to hear from the LSM people who they plan to deal with
> O_TMPFILE inodes.    But given that this seems to fix a real life bug
> let's go with it for now.

Is there a reason that xfs_init_security() isn't called from the inode
allocation function (e.g. xfs_ialloc), as in ext4 (__ext4_new_inode
calls ext4_init_security and also calls ext4_init_acl)?  That would have
ensured that tmpfile inodes would have been labeled without requiring a
separate change and more generally ensures complete coverage for all inodes.

For SELinux, we need the tmpfile inodes to be labeled at creation time,
not just if linked into the namespace, since they may be shared via
local socket IPC or inherited across a label-changing exec and since we
revalidate access on transfer or use.

Labeling based on the provided directory could be a bit random, although
it will work out with current policy if the provided directory
corresponds to existing tmpfile locations (e.g. /tmp, /var/tmp) and
therefore already has a label associated with temporary files.
Otherwise we might want some indication that it is a tmpfile passed into
security_inode_init_security() so that we can always select a stable
label irrespective of the directory.




_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH v3 2/4] xfs: initialize inode security on tmpfile creation

[Stephen Smalley]

On 04/15/2014 04:04 PM, Stephen Smalley wrote:
> On 04/15/2014 01:50 PM, Christoph Hellwig wrote:
>> On Tue, Apr 15, 2014 at 12:18:24PM -0400, Brian Foster wrote:
>>> +	error = xfs_init_security(inode, dir, &dentry->d_name);
>>> +	if (unlikely(error)) {
>>> +		iput(inode);
>>> +		return -error;
>>> +	}
>>> +
>>>  	d_tmpfile(dentry, inode);
>>>  
>>
>> I'd really love to hear from the LSM people who they plan to deal with
>> O_TMPFILE inodes.    But given that this seems to fix a real life bug
>> let's go with it for now.
> 
> Is there a reason that xfs_init_security() isn't called from the inode
> allocation function (e.g. xfs_ialloc), as in ext4 (__ext4_new_inode
> calls ext4_init_security and also calls ext4_init_acl)?  That would have
> ensured that tmpfile inodes would have been labeled without requiring a
> separate change and more generally ensures complete coverage for all inodes.
> 
> For SELinux, we need the tmpfile inodes to be labeled at creation time,
> not just if linked into the namespace, since they may be shared via
> local socket IPC or inherited across a label-changing exec and since we
> revalidate access on transfer or use.
> 
> Labeling based on the provided directory could be a bit random, although
> it will work out with current policy if the provided directory
> corresponds to existing tmpfile locations (e.g. /tmp, /var/tmp) and
> therefore already has a label associated with temporary files.
> Otherwise we might want some indication that it is a tmpfile passed into
> security_inode_init_security() so that we can always select a stable
> label irrespective of the directory.

Hmm...wondering if we can use the qstr as a distinguisher; pass NULL for
tmpfile and not for others as in ext4?

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH v3 2/4] xfs: initialize inode security on tmpfile creation

[Stephen Smalley]

On 04/15/2014 04:22 PM, Christoph Hellwig wrote:
> On Tue, Apr 15, 2014 at 04:04:32PM -0400, Stephen Smalley wrote:
>> Is there a reason that xfs_init_security() isn't called from the inode
>> allocation function (e.g. xfs_ialloc), as in ext4 (__ext4_new_inode
>> calls ext4_init_security and also calls ext4_init_acl)?  That would have
>> ensured that tmpfile inodes would have been labeled without requiring a
>> separate change and more generally ensures complete coverage for all inodes.
> 
> Really just code structuring - we don't like callouts to high level VFS
> functions from deep down in the guts of the filesystem.
> 
>> For SELinux, we need the tmpfile inodes to be labeled at creation time,
>> not just if linked into the namespace, since they may be shared via
>> local socket IPC or inherited across a label-changing exec and since we
>> revalidate access on transfer or use.
>>
>> Labeling based on the provided directory could be a bit random, although
>> it will work out with current policy if the provided directory
>> corresponds to existing tmpfile locations (e.g. /tmp, /var/tmp) and
>> therefore already has a label associated with temporary files.
>> Otherwise we might want some indication that it is a tmpfile passed into
>> security_inode_init_security() so that we can always select a stable
>> label irrespective of the directory.
> 
> Just check for I_LINKABLE in i_flags.

Thanks, that should allow us to handle it cleanly in the security modules!


_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH v3 2/4] xfs: initialize inode security on tmpfile creation

[Christoph Hellwig]

On Tue, Apr 15, 2014 at 04:04:32PM -0400, Stephen Smalley wrote:
> Is there a reason that xfs_init_security() isn't called from the inode
> allocation function (e.g. xfs_ialloc), as in ext4 (__ext4_new_inode
> calls ext4_init_security and also calls ext4_init_acl)?  That would have
> ensured that tmpfile inodes would have been labeled without requiring a
> separate change and more generally ensures complete coverage for all inodes.

Really just code structuring - we don't like callouts to high level VFS
functions from deep down in the guts of the filesystem.

> For SELinux, we need the tmpfile inodes to be labeled at creation time,
> not just if linked into the namespace, since they may be shared via
> local socket IPC or inherited across a label-changing exec and since we
> revalidate access on transfer or use.
> 
> Labeling based on the provided directory could be a bit random, although
> it will work out with current policy if the provided directory
> corresponds to existing tmpfile locations (e.g. /tmp, /var/tmp) and
> therefore already has a label associated with temporary files.
> Otherwise we might want some indication that it is a tmpfile passed into
> security_inode_init_security() so that we can always select a stable
> label irrespective of the directory.

Just check for I_LINKABLE in i_flags.

Re: [PATCH v3 2/4] xfs: initialize inode security on tmpfile creation

[Stephen Smalley]

On 04/15/2014 04:21 PM, Stephen Smalley wrote:
> On 04/15/2014 04:22 PM, Christoph Hellwig wrote:
>> On Tue, Apr 15, 2014 at 04:04:32PM -0400, Stephen Smalley wrote:
>>> Is there a reason that xfs_init_security() isn't called from the inode
>>> allocation function (e.g. xfs_ialloc), as in ext4 (__ext4_new_inode
>>> calls ext4_init_security and also calls ext4_init_acl)?  That would have
>>> ensured that tmpfile inodes would have been labeled without requiring a
>>> separate change and more generally ensures complete coverage for all inodes.
>>
>> Really just code structuring - we don't like callouts to high level VFS
>> functions from deep down in the guts of the filesystem.
>>
>>> For SELinux, we need the tmpfile inodes to be labeled at creation time,
>>> not just if linked into the namespace, since they may be shared via
>>> local socket IPC or inherited across a label-changing exec and since we
>>> revalidate access on transfer or use.
>>>
>>> Labeling based on the provided directory could be a bit random, although
>>> it will work out with current policy if the provided directory
>>> corresponds to existing tmpfile locations (e.g. /tmp, /var/tmp) and
>>> therefore already has a label associated with temporary files.
>>> Otherwise we might want some indication that it is a tmpfile passed into
>>> security_inode_init_security() so that we can always select a stable
>>> label irrespective of the directory.
>>
>> Just check for I_LINKABLE in i_flags.
> 
> Thanks, that should allow us to handle it cleanly in the security modules!

Maybe I spoke too soon.  IIUC, I_LINKABLE doesn't necessarily
distinguish tmpfiles from other files, as some tmpfiles may be linkable
and others not.  But what we want is a way to identify all tmpfiles when
security_inode_init_security() is called if we are going to label them
independently of the provided dir.

Also, in that situation, we would need to likewise distinguish them
during the create-time checking, i.e. when security_inode_create() is
called (from may_o_create), as we have to determine the label that will
be applied at that point for permission checking.  And there we do not
have the inode yet so we do not even have I_LINKABLE as a distinguisher.

So I think we need __O_TMPFILE or similar flag passed down to
may_o_create() -> security_inode_create() and to
security_inode_init_security() if we are going to label these files
independently of the provided directory.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH v3 2/4] xfs: initialize inode security on tmpfile creation

[Christoph Hellwig]

On Wed, Apr 16, 2014 at 08:51:38AM -0400, Stephen Smalley wrote:
> Maybe I spoke too soon.  IIUC, I_LINKABLE doesn't necessarily
> distinguish tmpfiles from other files, as some tmpfiles may be linkable
> and others not.  But what we want is a way to identify all tmpfiles when
> security_inode_init_security() is called if we are going to label them
> independently of the provided dir.

Oh, right.  If O_EXCL is specified (another annoying overload of the
flag..) the tmpfile can't ever be linked back into the filesystem
and thus doesn't have I_LINKABLE set.

I guess the best way to fix this is using the magic qstr you suggested
before.  That means security_inode_init_security would need to be
called after d_tmpfile, which most filesystems don't do right now.

Re: [PATCH v3 2/4] xfs: initialize inode security on tmpfile creation

[Stephen Smalley]

On 04/16/2014 10:14 AM, Christoph Hellwig wrote:
> On Wed, Apr 16, 2014 at 08:51:38AM -0400, Stephen Smalley wrote:
>> Maybe I spoke too soon.  IIUC, I_LINKABLE doesn't necessarily
>> distinguish tmpfiles from other files, as some tmpfiles may be linkable
>> and others not.  But what we want is a way to identify all tmpfiles when
>> security_inode_init_security() is called if we are going to label them
>> independently of the provided dir.
> 
> Oh, right.  If O_EXCL is specified (another annoying overload of the
> flag..) the tmpfile can't ever be linked back into the filesystem
> and thus doesn't have I_LINKABLE set.
> 
> I guess the best way to fix this is using the magic qstr you suggested
> before.  That means security_inode_init_security would need to be
> called after d_tmpfile, which most filesystems don't do right now.

I think one could just pass NULL for the qstr as an indicator, which
ext4 already does, so it doesn't require moving after d_tmpfile) IIUC.
However, that doesn't solve the problem for security_inode_create(),
which also needs to know it is dealing with a tmpfile.  So we might want
to just pass an explicit flag to both.