Bug 307431 - GFS2: security_eo_get() uses permission() when it should use security_inode_getxattr()
GFS2: security_eo_get() uses permission() when it should use security_inode_g...
Product: Red Hat Enterprise Linux 5
Classification: Red Hat
Component: kernel (Show other bugs)
All Linux
high Severity high
: rc
: ---
Assigned To: Don Zickus
GFS Bugs
Depends On: 242066
Blocks: 323111
  Show dependency treegraph
Reported: 2007-09-26 13:23 EDT by Eric Paris
Modified: 2008-05-21 10:56 EDT (History)
7 users (show)

See Also:
Fixed In Version: RHBA-2008-0314
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Last Closed: 2008-05-21 10:56:10 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---

Attachments (Terms of Use)
Remove permission checks from xattr ops (1.95 KB, patch)
2007-11-28 16:02 EST, Ryan O'Hara
no flags Details | Diff

  None (edit)
Description Eric Paris 2007-09-26 13:23:13 EDT
When a program tries to get (or set) the xattr on a file in a GFS2 filesystem
security_eo_get() is actually calling into the LSM through the function
permission() with either MAY_READ or MAY_WRITE.  But this is actually asking for
permission to read or write the data in the file, not just the extended attributes.

The proper LSM hook when something is just trying to get/set an xattr is

Maybe GFS needs to do some other permissions checking other than just
security_inode_getxattr I don't know how GFS works, but from the point of view
of the LSM requiring MAY_READ/MAY_WRITE is wrong.

I suspect other of the eo_ops have this wrong as well.  This makes relabeling
the SELinux context of files on GFS filesystems impossible with present tools.
Comment 1 Robert Peterson 2007-09-26 14:16:54 EDT
Adding GFS2 to summary so it shows up in Steve Whitehouse's lists.
Comment 2 Steve Whitehouse 2007-09-27 08:06:00 EDT
See also #242066 (there isn't a see also field, so I have to add a dep one way
or the other).
Comment 3 Steve Whitehouse 2007-10-04 09:46:26 EDT
It looks like we need xattr_permission, but that isn't exported at the moment.
Comment 5 Kiersten (Kerri) Anderson 2007-10-10 09:55:08 EDT
Reassigning to Ryan since this was his contribution in the first place.
Comment 6 Ryan O'Hara 2007-11-09 15:32:58 EST
Looked through the code and Steve (comment #) is correct. It appears
xattr_permission is what we want. Not sure why it is not exported. Looking into it.
Comment 7 Ryan O'Hara 2007-11-09 16:04:35 EST
Actually .. I retract my previous comment. I don't think xattr_permission is
what we want. Appears that xattr_permission places no restrictions on selinux
xattrs. security_inode_getxattr and security_inode_setxattr seem to be needed,
as Eric stated.

That said, we'll probably get proper handling/permission checks for free if we
use the generic xattr interfaces. That may be the best solution for GFS2, but
I'm not sure how we'll go about solving this for GFS1.
Comment 8 Steve Whitehouse 2007-11-11 10:41:53 EST
There are two issues here. In the code where permission() is currently being
(incorrectly) used it is the generic code which redoes the permission checks
after we got our glocks. Thats why xattr_permission isn't exported - no other
filesystem requires this and we'll need to argue that point before we'll be
allowed to export that function I'm sure.

As you say, xattr_permission doesn't do any checking if the xattr type is
"security". Thats correct, as the security handlers themselves should be doing
that, in this case with security_inode_get/setxattr as per the title of the bug.

Although we really need to redo the xattr code to use more of the VFS provided
library functions (and also to allow us to merge transactions during inode
creation and deletion) it should be possible to make these changes (for 5.2)
without changing a lot of code.

Having has a quick look at the VFS functions, I'd expect that when we do get
around to the rewrite, it will save us about 1000 lines of code compared with
the current implementation, plus (and more importantly) save us from extra
transactions when creating and deleting inodes. Thats all under bz #242066 though.
Comment 9 Ryan O'Hara 2007-11-15 14:21:01 EST
Would like to know how to recreate/test this problem so that I can verify the
fix. Any information would be helpful.
Comment 10 Daniel Walsh 2007-11-15 17:22:46 EST
Setup an SELinux box in enforcing mode.  Which say apache using gfs2 file system
to display its pages.

Directories need to be labeled 

chcon -R -t httpd_sys_content_t /PATHTOGFS2

Try to look at the web site.
Comment 11 Eric Paris 2007-11-15 17:40:53 EST
is that right dan?  is there a transition to chcon_t or something from
unconfined_t?  I'm reasonably certain there is a transition from unconfined_t to
restorecon_t.  Remember the problem is not with actual usage of GFS, but with
getting/setting the xattrs.

maybe a better test would be

mkdir -o /opt/httpd/test1dir/test2dir
semanage fcontext -a -t httpd_sys_content_t "/opt/httpd(/.*)?"
restorecon -R -v /opt/

if you get any selinux denials its not right.  If you can then ls -lZ /opt/httpd
and see the new httpd_sys_content_t on those subdirs you should be good.
Comment 12 Daniel Walsh 2007-11-16 11:02:15 EST
Yes that is good, but I also want to make sure confined domains can read the
privs.  restorecon is a fairly privledged application.
Comment 13 Ryan O'Hara 2007-11-20 15:22:59 EST
Verified this bug with procedure described above. GFS2 was unable to relabel
files/directories due to the permission() calls occuring in the
gfs2_security_eaops (permission was returning -EACCESS). Ripped our permission
check and restorecon worked properly. Appears that this is the fix. No need to
cal security_inode_setxattr, etc. since this in done in the vfs layer (ie.

What is the correct thing to do for the other xattr types? As far as I can tell,
we don't need to call permission() in any of the xattr operations.

Comments, please.
Comment 14 Eric Paris 2007-11-20 15:34:14 EST
I think you are right in that you don't need your own permission hooks inside
the FS.

If you gett a running machine with the perms ripped out can I get access?   I'm
not sure a good way to test to make sure the security hooks are getting hit
correctly, but i'm sure with enough playing I can verify (at least the selinux
Comment 15 Ryan O'Hara 2007-11-20 15:48:46 EST
OK. I am going to rip out all the permission() calls in each of the xattr ops.
It looks like xattr_permission() is doing the correct checks anyway. Going to
send as a single patch.

Comment 16 Ryan O'Hara 2007-11-28 16:02:30 EST
Created attachment 271791 [details]
Remove permission checks from xattr ops

Patch against RHEL5 branch.
Comment 17 Ryan O'Hara 2007-11-28 16:03:46 EST
Patch went upstream (Steve's git tree) and to rhkernel-list. Attached patch
against RHEL5 kernel (above).
Comment 18 Ryan O'Hara 2007-11-28 16:10:23 EST
Movied status to POST.
Comment 19 Don Zickus 2007-12-17 14:37:01 EST
in 2.6.18-61.el5
You can download this test kernel from http://people.redhat.com/dzickus/el5
Comment 22 errata-xmlrpc 2008-05-21 10:56:10 EDT
An advisory has been issued which should help the problem
described in this bug report. This report is therefore being
closed with a resolution of ERRATA. For more information
on the solution and/or where to find the updated files,
please follow the link below. You may reopen this bug report
if the solution does not work for you.


Note You need to log in before you can comment on or make changes to this bug.