Re: [PATCH]: chcon: no longer abort on SELinux disabled kernel
On Tue, 2009-10-06 at 10:14 +0200, Jim Meyering wrote:
Jim Meyering wrote:
Stephen Smalley wrote:
...
Must have previously booted an ancient kernel with SELinux permissive
and no policy loaded. Kernel was fixed by the commit below in 2006.
I'd recommend that he run the following to clean up the droppings in his
filesystem:
find / \( -fstype ext2 -o -fstype ext3 -o -fstype ext4 \) -exec setfattr
-x security.selinux {} \;
commit 8aad38752e81d1d4de67e3d8e2524618ce7c9276
Author: Stephen Smalley s...@tycho.nsa.gov
Date: Wed Mar 22 00:09:13 2006 -0800
[PATCH] selinux: Disable automatic labeling of new inodes when no
policy is loaded
Thanks for the quick explanation!
I've revised the commit not to say anything in NEWS
and to expand the log message. While the exit-early
change doesn't solve the problem in all cases, it is useful
and does make chcon consistent with runcon in that respect.
FWIW, there is a subtle difference here:
- chcon can in fact work on a SELinux-disabled kernel, as you can still
set the security.* extended attributes as long as the filesystem
provides handlers for the security.* namespace.
- runcon cannot work without a SELinux-enabled kernel, as only a
SELinux-enabled kernel allows you to set the security context of a
running process.
So by preventing chcon from running in the SELinux-disabled case, you
are imposing a restriction above and beyond what is strictly required.
The user can of course still use setfattr -n security.selinux -v
context path to set a SELinux security context on a file when
SELinux is disabled, or can run the setfiles program to set SELinux
security contexts on an entire file tree even when SELinux is disabled.
From 3a97d664b9f639fddb5a245775f47d27bfbb56c9 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ond=C5=99ej=20Va=C5=A1=C3=ADk?= ova...@redhat.com
Date: Mon, 5 Oct 2009 09:20:48 +0200
Subject: [PATCH] chcon: exit immediately if SELinux is disabled
This change happens to avoid an abort in chcon when SELinux is
disabled while operating on a file with an unlabeled context from
back in 2006. However, that same abort can still be triggered by the
same file when running chcon with SELinux enabled. This bug in chcon
will be fixed in a subsequent commit via a getfilecon wrapper. See
http://thread.gmane.org/gmane.comp.gnu.coreutils.bugs/18378/focus=18384
for how to correct your disk attributes to avoid triggering this bug.
* src/chcon.c (main): Exit immediately if SELinux is disabled.
Reported in http://bugzilla.redhat.com/527142 by Yanko Kaneti.
* src/runcon.c (main): Do not hardcode program name in error message.
* THANKS: Update.
---
THANKS |1 +
src/chcon.c |4
src/runcon.c |2 +-
3 files changed, 6 insertions(+), 1 deletions(-)
diff --git a/THANKS b/THANKS
index e0e14e5..65ac1bb 100644
--- a/THANKS
+++ b/THANKS
@@ -612,6 +612,7 @@ Wis Macomsonwis.macom...@intel.com
Wojciech Purczynski cl...@isec.pl
Wolfram Kleff kl...@cs.uni-bonn.de
Won-kyu Parkwkp...@chem.skku.ac.kr
+Yanko Kanetiyan...@declera.com
Yann Dirson dir...@debian.org
Zvi Har'El r...@math.technion.ac.il
diff --git a/src/chcon.c b/src/chcon.c
index fbfdb4d..c0da694 100644
--- a/src/chcon.c
+++ b/src/chcon.c
@@ -519,6 +519,10 @@ main (int argc, char **argv)
usage (EXIT_FAILURE);
}
+ if (is_selinux_enabled () != 1)
+error (EXIT_FAILURE, 0,
+ _(%s may be used only on a SELinux kernel), program_name);
+
if (reference_file)
{
if (getfilecon (reference_file, ref_context) 0)
diff --git a/src/runcon.c b/src/runcon.c
index e0019da..f87eada 100644
--- a/src/runcon.c
+++ b/src/runcon.c
@@ -195,7 +195,7 @@ main (int argc, char **argv)
if (is_selinux_enabled () != 1)
error (EXIT_FAILURE, 0,
- _(runcon may be used only on a SELinux kernel));
+ _(%s may be used only on a SELinux kernel), program_name);
if (context)
{
--
1.6.5.rc2.204.g8ea19
--
Stephen Smalley
National Security Agency
Re: [PATCH]: chcon: no longer abort on SELinux disabled kernel
On Wed, 2009-10-07 at 14:48 +0200, Jim Meyering wrote: Stephen Smalley wrote: ... FWIW, there is a subtle difference here: - chcon can in fact work on a SELinux-disabled kernel, as you can still set the security.* extended attributes as long as the filesystem provides handlers for the security.* namespace. - runcon cannot work without a SELinux-enabled kernel, as only a SELinux-enabled kernel allows you to set the security context of a running process. So by preventing chcon from running in the SELinux-disabled case, you are imposing a restriction above and beyond what is strictly required. The user can of course still use setfattr -n security.selinux -v context path to set a SELinux security context on a file when SELinux is disabled, or can run the setfiles program to set SELinux security contexts on an entire file tree even when SELinux is disabled. ... diff --git a/src/chcon.c b/src/chcon.c index fbfdb4d..c0da694 100644 --- a/src/chcon.c +++ b/src/chcon.c @@ -519,6 +519,10 @@ main (int argc, char **argv) usage (EXIT_FAILURE); } + if (is_selinux_enabled () != 1) +error (EXIT_FAILURE, 0, + _(%s may be used only on a SELinux kernel), program_name); + Thanks for the tip. I'll revert that part of the patch. I'll address the original problem by adding getfilecon and lgetfilecon wrappers that map those unusual cases (10,unlabeled and 0,NULL) to a return value of -1 with errno == ENOTSUPP. I'd suggest ENODATA instead - that means that the filesystem supports attributes but there was no value set for the particular file. -- Stephen Smalley National Security Agency
Re: [PATCH]: chcon: no longer abort on SELinux disabled kernel
On Wed, 2009-10-07 at 15:34 +0200, Jim Meyering wrote: Stephen Smalley wrote: On Wed, 2009-10-07 at 14:48 +0200, Jim Meyering wrote: Stephen Smalley wrote: ... FWIW, there is a subtle difference here: - chcon can in fact work on a SELinux-disabled kernel, as you can still set the security.* extended attributes as long as the filesystem provides handlers for the security.* namespace. - runcon cannot work without a SELinux-enabled kernel, as only a SELinux-enabled kernel allows you to set the security context of a running process. So by preventing chcon from running in the SELinux-disabled case, you are imposing a restriction above and beyond what is strictly required. The user can of course still use setfattr -n security.selinux -v context path to set a SELinux security context on a file when SELinux is disabled, or can run the setfiles program to set SELinux security contexts on an entire file tree even when SELinux is disabled. ... diff --git a/src/chcon.c b/src/chcon.c index fbfdb4d..c0da694 100644 --- a/src/chcon.c +++ b/src/chcon.c @@ -519,6 +519,10 @@ main (int argc, char **argv) usage (EXIT_FAILURE); } + if (is_selinux_enabled () != 1) +error (EXIT_FAILURE, 0, + _(%s may be used only on a SELinux kernel), program_name); + Thanks for the tip. I'll revert that part of the patch. I'll address the original problem by adding getfilecon and lgetfilecon wrappers that map those unusual cases (10,unlabeled and 0,NULL) to a return value of -1 with errno == ENOTSUPP. I'd suggest ENODATA instead - that means that the filesystem supports attributes but there was no value set for the particular file. ENODATA makes sense for the 10,unlabeled case. I viewed using a library so old that its getfilecon can return 0 and set context to NULL as lacking support (ENOTSUPP). But I'll do whatever is more consistent with the rest of SELinux. Ah, I see - we did change getfilecon() in libselinux in 2007 to check for a return value of zero from getxattr() and map that to -1 with errno EOPNOTSUPP. That was to address a change/regression in the kernel that caused getxattr() on /proc/sys nodes to return 0. So I guess EOPNOTSUPP aka ENOTSUP is fine for the zero-return case. -- Stephen Smalley National Security Agency
Re: should GNU install call matchpathcon by default?
On Wed, 2008-05-21 at 16:26 +0200, Jim Meyering wrote: Ondrej Vasik [EMAIL PROTECTED] wrote: Jim Meyering jim at meyering.net writes: In the multi-file case, the pre-patch performance penalty for enabling the ifdef'd-out code would range from probably-immeasurable (for just 2 or 3 files) to infinite, with enough files to make install exhaust virtual memory. Actually even for 2-3 files (copied to /usr/temp dir) it was measurable. On my machine the performance impact is following: Setdefaultcontext code if0'd: 0.0148 s. Current patch(matchpathcon_init_prefix just once): 0.0666 s. Old setdefaultcontext code:0.1702 s. For 100 files(copied to /usr/temp) it took 4.0342 s. - that's about 35,5 times more than with current patch from Jim and 128 times more than with if0'd setdefaultcontext code. And of course for more files it goes to infinity. Problem is that disabled code is causing bugzilla tickets like https://bugzilla.redhat.com/show_bug.cgi?id=319231 , so maybe the best solution Perhaps you mean some other BZ? That one doesn't involve performance. To enable the code in question, just define this symbol for install.c: ENABLE_WHEN_MATCHPATHCON_IS_MORE_EFFICIENT to reduce performance impact would be to patch automake to install multiple files in one directory at once or something like that - to reduce performance impact of the ifdefed code on installation of big portions of files. That will help, and part of it is already done in upstream automake: 2008-03-08 Ralf Wildenhues [EMAIL PROTECTED] Use `install' with multiple files at once for some primaries. With nobase targets, at most 50 files are installed at once, to avoid quadratic string concatenation and line length limits. This isn't yet done with base targets. One hope is that there, the typical file name length is lower. If this turns out to be a problem, it should be revisited. but that doesn't yet help when installing e.g., coreutils' 100 programs, since the existing code still loops, installing each individually. In a way, it has to, because with --program-transform-name, it may have to rename each one. However, automake *can* (and probably will, now that I've proposed it) special-case the very common situation in which there is no --program-transform-name and $(EXEEXT) is empty. Maybe someone will propose a patch to do that. A 30x performance improvement is worth a small compromise for the common case. However, the underlying problem still needs to be dealt with: the outrageous expense of the matchpathcon function. Is anyone planning to address that? There have been a number of small optimizations made over time, and we keep looking for other ways to improve the situation. There is also an experimental implementation in progress to replace the current use of pathname regexes with a simpler glob syntax (FCglob) that should help if it succeeds. -- Stephen Smalley National Security Agency ___ Bug-coreutils mailing list Bug-coreutils@gnu.org http://lists.gnu.org/mailman/listinfo/bug-coreutils
Re: should GNU install call matchpathcon by default?
On Mon, 2007-11-12 at 14:20 +0100, Jim Meyering wrote:
Jim Meyering [EMAIL PROTECTED] wrote:
Jim Meyering [EMAIL PROTECTED] wrote:
This morning I noticed a flagrant difference in the speed of
make install for the just-released gettext-0.17. It took 12(!)
times longer on a rawhide system than on a usually-slower debian
unstable system. (3min vs. 15s)
FYI,
Dan Walsh suggested to use
matchpathcon_init_prefix (NULL, /first_component_of_abs_dest/);
to limit the number of regular expressions matchpathcon will have to
compile. That works very well, as long as you're not installing into
/usr, in which case it's still better than nothing. When installing
into /tmp, the example above takes 21-22 seconds, rather than 180.
Much better. However, installing into /usr/tmp still required about 70
seconds, so there's room for improvement.
I've implemented it like this:
install+SELinux: reduce a 12x performance hit to ~1.5x
* src/install.c (setdefaultfilecon): Call matchpathcon_init_prefix,
to mitigate what would otherwise be a large performance hit due to
the use of matchpathcon.
Dan Walsh suggested the use of matchpathcon_init_prefix.
* gl/lib/se-selinux.in.h (matchpathcon_init_prefix): Define.
---
gl/lib/se-selinux.in.h |3 +++
src/install.c | 32
3 files changed, 44 insertions(+), 0 deletions(-)
diff --git a/gl/lib/se-selinux.in.h b/gl/lib/se-selinux.in.h
index 7bfe4c5..7be1e70 100644
--- a/gl/lib/se-selinux.in.h
+++ b/gl/lib/se-selinux.in.h
@@ -51,4 +51,7 @@ static inline int security_compute_create
(security_context_t scon,
security_class_t tclass,
security_context_t *newcon)
{ errno = ENOTSUP; return -1; }
+static inline int matchpathcon_init_prefix (char const *path,
+ char const *prefix)
+ { errno = ENOTSUP; return -1; }
#endif
diff --git a/src/install.c b/src/install.c
index 34f61ff..216715f 100644
--- a/src/install.c
+++ b/src/install.c
@@ -213,6 +213,38 @@ setdefaultfilecon (char const *file)
if (lstat (file, st) != 0)
return;
+ if (IS_ABSOLUTE_FILE_NAME (file))
+{
+ /* Calling matchpathcon_init_prefix (NULL, /first_component/)
+ is an optimization to minimize the expense of the following
+ matchpathcon call. */
+ char const *p0;
+ char const *p = file + 1;
+ while (ISSLASH (*p))
+ ++p;
+
+ /* Record final leading slash, for when FILE starts with two or more.
*/
+ p0 = p - 1;
+
+ if (*p)
+ {
+ char *prefix;
+ do
+ {
+ ++p;
+ }
+ while (*p !ISSLASH (*p));
+
+ prefix = malloc (p - p0 + 2);
+ if (prefix)
+ {
+ stpcpy (stpncpy (prefix, p0, p - p0), /);
+ matchpathcon_init_prefix (NULL, prefix);
+ free (prefix);
+ }
+ }
+}
+
/* If there's an error determining the context, or it has none,
return to allow default context */
if ((matchpathcon (file, st.st_mode, scontext) != 0) ||
This issue came up recently again, see:
https://bugzilla.redhat.com/show_bug.cgi?id=447410
It appears that the patch that was merged into coreutils ends up calling
matchpathcon_init_prefix() for each file being installed rather than
once upon startup, and without calling matchpathcon_fini() to free the
memory allocated by each matchpathcon_init_prefix() call.
That makes it slower than necessary and leaks memory.
See the bug report for the discussion.
Can we get this corrected in the upstream coreutils? Thanks.
--
Stephen Smalley
National Security Agency
___
Bug-coreutils mailing list
Bug-coreutils@gnu.org
http://lists.gnu.org/mailman/listinfo/bug-coreutils
