Re: [PATCH 0/8] checkpolicy, libsepol: add prefix/suffix matching to filename type transitions

From: Juraj Marcin <juraj@jurajmarcin.com>
To: James Carter <jwcart2@gmail.com>
Subject: Re: [PATCH 0/8] checkpolicy, libsepol: add prefix/suffix matching to filename type transitions
Date: Fri, 2 Jun 2023 01:59:50 +0200	[thread overview]
Message-ID: <20230601235948.nv6ypxqqlm3hkl2n@jmarcin-t14s-01> (raw)
In-Reply-To: <CAP+JOzS1nhxF8EttikRtYBW2QtW=ck+P4mC2dovgP4kp-g9WQQ@mail.gmail.com>

On 2023-06-01 17:03, James Carter wrote:
> I did a bit of testing.
> 
> These all work:
> ~/local/usr/bin/secil2tree -o test_01.cil.tree test_01.cil
> ~/local/usr/bin/secilc -o test_01.cil.bin test_01.cil
> ~/local/usr/bin/checkpolicy -C -b -o test_01.cil.bin.cil test_01.cil.bin
> 
> /local/usr/bin/checkpolicy -o test_01.conf.bin test_01.conf
> ~/local/usr/bin/checkpolicy -C -b -o test_01.conf.bin.cil test_01.conf.bin
> 
> ~/local/usr/bin/secil2conf -o test_01.cil.conf test_01.cil
> ~/local/usr/bin/checkpolicy -o test_01.cil.conf.bin test_01.cil.conf
> 
> ~/local/usr/bin/checkmodule -o base_01.mod base_01.te
> ~/local/usr/bin/checkmodule -m -o mod_02.mod mod_02.te
> ~/local/usr/bin/semodule_package -o base_01.pp -m base_01.mod
> ~/local/usr/bin/semodule_package -o mod_02.pp -m mod_02.mod
> ~/local/usr/bin/semodule_link -o test_01.pp.lnk base_01.pp mod_02.pp
> ~/local/usr/bin/semodule_expand test_01.pp.lnk test_01.pp.bin
> 
> These do not work:
> ~/local/usr/bin/checkpolicy -F -b -o test_01.cil.bin.conf test_01.cil.bin
> ~/local/usr/bin/checkpolicy -F -b -o test_01.conf.bin.conf test_01.conf.bin
> ~/local/usr/bin/checkpolicy -F -b -o test_01.pp.bin.conf test_01.pp.bin
> 
> It appears something is wrong with translating the binary to a
> policy.conf, but I haven't had a chance to look deeper.

Thank you for your feedback! I have managed to find the issue in the 6th
patch of the series (checkpolicy, libsepol: add prefix/suffix support to
kernel policy) where I forgot to increment the number of arguments.

With this fix all the mentioned tests work (I will also include it in v2
later):

diff --git a/libsepol/src/kernel_to_cil.c b/libsepol/src/kernel_to_cil.c
index fd036fa9..0d5fef33 100644
--- a/libsepol/src/kernel_to_cil.c
+++ b/libsepol/src/kernel_to_cil.c
@@ -1721,7 +1721,7 @@ static int name_trans_to_strs_helper(hashtab_key_t k, hashtab_datum_t d, void *a
                sepol_log_err("Unknown name match type: %" PRIu8, args->match);
                return SEPOL_ERR;
        }
