Bug 204030 - SELinux context cannot be changed from nautilus GUI
SELinux context cannot be changed from nautilus GUI
Product: Fedora
Classification: Fedora
Component: nautilus (Show other bugs)
All Linux
medium Severity medium
: ---
: ---
Assigned To: Alexander Larsson
Depends On:
Blocks: xattrs-tracker
  Show dependency treegraph
Reported: 2006-08-25 02:40 EDT by James Antill
Modified: 2007-11-30 17:11 EST (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Last Closed: 2006-09-19 11:50:14 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:

Attachments (Terms of Use)
Initial patch for nautilus SELinux editing (20.66 KB, patch)
2006-08-25 02:43 EDT, James Antill
no flags Details | Diff
Simple and advanced type setting (27.77 KB, patch)
2006-08-26 00:06 EDT, James Antill
no flags Details | Diff
SELinux nautilus patch, including matchpathcon etc. (34.92 KB, patch)
2006-08-26 17:33 EDT, James Antill
no flags Details | Diff
Style and minor bug fixes, still not recursive (37.57 KB, patch)
2006-08-28 19:45 EDT, James Antill
no flags Details | Diff
Does recursive, multiple nodes (with the same context) (49.10 KB, patch)
2006-09-18 17:34 EDT, James Antill
no flags Details | Diff

  None (edit)
Description James Antill 2006-08-25 02:40:44 EDT
Description of problem:
 SELinux context is only displayed, you can't edit it.


Additional info:

 I have a patch for review that adds editing capabilities to what is currently
the SELinux context label. The URLs to get the src rpm/patch are:


 It "works for me"(tm), now the bad news:

. Not sure I'm allowed to steal the c mnemonic, we really need a mnemonic though.

. MLS/MCS isn't tested by me yet, and the UI for that sucks the big one (it
really needs check boxes for MCS and two seperated combo boxes for MLS). You
could argue that about the entire UI, but this was the best I could come up with
and it's usable (IMO).

. Set context fail just resets the entry to the previous value, this sucks.

. Completion changes focus if the pointer is where the completion list appears,
this then emits a leave-focus event ... which is then tries to apply the "new
context", which fails and resets the entry box.

. We don't add the matchpathcon() type to the completion list (values in

. Added by hand user_home_t and tmp_t to the completion list ... this almost
certainly means there's more (adding the matchpathcon() type might make this go

. Completion doesn't help you if you want to change the entire label, but we can
probably mark that as unsupported for the short term. (and there are workarounds).

. some of the code could/should be cleaned up, but you could say that about a
lot of the preferences dialog C file :O.

. Probably other stuff I've forgotten about.
Comment 1 James Antill 2006-08-25 02:43:53 EDT
Created attachment 134890 [details]
Initial patch for nautilus SELinux editing

 Putting the first beta version here.
Comment 2 Alexander Larsson 2006-08-25 04:27:50 EDT
Upstream builds with -Werror from cvs, and this has warnings:
nautilus-file.c: In function 'nautilus_file_set_selinux_context':
nautilus-file.c:3647: warning: passing argument 1 of
'selinux_trans_to_raw_context' discards qualifiers from pointer target type
fm-properties-window.c: In function 'attach_selinux_comp_edit_field':
fm-properties-window.c:1728: warning: assignment discards qualifiers from
pointer target type

Furthermore, this is just an entry where you can modify a strange string that
you need a phd in selinux to understand. There is no way to know what you're
supposed/allowed to write there, nor is there a simple way to get back to an old
safe version if you accidentally changed it.

In this release we make the normal unix settings easier to understand (unless
you activate the advanced mode), and people seem to like that much better than
the old version that exposed the full unix lowlevel settings. However, we're
already getting requests to be able to remove the selinux label, because its
just scaring people. Just making it an entry isn't an option. 

Isn't there some way you can make this easier to use? For instance, have a
dropdown with nice translated natural language strings describing various common
contextes. Possibly with a "custom" element that pops up a dialog where you can
write whatever selinux context you want.
Comment 3 James Antill 2006-08-25 10:39:45 EDT
 Having a drop down available, with a small list of options (when in
not-advanced mode) ... isn't that hard to do, now. It'd only alter the type, but
that'll be all 99% of people want anyway (MCS checkboxes might be useful later,
as I said).
 It'll be highly specific and statically define a bunch of targeted policy
