diff mbox series

[10/15] package/refpolicy: allow providing user defined modules

Message ID 20200731101040.1723047-11-antoine.tenart@bootlin.com
State Accepted
Headers show
Series Improve SELinux support | expand

Commit Message

Antoine Tenart July 31, 2020, 10:10 a.m. UTC 
Allow users to provide custom SELinux modules to be part of the final
policy. A new configuration variable is added, pointing to list of
directories containing the custom modules.

SELinux modules do require a metadata.xml file to be well integrated in
the refpolicy build. If this file isn't provided, it will be
automatically created.

For now, this option requires the extra modules to be directly into the
BR2_REFPOLICY_EXTRA_MODULES directory, and subfolders aren't supported.
They may never be, as having subfolders could introduce issues when two
different modules have the same name (which isn't supported by the
refpolicy).

Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com>
---
 package/refpolicy/Config.in    | 10 ++++++++++
 package/refpolicy/refpolicy.mk | 23 ++++++++++++++++++++++-
 2 files changed, 32 insertions(+), 1 deletion(-)

Comments

Thomas Petazzoni Sept. 4, 2020, 1:05 p.m. UTC | #1 
Hello,

On Fri, 31 Jul 2020 12:10:35 +0200
Antoine Tenart <antoine.tenart@bootlin.com> wrote:

> +config BR2_REFPOLICY_EXTRA_MODULES_DIRS
> +	string "Extra modules directories"
> +	help
> +	  Specify directories containing SELinux modules that will be build
> +	  in the SELinux policy. The modules will be automatically enabled in
> +	  the policy.
> +
> +	  Each of those directories must contain the SELinux policy .fc, .if
> +	  and .te files directly at the top-level, with no sub-directories.

I've slightly tweaked the help text here:

+config BR2_REFPOLICY_EXTRA_MODULES_DIRS
+       string "Extra modules directories"
+       help
+         Specify a space-separated list of directories containing
+         SELinux modules that will be built into the SELinux
+         policy. The modules will be automatically enabled in the
+         policy.
+
+         Each of those directories must contain the SELinux policy
+         .fc, .if and .te files directly at the top-level, with no
+         sub-directories. Also, you cannot have several modules with
+         the same name in different directories.

Also, I think your lines were too long, causing "make check-package"
warnings.

