On Mon, Jul 04, 2016 at 04:34:22PM +0100, Richard W.M. Jones wrote:
> I don't exactly know where to post this, but I guess I have everyone's
> attention on this thread.
>
> Attached are patches which work for me. They could really do with
> review from someone who knows what they're doing. They also need much
> more testing than I've done, but I'll be doing that myself later.
>
> The first patch (against libselinux) sets SELinux to Permissive mode
> early in boot if the /.autorelabel file is found (or autorelabel on
> the command line).
>
> The second patch (against policycoreutils in Fedora) implements the
> generator itself.
This is great!
> Some problems I found:
>
> - It would be nice if systemd defined a %{_generatorsdir} RPM macro.
Yeah, we should. If nobody beats me to it, I'll file a pull request.
> - I couldn't get it to work only depending on local-fs.target. I had
> to depend on sysinit.target. With local-fs.target, /boot could not
> be mounted, so there may be something broken/missing in
> local-fs.target.
local-fs.target should mount /boot also. It would be much better to have
autorelabel.service only depend on local-fs.target, since sysinit.target
pulls in quite a few daemons. Do you maybe have the logs for the failed
attempt with local-fs.target?
Some more comments inline below.
> - There seems to be no upstream for selinux-autorelabel* since it was
> moved from systemd. It looks like the only upstream is Fedora's
> policycoreutils itself. Maybe I missed something there.
>
> Rich.
>
> --
> Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
> Read my programming and virtualization blog: http://rwmj.wordpress.com
> virt-p2v converts physical machines to virtual machines. Boot with a
> live CD or over the network (PXE) and turn machines into KVM guests.
> http://libguestfs.org/virt-v2v
> From c9b8d9da73d8f530df9a8672413d1db842ff45d5 Mon Sep 17 00:00:00 2001
> From: "Richard W.M. Jones" <rjones@xxxxxxxxxx>
> Date: Mon, 4 Jul 2016 11:42:14 +0100
> Subject: [PATCH] libselinux: If autorelabel, force permissive mode.
>
> Signed-off-by: Richard W.M. Jones <rjones@xxxxxxxxxx>
> ---
> libselinux/src/load_policy.c | 30 +++++++++++++++++++++++++-----
> 1 file changed, 25 insertions(+), 5 deletions(-)
>
> diff --git a/libselinux/src/load_policy.c b/libselinux/src/load_policy.c
> index 4f39fc7..337a8a9 100644
> --- a/libselinux/src/load_policy.c
> +++ b/libselinux/src/load_policy.c
> @@ -315,7 +315,8 @@ hidden_def(selinux_mkload_policy)
> */
> int selinux_init_load_policy(int *enforce)
> {
> - int rc = 0, orig_enforce = 0, seconfig = -2, secmdline = -1;
> + int rc = 0, orig_enforce = 0, seconfig = -2, secmdline = -1,
> + seautorelabel = -1;
If I'm reading the code correctly, seautorelabel only gets set to 0,
so it might be nicer to do away with the -1 init value, and initialize
it to 0, and then maybe set it to 1 if autorelabel request is found.
The code will be slightly easier to read this way.
> FILE *cfg;
> char *buf;
>
> @@ -332,6 +333,17 @@ int selinux_init_load_policy(int *enforce)
> */
> selinux_getenforcemode(&seconfig);
>
> + /*
> + * If /.autorelabel exists then we should start in permissive
> + * mode because (a) the labels on the filesystem are known to
> + * be bogus and so should not be trusted to make security
> + * decisions, but more practically (b) mislabelled files may
> + * cause services & processes required for relabelling to fail.
> + */
> + if (access("/.autorelabel", F_OK) == 0) {
> + seautorelabel = 0;
> + }
> +
> /* Check for an override of the mode via the kernel command line. */
> rc = mount("proc", "/proc", "proc", 0, 0);
> cfg = fopen("/proc/cmdline", "r");
> @@ -342,12 +354,18 @@ int selinux_init_load_policy(int *enforce)
> fclose(cfg);
> return -1;
> }
> - if (fgets(buf, selinux_page_size, cfg) &&
> - (tmp = strstr(buf, "enforcing="))) {
> - if (tmp == buf || isspace(*(tmp - 1))) {
> + if (fgets(buf, selinux_page_size, cfg)) {
> + if ((tmp = strstr(buf, "enforcing=")) &&
> + (tmp == buf || isspace(*(tmp - 1)))) {
> secmdline =
> atoi(tmp + sizeof("enforcing=") - 1);
> }
> + else if ((tmp = strstr(buf, "autorelabel")) &&
> + (tmp == buf || isspace(*(tmp - 1))) &&
> + (tmp + sizeof("autorelabel") - 1 == '\0' ||
> + isspace(tmp + sizeof("autorelabel") - 1))) {
> + seautorelabel = 0;
> + }
> }
> fclose(cfg);
> free(buf);
> @@ -357,7 +375,9 @@ int selinux_init_load_policy(int *enforce)
> * Determine the final desired mode.
> * Command line argument takes precedence, then config file.
> */
> - if (secmdline >= 0)
> + if (seautorelabel >= 0)
> + *enforce = seautorelabel;
> + else if (secmdline >= 0)
> *enforce = secmdline;
> else if (seconfig >= 0)
> *enforce = seconfig;
> --
> 2.7.4
>
> From f1f5cf751c72b658bae08161934ffac13aee8c5f Mon Sep 17 00:00:00 2001
> From: "Richard W.M. Jones" <rjones@xxxxxxxxxx>
> Date: Mon, 4 Jul 2016 13:15:08 +0100
> Subject: [PATCH] Use generator approach to fix autorelabel.
>
> See:
> https://lists.fedoraproject.org/archives/list/devel@xxxxxxxxxxxxxxxxxxxxxxx/message/CHCEGB2RUPHFCE4FVGIRO3CJYGNS75T7/
> ---
> policycoreutils.spec | 13 ++++++++++---
> selinux-autorelabel-generator.sh | 29 +++++++++++++++++++++++++++++
> selinux-autorelabel.service | 9 +++------
> selinux-autorelabel.target | 7 +++++++
> 4 files changed, 49 insertions(+), 9 deletions(-)
> create mode 100755 selinux-autorelabel-generator.sh
> create mode 100644 selinux-autorelabel.target
>
> diff --git a/policycoreutils.spec b/policycoreutils.spec
> index a77c243..cb784d9 100644
> --- a/policycoreutils.spec
> +++ b/policycoreutils.spec
> @@ -4,10 +4,12 @@
> %global libselinuxver 2.5-7
> %global sepolgenver 1.2.3
>
> +%global generatorsdir %{_prefix}/lib/systemd/system-generators
> +
> Summary: SELinux policy core utilities
> Name: policycoreutils
> Version: 2.5
> -Release: 11%{?dist}
> +Release: 11.rwmj1%{?dist}
> License: GPLv2
> Group: System Environment/Base
> # https://github.com/SELinuxProject/selinux/wiki/Releases
> @@ -20,6 +22,8 @@ Source4: sepolicy-icons.tgz
> Source5: selinux-autorelabel
> Source6: selinux-autorelabel.service
> Source7: selinux-autorelabel-mark.service
> +Source8: selinux-autorelabel.target
> +Source9: selinux-autorelabel-generator.sh
> # download https://raw.githubusercontent.com/fedora-selinux/scripts/master/selinux/make-fedora-selinux-patch.sh
> # run:
> # $ VERSION=2.5 ./make-fedora-selinux-patch.sh policycoreutils
> @@ -123,10 +127,12 @@ rm -f %{buildroot}%{_datadir}/system-config-selinux/system-config-selinux.deskto
>
> # https://bugzilla.redhat.com/show_bug.cgi?id=1328825
> mkdir -m 755 -p %{buildroot}/%{_unitdir}/basic.target.wants/
> +mkdir -m 755 -p %{buildroot}/%{generatorsdir}
> install -m 644 -p %{SOURCE6} %{buildroot}/%{_unitdir}/
> install -m 644 -p %{SOURCE7} %{buildroot}/%{_unitdir}/
> +install -m 644 -p %{SOURCE8} %{buildroot}/%{_unitdir}/
> +install -m 755 -p %{SOURCE9} %{buildroot}/%{generatorsdir}/
> install -m 755 -p %{SOURCE5} %{buildroot}/%{_libexecdir}/selinux/
> -ln -s ../selinux-autorelabel.service %{buildroot}/%{_unitdir}/basic.target.wants/
> ln -s ../selinux-autorelabel-mark.service %{buildroot}/%{_unitdir}/basic.target.wants/
>
> %find_lang %{name}
> @@ -371,7 +377,8 @@ fi
> %{_unitdir}/selinux-autorelabel-mark.service
> %{_unitdir}/basic.target.wants/selinux-autorelabel-mark.service
> %{_unitdir}/selinux-autorelabel.service
> -%{_unitdir}/basic.target.wants/selinux-autorelabel.service
> +%{_unitdir}/selinux-autorelabel.target
> +%{generatorsdir}/selinux-autorelabel-generator.sh
> %config(noreplace) %{_sysconfdir}/sestatus.conf
> # selinux-policy Requires: policycoreutils, so we own this set of directories and our files within them
> %{_mandir}/man5/selinux_config.5.gz
> diff --git a/selinux-autorelabel-generator.sh b/selinux-autorelabel-generator.sh
> new file mode 100755
> index 0000000..be60487
> --- /dev/null
> +++ b/selinux-autorelabel-generator.sh
> @@ -0,0 +1,29 @@
> +#!/bin/sh
> +
> +# This systemd.generator(7) detects if SELinux is running and if the
> +# user requested an autorelabel, and if so sets the default target to
> +# selinux-autorelabel.target, which will cause the filesystem to be
> +# relabelled and then the system will reboot again and boot into the
> +# real default target.
> +
> +PATH=/usr/sbin:$PATH
> +unitdir=/usr/lib/systemd/system
> +
> +# If invoked with no arguments (for testing) write to /tmp.
> +earlydir="/tmp"
> +if [ -n "$2" ]; then
> + earlydir="$2"
> +fi
> +
> +set_target ()
> +{
> + ln -sf "$unitdir/selinux-autorelabel.target" "$earlydir/default.target"
> +}
> +
> +if selinuxenabled; then
> + if test -f /.autorelabel; then
> + set_target
> + elif grep -sqE "\bautorelabel\b" /proc/cmdline; then
> + set_target
> + fi
> +fi
I think it's reasonable to start with a bash generator. Once things
settle down it could be useful to rewrite it in C to avoid the
dependency on grep, and a bunch of forks in early boot.
The generator should remove /.autorelabel as the first step.
Doing it as early as possible reduces the risk of a reboot loop.
I still see a potential for confusion, if "/.autorelabel" or "autorelabel"
are found, but selinux is turned off. In that case the generator would
redirect default.target, but selinux-autorelabel.target might not
run when ConditionSecurity=selinux is not matched. I don't think
ConditionSecurity=selinux is useful: the target should only be invoked
by the generator, so it's probably better to remove the condition
to avoid the potential for mismatch between the generator and what
systemd thinks about selinux.
> diff --git a/selinux-autorelabel.service b/selinux-autorelabel.service
> index a6cc332..b8461e6 100644
> --- a/selinux-autorelabel.service
> +++ b/selinux-autorelabel.service
> @@ -1,13 +1,10 @@
> [Unit]
> -Description=Relabel all filesystems, if necessary
> +Description=Relabel all filesystems
> DefaultDependencies=no
> -Requires=local-fs.target
> Conflicts=shutdown.target
> -After=local-fs.target
> -Before=sysinit.target shutdown.target
> +After=sysinit.target
> +Before=shutdown.target
> ConditionSecurity=selinux
> -ConditionKernelCommandLine=|autorelabel
> -ConditionPathExists=|/.autorelabel
>
> [Service]
> ExecStart=/usr/libexec/selinux/selinux-autorelabel
> diff --git a/selinux-autorelabel.target b/selinux-autorelabel.target
> new file mode 100644
> index 0000000..a4f63ab
> --- /dev/null
> +++ b/selinux-autorelabel.target
> @@ -0,0 +1,7 @@
> +[Unit]
> +Description=Relabel all filesystems and reboot
> +DefaultDependencies=no
> +Requires=sysinit.target selinux-autorelabel.service
> +Conflicts=shutdown.target
> +After=sysinit.target selinux-autorelabel.service
> +ConditionSecurity=selinux
> --
> 2.7.4
>
--
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxxx
https://lists.fedoraproject.org/admin/lists/devel@xxxxxxxxxxxxxxxxxxxxxxx
