| Message ID | 20250402010146.898864-1-inseob@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [RESEND] libsepol: Print line markers also for allow rules | expand |
On Tue, Apr 1, 2025 at 9:04 PM Inseob Kim <inseob@google.com> wrote: > > Currently, only line markers for neverallow rules are printed. This > makes people difficult to debug a neverallow failure with cil files > generated by checkpolicy. > > This change additionally prints line markers for allow and allowxperm > statements to make debugging easier. > > Signed-off-by: Inseob Kim <inseob@google.com> > --- > libsepol/src/module_to_cil.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/libsepol/src/module_to_cil.c b/libsepol/src/module_to_cil.c > index ae9a2b5d..76fe4739 100644 > --- a/libsepol/src/module_to_cil.c > +++ b/libsepol/src/module_to_cil.c > @@ -1196,7 +1196,7 @@ static int avrule_list_to_cil(int indent, struct policydb *pdb, struct avrule *a > struct type_set *ts; > > for (avrule = avrule_list; avrule != NULL; avrule = avrule->next) { > - if ((avrule->specified & (AVRULE_NEVERALLOW|AVRULE_XPERMS_NEVERALLOW)) && > + if ((avrule->specified & (AVRULE_ALLOWED|AVRULE_XPERMS_ALLOWED|AVRULE_NEVERALLOW|AVRULE_XPERMS_NEVERALLOW)) && > avrule->source_filename) { > cil_println(0, ";;* lmx %lu %s\n",avrule->source_line, avrule->source_filename); > } The problem is that currently line marks (which are converted to <src_info> rules when parsed) cannot be in booleanif statements, but allow rules can be, so this can create cil files that will not compile. I suspect that that restriction was not intentional and can be relaxed, but I don't remember. The other issue is that this will produce a lot of line marks for any real policy. A lot of line marks. It would be nice if this behavior could be made optional. Jim > @@ -1264,7 +1264,7 @@ static int avrule_list_to_cil(int indent, struct policydb *pdb, struct avrule *a > names_destroy(&snames, &num_snames); > names_destroy(&tnames, &num_tnames); > > - if ((avrule->specified & (AVRULE_NEVERALLOW|AVRULE_XPERMS_NEVERALLOW)) && > + if ((avrule->specified & (AVRULE_ALLOWED|AVRULE_XPERMS_ALLOWED|AVRULE_NEVERALLOW|AVRULE_XPERMS_NEVERALLOW)) && > avrule->source_filename) { > cil_println(0, ";;* lme\n"); > } > -- > 2.49.0.rc1.451.g8f38331e32-goog > >
On Thu, Apr 3, 2025 at 4:43 AM James Carter <jwcart2@gmail.com> wrote: > > On Tue, Apr 1, 2025 at 9:04 PM Inseob Kim <inseob@google.com> wrote: > > > > Currently, only line markers for neverallow rules are printed. This > > makes people difficult to debug a neverallow failure with cil files > > generated by checkpolicy. > > > > This change additionally prints line markers for allow and allowxperm > > statements to make debugging easier. > > > > Signed-off-by: Inseob Kim <inseob@google.com> > > --- > > libsepol/src/module_to_cil.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/libsepol/src/module_to_cil.c b/libsepol/src/module_to_cil.c > > index ae9a2b5d..76fe4739 100644 > > --- a/libsepol/src/module_to_cil.c > > +++ b/libsepol/src/module_to_cil.c > > @@ -1196,7 +1196,7 @@ static int avrule_list_to_cil(int indent, struct policydb *pdb, struct avrule *a > > struct type_set *ts; > > > > for (avrule = avrule_list; avrule != NULL; avrule = avrule->next) { > > - if ((avrule->specified & (AVRULE_NEVERALLOW|AVRULE_XPERMS_NEVERALLOW)) && > > + if ((avrule->specified & (AVRULE_ALLOWED|AVRULE_XPERMS_ALLOWED|AVRULE_NEVERALLOW|AVRULE_XPERMS_NEVERALLOW)) && > > avrule->source_filename) { > > cil_println(0, ";;* lmx %lu %s\n",avrule->source_line, avrule->source_filename); > > } > > The problem is that currently line marks (which are converted to > <src_info> rules when parsed) cannot be in booleanif statements, but > allow rules can be, so this can create cil files that will not > compile. > I suspect that that restriction was not intentional and can be > relaxed, but I don't remember. I tried supporting markers within booleanif statements and it seems to work with my tiny test. PoC patch: diff --git a/libsepol/cil/src/cil_binary.c b/libsepol/cil/src/cil_binary.c index 3d920182..90745110 100644 --- a/libsepol/cil/src/cil_binary.c +++ b/libsepol/cil/src/cil_binary.c @@ -2121,6 +2121,7 @@ static int __cil_cond_to_policydb_helper(struct cil_tree_node *node, __attribute break; case CIL_CALL: case CIL_TUNABLEIF: + case CIL_SRC_INFO: break; default: cil_tree_log(node, CIL_ERR, "Invalid statement within booleanif"); diff --git a/libsepol/cil/src/cil_build_ast.c b/libsepol/cil/src/cil_build_ast.c index 19fbb04e..619cd894 100644 --- a/libsepol/cil/src/cil_build_ast.c +++ b/libsepol/cil/src/cil_build_ast.c @@ -6158,6 +6158,7 @@ static int check_for_illegal_statement(struct cil_tree_node *parse_current, stru parse_current->data != CIL_KEY_AUDITALLOW && parse_current->data != CIL_KEY_TYPETRANSITION && parse_current->data != CIL_KEY_TYPECHANGE && + parse_current->data != CIL_KEY_SRC_INFO && parse_current->data != CIL_KEY_TYPEMEMBER) { if (((struct cil_booleanif*)args->boolif->data)->preserved_tunable) { cil_tree_log(parse_current, CIL_ERR, "%s is not allowed in tunableif being treated as a booleanif", (char *)parse_current->data); diff --git a/libsepol/cil/src/cil_resolve_ast.c b/libsepol/cil/src/cil_resolve_ast.c index da8863c4..d0d53b1d 100644 --- a/libsepol/cil/src/cil_resolve_ast.c +++ b/libsepol/cil/src/cil_resolve_ast.c @@ -3848,7 +3848,8 @@ static int __cil_resolve_ast_node_helper(struct cil_tree_node *node, uint32_t *f node->flavor != CIL_CONDBLOCK && node->flavor != CIL_AVRULE && node->flavor != CIL_TYPE_RULE && - node->flavor != CIL_NAMETYPETRANSITION) { + node->flavor != CIL_NAMETYPETRANSITION && + node->flavor != CIL_SRC_INFO) { rc = SEPOL_ERR; } else if (node->flavor == CIL_AVRULE) { struct cil_avrule *rule = node->data; diff --git a/libsepol/cil/src/cil_verify.c b/libsepol/cil/src/cil_verify.c index 9621a247..0b740c85 100644 --- a/libsepol/cil/src/cil_verify.c +++ b/libsepol/cil/src/cil_verify.c @@ -1175,6 +1175,9 @@ static int __cil_verify_booleanif_helper(struct cil_tree_node *node, __attribute booleanif statements if they don't have "*" as the file. We can't check that here. Or at least we won't right now. */ break; + case CIL_SRC_INFO: + //Fall through + break; default: { const char * flavor = cil_node_to_string(node); if (bif->preserved_tunable) { Snippet of test cil: (boolean foobarbaz true) (booleanif foobarbaz (true ;;* lmx 999 system/sepolicy/private/booleanif.te (allow domain system_file (file (open))) ;;* lme )) secilc neverallow check output: neverallow check failed at plat_sepolicy.cil:15155 from system/sepolicy/private/domain.te:1237 (neverallow base_typeattr_430 base_typeattr_438 (...)) <root> booleanif at plat_sepolicy.cil:9105 true at plat_sepolicy.cil:9105 allow at plat_sepolicy.cil:9108 from system/sepolicy/private/booleanif.te:999 (allow domain system_file (file (open))) I'll apply this in my next patchset. What do you think? > > The other issue is that this will produce a lot of line marks for any > real policy. A lot of line marks. It would be nice if this behavior > could be made optional. Ack, working on it. > > Jim > > > > @@ -1264,7 +1264,7 @@ static int avrule_list_to_cil(int indent, struct policydb *pdb, struct avrule *a > > names_destroy(&snames, &num_snames); > > names_destroy(&tnames, &num_tnames); > > > > - if ((avrule->specified & (AVRULE_NEVERALLOW|AVRULE_XPERMS_NEVERALLOW)) && > > + if ((avrule->specified & (AVRULE_ALLOWED|AVRULE_XPERMS_ALLOWED|AVRULE_NEVERALLOW|AVRULE_XPERMS_NEVERALLOW)) && > > avrule->source_filename) { > > cil_println(0, ";;* lme\n"); > > } > > -- > > 2.49.0.rc1.451.g8f38331e32-goog > > > > -- Inseob Kim | Software Engineer | inseob@google.com
While working on this change, I also found that
checkpolicy/policy_scan.l is a little broken.
#line[ ]1[ ]\"[^\n]*\" { set_source_file(yytext+9); }
#line[ ]{digit}+ {
errno = 0;
source_lineno = strtoul(yytext+6, NULL, 10) - 1;
if (errno) {
yywarn("source line number too big");
}
}
It assumes that the line marker with a filename always starts with
"#line 1 ...", but it isn't always the case if the file is
preprocessed and the very first line is skipped. We may want to fix
the rule like:
#line[ ]{digit}+[ ]\"[^\n]*\" { set_source_file(yytext+6); }
...
void set_source_file(const char *name)
{
// name points to "(number) (filename)".
// assign both source_lineno and source_file appropriately.
}
Let me apply that to my next patchset too.
diff --git a/libsepol/src/module_to_cil.c b/libsepol/src/module_to_cil.c index ae9a2b5d..76fe4739 100644 --- a/libsepol/src/module_to_cil.c +++ b/libsepol/src/module_to_cil.c @@ -1196,7 +1196,7 @@ static int avrule_list_to_cil(int indent, struct policydb *pdb, struct avrule *a struct type_set *ts; for (avrule = avrule_list; avrule != NULL; avrule = avrule->next) { - if ((avrule->specified & (AVRULE_NEVERALLOW|AVRULE_XPERMS_NEVERALLOW)) && + if ((avrule->specified & (AVRULE_ALLOWED|AVRULE_XPERMS_ALLOWED|AVRULE_NEVERALLOW|AVRULE_XPERMS_NEVERALLOW)) && avrule->source_filename) { cil_println(0, ";;* lmx %lu %s\n",avrule->source_line, avrule->source_filename); } @@ -1264,7 +1264,7 @@ static int avrule_list_to_cil(int indent, struct policydb *pdb, struct avrule *a names_destroy(&snames, &num_snames); names_destroy(&tnames, &num_tnames); - if ((avrule->specified & (AVRULE_NEVERALLOW|AVRULE_XPERMS_NEVERALLOW)) && + if ((avrule->specified & (AVRULE_ALLOWED|AVRULE_XPERMS_ALLOWED|AVRULE_NEVERALLOW|AVRULE_XPERMS_NEVERALLOW)) && avrule->source_filename) { cil_println(0, ";;* lme\n"); }
Currently, only line markers for neverallow rules are printed. This makes people difficult to debug a neverallow failure with cil files generated by checkpolicy. This change additionally prints line markers for allow and allowxperm statements to make debugging easier. Signed-off-by: Inseob Kim <inseob@google.com> --- libsepol/src/module_to_cil.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)