Bug#929063: init: delegate selinux operation to separate binary

classic Classic list List threaded Threaded
23 messages Options
12
Reply | Threaded
Open this post in threaded view
|

Bug#929063: init: delegate selinux operation to separate binary

Dmitry Bogatov-3

Package: sysvinit
Severity: wishlist
Tags: patch

From 7f6242e5f3d893e90b3ed44fb09abe5983c2d49a Mon Sep 17 00:00:00 2001
From: Dmitry Bogatov <[hidden email]>
Date: Wed, 15 May 2019 12:10:13 +0000
Subject: [PATCH] init: delegate selinux operation to separate binary

Move selinux-related logic from /sbin/init into separate binary
(/sbin/selinux-check) by default. This way, /sbin/init is no longer
linked aganist libselinux (and its transitive dependencies).

If user need selinux initialization, she can install /sbin/selinux-check
separately.
---
 src/Makefile        |  8 +++----
 src/init.c          | 51 +++++++++++++++++++++++++++------------------
 src/init.h          |  4 ++++
 src/paths.h         |  1 +
 src/selinux-check.c | 21 +++++++++++++++++++
 5 files changed, 61 insertions(+), 24 deletions(-)
 create mode 100644 src/selinux-check.c

diff --git a/src/Makefile b/src/Makefile
index e54c051c..a23fbe38 100644
--- a/src/Makefile
+++ b/src/Makefile
@@ -23,7 +23,7 @@ MNTPOINT=
 
 # For some known distributions we do not build all programs, otherwise we do.
 BIN =
-SBIN = init halt shutdown runlevel killall5 fstab-decode logsave
+SBIN = init halt shutdown runlevel killall5 fstab-decode logsave selinux-check
 USRBIN = last mesg readbootlog
 
 MAN1 = last.1 lastb.1 mesg.1 readbootlog.1
@@ -81,11 +81,9 @@ MANDIR = /usr/share/man
 
 ifeq ($(WITH_SELINUX),yes)
   SELINUX_DEF =  -DWITH_SELINUX
-  INITLIBS += -lsepol -lselinux
   SULOGINLIBS = -lselinux
 else
   SELINUX_DEF =
-  INITLIBS =
   SULOGINLIBS =
 endif
 
@@ -105,8 +103,10 @@ all: $(BIN) $(SBIN) $(USRBIN)
 # $(CC) $(CFLAGS) $(LDFLAGS) -o $@ $^ $(LDLIBS)
 #%.o: %.c
 # $(CC) $(CFLAGS) $(CPPFLAGS) -c $^ -o $@
+#
+selinux-check: -lsepol -lselinux
 
-init: LDLIBS += $(INITLIBS) $(STATIC)
+init: LDLIBS += $(STATIC)
 init: init.o init_utmp.o runlevellog.o
 
 halt: LDLIBS += $(STATIC)
diff --git a/src/init.c b/src/init.c
index 4369beb9..58e098fa 100644
--- a/src/init.c
+++ b/src/init.c
@@ -64,9 +64,6 @@ Version information is not placed in the top-level Makefile by default
 #include <sys/syslog.h>
 #include <sys/time.h>
 
-#ifdef WITH_SELINUX
-#  include <selinux/selinux.h>
-#endif
 #ifdef __FreeBSD__
 extern char **environ;
 #endif
@@ -3012,6 +3009,35 @@ int telinit(char *progname, int argc, char **argv)
  return 1;
 }
 
+static
+void maybe_init_selinux(char **argv)
+{
+ int wstatus, ret;
+
+ if (getenv("SELINUX_INIT")) return;           /* We were re-execed. */
+ if (access(SELINUX_CHECK, X_OK) != 0) return; /* Helper not installed */
+
+ if (fork() == 0) { /* child */
+ execl(SELINUX_CHECK, SELINUX_CHECK, NULL);
+ fprintf(stderr, "Failed to execute helper to init SELinux\n");
+ exit(SELINUX_CHECK_HALT);
+ }
+
+ /* parent */
+ wait(&wstatus);
+ ret = WIFEXITED(wstatus) ? WEXITSTATUS(wstatus) : SELINUX_CHECK_HALT;
+ switch (ret) {
+ case SELINUX_CHECK_CONTINUE: return;
+ case SELINUX_CHECK_REEXEC:
+ putenv("SELINUX_INIT=YES");
+ execv(myname, argv);
+ fprintf(stderr, "Failed to re-exec init\n");
+ /* fall through */
+ case SELINUX_CHECK_HALT:
+ default: exit(1);
+ }
+}
+
 /*
  * Main entry for init and telinit.
  */
