Note: This is a beta release of Red Hat Bugzilla 5.0. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Also email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback here.
Bug 223961 - Permissions changes crash with selinux disabled
Summary: Permissions changes crash with selinux disabled
Keywords:
Status: CLOSED CURRENTRELEASE
Alias: None
Product: Red Hat Enterprise Linux 5
Classification: Red Hat
Component: nautilus
Version: 5.0
Hardware: All
OS: Linux
urgent
urgent
Target Milestone: ---
: ---
Assignee: Alexander Larsson
QA Contact: David Lawrence
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2007-01-23 11:11 UTC by Alexander Larsson
Modified: 2007-11-30 22:07 UTC (History)
3 users (show)

Fixed In Version: 5.0.0
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2007-02-05 19:11:13 UTC


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

Description Alexander Larsson 2007-01-23 11:11:55 UTC
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:
http://bugzilla.gnome.org/show_bug.cgi?id=362302

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

Propose this for RHEL5

Comment 1 Alexander Larsson 2007-01-23 11:11:55 UTC
Created attachment 146283 [details]
New fixed version of selinux patch

Comment 2 RHEL Product and Program Management 2007-01-23 11:20:35 UTC
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
inclusion.

Comment 4 Alexander Larsson 2007-01-24 15:49:11 UTC
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 16:20:00 UTC
QE ack for RHEL5.  It's a regression and a very visible error.

Comment 7 Alexander Larsson 2007-01-24 16:35:56 UTC
Building fix as nautilus-2.16.2-6.el5


Comment 8 James Antill 2007-01-24 16:50:56 UTC
 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 10:31:42 UTC
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 21:45:20 UTC
No crashes here either.

Comment 13 Jay Turner 2007-02-05 19:11:13 UTC
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.