| Message ID | 20221215213429.998948-1-jwcart2@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Add CIL Deny Rule | expand |
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 >
On Fri, Dec 16, 2022 at 1:52 PM Daniel Burgener <dburgener@linux.microsoft.com> wrote: > > 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.) > I knew that I was creating some redundancy in the rules, but I was trying to eliminate the special cases. Now that I have something that appears to be working, it will probably be worth it to go back and see how messy it would be to remove some of the redundancy. > 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. > I am going to be on vacation for the last two weeks of December, so I won't be getting back to this until January anyway. Thanks for the quick look and comments! Jim > -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 > > >