> -	$(PACKAGES_SELINUX_MODULES)
> +	$(PACKAGES_SELINUX_MODULES) \
> +	$(foreach d,$(call qstrip,$(REFPOLICY_EXTRA_MODULES)),\
> +		$(basename $(notdir $(wildcard $(d)/*.te))))
> +
> +# Allow to provide out-of-tree SELinux modules in addition to the ones in the
> +# refpolicy.
> +REFPOLICY_EXTRA_MODULES = $(BR2_REFPOLICY_EXTRA_MODULES_DIRS)

It was a bit silly to not do the qstrip here once for all, and use that
everywhere else. Also, the variable name REFPOLICY_EXTRA_MODULES wasn't
so good, since it really contains a list of directories, not a list of
modules.

So I've changed that to:

REFPOLICY_EXTRA_MODULES_DIRS = $(call qstrip,$(BR2_REFPOLICY_EXTRA_MODULES_DIRS))

I've moved it a bit further up so that the REFPOLICY_MODULES variable
can use it:

-       $(PACKAGES_SELINUX_MODULES)
+       $(PACKAGES_SELINUX_MODULES) \
+       $(foreach d,$(REFPOLICY_EXTRA_MODULES_DIRS),\
+               $(basename $(notdir $(wildcard $(d)/*.te))))

> +$(foreach dir,$(call qstrip,$(BR2_REFPOLICY_EXTRA_MODULES_DIRS)),\

I've used REFPOLICY_EXTRA_MODULES_DIRS here as well.

> +	$(if $(wildcard $(dir)),,\
> +		$(error BR2_REFPOLICY_EXTRA_MODULES_DIRS contains nonexistent directory $(dir))))
> +
> +define REFPOLICY_COPY_MODULES
> +	mkdir -p $(@D)/policy/modules/buildroot
> +	rsync -au $(addsuffix /*,$(call qstrip,$(REFPOLICY_EXTRA_MODULES))) \

And here as well.

> +		$(@D)/policy/modules/buildroot/
> +	if [ ! -f $(@D)/policy/modules/buildroot/metadata.xml ]; then \
> +		echo "<summary>Buildroot extra modules</summary>" > \
> +			$(@D)/policy/modules/buildroot/metadata.xml; \
> +	fi
> +endef

I've enclosed this REFPOLICY_COPY_MODULES macro definition in a:

ifneq ($(REFPOLICY_EXTRA_MODULES_DIRS),)
...
endif 

condition.

>  # In the context of a monolithic policy enabling a piece of the policy as
>  # 'base' or 'module' is equivalent, so we enable them as 'base'.
> @@ -72,6 +91,8 @@ define REFPOLICY_CONFIGURE_CMDS
>  endef
>  
>  define REFPOLICY_BUILD_CMDS
> +	$(if $(call qstrip,$(REFPOLICY_EXTRA_MODULES)),\
> +		$(REFPOLICY_COPY_MODULES))

So that we don't need a condition here.

Final commit looks like this:

  https://git.buildroot.org/buildroot/commit/?id=1e2e3cc9519ab0fd6ed5411fe88cce14b4b7a2a9

Thanks!

Thomas
Antoine Tenart Sept. 4, 2020, 3 p.m. UTC | #2 
Hello Thomas,

Quoting Thomas Petazzoni (2020-09-04 15:05:32)
> On Fri, 31 Jul 2020 12:10:35 +0200
> Antoine Tenart <antoine.tenart@bootlin.com> wrote:
> 
> > -     $(PACKAGES_SELINUX_MODULES)
> > +     $(PACKAGES_SELINUX_MODULES) \
> > +     $(foreach d,$(call qstrip,$(REFPOLICY_EXTRA_MODULES)),\
> > +             $(basename $(notdir $(wildcard $(d)/*.te))))
> > +
> > +# Allow to provide out-of-tree SELinux modules in addition to the ones in the
> > +# refpolicy.
> > +REFPOLICY_EXTRA_MODULES = $(BR2_REFPOLICY_EXTRA_MODULES_DIRS)
> 
> It was a bit silly to not do the qstrip here once for all, and use that
> everywhere else. Also, the variable name REFPOLICY_EXTRA_MODULES wasn't
> so good, since it really contains a list of directories, not a list of
> modules.
> 
> So I've changed that to:
> 
> REFPOLICY_EXTRA_MODULES_DIRS = $(call qstrip,$(BR2_REFPOLICY_EXTRA_MODULES_DIRS))
> 
> I've moved it a bit further up so that the REFPOLICY_MODULES variable
> can use it:
> 
> -       $(PACKAGES_SELINUX_MODULES)
> +       $(PACKAGES_SELINUX_MODULES) \
> +       $(foreach d,$(REFPOLICY_EXTRA_MODULES_DIRS),\
> +               $(basename $(notdir $(wildcard $(d)/*.te))))
> > +
> > +define REFPOLICY_COPY_MODULES
> > +     mkdir -p $(@D)/policy/modules/buildroot
> > +     rsync -au $(addsuffix /*,$(call qstrip,$(REFPOLICY_EXTRA_MODULES))) \
> > +             $(@D)/policy/modules/buildroot/
> > +     if [ ! -f $(@D)/policy/modules/buildroot/metadata.xml ]; then \
> > +             echo "<summary>Buildroot extra modules</summary>" > \
> > +                     $(@D)/policy/modules/buildroot/metadata.xml; \
> > +     fi
> > +endef
> 
> I've enclosed this REFPOLICY_COPY_MODULES macro definition in a:
> 
> ifneq ($(REFPOLICY_EXTRA_MODULES_DIRS),)
> ...
> endif 
> 
> condition.

If there are no extra modules provided by BR2_REFPOLICY_EXTRA_MODULES_DIRS
nor PACKAGES_SELINUX_EXTRA_MODULES_DIRS, REFPOLICY_EXTRA_MODULES_DIRS
would still be different than an empty string as it is now a list. As a
result, REFPOLICY_COPY_EXTRA_MODULES will always be called and the
'buildroot/metadata.xml' file will be installed. This would break the
build.

But using:

ifneq ($(qstrip,$(REFPOLICY_EXTRA_MODULES_DIRS)),)

would also not work as REFPOLICY_EXTRA_MODULES_DIRS would be expanded
too early, and PACKAGES_SELINUX_EXTRA_MODULES_DIRS might not already
contain all the packages selinux's custom modules.

That's why there were deferred calls to qstrip in the original patch.

Thanks!
Antoine
Thomas Petazzoni Sept. 4, 2020, 3:10 p.m. UTC | #3 
Hello,

On Fri, 04 Sep 2020 17:00:17 +0200
Antoine Tenart <antoine.tenart@bootlin.com> wrote:

> If there are no extra modules provided by BR2_REFPOLICY_EXTRA_MODULES_DIRS
> nor PACKAGES_SELINUX_EXTRA_MODULES_DIRS, REFPOLICY_EXTRA_MODULES_DIRS
> would still be different than an empty string as it is now a list.

There is no such thing as a "list" in make. Everything is string, and
space-separated words in a string can somehow be manipulated as a list.

> As a result, REFPOLICY_COPY_EXTRA_MODULES will always be called and
> the 'buildroot/metadata.xml' file will be installed. This would break
> the build.
> 
> But using:
> 
> ifneq ($(qstrip,$(REFPOLICY_EXTRA_MODULES_DIRS)),)

But indeed, there will always be a space in
REFPOLICY_EXTRA_MODULES_DIRS. So I guess the most logical change to do
is:

REFPOLICY_EXTRA_MODULES_DIRS = \
	$(strip \
		$(call qstrip,$(BR2_REFPOLICY_EXTRA_MODULES_DIRS)) \
		$(PACKAGES_SELINUX_EXTRA_MODULES_DIRS))

The below snippet of Makefile illustrates that:

# Strip quotes and then whitespaces
qstrip = $(strip $(subst ",,$(1)))
#"))

BR2_REFPOLICY_EXTRA_MODULES_DIRS = ""
PACKAGES_SELINUX_EXTRA_MODULES_DIRS =

REFPOLICY_EXTRA_MODULES_DIRS = \
	$(strip \
		$(call qstrip,$(BR2_REFPOLICY_EXTRA_MODULES_DIRS)) \
		$(PACKAGES_SELINUX_EXTRA_MODULES_DIRS))

all:
ifneq ($(REFPOLICY_EXTRA_MODULES_DIRS),)
	@echo "REFPOLICY_EXTRA_MODULES_DIRS is not empty"
else
	@echo "REFPOLICY_EXTRA_MODULES_DIRS is empty"
endif


> would also not work as REFPOLICY_EXTRA_MODULES_DIRS would be expanded
> too early, and PACKAGES_SELINUX_EXTRA_MODULES_DIRS might not already
> contain all the packages selinux's custom modules.

I'm not sure it's related to being expanded "too early". All those
variables are recursively expanded, i.e expanded at time of use.

Thomas
Antoine Tenart Sept. 4, 2020, 3:28 p.m. UTC | #4 
Hello,

Quoting Thomas Petazzoni (2020-09-04 17:10:09)
> On Fri, 04 Sep 2020 17:00:17 +0200
> Antoine Tenart <antoine.tenart@bootlin.com> wrote:
> 
> > As a result, REFPOLICY_COPY_EXTRA_MODULES will always be called and
> > the 'buildroot/metadata.xml' file will be installed. This would break
> > the build.
> > 
> > But using:
> > 
> > ifneq ($(qstrip,$(REFPOLICY_EXTRA_MODULES_DIRS)),)
> 
> But indeed, there will always be a space in
> REFPOLICY_EXTRA_MODULES_DIRS. So I guess the most logical change to do
> is:
> 
> REFPOLICY_EXTRA_MODULES_DIRS = \
>         $(strip \
>                 $(call qstrip,$(BR2_REFPOLICY_EXTRA_MODULES_DIRS)) \
>                 $(PACKAGES_SELINUX_EXTRA_MODULES_DIRS))
> 
> > would also not work as REFPOLICY_EXTRA_MODULES_DIRS would be expanded
> > too early, and PACKAGES_SELINUX_EXTRA_MODULES_DIRS might not already
> > contain all the packages selinux's custom modules.
> 
> I'm not sure it's related to being expanded "too early". All those
> variables are recursively expanded, i.e expanded at time of use.

Calling "ifneq ($(REFPOLICY_EXTRA_MODULES_DIRS),)" would expend the
variables in REFPOLICY_EXTRA_MODULES_DIRS, including
REFPOLICY_EXTRA_MODULES_DIRS. But at the time this is done not all the
packages would have been parsed and expanded, and
REFPOLICY_EXTRA_MODULES_DIRS will therefor not contain the full list of
"selinux" folders within packages.

Deferring its expansion to the configure/build target do ensure the
REFPOLICY_EXTRA_MODULES_DIRS list is complete, as all the other packages
would be parsed and expanded by then.

Thanks,
Antoine
diff mbox series

Patch

diff --git a/package/refpolicy/Config.in b/package/refpolicy/Config.in
index b50b2f09ff79..030b1e93c9bd 100644
--- a/package/refpolicy/Config.in
+++ b/package/refpolicy/Config.in
@@ -54,6 +54,16 @@  config BR2_PACKAGE_REFPOLICY_POLICY_STATE
 	default "enforcing" if BR2_PACKAGE_REFPOLICY_POLICY_STATE_ENFORCING
 	default "disabled" if BR2_PACKAGE_REFPOLICY_POLICY_STATE_DISABLED
 
+config BR2_REFPOLICY_EXTRA_MODULES_DIRS
+	string "Extra modules directories"
+	help
+	  Specify directories containing SELinux modules that will be build
+	  in the SELinux policy. The modules will be automatically enabled in
+	  the policy.
+
+	  Each of those directories must contain the SELinux policy .fc, .if
+	  and .te files directly at the top-level, with no sub-directories.
+
 endif
 
 comment "refpolicy needs a toolchain w/ threads"
diff --git a/package/refpolicy/refpolicy.mk b/package/refpolicy/refpolicy.mk
index c29912a53b0b..edbb5a228f55 100644
--- a/package/refpolicy/refpolicy.mk
+++ b/package/refpolicy/refpolicy.mk
@@ -46,7 +46,26 @@  REFPOLICY_MODULES = \
 	sysnetwork \
 	unconfined \
 	userdomain \
-	$(PACKAGES_SELINUX_MODULES)
+	$(PACKAGES_SELINUX_MODULES) \
+	$(foreach d,$(call qstrip,$(REFPOLICY_EXTRA_MODULES)),\
+		$(basename $(notdir $(wildcard $(d)/*.te))))
+
+# Allow to provide out-of-tree SELinux modules in addition to the ones in the
+# refpolicy.
+REFPOLICY_EXTRA_MODULES = $(BR2_REFPOLICY_EXTRA_MODULES_DIRS)
+$(foreach dir,$(call qstrip,$(BR2_REFPOLICY_EXTRA_MODULES_DIRS)),\
+	$(if $(wildcard $(dir)),,\
+		$(error BR2_REFPOLICY_EXTRA_MODULES_DIRS contains nonexistent directory $(dir))))
+
+define REFPOLICY_COPY_MODULES
+	mkdir -p $(@D)/policy/modules/buildroot
+	rsync -au $(addsuffix /*,$(call qstrip,$(REFPOLICY_EXTRA_MODULES))) \
+		$(@D)/policy/modules/buildroot/
+	if [ ! -f $(@D)/policy/modules/buildroot/metadata.xml ]; then \
+		echo "<summary>Buildroot extra modules</summary>" > \
+			$(@D)/policy/modules/buildroot/metadata.xml; \
+	fi
+endef
 
 # In the context of a monolithic policy enabling a piece of the policy as
 # 'base' or 'module' is equivalent, so we enable them as 'base'.
@@ -72,6 +91,8 @@  define REFPOLICY_CONFIGURE_CMDS
 endef
 
 define REFPOLICY_BUILD_CMDS
+	$(if $(call qstrip,$(REFPOLICY_EXTRA_MODULES)),\
+		$(REFPOLICY_COPY_MODULES))
 	$(REFPOLICY_MAKE) -C $(@D) DESTDIR=$(STAGING_DIR) bare conf
 	$(REFPOLICY_CONFIGURE_MODULES)
 endef