linux-kernel - Re: [PATCH 5/7] SELinux: Handle opening of a unioned file

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <55805AEA.3050407@tycho.nsa.gov>
Date:	Tue, 16 Jun 2015 13:20:42 -0400
From:	Stephen Smalley <sds@...ho.nsa.gov>
To:	David Howells <dhowells@...hat.com>
CC:	linux-unionfs@...r.kernel.org, linux-kernel@...r.kernel.org,
	drquigl <drquigl@...ho.nsa.gov>,
	linux-security-module@...r.kernel.org, selinux@...ho.nsa.gov,
	linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH 5/7] SELinux: Handle opening of a unioned file

On 06/16/2015 12:49 PM, David Howells wrote:
> Stephen Smalley <sds@...ho.nsa.gov> wrote:
> 
>> It looks like commit 415103f9932d45f7927f4b17e3a9a13834cdb9a1 changed
>> selinux_inode_init_security()'s handling of SECURITY_FS_USE_MNTPOINT,
>> and this change was never propagated to selinux_dentry_init_security().
>>  However, that commit also did not update
>> security/selinux/hooks.c:may_create()'s logic for computing the new file
>> label when checking CREATE permission, and therefore introduced a
>> potential inconsistency between the label used for the permission check
>> and the label assigned to the inode.
>>
>> That's why I suggested that we need a common helper for all three to
>> ensure consistency there.
> 
> I think a common helper is harder than it seems.  We need the parent dir in
> one of the cases the helper has to consider, but finding it is done in three
> different ways, depending on the caller:
> 
>  (1) dentry_init can just use ->d_parent as there's a lock held that prevents
>      it changing (I think).  This could use (2) instead, however.
> 
>  (2) file_open has to use dget_parent().
> 
>  (3) inode_init doesn't have any dentries, but rather has the object and
>      parent inodes.
> 
> If we don't mind file_open() always calling dget_parent(), then the common
> helper can take the dir inode.
> 
> Also, thinking ahead to the possibility of bringing unionmount into the kernel
> at some point: union non-dir dentries that are not yet copied up have no inode
> attached, but rather fall through to the underlying lower inode in the VFS.
> This, however, gives us nowhere to hang the inode label.  How expensive is the
> security_transition_sid() call?

Why are you talking about file_open()?  It is may_create() that has the
duplicated logic, which has the parent dir passed to it by its callers
(selinux_inode_create, selinux_inode_symlink, selinux_inode_mkdir,
selinux_mknod).  selinux_inode_init_security() also gets passed the
parent dir directly.  Only selinux_dentry_init_security() has to use
d_parent, which as you say is safe, so it can just pass the resulting
dir to the helper.

Until a process writes to the file, we just want to use the lower inode
label, right?  At the point a process writes to the file and a copy-up
is produced, we could perform a one-time computation, and then once the
upper inode is created, it should get set accordingly.  So I wouldn't
think we would need to call security_transition_sid() frequently.  If
so, we might want to do what was previously done in the userspace AVC in
libselinux, and start caching security_transition_sid() results in the
AVC itself and add an avc_transition_sid() interface (in the userspace
AVC, this is avc_compute_create()).

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/