Re: [PATCH V2] Ensure correct monolithic binary policy is loaded

On 12/20/20 11:40 AM, Richard Haines wrote:
> On Sun, 2020-12-20 at 10:01 -0500, Chris PeBenito wrote:
>> On 12/20/20 7:31 AM, Richard Haines wrote:
>>> On Sat, 2020-12-19 at 15:49 -0500, Chris PeBenito wrote:
>>>> On 12/18/20 10:03 AM, Richard Haines wrote:
>>>>> When building a monolithic policy with 'make load', the
>>>>> selinux_config(5) file 'SELINUXTYPE' entry determines what
>>>>> policy
>>>>> is loaded as load_policy(8) does not take a path value (it
>>>>> always
>>>>> loads
>>>>> the active system policy as defined by /etc/selinux/config).
>>>>>
>>>>> Currently it is possible to load the wrong binary policy, for
>>>>> example if
>>>>> the Reference Policy source is located at:
>>>>> /etc/selinux/refpolicy
>>>>> and the /etc/selinux/config file has the following entry:
>>>>> SELINUXTYPE=targeted
>>>>> Then the /etc/selinux/targeted/policy/policy.<ver> is loaded
>>>>> when
>>>>> 'make load' is executed.
>>>>>
>>>>> Another example is that if the Reference Policy source is
>>>>> located
>>>>> at:
>>>>> /tmp/custom-rootfs/etc/selinux/refpolicy
>>>>> and the /etc/selinux/config file has the following entry:
>>>>> SELINUXTYPE=refpolicy
>>>>> Then the /etc/selinux/refpolicy/policy/policy.<ver> is loaded
>>>>> when
>>>>> 'make DESTDIR=/tmp/custom-rootfs load' is executed (not the
>>>>> /tmp/custom-rootfs/etc/selinux/refpolicy/policy/policy.<ver>
>>>>> that
>>>>> the
>>>>> developer thought would be loaded).
>>>>>
>>>>> Resolve these issues by using selinux_path(3) to resolve the
>>>>> policy
>>>>> root,
>>>>> then checking the selinux_config(5) file for the appropriate
>>>>> SELINUXTYPE
>>>>> entry.
>>>>>
>>>>> Remove the '@touch $(tmpdir)/load' line as the file is never
>>>>> referenced.
>>>>>
>>>>> Signed-off-by: Richard Haines <[email protected]>
>>>>> ---
>>>>> V2 Changes: Use $(error .. instead of NO_LOAD logic. Use python
>>>>> script to
>>>>> find selinux path not sestatus. Reword error messages.
>>>>>
>>>>>     Makefile                |  1 +
>>>>>     Rules.monolithic        | 15 ++++++++++++++-
>>>>>     support/selinux_path.py | 13 +++++++++++++
>>>>>     3 files changed, 28 insertions(+), 1 deletion(-)
>>>>>     create mode 100644 support/selinux_path.py
>>>>>
>>>>> diff --git a/Makefile b/Makefile
>>>>> index 6ba215f1..e49d43d0 100644
>>>>> --- a/Makefile
>>>>> +++ b/Makefile
>>>>> @@ -97,6 +97,7 @@ genxml := $(PYTHON) $(support)/segenxml.py
>>>>>     gendoc := $(PYTHON) $(support)/sedoctool.py
>>>>>     genperm := $(PYTHON) $(support)/genclassperms.py
>>>>>     policyvers := $(PYTHON) $(support)/policyvers.py
>>>>> +selinux_path := $(PYTHON) $(support)/selinux_path.py
>>>>>     fcsort := $(PYTHON) $(support)/fc_sort.py
>>>>>     setbools := $(AWK) -f $(support)/set_bools_tuns.awk
>>>>>     get_type_attr_decl := $(SED) -r -f
>>>>> $(support)/get_type_attr_decl.sed
>>>>> diff --git a/Rules.monolithic b/Rules.monolithic
>>>>> index a8ae98d1..cd065362 100644
>>>>> --- a/Rules.monolithic
>>>>> +++ b/Rules.monolithic
>>>>> @@ -42,6 +42,12 @@ vpath %.te $(all_layers)
>>>>>     vpath %.if $(all_layers)
>>>>>     vpath %.fc $(all_layers)
>>>>>     
>>>>> +# load_policy(8) loads policy from
>>>>> <SELINUX_PATH>/<SELINUXTYPE>/policy/policy.<ver>
>>>>> +# It does this by reading the <SELINUX_PATH>/config file and
>>>>> using
>>>>> the
>>>>> +# SELINUX_PATH/SELINUXTYPE entries to form the initial path.
>>>>> +SELINUX_PATH := $(shell $(selinux_path))
>>>>> +SELINUXTYPE := $(strip $(shell $(AWK) -F= '/^SELINUXTYPE/{
>>>>> print
>>>>> $$2 }' $(SELINUX_PATH)/config))
>>>>> +
>>>>>     ########################################
>>>>>     #
>>>>>     # default action: build policy locally
>>>>> @@ -91,9 +97,16 @@ endif
>>>>>     # Load the binary policy
>>>>>     #
>>>>>     reload $(tmpdir)/load: $(loadpath) $(fcpath) $(appfiles)
>>>>> +ifneq ($(SELINUXTYPE),$(NAME))
>>>>> +       $(error Cannot load policy as $(SELINUX_PATH)/config
>>>>> file
>>>>> contains SELINUXTYPE=$(SELINUXTYPE) - \
>>>>> +               Edit $(SELINUX_PATH)/config and set
>>>>> "SELINUXTYPE=$(NAME)")
>>>>> +endif
>>>>> +ifneq ($(topdir),$(SELINUX_PATH))
>>>>> +       $(error Cannot load policy as policy root MUST be
>>>>> $(SELINUX_PATH)/$(NAME) - \
>>>>> +               Current policy root is: $(topdir)/$(NAME))
>>>>> +endif
>>>>>           @echo "Loading $(NAME) $(loadpath)"
>>>>>           $(verbose) $(LOADPOLICY) -q $(loadpath)
>>>>> -       @touch $(tmpdir)/load
>>>>>     
>>>>>     ########################################
>>>>>     #
>>>>> diff --git a/support/selinux_path.py b/support/selinux_path.py
>>>>> new file mode 100644
>>>>> index 00000000..b663ff09
>>>>> --- /dev/null
>>>>> +++ b/support/selinux_path.py
>>>>> @@ -0,0 +1,13 @@
>>>>> +#!/usr/bin/env python3
>>>>> +
>>>>> +try:
>>>>> +    import warnings
>>>>> +    with warnings.catch_warnings():
>>>>> +        warnings.filterwarnings("ignore",
>>>>> category=PendingDeprecationWarning)
>>>>> +        import selinux
>>>>> +
>>>>> +    if selinux.is_selinux_enabled():
>>>>> +        # Strip the trailing '/'
>>>>> +        print(selinux.selinux_path()[:-1])
>>>>
>>>> Why not use selinux.selinux_binary_policy_path()? Then you don't
>>>> need
>>>> to parse
>>>> for SELINUXTYPE above.
>>>
>>> Because it has more information than needed. How about using
>>> selinux.selinux_policy_root() to give:
>>>
>>> # load_policy(8) loads policy from
>>> <POLICY_ROOT>/policy/policy.<ver>
>>> # It does this by reading the <SELINUX_PATH>/config file and using
>>> the
>>> # SELINUX_PATH/SELINUXTYPE entry to form the <POLICY_ROOT>.
>>> POLICY_ROOT := $(shell $(selinux_policy_root))
>>> SELINUXTYPE := $(shell basename $(POLICY_ROOT))
>>> SELINUX_PATH := $(shell dirname $(POLICY_ROOT))
>>
>> On second thought, isn't another way of doing the check:
>>
>> selinux.selinux_binary_policy_path() + "." + POLICYVER ==
>> $(loadpath)?
> 
> Yes I could, however that accounts for only one of the checks:
>     Cannot load policy as policy root MUST be ...
> 
> The other check uses SELINUXTYPE against $NAME to see if the config
> file has the correct entry:
>     Cannot load policy as $(SELINUX_PATH)/config file ...
> 
> I could just use one test and say that the policy must be based at
> /etc/selinux/$(NAME) plus the /etc/selinux/config file SELINUXTYPE
> entry must be $(NAME) ???

Since the Makefile creates $(loadpath) to be $(topdir)/$NAME/policy/policy.$ver, 
and selinux.selinux_binary_policy_path() takes into account SELINUXTYPE, the 
only reason the $topdir  ($DESTDIR/etc/selinux), would be a problem is if 
DESTDIR is set.  We should instead fail the load if DESTDIR is set.  If the user 
alters the $(topdir) of the Makefile to change /etc/selinux into something else, 
that is undefined behavior since lowercase variables aren't meant to be altered 
by users.  I see no reason to check that case.



-- 
Chris PeBenito