-       return strs_create_and_add(args->strs, "(%s %s %s %s \"%s\"%s %s)", 6,
+       return strs_create_and_add(args->strs, "(%s %s %s %s \"%s\"%s %s)", 7,
                                   args->flavor, args->src, args->tgt,
                                   args->class, name, match_str,
                                   args->pdb->p_type_val_to_name[*otype - 1]);
diff --git a/libsepol/src/kernel_to_conf.c b/libsepol/src/kernel_to_conf.c
index 50fad1fb..e58eb67d 100644
--- a/libsepol/src/kernel_to_conf.c
+++ b/libsepol/src/kernel_to_conf.c
@@ -1699,7 +1699,7 @@ static int name_trans_to_strs_helper(hashtab_key_t k, hashtab_datum_t d, void *a
                sepol_log_err("Unknown name match type: %" PRIu8, args->match);
                return SEPOL_ERR;
        }
-       return strs_create_and_add(args->strs, "%s %s %s:%s %s \"%s\"%s;", 6,
+       return strs_create_and_add(args->strs, "%s %s %s:%s %s \"%s\"%s;", 7,
                                   args->flavor, args->src, args->tgt,
                                   args->class,
                                   args->pdb->p_type_val_to_name[*otype - 1],


-- 
Juraj Marcin

> 
> I've attached the very simple test policies I used.
> 
> Thanks,
> Jim
> 
> On Wed, May 31, 2023 at 7:51 AM Juraj Marcin <juraj@jurajmarcin.com> wrote:
> >
> > Currently, filename transitions are stored separately from other type
> > enforcement rules and only support exact name matching. However, in
> > practice, the names contain variable parts. This leads to many
> > duplicated rules in the policy that differ only in the part of the name,
> > or it is even impossible to cover all possible combinations.
> >
> > This series implements equivalent changes made by this kernel patch
> > series [1].
> >
> > First, this series of patches moves the filename transitions to be part
> > of the avtab and avrule structures. This not only makes the
> > implementation of prefix/suffix matching and future enhancements easier,
> > but also reduces the technical debt regarding the filename transitions.
> > Next, the last three patches implement the support for prefix/suffix
> > name matching itself by extending the structures added in previous
> > patches in this series and adding the support to CIL in the last of the
> > triple.
> >
> > Even though, moving everything to avtab increases the memory usage and
> > the size of the binary policy itself and thus the loading time, the
> > ability to match the prefix or suffix of the name will reduce the
> > overall number of rules in the policy which should mitigate this issue.
> >
> > [1]: https://lore.kernel.org/selinux/20230531112927.1957093-1-juraj@jurajmarcin.com/
> >
> > Juraj Marcin (8):
> >   checkpolicy, libsepol: move transition to separate structure in avtab
> >   checkpolicy, libsepol: move filename transitions to avtab
> >   checkpolicy, libsepol: move filename transition rules to avrule
> >   libsepol: implement new kernel binary format for avtab
> >   libsepol: implement new module binary format of avrule
> >   checkpolicy, libsepol: add prefix/suffix support to kernel policy
> >   checkpolicy, libsepol: add prefix/suffix support to module policy
> >   libsepol/cil: add support for prefix/suffix filename transtions to CIL
> >
> >  checkpolicy/checkmodule.c                  |   9 +
> >  checkpolicy/module_compiler.c              |  12 -
> >  checkpolicy/module_compiler.h              |   1 -
> >  checkpolicy/policy_define.c                | 211 +-----
> >  checkpolicy/policy_define.h                |   3 +-
> >  checkpolicy/policy_parse.y                 |  15 +-
> >  checkpolicy/policy_scan.l                  |   6 +
> >  checkpolicy/test/dismod.c                  |  39 +-
> >  checkpolicy/test/dispol.c                  | 106 +--
> >  libsepol/cil/src/cil.c                     |   8 +
> >  libsepol/cil/src/cil_binary.c              |  63 +-
> >  libsepol/cil/src/cil_build_ast.c           |  25 +-
> >  libsepol/cil/src/cil_copy_ast.c            |   1 +
> >  libsepol/cil/src/cil_internal.h            |   5 +
> >  libsepol/cil/src/cil_policy.c              |  17 +-
> >  libsepol/cil/src/cil_resolve_ast.c         |  10 +
> >  libsepol/cil/src/cil_write_ast.c           |   2 +
> >  libsepol/include/sepol/policydb/avtab.h    |  19 +-
> >  libsepol/include/sepol/policydb/hashtab.h  |  14 +
> >  libsepol/include/sepol/policydb/policydb.h |  50 +-
> >  libsepol/src/avrule_block.c                |   1 -
> >  libsepol/src/avtab.c                       | 336 +++++++++-
> >  libsepol/src/conditional.c                 |   6 +-
> >  libsepol/src/expand.c                      | 149 ++---
> >  libsepol/src/kernel_to_cil.c               | 182 ++----
> >  libsepol/src/kernel_to_common.h            |  10 +
> >  libsepol/src/kernel_to_conf.c              | 178 ++----
> >  libsepol/src/link.c                        |  57 +-
> >  libsepol/src/module_to_cil.c               |  86 +--
> >  libsepol/src/optimize.c                    |   8 +
> >  libsepol/src/policydb.c                    | 479 +++-----------
> >  libsepol/src/policydb_validate.c           | 100 +--
> >  libsepol/src/services.c                    |   5 +-
> >  libsepol/src/write.c                       | 712 ++++++++++++++++-----
> >  34 files changed, 1534 insertions(+), 1391 deletions(-)
> >
> > --
> > 2.40.0
> >
