On Thu, 2017-05-18 at 12:04 -0400, Paul Moore wrote:
> On Fri, May 12, 2017 at 12:41 PM, Stephen Smalley <sds@xxxxxxxxxxxxx>
> wrote:
> > open permission is currently only defined for files in the kernel
> > (COMMON_FILE_PERMS rather than COMMON_FILE_SOCK_PERMS).
> > Construction of
> > an artificial test case that tries to open a socket via
> > /proc/pid/fd will
> > generate a recvfrom avc denial because recvfrom and open happen to
> > map to
> > the same permission bit in socket vs file classes.
> >
> > open of a socket via /proc/pid/fd is not supported by the kernel
> > regardless
> > and will ultimately return ENXIO. But we hit the permission check
> > first and
> > can thus produce these odd/misleading denials. Omit the open check
> > when
> > operating on a socket.
> >
> > Signed-off-by: Stephen Smalley <sds@xxxxxxxxxxxxx>
> > ---
> > security/selinux/hooks.c | 10 +++++++---
> > 1 file changed, 7 insertions(+), 3 deletions(-)
> >
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index e67a526..dd356ed 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -2063,8 +2063,9 @@ static inline u32 file_to_av(struct file
> > *file)
> > static inline u32 open_file_to_av(struct file *file)
> > {
> > u32 av = file_to_av(file);
> > + struct inode *inode = file_inode(file);
> >
> > - if (selinux_policycap_openperm)
> > + if (selinux_policycap_openperm && inode->i_sb->s_magic !=
> > SOCKFS_MAGIC)
> > av |= FILE__OPEN;
>
> No real objection to this patch, I'm just wondering if we are better
> served by only adding the open permission on a specific whitelist of
> object classes? From a practical point of view it probably has
> little
> impact either way and one check to rule out sockets is arguably
> faster
> than checking a handful of object classes, but I wonder how likely it
> would be that we would ever see someone trying to open other non-file
> objects and hitting this code?
We only ever assign either file or socket classes to inode security
blobs. There are 7 distinct file classes, so we'd have to compare
against 7 values (unless we use a range check, which would be two
comparisons, and if we want 100% accuracy, would require tweaking the
classmap to ensure they are all contiguous since "fd" is presently
intermingled). Also, we initialize isec->sclass to SECCLASS_FILE in
inode_alloc_security() for all inodes (for some inodes, we never get
another chance to initialize it); sockets are later switched to their
appropriate class in selinux_socket_post_create(). Testing for
SOCKFS_MAGIC seems simpler, more efficient, and less prone to error.
Up to you, but I'm not sure we gain anything by switching to a class-
based test.
> Also, as an aside, it seems rather odd that we don't have a macro
> somewhere to check if an inode is a socket. Oh well.
Smack uses this test as well in various place to see if an inode
represents a socket. I suspect the vfs folks would view it as a
layering violation, but we do need to tell at times...
>
> > return av;
> > @@ -3059,6 +3060,7 @@ static int selinux_inode_permission(struct
> > inode *inode, int mask)
> > static int selinux_inode_setattr(struct dentry *dentry, struct
> > iattr *iattr)
> > {
> > const struct cred *cred = current_cred();
> > + struct inode *inode = d_backing_inode(dentry);
> > unsigned int ia_valid = iattr->ia_valid;
> > __u32 av = FILE__WRITE;
> >
> > @@ -3074,8 +3076,10 @@ static int selinux_inode_setattr(struct
> > dentry *dentry, struct iattr *iattr)
> > ATTR_ATIME_SET | ATTR_MTIME_SET |
> > ATTR_TIMES_SET))
> > return dentry_has_perm(cred, dentry,
> > FILE__SETATTR);
> >
> > - if (selinux_policycap_openperm && (ia_valid & ATTR_SIZE)
> > - && !(ia_valid & ATTR_FILE))
> > + if (selinux_policycap_openperm &&
> > + inode->i_sb->s_magic != SOCKFS_MAGIC &&
> > + (ia_valid & ATTR_SIZE) &&
> > + !(ia_valid & ATTR_FILE))
> > av |= FILE__OPEN;
> >
> > return dentry_has_perm(cred, dentry, av);
> > --
> > 2.9.3
> >
>
>