information, but it can say things like "Allowed to run as CGI, without file IO"
and "Allowed to run as CGI, and can read files". But I wanted to get the "can do
anything" UI working first.

 I'm surprised about the warnings, as I fixed a few warnings in
fm-properties-window.c, I've fixed those two.
Comment 4 James Antill 2006-08-26 00:04:00 EDT
 Ok, this contains the "simple" version. As I said above, the "explanations" are
all hand coded (there's no where external to get them from atm.) ... and they
could do with Dan/etc. checking them.
 It might be useful to have these as tooltips or something for completion to,
but I'm not sure how/if I can do that.

 However it does a weird thing when you select the popup (it has a large blank
space), which I assume is something obvious that I'm not doing ... but I can't
see it.

 Still doesn't include a matchpathcon() like type, and MCS (both of which we
probably need for GA).
Comment 5 James Antill 2006-08-26 00:06:02 EDT
Created attachment 134967 [details]
Simple and advanced type setting

 Uploading the new patch, also have src and binary rpms at my people page.
Comment 6 James Antill 2006-08-26 17:33:53 EDT
Created attachment 134987 [details]
SELinux nautilus patch, including matchpathcon etc.

 This is ready to go into rawhide/FC6-betaX now, IMO. You can change everything
in advanced mode, and can change to any of the customizable types in simple
mode (and there are convertions for a significant number of types into
english). Both have sorted lists, with pixmaps for current and default types

 Packages from:

Comment 7 Alexander Larsson 2006-08-28 08:40:07 EDT
First of all, there are lots of indentation and code style-guide issue.
Upstream (== me) is pretty picky about this, and I want to be able to just commit
this patch upstream later.

The nautilus styleguide is in docs/style-guide.html.
Examples of things that need to be fixed:

+        security_context_t rcontext = NULL;

No initializations at declaration.

+static void maybe_gtk_entry_set_text(GtkEntry *entry, const char *val)

No space before "(".
Type and function name on separate rows

+static void attach_selinux_data_edit_field(GtkEntry *entry,
+					   char *attr_value,
+					   char *def_attr_value);

Declarations on top of file. (and no space before "(")

+	//	fm_report_error_setting_selinux (file, result, NULL);

No c++ comments

+	if (!gtk_combo_box_get_active_iter (comb, &iter))
+	        return;
+	else

Always use brackets

