Add the functions cil_write_parse_ast(), cil_write_build_ast(),
and cil_write_resolve_ast() that can be used outside of libsepol.
These functions take a FILE pointer and CIL db, do the CIL build
through the desired phase, and then call cil_write_ast() to write
the CIL AST at that point.
Signed-off-by: James Carter <jwcart2@gmail.com>
The function cil_print_tree() has existed in cil_tree.c since the
beginning of the development of CIL and secilc. Unfortunately, it
used cil_log() at log level CIL_INFO to print out the AST and
has suffered greatly from bit rot.
Move the functions to write the CIL AST to cil_write_ast.c, update
the functions, and write the AST to the FILE pointer passed in as
an argument.
The function cil_write_ast() supports writing the CIL AST at three
different phases of the compiling a CIL policy. After parsing has
been done and the parse tree has been created, after the CIL AST
has been built, and after the CIL AST has been resolved.
Signed-off-by: James Carter <jwcart2@gmail.com>
In cil_compile(), CIL_INFO is being used as the priority for
error messages. This can make it difficult to tell when the error
occurred.
Instead, use CIL_ERR as the priority for the error messages in
cil_compile().
Signed-off-by: James Carter <jwcart2@gmail.com>
Use a consistent style for the error messages when an invalid
statement is found within tunableif, in-statement, block, macro,
optional, and booleanif blocks.
Signed-off-by: James Carter <jwcart2@gmail.com>
Since tunableifs are resolved before in-statements, do not allow
tuanble declarations in in-statements.
Since in-statements are the first flavor of statement that causes
part of the AST to be copied to another part, there is no need to
check the in-statements when resolving the AST.
Signed-off-by: James Carter <jwcart2@gmail.com>
When resolving the AST, tunable and in-statements are not considered
to be invalid in macros. This is inconsistent with the checks when
building the AST.
Add checks to make tunable and in-statments invalid in macros when
resolving the AST.
Signed-off-by: James Carter <jwcart2@gmail.com>
While there are some checks for invalid statements in an optional
block when resolving the AST, there are no checks when building the
AST.
OSS-Fuzz found the following policy which caused a null dereference
in cil_tree_get_next_path().
(blockinherit b3)
(sid SID)
(sidorder(SID))
(optional o
(ibpkeycon :(1 0)s)
(block b3
(filecon""block())
(filecon""block())))
The problem is that the blockinherit copies block b3 before
the optional block is disabled. When the optional is disabled,
block b3 is deleted along with everything else in the optional.
Later, when filecon statements with the same path are found an
error message is produced and in trying to find out where the block
was copied from, the reference to the deleted block is used. The
error handling code assumes (rightly) that if something was copied
from a block then that block should still exist.
It is clear that in-statements, blocks, and macros cannot be in an
optional, because that allows nodes to be copied from the optional
block to somewhere outside even though the optional could be disabled
later. When optionals are disabled the AST is reset and the
resolution is restarted at the point of resolving macro calls, so
anything resolved before macro calls will never be re-resolved.
This includes tunableifs, in-statements, blockinherits,
blockabstracts, and macro definitions. Tunable declarations also
cannot be in an optional block because they are needed to resolve
tunableifs. It should be fine to allow blockinherit statements in
an optional, because that is copying nodes from outside the optional
to the optional and if the optional is later disabled, everything
will be deleted anyway.
Check and quit with an error if a tunable declaration, in-statement,
block, blockabstract, or macro definition is found within an
optional when either building or resolving the AST.
Signed-off-by: James Carter <jwcart2@gmail.com>
When building the AST, typemember rules in a booleanif block will
be incorrectly called invalid. They are allowed in the kernel
policy and should be allowed in CIL.
When resolving the AST, if a neverallow rule is copied into a
booleanif block, it will not be considered an invalid rule, even
though this is not allowed in the kernel policy.
Update the booleanif checks to allow typemember rules and to not
allow neverallow rules in booleanifs. Also use the same form of
conditional for the checks when building and resolving the AST.
Signed-off-by: James Carter <jwcart2@gmail.com>
Reorder checks for invalid rules in the blocks of tunableifs,
in-statements, macros, and booleanifs when resolving the AST for
consistency.
Order the checks in the same order the blocks will be resolved in,
so tuanbleif, in-statement, macro, booleanif, and then non-block
rules.
Signed-off-by: James Carter <jwcart2@gmail.com>
When resolving the AST, block and optional stacks are used to
determine if the current rule being resolved is in a block or
an optional. There is no need to do this since the parent node
pointers can be used when exiting a block or an optional to
determine if resolution is still within a block or an optional.
When entering either a block or an optional, update the appropriate
tree node pointer. When finished with the last child of a block or
optional, set the appropriate pointer to NULL. If a parent of the
same kind is found when the parent node pointers are followed back
to the root node, then set the pointer to that tree node.
Signed-off-by: James Carter <jwcart2@gmail.com>
In order to find statements not allowed in tunableifs, in-statements,
macros, and booleanifs, there are tree node pointers that point to
each of these kinds of statements when its block is being parsed.
If the pointer is non-NULL, then the rule being parsed is in the block
of that kind of statement.
The tree node pointers were being updated at the wrong point which
prevented an invalid statement from being found if it was the first
statement in the block of a tunableif, in-statement, macro, or
booleanif.
Create a first child helper function for walking the parse tree and
in that function set the appropriate tree node pointer if the
current AST node is a tunableif, in-statement, macro, or booleanif.
This also makes the code symmetrical with the last child helper
where the tree node pointers are set to NULL.
Signed-off-by: James Carter <jwcart2@gmail.com>
Since parse_current, finished, and extra_args can never be NULL,
remove the useless check and directly assign local variables from
extra_args.
Signed-off-by: James Carter <jwcart2@gmail.com>
Reorder checks for invalid rules in the blocks of tunableifs,
in-statements, macros, and booleanifs when building the AST for
consistency.
Order the checks in the same order the blocks will be resolved in,
so tuanbleif, in-statement, macro, booleanif, and then non-block
rules.
Signed-off-by: James Carter <jwcart2@gmail.com>
In cil_gen_node(), after the declaration is added to the symbol
table, if the parent is a macro, then a check is made to ensure
the declaration does not shadow any of the macro's parameters.
This check also needs to be done when copying the AST.
Move the check for the shadowing of macro parameters to its own
function, cil_verify_decl_does_not_shadow_macro_parameter(), and
refactor cil_gen_node() and __cil_copy_node_helper() to use the
new function.
Signed-off-by: James Carter <jwcart2@gmail.com>
The functionality of adding a declaration to a symbol table is also
needed in __cil_copy_node_helper() and not just cil_gen_node().
Create a new function called cil_add_decl_to_symtab() to add a
declaration to a symtab and refactor cil_gen_node() and
__cil_copy_node_helper() to use the new function.
By using the new function, __cil_copy_node_helper() will now allow
duplicate declarations when appropriate.
Signed-off-by: James Carter <jwcart2@gmail.com>
Change the name of cil_is_datum_multiple_decl() to
cil_allow_multiple_decls() and make it static. The new function
takes the CIL db and the flavors of the old and new datum as
arguments. Also, put all of the logic of determining if multiple
declarations are allowed into the new function. Finally, update
the call from cil_gen_node().
Signed-off-by: James Carter <jwcart2@gmail.com>
The following policy will cause a segfault:
(class CLASS (PERM))
(class C (P1 P2 P3))
(classorder (CLASS C))
(sid SID)
(sidorder (SID))
(user USER)
(role ROLE)
(type TYPE)
(category CAT)
(categoryorder (CAT))
(sensitivity SENS)
(sensitivityorder (SENS))
(sensitivitycategory SENS (CAT))
(allow TYPE self (CLASS (PERM)))
(roletype ROLE TYPE)
(userrole USER ROLE)
(userlevel USER (SENS))
(userrange USER ((SENS)(SENS (CAT))))
(sidcontext SID (USER ROLE TYPE ((SENS)(SENS))))
(classmap CM (PM1 PM2 PM3))
(classmapping CM PM1 (C (P1)))
(classmapping CM PM2 (C (P2)))
(classmapping CM PM3 (C (P3)))
(allow TYPE self (CM (and (all) (not PM2))))
The problem is that, while permission expressions are allowed for
normal classes, map classes are expected to only have permission
lists and no check is done to verify that only a permission list
is being used.
When the above policy is parsed, the "and" and "all" are seen as
expression operators, but when the map permissions are converted to
normal class and permissions, the permission expression is assumed
to be a list of datums and since the operators are not datums a
segfault is the result.
There is no reason to limit map classes to only using a list of
permissions and, in fact, it would be better to be able to use them
in the same way normal classes are used.
Allow permissions expressions to be used for map classes by first
evaluating the permission expression and then converting the
resulting list to normal classes and permissions.
Signed-off-by: James Carter <jwcart2@gmail.com>
When CIL parses sets or conditional expressions, any identifier that
matches an operator name will always be taken as an operator. If a
declaration has the same name as an operator, then there is the
possibility of causing either confusion or a syntax error if it is
used in an expression. The potential for problems is much greater
than any possible advantage in allowing a declaration to share the
name of a reserved word.
Create a new function, __cil_is_reserved_name() that is called when
an identifier is declared and its name is being validated. In this
function, check if the declaration has the same name as a reserved
word for an expression operator that can be used with the identifer's
flavor and exit with an error if it does.
Also, move the check for types, type aliases, and type attributes
matching the reserved word "self" to this new function.
Finally, change the name of the function __cil_verify_name() to
cil_verify_name(), since this function is neither static nor a
helper function.
Signed-off-by: James Carter <jwcart2@gmail.com>
In constraint expressions u1, u3, r1, r3, t1, and t3 are never
allowed on the right side of an expression, but there were no checks
to verify that they were not used on the right side. The result was
that the expression "(eq t1 t1)" would be silently turned into
"(eq t1 t2)" when the binary policy was created.
Verify that u1, u3, r1, r3, t1, and t3 are not used on the right
side of a constraint expression.
Signed-off-by: James Carter <jwcart2@gmail.com>
The class field of a struct cil_classperms points to the class looked
up in the symbol table, so that field should be set to NULL when
the cil_classperms is reset.
Set the class field to NULL when resetting the struct cil_classperms.
Signed-off-by: James Carter <jwcart2@gmail.com>
In struct cil_classperms_set, the set field is a pointer to a
struct cil_classpermission which is looked up in the symbol table.
Since the cil_classperms_set does not create the cil_classpermission,
it should not reset it.
Set the set field to NULL instead of resetting the classpermission
that it points to.
Signed-off-by: James Carter <jwcart2@gmail.com>
Map perms share the same struct as regular perms, but only the
map perms use the classperms field. This field is a pointer to a
list of classperms that is created and added to when resolving
classmapping rules, so the map permission doesn't own any of the
data in the list and this list should be destroyed when the AST is
reset.
When resetting a perm, destroy the classperms list without destroying
the data in the list.
Signed-off-by: James Carter <jwcart2@gmail.com>
Nicolas Iooss reports:
A few months ago, OSS-Fuzz found a crash in the CIL compiler, which
got reported as
https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=28648 (the title
is misleading, or is caused by another issue that conflicts with the
one I report in this message). Here is a minimized CIL policy which
reproduces the issue:
(class CLASS (PERM))
(classorder (CLASS))
(sid SID)
(sidorder (SID))
(user USER)
(role ROLE)
(type TYPE)
(category CAT)
(categoryorder (CAT))
(sensitivity SENS)
(sensitivityorder (SENS))
(sensitivitycategory SENS (CAT))
(allow TYPE self (CLASS (PERM)))
(roletype ROLE TYPE)
(userrole USER ROLE)
(userlevel USER (SENS))
(userrange USER ((SENS)(SENS (CAT))))
(sidcontext SID (USER ROLE TYPE ((SENS)(SENS))))
(classpermission CLAPERM)
(optional OPT
(roletype nonexistingrole nonexistingtype)
(classpermissionset CLAPERM (CLASS (PERM)))
)
The CIL policy fuzzer (which mimics secilc built with clang Address
Sanitizer) reports:
==36541==ERROR: AddressSanitizer: heap-use-after-free on address
0x603000004f98 at pc 0x56445134c842 bp 0x7ffe2a256590 sp
0x7ffe2a256588
READ of size 8 at 0x603000004f98 thread T0
#0 0x56445134c841 in __cil_verify_classperms
/selinux/libsepol/src/../cil/src/cil_verify.c:1620:8
#1 0x56445134a43e in __cil_verify_classpermission
/selinux/libsepol/src/../cil/src/cil_verify.c:1650:9
#2 0x56445134a43e in __cil_pre_verify_helper
/selinux/libsepol/src/../cil/src/cil_verify.c:1715:8
#3 0x5644513225ac in cil_tree_walk_core
/selinux/libsepol/src/../cil/src/cil_tree.c:272:9
#4 0x564451322ab1 in cil_tree_walk
/selinux/libsepol/src/../cil/src/cil_tree.c:316:7
#5 0x5644513226af in cil_tree_walk_core
/selinux/libsepol/src/../cil/src/cil_tree.c:284:9
#6 0x564451322ab1 in cil_tree_walk
/selinux/libsepol/src/../cil/src/cil_tree.c:316:7
#7 0x5644512b88fd in cil_pre_verify
/selinux/libsepol/src/../cil/src/cil_post.c:2510:7
#8 0x5644512b88fd in cil_post_process
/selinux/libsepol/src/../cil/src/cil_post.c:2524:7
#9 0x5644511856ff in cil_compile
/selinux/libsepol/src/../cil/src/cil.c:564:7
The classperms list of a classpermission rule is created and filled
in when classpermissionset rules are processed, so it doesn't own any
part of the list and shouldn't retain any of it when it is reset.
Destroy the classperms list (without destroying the data in it) when
resetting a classpermission rule.
Reported-by: Nicolas Iooss <nicolas.iooss@m4x.org>
Signed-off-by: James Carter <jwcart2@gmail.com>
Based on patch by Nicolas Iooss, who writes:
OSS-Fuzz found a Heap-buffer-overflow in the CIL compiler when trying
to compile the following policy:
(sid SID)
(sidorder(SID))
(filecon "\" any ())
(filecon "" any ())
When cil_post_fc_fill_data() processes "\", it goes beyond the NUL
terminator of the string. Fix this by returning when '\0' is read
after a backslash.
To be consistent with the function compute_diffdata() in
refpolicy/support/fc_sort.py, also increment str_len in this case.
Fixes: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=28484
Reported-by: Nicolas Iooss <nicolas.iooss@m4x.org>
Signed-off-by: James Carter <jwcart2@gmail.com>
In CIL, blocks, optionals, and macros share the same symbol table so
that the targets of "in" statements can be located. Because of this,
they cannot have the same name in the same namespace, but, because
they do not show up in the final policy, they can have the same name
as long as they are in different namespaces. Unfortunately, when
copying from one namespace to another, no check was being done to see
if there was a conflict.
When copying blocks, optionals, and macros, if a datum is found in
the destination namespace, then there is a conflict with a previously
declared block, optional, or macro, so exit with an error.
Reported-by: Nicolas Iooss <nicolas.iooss@m4x.org>
Reported-by: Evgeny Vereshchagin <evvers@ya.ru>
Signed-off-by: James Carter <jwcart2@gmail.com>
The expectation in CIL was to use user, role, or type attributes in
constraint expressions. The problem is that neither user nor role
attributes are part of the kernel binary policy, so when converting
from a kernel policy to CIL, that would require the creation of a
role or user attribute. The better solution is to just allow a list
to be used. In fact, the only thing preventing a list to be used
is a check in cil_verify_constraint_leaf_expr_syntax().
Remove the check and allow lists in constraint expressions.
The following is now allowed:
(constrain (CLASS1 (PERM1)) (eq r1 (ROLE1 ROLE2 ROLE_ATTR3)))
Signed-off-by: James Carter <jwcart2@gmail.com>
OSS-Fuzz found a Null-dereference in __cil_insert_name when trying to
compile the following policy:
(macro MACRO ()
(classmap CLASS (PERM))
(type TYPE)
(typetransition TYPE TYPE CLASS "name" TYPE)
)
(call MACRO)
When using a macro with no argument, macro->params is NULL and
cil_list_for_each(item, macro->params) dereferenced a NULL pointer.
Fix this by checking that macro->params is not NULL before using it.
Fixes: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=28565
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
All functions of the CIL compiler use cil_log or cil_tree_log to report
errors, but in two places which still uses printf. Replace these printf
invocation with cil_tree_log.
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
printf("%i\n", node->flavor); looks very much like a statement which was
added for debugging purpose and was unintentionally left.
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
cil_post_fc_fill_data() is not used outside of cil_post.c, and is not
exported in libsepol.so. Make it static, in order to ease the analysis
of static analyzers.
While at it, make its path argument "const char*" and the fields of
"struct fc_data" "unsigned int" or "size_t", in order to make the types
better match the values.
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
CIL permits not assigning a context to a SID, e.g. to an unused initial
SID, e.g. 'any_socket'.
When using the example policy from the SELinux Notebook,
https://github.com/SELinuxProject/selinux-notebook/blob/main/src/notebook-examples/cil-policy/cil-policy.cil,
secilc logs:
No context assigned to SID any_socket, omitting from policy at cil-policy.cil:166
But secil2conf segfaults when writing the policy.conf:
../cil/src/cil_policy.c:274:2: runtime error: member access within null pointer of type 'struct cil_context'
Only print the sid context statement if a context was actually assigned.
The sid declaration is still included via cil_sid_decls_to_policy().
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
Acked-by: James Carter <jwcart2@gmail.com>
Nicolas Iooss reports:
I am continuing to investigate OSS-Fuzz crashes and the following one
is quite complex. Here is a CIL policy which triggers a
heap-use-after-free error in the CIL compiler:
(class CLASS (PERM2))
(classorder (CLASS))
(classpermission CLSPRM)
(optional o
(mlsvalidatetrans x (domby l1 h1))
(common CLSCOMMON (PERM1))
(classcommon CLASS CLSCOMMON)
)
(classpermissionset CLSPRM (CLASS (PERM1)))
The issue is that the mlsvalidatetrans fails to resolve in pass
CIL_PASS_MISC3, which comes after the resolution of classcommon (in
pass CIL_PASS_MISC2). So:
* In pass CIL_PASS_MISC2, the optional block still exists, the
classcommon is resolved and class CLASS is linked with common
CLSCOMMON.
* In pass CIL_PASS_MISC3, the optional block is destroyed, including
the common CLSCOMMON.
* When classpermissionset is resolved, function cil_resolve_classperms
uses "common_symtab = &class->common->perms;", which has been freed.
The use-after-free issue occurs in __cil_resolve_perms (in
libsepol/cil/src/cil_resolve_ast.c):
// common_symtab was freed
rc = cil_symtab_get_datum(common_symtab, curr->data, &perm_datum);
The fundamental problem here is that when the optional block is
disabled it is immediately destroyed in the middle of the pass, so
the class has not been reset and still refers to the now destroyed
common when the classpermissionset is resolved later in the same pass.
Added a list, disabled_optionals, to struct cil_args_resolve which is
passed when resolving the tree. When optionals are disabled, they are
now added to this list and then are destroyed after the tree has been
reset between passes.
Reported-by: Nicolas Iooss <nicolas.iooss@m4x.org>
Signed-off-by: James Carter <jwcart2@gmail.com>
Acked-by: Nicolas Iooss <nicolas.iooss@m4x.org>
clang 11.0.0 reports the following warning several times (when building
with "make CC=clang" from libsepol directory, in the default
configuration of the git repository):
../cil/src/cil_binary.c:1980:8: error: cast to smaller integer type
'enum cil_flavor' from 'void *' [-Werror,-Wvoid-pointer-to-enum-cast]
op = (enum cil_flavor)curr->data;
^~~~~~~~~~~~~~~~~~~~~~~~~~~
Silence this warning by casting the pointer to an integer the cast to
enum cil_flavor.
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
OSS-Fuzz found a Null-dereference READ in the CIL compiler when trying
to compile the following policy:
(<src_info>)
In cil_gen_src_info(), parse_current->next is NULL even though the code
expects that both parse_current->next and parse_current->next->next
exists.
Fixes: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=28457
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
OSS-Fuzz found a Null-dereference READ in the CIL compiler when trying
to compile the following policy:
(macro m((name n))) (call m(()))
When calling the macro, the name (in variable "pc") is NULL, which
triggers a NULL pointer dereference when using it as a key in
__cil_insert_name(). The stack trace is:
#0 0x7f4662655a85 in __strlen_avx2 (/usr/lib/libc.so.6+0x162a85)
#1 0x556d0b6d150c in __interceptor_strlen.part.0 (/selinux/libsepol/fuzz/fuzz-secilc+0x44850c)
#2 0x556d0ba74ed6 in symhash /selinux/libsepol/src/symtab.c:22:9
#3 0x556d0b9ef50d in hashtab_search /selinux/libsepol/src/hashtab.c:186:11
#4 0x556d0b928e1f in cil_symtab_get_datum /selinux/libsepol/src/../cil/src/cil_symtab.c:121:37
#5 0x556d0b8f28f4 in __cil_insert_name /selinux/libsepol/src/../cil/src/cil_resolve_ast.c:96:2
#6 0x556d0b908184 in cil_resolve_call1 /selinux/libsepol/src/../cil/src/cil_resolve_ast.c:2835:12
#7 0x556d0b91b404 in __cil_resolve_ast_node /selinux/libsepol/src/../cil/src/cil_resolve_ast.c
#8 0x556d0b91380f in __cil_resolve_ast_node_helper /selinux/libsepol/src/../cil/src/cil_resolve_ast.c:3773:7
#9 0x556d0b932230 in cil_tree_walk_core /selinux/libsepol/src/../cil/src/cil_tree.c:263:9
#10 0x556d0b932230 in cil_tree_walk /selinux/libsepol/src/../cil/src/cil_tree.c:307:7
#11 0x556d0b932326 in cil_tree_walk_core /selinux/libsepol/src/../cil/src/cil_tree.c:275:9
#12 0x556d0b932326 in cil_tree_walk /selinux/libsepol/src/../cil/src/cil_tree.c:307:7
#13 0x556d0b911189 in cil_resolve_ast /selinux/libsepol/src/../cil/src/cil_resolve_ast.c:3941:8
#14 0x556d0b798729 in cil_compile /selinux/libsepol/src/../cil/src/cil.c:550:7
Fixes: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=28544
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
Nicolas Iooss reports:
OSS-Fuzz found an integer overflow when compiling the following
(empty) CIL policy:
;;*lms 2147483647 a
; (empty line)
Change hll_lineno to uint32_t which is the type of the field hll_line
in struct cil_tree_node where the line number will be stored eventually.
Read the line number into an unsigned long variable using strtoul()
instead of strtol(). On systems where ULONG_MAX > UINT32_MAX, return
an error if the value is larger than UINT32_MAX.
Also change hll_expand to uint32_t, since its value will be either
0 or 1 and there is no need for it to be signed.
Reported-by: Nicolas Iooss <nicolas.iooss@m4x.org>
Signed-off-by: James Carter <jwcart2@gmail.com>
It is good practise in C to include the header file that specifies the
prototype of functions which are defined in the source file. Otherwise,
the function prototypes which be different, which could cause unexpected
issues.
Add the include directives to do this.
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
This is based on a patch by Nicolas Iooss. He writes:
When secilc compiles the following policy:
(block b1
(optional o1
(blockinherit b1)
(blockinherit x)
)
)
it disables the optional block at pass 3 (CIL_PASS_BLKIN_LINK)
because the block "x" does not exist.
__cil_resolve_ast_last_child_helper() calls
cil_tree_children_destroy() on the optional block, which destroys
the two blockinherit statements. But the (blockinherit b1) node
was referenced inside (block b1) node, in its block->bi_nodes list.
Therefore, when this list is used at pass 4 (CIL_PASS_BLKIN_COPY),
it contains a node which was freed: this triggers a use-after-free
issue
Fix this issue by removing blockinherit nodes from their lists of
nodes block->bi_nodes when they are being destroyed. As
cil_destroy_blockinherit() does not have a reference to the node
containing the blockinherit data, implement this new logic in
cil_tree_node_destroy().
This issue was found while investigating a testcase from an OSS-Fuzz
issue which seems unrelated (a Null-dereference READ in
cil_symtab_get_datum,
https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=29861).
Reported-by: Nicolas Iooss <nicolas.iooss@m4x.org>
Signed-off-by: James Carter <jwcart2@gmail.com>
Acked-by: Nicolas Iooss <nicolas.iooss@m4x.org>
The following CIL policy triggers a heap use-after-free in secilc
because when the blockinherit node is destroyed, the block node was
already destroyed:
(block b2a)
(blockinherit b2a)
Fix this by setting blockinherit->block to NULL when destroying block.
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
Acked-by: James Carter <jwcart2@gmail.com>
When __cil_validate_constrain_expr() fails,
cil_constrain_to_policydb_helper() does not destroy the constraint
expression. This leads to a memory leak reported by OSS-Fuzz with the
following CIL policy:
(class CLASS (PERM))
(classorder (CLASS))
(sid SID)
(sidorder (SID))
(user USER)
(role ROLE)
(type TYPE)
(category CAT)
(categoryorder (CAT))
(sensitivity SENS)
(sensitivityorder (SENS))
(sensitivitycategory SENS (CAT))
(allow TYPE self (CLASS (PERM)))
(roletype ROLE TYPE)
(userrole USER ROLE)
(userlevel USER (SENS))
(userrange USER ((SENS)(SENS (CAT))))
(sidcontext SID (USER ROLE TYPE ((SENS)(SENS))))
(constrain
(CLASS (PERM))
(or
(eq t1 TYPE)
(or
(eq t1 TYPE)
(or
(eq t1 TYPE)
(or
(eq t1 TYPE)
(or
(eq t1 TYPE)
(eq t1 TYPE)
)
)
)
)
)
)
Add constraint_expr_destroy(sepol_expr) to destroy the expression.
Moreover constraint_expr_destroy() was not freeing all items of an
expression. Code in libsepol/src and checkpolicy contained while loop to
free all the items of a constraint expression, but not the one in
libsepol/cil. As freeing only the first item of an expression is
misleading, change the semantic of constraint_expr_destroy() to iterate
the list of constraint_expr_t and to free all items.
Fixes: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=28938
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
Acked-by: James Carter <jwcart2@gmail.com>
Nicolas Iooss reports:
A few weeks ago, OSS-Fuzz got configured in order to fuzz the CIL
policy compiler (cf.
https://github.com/SELinuxProject/selinux/issues/215 and
https://github.com/google/oss-fuzz/pull/4790). It reported a bunch of
simple issues, for which I will submit patches. There are also more
subtle bugs, like the one triggered by this CIL policy:
(class CLASS (PERM))
(classorder (CLASS))
(sid SID)
(sidorder (SID))
(sensitivity SENS)
(sensitivityorder (SENS))
(type TYPE)
(allow TYPE self (CLASS (PERM)))
(block b
(optional o
(sensitivitycategory SENS (C)) ; Non-existing category
triggers disabling the optional
(common COMMON (PERM1))
(classcommon CLASS COMMON)
(allow TYPE self (CLASS (PERM1)))
)
)
On my computer, secilc manages to build this policy fine, but when
clang's Address Sanitizer is enabled, running secilc leads to the
following report:
$ make -C libsepol/src CC=clang CFLAGS='-g -fsanitize=address' libsepol.a
$ clang -g -fsanitize=address secilc/secilc.c libsepol/src/libsepol.a
-o my_secilc
$ ./my_secilc -vv testcase.cil
Parsing testcase.cil
Building AST from Parse Tree
Destroying Parse Tree
Resolving AST
Failed to resolve sensitivitycategory statement at testcase.cil:12
Disabling optional 'o' at testcase.cil:11
Resetting declarations
=================================================================
==181743==ERROR: AddressSanitizer: heap-use-after-free on address
0x6070000000c0 at pc 0x55ff7e445d24 bp 0x7ffe7eecfba0 sp
0x7ffe7eecfb98
READ of size 4 at 0x6070000000c0 thread T0
#0 0x55ff7e445d23 in __class_reset_perm_values
/git/selinux-userspace/libsepol/src/../cil/src/cil_reset_ast.c:17:17
The problem is that the optional branch is destroyed when it is disabled,
so the common has already been destroyed when the reset code tries to
access the number of common permissions, so that it can change the
value of the class permissions back to their original values.
The solution is to count the number of class permissions and then
calculate the number of common permissions.
Reported-by: Nicolas Iooss <nicolas.iooss@m4x.org>
Signed-off-by: James Carter <jwcart2@gmail.com>
This field is suppose to be used to track the number of primary names in
the symtab. It was not being updated or used.
Increment the nprim field when a new datum is added to the symtab and
decrement the field when a datum is removed.
Signed-off-by: James Carter <jwcart2@gmail.com>
OSS-Fuzz found a NULL pointer dereference when the CIL compiler tries to
compile a policy with an invalid integer:
$ echo '(ioportcon(2())n)' > tmp.cil
$ secilc tmp.cil
Segmentation fault (core dumped)
This is because strtol() is called with a NULL pointer, in
cil_fill_integer().
Fix this by checking that int_node->data is not NULL. While at it, use
strtoul() instead of strtol() to parse an unsigned integer.
When using "val > UINT32_MAX" with "unsigned long val;", it is expected
that some compilers emit a warning when the size of "unsigned long" is
32 bits. In theory gcc could be such a compiler (with warning
-Wtype-limits, which is included in -Wextra). Nevertheless this is
currently broken, according to
https://gcc.gnu.org/pipermail/gcc-help/2021-January/139755.html and
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=89126 (this bug was
opened in January 2019).
In order to prevent this warning from appearing, introduce some
preprocessor macros around the bound check.
Fixes: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=28456
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
Acked-by: James Carter <jwcart2@gmail.com>
When __cil_resolve_perms fails, it does not destroy perm_datums, which
leads to a memory leak reported by OSS-Fuzz with the following CIL
policy:
(class cl01())
(classorder(cl01))
(type at02)
(type tpr3)
(allow at02 tpr3(cl01((s))))
Calling cil_list_destroy() fixes the issue.
Fixes: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=28466
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
OSS-Fuzz found a heap buffer overflow (out-of-bound reads) when the CIL
compiler tries to report a recursive blockinherit with an optional
block:
$ echo '(block b (optional o (blockinherit b)))' > tmp.cil
$ secilc tmp.cil
Segmentation fault (core dumped)
This is because cil_print_recursive_blockinherit() assumes that all
nodes are either CIL_BLOCK or CIL_BLOCKINHERIT. Add support for other
block kinds, using cil_node_to_string() to show them.
Fixes: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=28462
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
Function cil_add_file() copies its input into a newly-allocated buffer,
and does not modify "name". State these properties in the types of
parameters by adding "const" qualifiers.
This enables using LibFuzzer directly on cil_add_file(), without a
warning about discarding "const" qualifier:
fuzz-secilc.c: In function ‘LLVMFuzzerTestOneInput’:
fuzz-secilc.c:57:31: warning: passing argument 3 of ‘cil_add_file’
discards ‘const’ qualifier from pointer target type
[-Wdiscarded-qualifiers]
57 | if (cil_add_file(db, "fuzz", data, size) != SEPOL_OK)
| ^~~~
In file included from fuzz-secilc.c:26:
/usr/include/sepol/cil/cil.h:45:57: note: expected ‘char *’ but
argument is of type ‘const uint8_t *’ {aka ‘const unsigned char *’}
45 | extern int cil_add_file(cil_db_t *db, char *name, char *data, size_t size);
| ~~~~~~^~~~
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
OSS-Fuzz found a Null-dereference READ in the CIL compiler when trying
to compile the following policy:
(optional o (validatetrans x (eq t3 (a ()))))
With some logs, secilc reports:
Invalid syntax
Destroying Parse Tree
Resolving AST
Failed to resolve validatetrans statement at fuzz:1
Disabling optional 'o' at tmp.cil:1
So there is an "Invalid syntax" error, but the compilation continues.
Fix this issue by stopping the compilation when cil_fill_list() reports
an error:
Invalid syntax
Bad expression tree for constraint
Bad validatetrans declaration at tmp.cil:1
Fixes: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=29061
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
OSS-Fuzz found a heap use-after-free when the CIL compiler destroys its
database after failing to compile the following policy:
(validatetrans x (eq t3 (a)))
This is caused by the fact that the validatetrans AST object references
a stack variable local to __cil_fill_constraint_leaf_expr, when parsing
the list "(a)":
struct cil_list *sub_list;
cil_fill_list(current->next->next->cl_head, leaf_expr_flavor, &sub_list);
cil_list_append(*leaf_expr, CIL_LIST, &sub_list);
Drop the & sign to really add the list like it is supposed to be.
Fixes: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=28507
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