@@ -3095,23 +3121,8 @@ int main(int argc, char **argv)
  maxproclen += strlen(argv[f]) + 1;
  }
 
-#ifdef WITH_SELINUX
- if (getenv("SELINUX_INIT") == NULL) {
-         if (is_selinux_enabled() != 1) {
-    if (selinux_init_load_policy(&enforce) == 0) {
-             putenv("SELINUX_INIT=YES");
-      execv(myname, argv);
-    } else {
-      if (enforce > 0) {
- /* SELinux in enforcing mode but load_policy failed */
- /* At this point, we probably can't open /dev/console, so log() won't work */
- fprintf(stderr,"Unable to load SELinux Policy. Machine is in enforcing mode. Halting now.\n");
- exit(1);
-      }
-    }
-  }
- }
-#endif  
+ maybe_init_selinux(argv);
+
  /* Start booting. */
  argv0 = argv[0];
  argv[1] = NULL;
diff --git a/src/init.h b/src/init.h
index 1b70d6d1..b9440e93 100644
--- a/src/init.h
+++ b/src/init.h
@@ -50,6 +50,10 @@
 /* Default path inherited by every child. */
 #define PATH_DEFAULT   "/sbin:/usr/sbin:/bin:/usr/bin"
 
+/* Interface with SELINUX_CHECK */
+#define SELINUX_CHECK_CONTINUE 0
+#define SELINUX_CHECK_REEXEC   1
+#define SELINUX_CHECK_HALT     2
 
 /* Prototypes. */
 void write_utmp_wtmp(char *user, char *id, int pid, int type, char *line);
diff --git a/src/paths.h b/src/paths.h
index c9b82c1a..8230c8a9 100644
--- a/src/paths.h
+++ b/src/paths.h
@@ -38,6 +38,7 @@
 #define PWRSTAT_OLD "/etc/powerstatus" /* COMPAT: SIGPWR reason (OK/BAD) */
 #define PWRSTAT "/var/run/powerstatus" /* COMPAT: SIGPWR reason (OK/BAD) */
 #define RUNLEVEL_LOG    "/var/run/runlevel"     /* neutral place to store run level */
+#define SELINUX_CHECK   "/sbin/selinux-check"   /* optional selinux initialization */
 
 #if 0
 #define INITLVL "/etc/initrunlvl" /* COMPAT: New runlevel */
diff --git a/src/selinux-check.c b/src/selinux-check.c
new file mode 100644
index 00000000..f530b92c
--- /dev/null
+++ b/src/selinux-check.c
@@ -0,0 +1,21 @@
+#include <selinux/selinux.h>
+#include <stdio.h>
+#include <time.h>
+
+#include "init.h"
+
+int main(void)
+{
+ int enforce;
+
+ if (is_selinux_enabled() != 1) {
+ if (selinux_init_load_policy(&enforce) == 0) {
+ return SELINUX_CHECK_REEXEC;
+ } else if (enforce > 0) {
+ fprintf(stderr,"Unable to load SELinux Policy. Machine is in enforcing mode. Halting now.\n");
+ return SELINUX_CHECK_HALT;
+ }
+ }
+
+ return SELINUX_CHECK_CONTINUE;
+}
--
        Note, that I send and fetch email in batch, once every 24 hours.
                 If matter is urgent, try https://t.me/kaction
                                                                             --

