Bug 223961 - Permissions changes crash with selinux disabled
Permissions changes crash with selinux disabled
Product: Red Hat Enterprise Linux 5
Classification: Red Hat
Component: nautilus (Show other bugs)
All Linux
urgent Severity urgent
: ---
: ---
Assigned To: Alexander Larsson
David Lawrence
: Desktop, Regression
Depends On:
  Show dependency treegraph
Reported: 2007-01-23 06:11 EST by Alexander Larsson
Modified: 2007-11-30 17:07 EST (History)
3 users (show)

See Also:
Fixed In Version: 5.0.0
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Last Closed: 2007-02-05 14:11:13 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---

Attachments (Terms of Use)
New fixed version of selinux patch (47.52 KB, patch)
2007-01-23 06:11 EST, Alexander Larsson
no flags Details | Diff

  None (edit)
Description Alexander Larsson 2007-01-23 06:11:55 EST
There is a bug in the selinux patch we ship in RHEL5. When changing permissions
or selinux context, if we were unable to read the selinux context before, then
we crash. This happens e.g. if you disable selinux and create a file without a
selinux context, but its also been seen on filesystems without selinux support
(like ntfs).

This is the upstream bug:

The fix is simple, just avoid strcmp() with NULL in this case.

Propose this for RHEL5
Comment 1 Alexander Larsson 2007-01-23 06:11:55 EST
Created attachment 146283 [details]
New fixed version of selinux patch
Comment 2 RHEL Product and Program Management 2007-01-23 06:20:35 EST
This request was evaluated by Red Hat Product Management for inclusion in a Red
Hat Enterprise Linux major release.  Product Management has requested further
review of this request by Red Hat Engineering, for potential inclusion in a Red
Hat Enterprise Linux Major release.  This request is not yet committed for
Comment 4 Alexander Larsson 2007-01-24 10:49:11 EST
To reproduce easily, run with selinux disabled, create a new file (this should
make it not have a selinux context), then bring up the properties dialog for the
file and try to change the selinux context.
Comment 5 Jay Turner 2007-01-24 11:20:00 EST
QE ack for RHEL5.  It's a regression and a very visible error.
Comment 7 Alexander Larsson 2007-01-24 11:35:56 EST
Building fix as nautilus-2.16.2-6.el5
Comment 8 James Antill 2007-01-24 11:50:56 EST
 Alex, how much of that patch is new vs. changed.

 I assume the NULL check was added in:

+	if (file->details->info->selinux_context != NULL &&
+	    strcmp(selinux_context, file->details->info->selinux_context) == 0) {

...but I'm trying to remember if the matchpathcon() stuff was there before (I
think not, and it's just similar to another usage of matchpathcon() for "reset
to default").

 I'm also not sure the patch is "right", it'll fix the crash and it'll "work" on
ext3 ... but I'm not sure what happens on NFS/VFAT etc. when you can't change
the context (as I think it'll want to change it a lot now).
 I'm not 100% on when nautilus will try to set that new context: only when
something else changes, only when the context is changed to something else, or
all the time? -- I think it's the later.

 Also AIUI the result of matchpathcon() is being used to display a context to
the user, when no context is available. This isn't right, as the kernel will not
be treating that file as if it has that context (it'll treating it as a
*:file_t). This will only confuse the user.
 This almost seems like the kernel should be fixed to return the "default" type
it's using for the file, instead of nothing. Of course getting that change
through for RHEL-5 might be a bigger PITA than this change.
Comment 9 Alexander Larsson 2007-01-25 05:31:42 EST
The file->details->info->selinux_context != NULL part is the only thing i added.

I tried to run it with selinux turned off on ext3 and didn't see any problems.
Also, setting the selinux context via gnome-vfs never fails if selinux is
disabled, as it checks set_selinux_context() and if that fails always returns OK.

So, i think the change is "safe".

Whether it does the right thing wrt selinux and matchpathcon() etc. I don't
know, i don't really know much about selinux.

Comment 12 Zack Cerza 2007-01-26 16:45:20 EST
No crashes here either.
Comment 13 Jay Turner 2007-02-05 14:11:13 EST
Closing out with results from 20070126.0 trees.

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