+	else
+	{

Brackets on same line as if/else

+	return (NULL);

No paranthesis around return value.

+# include <selinux/selinux.h>

Includes at top of file

Real comments:

There is no support for selinux context setting in the recursive permissions

I'm not sure about the icons.
First of all, the icon should be to the left of the text. That way its not
indented to fit all text lengths.
Secondly, i'm not sure what the OK icon is supposed to mean. When experimenting it
seems to only show what value you last changed it to. And it overrides the home icon
for the users home dir.
Furthermore i don't think OK is a good icon to use here. GTK_STOCK_APPLY might be
better, but as i said, i'm not sure what its supposed to mean.

+	HACK_TYPE("cupsd_etc_t", "CUPS printer configuration.");

These strings are not translated, they need _("") markup around the actual string.
gettext works on the source, and doesn't expand macros.
Also, the translations won't be in FC6, because this code is not upstream, so the
gnome tranlators won't see it.
Can't we put all these strings things in a file somewhere (with translations) that
nautilus reads? That way selinux can ship with and update these strings.
The strings shouldn't end with a period. That looks silly in the UI.

+	HACK_TYPE("samba_share_t", "Shared via. CIFS (samba).");

This has a stray dot in it.

+	if (!nautilus_file_is_local (file))
+	        return (NULL);
+	uri = nautilus_file_get_uri (file);
+	fname = gnome_vfs_get_local_path_from_uri (uri);

nautilus_file_is_local doesn't do what you think it does. It will return FALSE
for a mounted NFS filesystem. Just look at the value returned from
gnome_vfs_get_local_path_from_uri and check for NULL.

-	GtkWidget *icon_chooser;
+        gpointer   icon_chooser; /* passing & to g_object_add_weak_pointer */

I don't think its actually nicer to save it as a gpointer even if we avoid a cast.
Its better to have the real type info in the data structures so you know what
type of object it is when reading the code.

+static void selinux_split_cntx (char *cntx,
+				const char **ret_attr_u,
+				const char **ret_attr_r,
+				const char **ret_attr_t,
+				const char **ret_attr_s)

This modifies cntx. Would be nice with a comment on that.

+	fname_ctypes = selinux_customizable_types_path ();
+	/* read types, one per line... */
+	if ((ioc_ctypes = g_io_channel_new_file (fname_ctypes, "r", &errc)))
+	{
+	        char *data = NULL;
+		while ((data = cust_type_next_line (ioc_ctypes)))
+		        cust_types = g_slist_prepend (cust_types, data);
+		g_io_channel_unref (ioc_ctypes);
+	}

Seem easier to use g_file_get_contents & g_strsplit than this.

+	/* FIXME: this should be cached, but threading makes it hurt */
+	for (scan = cust_types; scan; scan = scan->next)
+		g_free (scan->data);

Threading? Nautilus doesn't use threads.
Comment 8 James Antill 2006-08-28 19:45:17 EDT
Created attachment 135096 [details]
 Style and minor bug fixes, still not recursive

 This contains almost all of the above, note that icon_chooser now produces a
warning about aliasing again (which is what you want). I also haven't converted
to using g_file_get_contents(), although there is caching of the customizable
types now.

 It still doesn't change the label recursively, it'll be another day or two to
get that working I think. One problem is that it's not obvious how to tweak the
UI so that you can apply permission changes recursively without affecting the
security context.

 Also, with regard to the icons. The Home icon represents the default security
context for that file (Ie. what install would label it as) ... the _OK icon
shows what label it had when the preferences dialog was opened.
 I tried to choose icons that made some sense, but...
Comment 9 Alexander Larsson 2006-08-29 04:25:57 EDT
Bringing up a properties dialog with multiple files with different selinux
context selected looks weird. I get the home icon on a line that says "--" in
the dropdown, and the context displayed doesn't seem right. The other
permissions widgets have a third "inconsistent" state for this meaning "use the
current value for each file". The other combos use "---" for this. We also track
the original setting for each file so that changing the value back to "---"
restores the old values.

The inconsistent state is also what is used for the recursive permission
handling. If a folder is selected (so recursive permissions changing is
availible) you can manually set a combo in an inconsistent state to not change
something when applying recursively. Something similar could be done with the
selinux stuff.

+	gtk_cell_layout_pack_start (GTK_CELL_LAYOUT (comb), cell, TRUE);
+	gtk_cell_layout_set_attributes (GTK_CELL_LAYOUT (comb), cell,
+					"stock-id", 2, NULL);
+	cell = gtk_cell_renderer_text_new ();
+	gtk_cell_layout_pack_start (GTK_CELL_LAYOUT (comb), cell, TRUE);

Pass expand = FALSE in the pack_start calls to match the rest of the dialog.

Some further style issues:

+	char *translated = NULL;
+	char *raw = NULL;
+	char *uri = NULL;
+	char *fname = NULL;

+        security_context_t rcontext = NULL;

+	set_permissions_callback(handle, result, new_info, callback_data);

+static void maybe_gtk_entry_set_text(GtkEntry *entry, const char *val)

+        char *old_val = NULL;

+static void selinux_done_editing(FMPropertiesWindow *window,
+				 char *selinux_context)

+	}
+	else {

fm-properties-window.c: In function 'attach_selinux_data_edit_field':
fm-properties-window.c:1839: warning: unused variable 'fname_ctypes'

Comment 10 James Antill 2006-09-18 17:34:33 EDT
Created attachment 136586 [details]
Does recursive, multiple nodes (with the same context)

 Ok, I think this is basically it. It might be nice to be able to change from
inconsistent label's when you have multiple contexts and changing MLS/MCS/etc.
when in simple mode isn't possible, but those can be other features for other
 This should do everything, and follow the nautilus rules. Can you take a look
and add it to FC6, if there are no major problems?
Comment 11 Alexander Larsson 2006-09-19 10:57:34 EDT
The recursive change is a bit weird. Whenever you change the permissions
recursively on a directory you will make all the files in the directory have the
same context as the directory, as there is no way to say "don't change context"
by setting it to inconsistent.

Also, i fixed a bug in:
+	if (def_attr_val) {
+	          selinux_split_cntx (def_attr_val, &dattr_u, &dattr_r,
+				      &dattr_t, &dattr_s);
+	}

Where dattr_t etc can be read uninitialized.
Comment 12 Alexander Larsson 2006-09-19 11:50:14 EDT
This is in nautilus-2.16.0-4.fc6.
Comment 13 Xiaohong Wang 2006-10-17 02:15:45 EDT
Confirmed it has been fixed in nautilus-2.16.0-5.el5

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