Re: [RFC PATCH 0/9] Add CIL Deny Rule

[Daniel Burgener <dburgener@linux.microsoft.com>]

On 12/15/2022 4:34 PM, James Carter wrote:
> I don't expect this to be part of the upcoming userspace release,
> but I did want to see if this is going to be what Cascade needs.

Thank you!  I've been playing with this series locally all morning and 
so far it mostly works as expected.  The only minor surprise so far is 
if I do something like:

(type my_type1)
(type my_type2)
(type my_type3)
(typeattributeset my_attr (my_type1 my_type2 my_type3))
(allow my_attr my_attr (process (signal signull)))
(deny my_type1 my_attr (process (signal)))

I get rules like this:

$ sesearch -A -s my_attr policy.33
allow deny_rule_attr_11 my_attr:process { signal signull };
allow my_attr my_attr:process signull;

Since deny_rule_attr_11 is a subset of my_attr (specifically, my_type2 
and my_type3), both of those types get the "signull" permission from the 
second rule on the attribute, and only strictly need the "signal" 
permission (which my_type1 doesn't get)

It's obviously not a real problem, since it's just redundant policy and 
both versions are functionally equivalent.  It's just a little weird 
browsing the rules using sesearch, particularly with large permission 
sets (My first attribute test involved using "all" on the file class and 
denying one permission from a type, and it was mildly challenging to 
manually verify the results looking at the allow rules in a large list.)

Anyways, besides that minor issue everything I've tried so far has 
worked well and the overall concept seems sensible and helpful. 
Although, I still need to get my head around the specifics of the 
attribute handling, and I haven't looked thoroughly at the code yet.

I intend to do more thorough testing and code review, but I don't expect 
I'll have time before the holidays, so that will likely come in January. 
  Since I don't have time for anything more thorough now, hopefully some 
first impressions are helpful in the interim.

-Daniel

> 
> This series of patches implements a deny rule in CIL. A deny rule will remove
> the stated permissions in it from the policy. CIL does this by searching for
> allow rules that match the deny rule and then writing new allow rules that
> correspond to the matched allow rule with the permissions from the deny rule
> removed. The rule uses the same syntax as an allow rule, but with "deny"
> instead of "allow".
> 
>    (deny SRC TGT (CLASS (PERMS)))
> 
> Deny rules are processed during post processing (after the AST is resolved,
> but before the binary policy is written). This means that neverallow checking
> is done after deny rules are resolved. Deny rules are complimentary to
> neverallow checking. When an allow rule is found that matches, a deny rule
> removes permissions while a neverallow rule reports an error.
> 
> Patch 4 is biggest and most complex since it is the one doing the processing.
> 
> James Carter (9):
>    libsepol/cil: Parse and add deny rule to AST, but do not process
>    libsepol/cil: Add cil_list_is_empty macro
>    libsepol/cil: Add cil_tree_remove_node function
>    libsepol/cil: Process deny rules
>    libsepol/cil: Add cil_write_post_ast function
>    libsepol: Export the cil_write_post_ast function
>    secilc/secil2tree: Add option to write CIL AST after post processing
>    secilc/test: Add a deny rule test
>    secilc/docs: Add deny rule to CIL documentation
> 
>   libsepol/cil/include/cil/cil.h         |   1 +
>   libsepol/cil/src/cil.c                 |  68 ++
>   libsepol/cil/src/cil_build_ast.c       |  56 ++
>   libsepol/cil/src/cil_build_ast.h       |   2 +
>   libsepol/cil/src/cil_copy_ast.c        |  19 +
>   libsepol/cil/src/cil_copy_ast.h        |   1 +
>   libsepol/cil/src/cil_deny.c            | 957 +++++++++++++++++++++++++
>   libsepol/cil/src/cil_deny.h            |  34 +
>   libsepol/cil/src/cil_flavor.h          |   1 +
>   libsepol/cil/src/cil_internal.h        |  10 +
>   libsepol/cil/src/cil_list.h            |   3 +
>   libsepol/cil/src/cil_post.c            |   7 +
>   libsepol/cil/src/cil_reset_ast.c       |   8 +
>   libsepol/cil/src/cil_resolve_ast.c     |  44 ++
>   libsepol/cil/src/cil_resolve_ast.h     |   1 +
>   libsepol/cil/src/cil_tree.c            |  27 +
>   libsepol/cil/src/cil_tree.h            |   1 +
>   libsepol/cil/src/cil_verify.c          |   9 +
>   libsepol/cil/src/cil_write_ast.c       |  10 +
>   libsepol/cil/src/cil_write_ast.h       |   1 +
>   libsepol/src/libsepol.map.in           |   5 +
>   secilc/docs/cil_access_vector_rules.md |  68 ++
>   secilc/secil2tree.c                    |   8 +-
>   secilc/test/deny_rule_test.cil         | 384 ++++++++++
>   24 files changed, 1724 insertions(+), 1 deletion(-)
>   create mode 100644 libsepol/cil/src/cil_deny.c
>   create mode 100644 libsepol/cil/src/cil_deny.h
>   create mode 100644 secilc/test/deny_rule_test.cil
>