attachment0 (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Bug#929063: init: delegate selinux operation to separate binary

Laurent Bigonville-5
On Thu, 16 May 2019 08:54:43 +0000 Dmitry Bogatov <[hidden email]>
wrote:
 >
 > From 7f6242e5f3d893e90b3ed44fb09abe5983c2d49a Mon Sep 17 00:00:00 2001
 > From: Dmitry Bogatov <[hidden email]>
 > Date: Wed, 15 May 2019 12:10:13 +0000
 > Subject: [PATCH] init: delegate selinux operation to separate binary
 >
 > Move selinux-related logic from /sbin/init into separate binary
 > (/sbin/selinux-check) by default. This way, /sbin/init is no longer
 > linked aganist libselinux (and its transitive dependencies).
 >
 > If user need selinux initialization, she can install /sbin/selinux-check
 > separately.

Can you please explain the rational behind this?

This looks like a bad idea to me. SELinux needs to be initialized as
soon as possible during the boot otherwise this will call for issues.

Was that discussed with anybody involved in SELinux in debian and/or
upstream?

Reply | Threaded
Open this post in threaded view
|

Bug#929063: init: delegate selinux operation to separate binary

Laurent Bigonville-5
Le 16/05/19 à 11:43, Laurent Bigonville a écrit :

> On Thu, 16 May 2019 08:54:43 +0000 Dmitry Bogatov <[hidden email]>
> wrote:
> >
> > From 7f6242e5f3d893e90b3ed44fb09abe5983c2d49a Mon Sep 17 00:00:00 2001
> > From: Dmitry Bogatov <[hidden email]>
> > Date: Wed, 15 May 2019 12:10:13 +0000
> > Subject: [PATCH] init: delegate selinux operation to separate binary
> >
> > Move selinux-related logic from /sbin/init into separate binary
> > (/sbin/selinux-check) by default. This way, /sbin/init is no longer
> > linked aganist libselinux (and its transitive dependencies).
> >
> > If user need selinux initialization, she can install
> /sbin/selinux-check
> > separately.
>
> Can you please explain the rational behind this?
>
> This looks like a bad idea to me. SELinux needs to be initialized as
> soon as possible during the boot otherwise this will call for issues.
>
> Was that discussed with anybody involved in SELinux in debian and/or
> upstream?
>
Note that if you want to reduce the dependencies, you can drop
"-libsepol" from INITLIBS in src/Makefile.

This is not needed (anymore?)

Reply | Threaded
Open this post in threaded view
|

Bug#929063: init: delegate selinux operation to separate binary

Thorsten Glaser-6
In reply to this post by Dmitry Bogatov-3
On Thu, 16 May 2019, Dmitry Bogatov wrote:

> + if (fork() == 0) { /* child */
        if ((ret = fork()) == 0) { /* child */
> + execl(SELINUX_CHECK, SELINUX_CHECK, NULL);
> + fprintf(stderr, "Failed to execute helper to init SELinux\n");
> + exit(SELINUX_CHECK_HALT);
> + }
        } else if (ret == -1) { /* fork failed */
                fprintf(stderr, "Failed to fork to execute helper to init SELinux\n");
                ret = SELINUX_CHECK_HALT;
        } else { /* parent */
+ wait(&wstatus);
+ ret = WIFEXITED(wstatus) ? WEXITSTATUS(wstatus) : SELINUX_CHECK_HALT;
        }
> + switch (ret) {
> + case SELINUX_CHECK_CONTINUE: return;
[...]

Reply | Threaded
Open this post in threaded view
|

Bug#929063: init: delegate selinux operation to separate binary

Dmitry Bogatov-3
In reply to this post by Laurent Bigonville-5

[2019-05-16 11:43] Laurent Bigonville <[hidden email]>
> On Thu, 16 May 2019 08:54:43 +0000 Dmitry Bogatov <[hidden email]>
> wrote:
>  >
>  > From 7f6242e5f3d893e90b3ed44fb09abe5983c2d49a Mon Sep 17 00:00:00 2001
>  > From: Dmitry Bogatov <[hidden email]>
>  > Date: Wed, 15 May 2019 12:10:13 +0000
>  > Subject: [PATCH] init: delegate selinux operation to separate binary
> Can you please explain the rational behind this?

        This way, /sbin/init is no longer linked aganist libselinux (and its
        transitive dependencies).

        If user need selinux initialization, she can install
        /sbin/selinux-check separately.

> This looks like a bad idea to me. SELinux needs to be initialized as
> soon as possible during the boot otherwise this will call for issues.

As you may see, this patch does not change time during boot, when
selinux functions are called -- only moves them into child process.

> Was that discussed with anybody involved in SELinux in debian and/or
> upstream?

That is exactly place to start discussion. Luckily, Jesse is following
BTS, and I do not have to go through Savannah issue tracker.

PS. I removed -lselinux from INITLIBS in src/Makefile.
--
        Note, that I send and fetch email in batch, once every 24 hours.
                 If matter is urgent, try https://t.me/kaction
                                                                             --

Reply | Threaded
Open this post in threaded view
|

Bug#929063: init: delegate selinux operation to separate binary

Laurent Bigonville-5
Le 18/05/19 à 13:31, Dmitry Bogatov a écrit :
[2019-05-16 11:43] Laurent Bigonville [hidden email]
On Thu, 16 May 2019 08:54:43 +0000 Dmitry Bogatov [hidden email] 
wrote:
 >
 > From 7f6242e5f3d893e90b3ed44fb09abe5983c2d49a Mon Sep 17 00:00:00 2001
 > From: Dmitry Bogatov [hidden email]
 > Date: Wed, 15 May 2019 12:10:13 +0000
 > Subject: [PATCH] init: delegate selinux operation to separate binary
Can you please explain the rational behind this?
	This way, /sbin/init is no longer linked aganist libselinux (and its
	transitive dependencies).

	If user need selinux initialization, she can install
	/sbin/selinux-check separately.

I've seen that in your commit, I just don't understand why this is even a goal. libselinux is really small and only pulls libpcre3 which is pulled by grep (which is Essential). It's not possible today to install debian without libselinux installed anyway.

Also, what's your plan regarding packaging? Would that executable be put in a separate package? TBRH I spent a lot of time working opening bugs/submitting patches in debian so the user who wants to use SELinux can get (an almost) out of the box experience in debian and I would not really be happy to see that attempt to revert that in a core component.

If you really (really) want to go that way, maybe you should use a private path for the helper (as it shouldn't be called my regular users after the initial load) and/or use a less common name than "selinux-check".

This looks like a bad idea to me. SELinux needs to be initialized as 
soon as possible during the boot otherwise this will call for issues.
As you may see, this patch does not change time during boot, when
selinux functions are called -- only moves them into child process.

But you are forking/exec() that usually requires SELinux permission, however as there would be no policy loaded yet at that moment that should be allowed by SELinux.

I don't know whether or not that would require a change in existing policies.


Was that discussed with anybody involved in SELinux in debian and/or 
upstream?
That is exactly place to start discussion. Luckily, Jesse is following
BTS, and I do not have to go through Savannah issue tracker.

I was more thinking about upstream SELinux people

PS. I removed -lselinux from INITLIBS in src/Makefile.
You mean -lsepol?

Reply | Threaded
Open this post in threaded view
|

Bug#929063: Moving SELinux check

Jesse Smith-2
In reply to this post by Dmitry Bogatov-3
I've looked over the patch and the logic seems straight forward enough.
Philosophically, I can see arguments for doing this (simplify the core
of init, remove a dependency) and against this idea (it adds a new
program to the sysvinit package and start-up process). So from a
philosophical stand point I'm fairly neutral on this new approach.

From a practical perspective, I'm curious if there is any benefit or
drawback. Is this patch fixing a known bug, does it  significantly
reduce the size of PID 1 in memory? Is there a flaw in libselinux is
known to cause problems if init is linked to it? I'd like to hear some
options on why we might apply this (or not) upstream.

- Jesse

Reply | Threaded
Open this post in threaded view
|

Bug#929063: init: delegate selinux operation to separate binary

Dmitry Bogatov-3
In reply to this post by Laurent Bigonville-5

[2019-05-18 15:00] Laurent Bigonville <[hidden email]>
> I've seen that in your commit, I just don't understand why this is even
> a goal.

Because I do not want to pay for what I do not use. It is matter of good
design and Unix way.

> libselinux is really small and only pulls libpcre3 which is pulled by
> grep (which is Essential). It's not possible today to install debian
> without libselinux installed anyway.

Path of a thousand miles starts with a single step.

> Also, what's your plan regarding packaging? Would that executable be
> put in a separate package?

Yes, that the plan.

> TBRH I spent a lot of time working opening bugs/submitting patches in
> debian so the user who wants to use SELinux can get (an almost) out of
> the box experience in debian and I would not really be happy to see
> that attempt to revert that in a core component.

And I spend a lot of time of not having useless things installed on my
box.

What is wrong with having selinux support in separate package? Just
enable "Apt::InstallSuggests" and you are golden.

> If you really (really) want to go that way, maybe you should use a
> private path for the helper (as it shouldn't be called my regular users
> after the initial load) and/or use a less common name than "selinux-check".

No problem. I do not insist on any particular naming of helper and I
installed it into sbin just to reduce Makefile part of patch.

It was my plan to install it into /lib/init/ anyway, and you are welcome
to propose any name. What do you suggest?

> >> Was that discussed with anybody involved in SELinux in debian and/or
> >> upstream?
> > That is exactly place to start discussion. Luckily, Jesse is following
> > BTS, and I do not have to go through Savannah issue tracker.
>
> I was more thinking about upstream SELinux people

Okay. I see you added "selinux-devel" into thread. Thank you.

Dear selinux-devel maintainers, we are considering moving following
check from /sbin/init into subprocess:

   if (getenv("SELINUX_INIT") == NULL) {
     if (is_selinux_enabled() != 1) {
       if (selinux_init_load_policy(&enforce) == 0) {
         putenv("SELINUX_INIT=YES");
         execv(myname, argv);
       } else {
         if (enforce > 0) {
           /* SELinux in enforcing mode but load_policy failed */
           /* At this point, we probably can't open /dev/console, so log() won't work */
           fprintf(stderr,"Unable to load SELinux Policy. Machine is in enforcing mode. Halting now.\n");
           exit(1);
         }
       }
     }
   }

Are there any possible unwanted side-effects? Any suggestions about it?
--
        Note, that I send and fetch email in batch, once every 24 hours.
                 If matter is urgent, try https://t.me/kaction
                                                                             --

Reply | Threaded
Open this post in threaded view
|

Bug#929063: Moving SELinux check

Dmitry Bogatov-3
In reply to this post by Jesse Smith-2

[2019-05-18 16:14] Jesse Smith <[hidden email]>
> From a practical perspective, I'm curious if there is any benefit or
> drawback. Is this patch fixing a known bug,
> does it significantly reduce the size of PID 1 in memory?

Not that I really care about 1Mb of RAM, but:

152K /lib/x86_64-linux-gnu/libselinux.so.1
692K /lib/x86_64-linux-gnu/libsepol.so.1
460K /lib/x86_64-linux-gnu/libpcre.so.3.13.3
--
        Note, that I send and fetch email in batch, once every 24 hours.
                 If matter is urgent, try https://t.me/kaction
                                                                             --

Reply | Threaded
Open this post in threaded view
|

Bug#929063: Path of a thousand miles starts with a single step.

Dominick Grift-3
In reply to this post by Dmitry Bogatov-3
You should probably present your real question using the proper chanells, instead of beating around the bush to get your foot in the door.

Does Debian want to get rid of its SELinux support, yes or no? Regardless of the answer to this question, i do not believe that this patch makes sense.

--
Key fingerprint = 5F4D 3CDB D3F8 3652 FBD8 02D5 3B6C 5F1D 2C7B 6B02
https://sks-keyservers.net/pks/lookup?op=get&search=0x3B6C5F1D2C7B6B02
Dominick Grift

Reply | Threaded
Open this post in threaded view
|

Bug#929063: init: delegate selinux operation to separate binary

Laurent Bigonville-5
In reply to this post by Dmitry Bogatov-3
Le 22/05/19 à 01:45, Dmitry Bogatov a écrit :

> [2019-05-18 15:00] Laurent Bigonville<[hidden email]>
>> I've seen that in your commit, I just don't understand why this is even
>> a goal.
> Because I do not want to pay for what I do not use. It is matter of good
> design and Unix way.
>
>> libselinux is really small and only pulls libpcre3 which is pulled by
>> grep (which is Essential). It's not possible today to install debian
>> without libselinux installed anyway.
> Path of a thousand miles starts with a single step.
>
>> Also, what's your plan regarding packaging? Would that executable be
>> put in a separate package?
> Yes, that the plan.

So let's be it clear for the record. I'll personally oppose all patches
that would undermine the consistency and the experience of using SELinux
in debian.

As a distribution, debian has historically always been on the side of
enabling as many build options as possible to provide by default the
"full experience" to the users. I think that good and consistent
integration of different options and technologies is more beneficial for
our users than winning 205kb on the default installation (libpcre is
already pulled by grep and the sysvinit dependency against libsepol can
be dropped).

Also, removing selinux support by default would require many packages to
create different flavors (which is usually a big no-no in debian).

If people feel the urge of removing libselinux library (or other
libraries starting with "libs") from their system that still something
that could be done on their side at their cost ; especially that the
current situation exists for more than 10 years (SELinux support is
enabled by default in sysvinit and other base packages like PAM since
2005) and is absolutely not causing any issues what's however to the
users not enabling SELinux on their system (the library is a noop in
that case).

Reply | Threaded
Open this post in threaded view
|

Bug#929063: init: delegate selinux operation to separate binary

Thorsten Glaser-6
On Wed, 22 May 2019, Laurent Bigonville wrote:

> So let's be it clear for the record. I'll personally oppose all patches that
> would undermine the consistency and the experience of using SELinux in debian.

Erm… all the patch does is move the SELinux call into a separate
executable so that init itself does not need to be linked against
those libraries and doesn’t need to keep them resident (and will
not be affected by flaws in those libraries, keeping the attack
surface small, unlike *cough* others).

As long as that other executable will end up in the same binary
package in Debian, there will be no user-visible change save for
saving some RAM.

(I’m not quite convinced the effort is worth it, but given that
this would be changed upstream, and that there are likely other
users of the same upstream code who’re _not_ using SELinux, this
would be very welcomed by those, so I’m okay with it.)

bye,
//mirabilos
--
tarent solutions GmbH
Rochusstraße 2-4, D-53123 Bonn • http://www.tarent.de/
Tel: +49 228 54881-393 • Fax: +49 228 54881-235
HRB 5168 (AG Bonn) • USt-ID (VAT): DE122264941
Geschäftsführer: Dr. Stefan Barth, Kai Ebenrett, Boris Esser, Alexander Steeg

**********

Mit der tarent Academy bieten wir auch Trainings und Schulungen in den
Bereichen Softwareentwicklung, Agiles Arbeiten und Zukunftstechnologien an.

Besuchen Sie uns auf www.tarent.de/academy. Wir freuen uns auf Ihren Kontakt.

**********

Reply | Threaded
Open this post in threaded view
|

Bug#929063: Moving SELinux check

Jesse Smith-2
In reply to this post by Dmitry Bogatov-3
On 5/21/19 8:45 PM, Dmitry Bogatov wrote:
> [2019-05-18 16:14] Jesse Smith <[hidden email]>
>> From a practical perspective, I'm curious if there is any benefit or
>> drawback. Is this patch fixing a known bug,
>> does it significantly reduce the size of PID 1 in memory?
> Not that I really care about 1Mb of RAM, but:
>
> 152K /lib/x86_64-linux-gnu/libselinux.so.1
> 692K /lib/x86_64-linux-gnu/libsepol.so.1
> 460K /lib/x86_64-linux-gnu/libpcre.so.3.13.3

I don't think removing the SELinux dependency from init actually saves
us any RAM. Several other services link to these libraries too, so the
libraries are loaded into RAM anyway and should be shared between the
various services. Unless SELinux is culled from every low-level daemon
that 1MB RAM is still going to be used.

Reply | Threaded
Open this post in threaded view
|

Bug#929063: Moving SELinux check

Thorsten Glaser-6
On Wed, 22 May 2019, Jesse Smith wrote:

> I don't think removing the SELinux dependency from init actually saves
> us any RAM. Several other services link to these libraries too, so the

Maybe, maybe not. (I’m fairly sure I’ve got some VMs without.)

Other services can, however, be more easily restarted than the entire
system, in case of a security fix for that library.

bye,
//mirabilos
--
tarent solutions GmbH
Rochusstraße 2-4, D-53123 Bonn • http://www.tarent.de/
Tel: +49 228 54881-393 • Fax: +49 228 54881-235
HRB 5168 (AG Bonn) • USt-ID (VAT): DE122264941
Geschäftsführer: Dr. Stefan Barth, Kai Ebenrett, Boris Esser, Alexander Steeg

**********

Mit der tarent Academy bieten wir auch Trainings und Schulungen in den
Bereichen Softwareentwicklung, Agiles Arbeiten und Zukunftstechnologien an.

Besuchen Sie uns auf www.tarent.de/academy. Wir freuen uns auf Ihren Kontakt.

**********

Reply | Threaded
Open this post in threaded view
|

Bug#929063: Moving SELinux check

Jesse Smith-2
On 5/22/19 11:57 AM, Thorsten Glaser wrote:

> On Wed, 22 May 2019, Jesse Smith wrote:
>
>> I don't think removing the SELinux dependency from init actually saves
>> us any RAM. Several other services link to these libraries too, so the
> Maybe, maybe not. (I’m fairly sure I’ve got some VMs without.)
>
> Other services can, however, be more easily restarted than the entire
> system, in case of a security fix for that library.
>
>

How do you think an attacker would exploit a flaw in a SELinux library
through init? SysV init doesn't interact with the user, doesn't read any
files directly after it's up and running, doesn't listen on any sockets.
About the only way to interact with PID1 is through a pipe that can only
be written to by root.

Reply | Threaded
Open this post in threaded view
|

Bug#929063: init: delegate selinux operation to separate binary

Jesse Smith-2
In reply to this post by Dmitry Bogatov-3
On Wed, 22 May 2019 13:28:39 +0200 (CEST) Thorsten Glaser wrote:
>
> (I’m not quite convinced the effort is worth it, but given that
> this would be changed upstream, and that there are likely other
> users of the same upstream code who’re _not_ using SELinux, this
> would be very welcomed by those, so I’m okay with it.)

I'd like to point out that init already has compile-time defines in the code which check for the existence of SELinux (using the variable WITH_SELINUX). If WITH_SELINUX is not defined at compile time, then the SELinux code isn't built into init. So other projects, perhaps Debian Hurd or FreeBSD, can already build init without SELinux features.
Reply | Threaded
Open this post in threaded view
|

Bug#929063: init: delegate selinux operation to separate binary

Dmitry Bogatov-3

[2019-05-22 18:24] Jesse Smith <[hidden email]>

> On Wed, 22 May 2019 13:28:39 +0200 (CEST) Thorsten Glaser wrote:
> >
> > (I’m not quite convinced the effort is worth it, but given that
> > this would be changed upstream, and that there are likely other
> > users of the same upstream code who’re _not_ using SELinux, this
> > would be very welcomed by those, so I’m okay with it.)
>
> I'd like to point out that init already has compile-time defines in the
> code which check for the existence of SELinux (using the variable
> WITH_SELINUX). If WITH_SELINUX is not defined at compile time, then the
> SELinux code isn't built into init. So other projects, perhaps Debian
> Hurd or FreeBSD, can already build init without SELinux features.

Sure. Difference is in convenience. One thing is when you have to
re-compile program to get those and only those features you need (hi,
Gentoo) and another is when you just install and uninstall pre-compiled
binaries.

Also, every WITH_FOO flag doubles number of configurations your program
have.  Once you have dozen of flags, you no longer can test all of
configurations.

I am surprised, that there is so much controversy on whether it is good
to have some feature of program pluggable without re-compilation.  The
only real concern that was raised, as I see it, is how SELinux interacts
with extra fork/exec.
--
        Note, that I send and fetch email in batch, once every 24 hours.
                 If matter is urgent, try https://t.me/kaction
                                                                             --

Reply | Threaded
Open this post in threaded view
|

Bug#929063: init: delegate selinux operation to separate binary

Jesse Smith-2
In reply to this post by Dmitry Bogatov-3
On Thu, 23 May 2019 16:27:00 +0000 Dmitry Bogatov wrote:
>
>
> Also, every WITH_FOO flag doubles number of configurations your program
> have. Once you have dozen of flags, you no longer can test all of
> configurations.

Why not?

>
> I am surprised, that there is so much controversy on whether it is good
> to have some feature of program pluggable without re-compilation. The
> only real concern that was raised, as I see it, is how SELinux interacts
> with extra fork/exec.

The proposed patch doesn't seem to make the feature pluggable, it just moves the feature to a second binary. There isn't anything here which toggles the feature on/off, or lets the admin enable/disable it.

That being said, I'd be open to applying this upstream, with four conditions or changes:

1. The selinux-check binary should probably be renamed so it is clear this program is part of the SysV init suite. Maybe calling it something like /sbin/sysvinit-selinux-check? I'm open to suggestions as to what it should be called.

2. The patch should respect the compile-time define WITH_SELINUX in both init.c and in the check-selinux.c code. Otherwise the build will either fail or attempt to perform unnecessary fork/exec on systems without SELinux, like Hurd and kFreeBSD. In fact, the Makefile should skip building & installing check-selinux.c completely if SELinux is not available on that platform. Otherwise we are making things more resource hungry and complex on systems where SELinux is not included, rather than less.

3. The new binary/helper program should have a man page.

4. There should be a test or script presented that demonstrates SELinux still works properly with this approach. I don't want to open a security issue by moving SELinux support to a non-PID1 binary.


Reply | Threaded
Open this post in threaded view
|

Bug#929063: SELinux integration in sysVinit

Jason Zaman
In reply to this post by Dmitry Bogatov-3
Bigon asked me to forward this so its part of the bug tracker.

On Fri, May 24, 2019 at 08:55:22PM +0800, Jason Zaman wrote:

> On Fri, May 24, 2019 at 01:17:00PM +0200, Laurent Bigonville wrote:
> > Hello,
> >
> > There is currently some discussion at [0] about SELinux integration in
> > sysVinit and the fact that somebody wants to delegate the loading of the
> > policy to an other binary than PID1.
> >
> > Has somebody a remark or an objection to that proposal?
>
> I object too. There is a *huge* change in functionality. Originally if
> you boot with SELinux enforcing, there are only two things that can
> happen. Either you end up with the policy loaded or the machine halts.
>
> In the new patch, an attacker can just chmod -x /sbin/selinux-check then
> next boot there will be no selinux enabled.
>
> if (access(SELINUX_CHECK, X_OK) != 0) fails, the machine will continue
> to boot without SELinux enabled. There is no difference between a user
> without /sbin/selinux-check on purpose and an attacker just chmod -x'd
> the tool.
>
> SELinux does not protect /sbin anywhere near as much as /etc/selinux
> (and doing that would probably be impossible). You'd need to check if
> selinux is enabled and enforcing before skipping the loading ... which
> is done by calling is_selinux_enabled() which needs linking to
> libselinux. The important part of the original design is not
> selinux_init_load_policy(), its is_selinux_enabled().
>
> If someone really wants to run sysvinit without libselinux then just
> switch to Gentoo, if you're on a non-selinux profile then you dont have
> libselinux.so. But I'd definitely not want to have this patch in Gentoo
> either.
>
> -- Jason
>
>
> > I already gave my POV regarding SELinux integration in debian.
> >
> > Kind regards,
> >
> > Laurent Bigonville
> >
> > [0] https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=929063
> >

Reply | Threaded
Open this post in threaded view
|

Bug#929063: SELinux integration in sysVinit

Stephen Smalley
On 5/24/19 9:00 AM, Jason Zaman wrote:

> Bigon asked me to forward this so its part of the bug tracker.
>
> On Fri, May 24, 2019 at 08:55:22PM +0800, Jason Zaman wrote:
>> On Fri, May 24, 2019 at 01:17:00PM +0200, Laurent Bigonville wrote:
>>> Hello,
>>>
>>> There is currently some discussion at [0] about SELinux integration in
>>> sysVinit and the fact that somebody wants to delegate the loading of the
>>> policy to an other binary than PID1.
>>>
>>> Has somebody a remark or an objection to that proposal?
>>
>> I object too. There is a *huge* change in functionality. Originally if
>> you boot with SELinux enforcing, there are only two things that can
>> happen. Either you end up with the policy loaded or the machine halts.
>>
>> In the new patch, an attacker can just chmod -x /sbin/selinux-check then
>> next boot there will be no selinux enabled.
>>
>> if (access(SELINUX_CHECK, X_OK) != 0) fails, the machine will continue
>> to boot without SELinux enabled. There is no difference between a user
>> without /sbin/selinux-check on purpose and an attacker just chmod -x'd
>> the tool.

NB even amending the patch to use F_OK still leaves one vulnerable to an
attacker doing rm /sbin/selinux-check to silently disable SELinux on
reboot.  And you can't assume that anyone who can do that can already
modify or replace /sbin/init; under existing policies /sbin/init has its
own distinct SELinux type, which would not be assigned to
/sbin/selinux-check by default (and probably shouldn't be even in newer
policies since it is the entrypoint type for the init domain).

>>
>> SELinux does not protect /sbin anywhere near as much as /etc/selinux
>> (and doing that would probably be impossible). You'd need to check if
>> selinux is enabled and enforcing before skipping the loading ... which
>> is done by calling is_selinux_enabled() which needs linking to
>> libselinux. The important part of the original design is not
>> selinux_init_load_policy(), its is_selinux_enabled().
>>
>> If someone really wants to run sysvinit without libselinux then just
>> switch to Gentoo, if you're on a non-selinux profile then you dont have
>> libselinux.so. But I'd definitely not want to have this patch in Gentoo
>> either.
>>
>> -- Jason
>>
>>
>>> I already gave my POV regarding SELinux integration in debian.
>>>
>>> Kind regards,
>>>
>>> Laurent Bigonville
>>>
>>> [0] https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=929063
>>>

12