From: David Howells <dhowells@redhat.com>
To: Stephen Smalley <sds@tycho.nsa.gov>, eparis@redhat.com
Cc: dhowells@redhat.com, linux-unionfs@vger.kernel.org,
linux-kernel@vger.kernel.org, drquigl <drquigl@tycho.nsa.gov>,
linux-security-module@vger.kernel.org, selinux@tycho.nsa.gov,
linux-fsdevel@vger.kernel.org
Subject: Re: [PATCH 5/7] SELinux: Handle opening of a unioned file
Date: Thu, 18 Jun 2015 11:15:39 +0100 [thread overview]
Message-ID: <8182.1434622539@warthog.procyon.org.uk> (raw)
In-Reply-To: <558187EA.60805@tycho.nsa.gov>
Stephen Smalley <sds@tycho.nsa.gov> wrote:
> >> Until a process writes to the file, we just want to use the lower inode
> >> label, right?
> >
> > No.
> >
> > There are two issues:
> >
> > (1) Non-fd accesses to an overlayfs file use the security label on the
> > overlay inode, not the lower inode, even before copy up because they
> > go through the inode ops of the overlayfs file first.
> >
> > (2) I'm told that we want the ability to have a different label on the
> > upper file to that on the lower file. This is trivial in overlayfs
> > since you always have an overlay inode off which to hang the security
> > label, but tricky with unionmount since you may only have a dentry.
>
> I recall that being controversial. I agree that if a process attempts
> to write to the file and a copy-up is triggered, then we want to be able
> to label the copy of the file differently. But until that happens, why
> wouldn't we simply treat the file as having the lower file label for
> access control purposes on read operations?
Actually, for overlayfs, I've made it such that the label off of the overlay
inode is used for the open file. This label gets determined when the overlay
inode is set up and so is cached for the lifetime of the inode struct. Like
this in the patch I posted:
+ if (inode) {
+ isec = inode->i_security;
+ fsec->union_isid = isec->sid;
+ } ...
where 'inode' is the overlay inode. The subsequent bits:
+ } else if ((sbsec->flags & SE_SBINITIALIZED) &&
+ (sbsec->behavior == SECURITY_FS_USE_MNTPOINT)) {
+ fsec->union_isid = sbsec->mntpoint_sid;
+ } else {
+ dir = dget_parent(union_path->dentry);
+ dsec = dir->d_inode->i_security;
+
+ rc = security_transition_sid(
+ tsec->sid, dsec->sid,
+ inode_mode_to_security_class(file_inode(file)->i_mode),
+ &union_path->dentry->d_name,
+ &fsec->union_isid);
+ dput(dir);
+ if (rc) {
+ printk(KERN_WARNING "%s: "
+ "security_transition_sid failed, rc=%d (name=%pD)\n",
+ __func__, -rc, file);
+ return rc;
+ }
+ }
are for future unionmount support where there isn't an inode in the union
layer but only an inode in a lower layer.
In patch 7/7, if fsec->union_isid is non-zero in file_has_perm() then that is
used directly with avc_has_perm() in preference to calling inode_has_perm() on
the overlay or lower inodes.
Btw, is it correct that file operations must follow the label currently on the
inode rather than caching it in file_security_struct?
> > (2) I'm told that we want the ability to have a different label on the
> > upper file to that on the lower file. This is trivial in overlayfs
> > since you always have an overlay inode off which to hang the security
> > label, but tricky with unionmount since you may only have a dentry.
>
> I recall that being controversial. I agree that if a process attempts
> to write to the file and a copy-up is triggered, then we want to be able
> to label the copy of the file differently. But until that happens, why
> wouldn't we simply treat the file as having the lower file label for
> access control purposes on read operations?
I'm hoping Eric Paris might address this. As I understand it, we don't want
to give the container access to the lower layer labels, but would rather
consistently use the overlay labels.
David
WARNING: multiple messages have this Message-ID (diff)
From: David Howells <dhowells@redhat.com>
To: Stephen Smalley <sds@tycho.nsa.gov>, eparis@redhat.com
Cc: linux-unionfs@vger.kernel.org, linux-kernel@vger.kernel.org,
dhowells@redhat.com, linux-fsdevel@vger.kernel.org,
linux-security-module@vger.kernel.org, selinux@tycho.nsa.gov,
drquigl <drquigl@tycho.nsa.gov>
Subject: Re: [PATCH 5/7] SELinux: Handle opening of a unioned file
Date: Thu, 18 Jun 2015 11:15:39 +0100 [thread overview]
Message-ID: <8182.1434622539@warthog.procyon.org.uk> (raw)
In-Reply-To: <558187EA.60805@tycho.nsa.gov>
Stephen Smalley <sds@tycho.nsa.gov> wrote:
> >> Until a process writes to the file, we just want to use the lower inode
> >> label, right?
> >
> > No.
> >
> > There are two issues:
> >
> > (1) Non-fd accesses to an overlayfs file use the security label on the
> > overlay inode, not the lower inode, even before copy up because they
> > go through the inode ops of the overlayfs file first.
> >
> > (2) I'm told that we want the ability to have a different label on the
> > upper file to that on the lower file. This is trivial in overlayfs
> > since you always have an overlay inode off which to hang the security
> > label, but tricky with unionmount since you may only have a dentry.
>
> I recall that being controversial. I agree that if a process attempts
> to write to the file and a copy-up is triggered, then we want to be able
> to label the copy of the file differently. But until that happens, why
> wouldn't we simply treat the file as having the lower file label for
> access control purposes on read operations?
Actually, for overlayfs, I've made it such that the label off of the overlay
inode is used for the open file. This label gets determined when the overlay
inode is set up and so is cached for the lifetime of the inode struct. Like
this in the patch I posted:
+ if (inode) {
+ isec = inode->i_security;
+ fsec->union_isid = isec->sid;
+ } ...
where 'inode' is the overlay inode. The subsequent bits:
+ } else if ((sbsec->flags & SE_SBINITIALIZED) &&
+ (sbsec->behavior == SECURITY_FS_USE_MNTPOINT)) {
+ fsec->union_isid = sbsec->mntpoint_sid;
+ } else {
+ dir = dget_parent(union_path->dentry);
+ dsec = dir->d_inode->i_security;
+
+ rc = security_transition_sid(
+ tsec->sid, dsec->sid,
+ inode_mode_to_security_class(file_inode(file)->i_mode),
+ &union_path->dentry->d_name,
+ &fsec->union_isid);
+ dput(dir);
+ if (rc) {
+ printk(KERN_WARNING "%s: "
+ "security_transition_sid failed, rc=%d (name=%pD)\n",
+ __func__, -rc, file);
+ return rc;
+ }
+ }
are for future unionmount support where there isn't an inode in the union
layer but only an inode in a lower layer.
In patch 7/7, if fsec->union_isid is non-zero in file_has_perm() then that is
used directly with avc_has_perm() in preference to calling inode_has_perm() on
the overlay or lower inodes.
Btw, is it correct that file operations must follow the label currently on the
inode rather than caching it in file_security_struct?
> > (2) I'm told that we want the ability to have a different label on the
> > upper file to that on the lower file. This is trivial in overlayfs
> > since you always have an overlay inode off which to hang the security
> > label, but tricky with unionmount since you may only have a dentry.
>
> I recall that being controversial. I agree that if a process attempts
> to write to the file and a copy-up is triggered, then we want to be able
> to label the copy of the file differently. But until that happens, why
> wouldn't we simply treat the file as having the lower file label for
> access control purposes on read operations?
I'm hoping Eric Paris might address this. As I understand it, we don't want
to give the container access to the lower layer labels, but would rather
consistently use the overlay labels.
David
next prev parent reply other threads:[~2015-06-18 10:15 UTC|newest]
Thread overview: 89+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-11-05 15:42 [PATCH 0/7] Security: Provide unioned file support David Howells
2014-11-05 15:42 ` [PATCH 1/7] Security: Provide copy-up security hooks for unioned files David Howells
2014-11-06 17:46 ` Casey Schaufler
2014-11-07 14:49 ` David Howells
2014-11-07 14:49 ` David Howells
2014-11-07 21:22 ` Paul Moore
2014-11-07 21:22 ` Paul Moore
2014-11-07 22:10 ` David Howells
2014-11-07 22:10 ` David Howells
2014-11-10 15:28 ` Paul Moore
2014-11-10 15:28 ` Paul Moore
2014-11-05 15:42 ` [PATCH 2/7] Overlayfs: Use copy-up security hooks David Howells
2014-11-07 21:39 ` Paul Moore
2014-11-07 21:39 ` Paul Moore
2014-11-07 22:05 ` David Howells
2014-11-07 22:05 ` David Howells
2014-11-10 15:45 ` Paul Moore
2014-11-10 15:45 ` Paul Moore
2014-11-05 15:42 ` [PATCH 3/7] SELinux: Stub in copy-up handling David Howells
2014-11-07 21:44 ` Paul Moore
2014-11-07 21:44 ` Paul Moore
2014-11-07 22:08 ` David Howells
2014-11-07 22:08 ` David Howells
2014-11-10 15:47 ` Paul Moore
2014-11-10 15:47 ` Paul Moore
2014-11-05 15:42 ` [PATCH 4/7] Security: Pass the union-layer file path into security_file_open() David Howells
2014-11-05 15:43 ` [PATCH 5/7] SELinux: Handle opening of a unioned file David Howells
2014-11-05 16:35 ` Stephen Smalley
2014-11-06 12:03 ` David Howells
2014-11-06 12:03 ` David Howells
2014-11-06 13:13 ` Stephen Smalley
2014-11-06 13:13 ` Stephen Smalley
2014-11-06 13:34 ` David Howells
2014-11-06 13:34 ` David Howells
2014-11-27 14:15 ` David Howells
2014-11-27 14:15 ` David Howells
2014-11-06 12:27 ` David Howells
2014-11-06 12:27 ` David Howells
2014-11-06 12:27 ` David Howells
2014-11-27 17:25 ` David Howells
2014-11-27 17:25 ` David Howells
2015-06-12 15:30 ` David Howells
2015-06-12 15:30 ` David Howells
2015-06-15 12:57 ` Stephen Smalley
2015-06-15 12:57 ` Stephen Smalley
2015-06-16 9:41 ` David Howells
2015-06-16 9:41 ` David Howells
2015-06-16 16:49 ` David Howells
2015-06-16 16:49 ` David Howells
2015-06-16 17:20 ` Stephen Smalley
2015-06-16 17:20 ` Stephen Smalley
2015-06-16 21:34 ` David Howells
2015-06-16 21:34 ` David Howells
2015-06-17 14:44 ` Stephen Smalley
2015-06-17 14:44 ` Stephen Smalley
2015-06-18 10:15 ` David Howells [this message]
2015-06-18 10:15 ` David Howells
2015-06-18 12:48 ` Stephen Smalley
2015-06-18 12:48 ` Stephen Smalley
2015-06-18 15:26 ` David Howells
2015-06-18 15:26 ` David Howells
2015-06-18 10:32 ` David Howells
2015-06-18 10:32 ` David Howells
2015-06-18 12:16 ` Stephen Smalley
2015-06-18 12:16 ` Stephen Smalley
2014-11-05 15:43 ` [PATCH 6/7] SELinux: The copy-up operation must have read permission on the lower file David Howells
2014-11-05 16:43 ` Stephen Smalley
2014-11-05 17:54 ` Stephen Smalley
2014-11-06 13:39 ` Stephen Smalley
2014-11-27 14:17 ` David Howells
2014-11-27 14:17 ` David Howells
2014-11-27 14:21 ` David Howells
2014-11-27 14:21 ` David Howells
2014-11-27 14:21 ` David Howells
2014-11-05 15:43 ` [PATCH 7/7] SELinux: Check against union and lower labels for file ops on lower files David Howells
2014-11-06 17:35 ` [PATCH 0/7] Security: Provide unioned file support Casey Schaufler
2014-11-06 17:35 ` Casey Schaufler
2014-11-06 17:58 ` David Howells
2014-11-06 17:58 ` David Howells
2014-11-06 18:40 ` Casey Schaufler
2014-11-06 18:40 ` Casey Schaufler
2014-11-07 15:21 ` David Howells
2014-11-07 15:21 ` David Howells
2014-11-07 18:54 ` Daniel J Walsh
2014-11-07 18:54 ` Daniel J Walsh
2014-11-09 1:31 ` Casey Schaufler
2014-11-09 1:31 ` Casey Schaufler
2014-11-10 13:59 ` Daniel J Walsh
2014-11-10 13:59 ` Daniel J Walsh
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=8182.1434622539@warthog.procyon.org.uk \
--to=dhowells@redhat.com \
--cc=drquigl@tycho.nsa.gov \
--cc=eparis@redhat.com \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-security-module@vger.kernel.org \
--cc=linux-unionfs@vger.kernel.org \
--cc=sds@tycho.nsa.gov \
--cc=selinux@tycho.nsa.gov \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.