libsepol/src/roles.c contains functions which do not match its header
file libsepol/include/sepol/roles.h:
// In roles.c
int sepol_role_exists(sepol_handle_t * handle __attribute__ ((unused)),
sepol_policydb_t * p, const char *role, int *response)
// In roles.h
extern int sepol_role_exists(const sepol_policydb_t * policydb,
const char *role, int *response);
and:
// In roles.c
int sepol_role_list(sepol_handle_t * handle,
sepol_policydb_t * p, char ***roles, unsigned int *nroles)
// In roles.h
extern int sepol_role_list(const sepol_policydb_t * policydb,
char ***roles, unsigned int *nroles);
Instead of fixing the parameter type (using sepol_handle_t or
sepol_policydb_t but not different ones), remove these functions, as
they appear not to be used. They are not exported in libsepol.so.
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 direct memory leak in policydb_filetrans_insert()
because filenametr_destroy() does not fully destroy the list associated
with a typetransition.
More precisely, let's consider this (minimized) CIL policy:
(class CLASS (PERM))
(classorder (CLASS))
(sid SID)
(sidorder (SID))
(user USER)
(role ROLE)
(type TYPE) ; "type 1" in libsepol internal structures
(type TYPE2) ; "type 2" in libsepol internal structures
(type TYPE3) ; "type 3" in libsepol internal structures
(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))))
(typetransition TYPE2 TYPE CLASS "some_file" TYPE2)
(typetransition TYPE3 TYPE CLASS "some_file" TYPE3)
The two typetransition statements make policydb_filetrans_insert()
insert an item with key {ttype=1, tclass=1, name="some_file"} in the
hashmap p->filename_trans. This item contains a linked list of two
filename_trans_datum_t elements:
* The first one uses {otype=2, stypes=bitmap containing 2}
* The second one uses {otype=3, stypes=bitmap containing 3}
Nevertheless filenametr_destroy() (called by
hashtab_map(p->filename_trans, filenametr_destroy, NULL);) only frees
the first element. Fix this memory leak by freeing all elements.
This issue was introduced by commit 42ae834a74 ("libsepol,checkpolicy:
optimize storage of filename transitions") and was never present in the
kernel, as filenametr_destroy() was modified appropriately in commit
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=c3a276111ea2572399281988b3129683e2a6b60b
Fixes: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=29138
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
Acked-by: Ondrej Mosnacek <omosnace@redhat.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>
OSS-Fuzz found a NULL pointer dereference when the CIL compiler tries to
compile a policy where a categoryalias references an unused
categoryalias:
$ echo '(categoryalias c0)(categoryalias c1)(categoryaliasactual c0 c1)' > tmp.cil
$ secil tmp.cil
Segmentation fault (core dumped)
In such a case, a1 can become NULL in cil_resolve_alias_to_actual().
Add a check to report an error when this occurs. Now the error message
is:
Alias c0 references an unused alias c1 at tmp.cil:1
Fixes: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=28471
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
cil_copy_expandtypeattribute prints "cil_copy_expandtypeattribute 656"
which is quite annoying. Remove the fprintf statement responsible for
this.
While at it, remove another one in cil_tree_print_node()
Fixes: https://lore.kernel.org/selinux/3c2ab876-b0b7-42eb-573d-e5b450a7125a@gmail.com/T/#u
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
It was found in https://github.com/google/oss-fuzz/pull/4790:
```
Invalid token '' at line 2 of fuzz
NEW_FUNC[1/2]: 0x67fff0 in yy_get_previous_state /src/selinux/libsepol/src/../cil/src/cil_lexer.c:1143
NEW_FUNC[2/2]: 0x6803e0 in yy_try_NUL_trans /src/selinux/libsepol/src/../cil/src/cil_lexer.c:1176
=================================================================
==12==ERROR: AddressSanitizer: heap-use-after-free on address 0x602000007992 at pc 0x000000681800 bp 0x7ffccddee530 sp 0x7ffccddee528
WRITE of size 1 at 0x602000007992 thread T0
SCARINESS: 41 (1-byte-write-heap-use-after-free)
#0 0x6817ff in cil_yy_switch_to_buffer /src/selinux/libsepol/src/../cil/src/cil_lexer.c:1315:17
#1 0x6820cc in cil_yy_scan_buffer /src/selinux/libsepol/src/../cil/src/cil_lexer.c:1571:2
#2 0x682662 in cil_lexer_setup /src/selinux/libsepol/src/../cil/src/cil_lexer.l:73:6
#3 0x5cf2ae in cil_parser /src/selinux/libsepol/src/../cil/src/cil_parser.c:220:2
#4 0x56d5e2 in cil_add_file /src/selinux/libsepol/src/../cil/src/cil.c:514:7
#5 0x556e91 in LLVMFuzzerTestOneInput /src/secilc-fuzzer.c:434:7
#6 0x459ab1 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:599:15
#7 0x45a755 in fuzzer::Fuzzer::TryDetectingAMemoryLeak(unsigned char const*, unsigned long, bool) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:675:3
#8 0x45acd9 in fuzzer::Fuzzer::MutateAndTestOne() /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:747:5
#9 0x45b875 in fuzzer::Fuzzer::Loop(std::__Fuzzer::vector<fuzzer::SizedFile, fuzzer::fuzzer_allocator<fuzzer::SizedFile> >&) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:883:5
#10 0x4499fb in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:906:6
#11 0x473a32 in main /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerMain.cpp:20:10
#12 0x7f982296d83f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2083f)
#13 0x41e758 in _start (/out/secilc-fuzzer+0x41e758)
DEDUP_TOKEN: cil_yy_switch_to_buffer--cil_yy_scan_buffer--cil_lexer_setup
0x602000007992 is located 2 bytes inside of 4-byte region [0x602000007990,0x602000007994)
freed by thread T0 here:
#0 0x521ef2 in free /src/llvm-project/compiler-rt/lib/asan/asan_malloc_linux.cpp:127:3
#1 0x56d630 in cil_add_file /src/selinux/libsepol/src/../cil/src/cil.c:526:2
#2 0x556e91 in LLVMFuzzerTestOneInput /src/secilc-fuzzer.c:434:7
#3 0x459ab1 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:599:15
#4 0x458fba in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool, bool*) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:505:3
#5 0x45acc7 in fuzzer::Fuzzer::MutateAndTestOne() /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:745:19
#6 0x45b875 in fuzzer::Fuzzer::Loop(std::__Fuzzer::vector<fuzzer::SizedFile, fuzzer::fuzzer_allocator<fuzzer::SizedFile> >&) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:883:5
#7 0x4499fb in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:906:6
#8 0x473a32 in main /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerMain.cpp:20:10
#9 0x7f982296d83f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2083f)
DEDUP_TOKEN: free--cil_add_file--LLVMFuzzerTestOneInput
previously allocated by thread T0 here:
#0 0x52215d in malloc /src/llvm-project/compiler-rt/lib/asan/asan_malloc_linux.cpp:145:3
#1 0x5cecb8 in cil_malloc /src/selinux/libsepol/src/../cil/src/cil_mem.c:39:14
#2 0x56d584 in cil_add_file /src/selinux/libsepol/src/../cil/src/cil.c:510:11
#3 0x556e91 in LLVMFuzzerTestOneInput /src/secilc-fuzzer.c:434:7
#4 0x459ab1 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:599:15
#5 0x458fba in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool, bool*) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:505:3
#6 0x45acc7 in fuzzer::Fuzzer::MutateAndTestOne() /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:745:19
#7 0x45b875 in fuzzer::Fuzzer::Loop(std::__Fuzzer::vector<fuzzer::SizedFile, fuzzer::fuzzer_allocator<fuzzer::SizedFile> >&) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:883:5
#8 0x4499fb in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:906:6
#9 0x473a32 in main /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerMain.cpp:20:10
#10 0x7f982296d83f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2083f)
DEDUP_TOKEN: malloc--cil_malloc--cil_add_file
SUMMARY: AddressSanitizer: heap-use-after-free /src/selinux/libsepol/src/../cil/src/cil_lexer.c:1315:17 in cil_yy_switch_to_buffer
Shadow bytes around the buggy address:
0x0c047fff8ee0: fa fa fd fa fa fa fd fd fa fa fd fa fa fa fd fd
0x0c047fff8ef0: fa fa fd fa fa fa fd fd fa fa fd fa fa fa fd fd
0x0c047fff8f00: fa fa fd fa fa fa fd fa fa fa fd fa fa fa fd fd
0x0c047fff8f10: fa fa fd fa fa fa fd fd fa fa fd fa fa fa fd fd
0x0c047fff8f20: fa fa fd fa fa fa fd fd fa fa fd fa fa fa fd fa
=>0x0c047fff8f30: fa fa[fd]fa fa fa fd fa fa fa fd fa fa fa fd fa
0x0c047fff8f40: fa fa fd fa fa fa fd fa fa fa fd fa fa fa fd fa
0x0c047fff8f50: fa fa fd fa fa fa fd fd fa fa fd fa fa fa fd fa
0x0c047fff8f60: fa fa fd fd fa fa fd fa fa fa fd fd fa fa fd fa
0x0c047fff8f70: fa fa 00 00 fa fa 02 fa fa fa 02 fa fa fa 00 fa
0x0c047fff8f80: fa fa 03 fa fa fa 00 fa fa fa 03 fa fa fa 00 fa
Shadow byte legend (one shadow byte represents 8 application bytes):
Addressable: 00
Partially addressable: 01 02 03 04 05 06 07
Heap left redzone: fa
Freed heap region: fd
Stack left redzone: f1
Stack mid redzone: f2
Stack right redzone: f3
Stack after return: f5
Stack use after scope: f8
Global redzone: f9
Global init order: f6
Poisoned by user: f7
Container overflow: fc
Array cookie: ac
Intra object redzone: bb
ASan internal: fe
Left alloca redzone: ca
Right alloca redzone: cb
Shadow gap: cc
==12==ABORTING
```
Signed-off-by: Evgeny Vereshchagin <evvers@ya.ru>
Acked-by: Nicolas Iooss <nicolas.iooss@m4x.org>
In cil_symtab.h, the macro FLAVOR() is defined. It refers to the
flavor of the first node in the list of nodes that declare the datum.
(The flavors of every node should be the same.) While the macro was
used in many places, it was not used everywhere that it could be.
Change all the remaining places to use FLAVOR().
Signed-off-by: James Carter <jwcart2@gmail.com>
In cil_symtab.h, the macro NODE() is defined. It refers to the first
node in the list of nodes that declare that datum. (It is rare for
a datum to have more than one node in this list.) While the macro was
used in many places, it was not used everywhere that it could be.
Change all the remaining places to use NODE().
Signed-off-by: James Carter <jwcart2@gmail.com>
Block, macro, and optional names are all in stored in a block symtab. A
declarations fully-qualified name includes all of the block names from
the root node to the declaration separated by dots. Macro and optional
names are only used when trying to determine the block referred to by
an "in" block. An optional block name might be stored in a macro's
symtab, but optional blocks have no symtab and (*datum)->symtab just
refers to the symtab of the datum which would be the current symtab.
Since the assignment is not needed, remove it so the code is clearer.
Signed-off-by: James Carter <jwcart2@gmail.com>
When resolving names, the struct cil_args_resolve is passed to the
various resolve functions. The field last_resolved_name is not used.
Remove the last_resolved_name field from struct cil_args_resolve.
Signed-off-by: James Carter <jwcart2@gmail.com>
Since cil_gen_node() is only called from declarations, the check to
determine if the node is a declaration is not needed, so remove it.
Signed-off-by: James Carter <jwcart2@gmail.com>
The function cil_tree_walk() has an argument that can be used by
the process_node helper function to tell cil_tree_walk() to skip
the node's sub-tree or the rest of the current branch. The constants
CIL_TREE_SKIP_NOTHING, CIL_TREE_SKIP_NEXT and CIL_TREE_SKIP_HEAD are
defined to be used by that argument.
Fixed two instances in the function __cil_build_ast_node_helper()
where the value 1 is used instead of the more informative
CIL_TREE_SKIP_NEXT.
Signed-off-by: James Carter <jwcart2@gmail.com>
In get_class_info(), if realloc(class_buf, new_class_buf_len) fails to
grow the memory, the function returns NULL without freeing class_buf.
This leads to a memory leak which is reported by clang's static
analyzer:
https://580-118970575-gh.circle-artifacts.com/0/output-scan-build/2020-11-11-194150-6152-1/report-42a899.html#EndPath
Fix the memory leak by calling free(class_buf).
While at it, use size_t insted of int to store the size of the buffer
which is growing.
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
Both tunableif and booleanif use conditional blocks (either true or
false). No ordering is imposed, so a false block can be first (or even
the only) block. Checks are made to ensure that the first and second
(if it exists) blocks are either true or false, but no checks are made
to ensure that there is only one true and/or one false block. If there
are more than one true or false block, only the first will be used and
the other will be ignored.
Create a function, cil_verify_conditional_blocks(), that gives an error
along with a message if more than one true or false block is specified
and call that function when building tunableif and booleanif blocks in
the AST.
Signed-off-by: James Carter <jwcart2@gmail.com>
Acked-by: Ondrej Mosnacek <omosnace@redhat.com>
Previous commits removed some symbols and broke ABI, therefore we need to change
SONAME.
See the following quotes from distribution guidelines:
https://www.debian.org/doc/debian-policy/ch-sharedlibs.html#run-time-shared-libraries
Every time the shared library ABI changes in a way that may break
binaries linked against older versions of the shared library, the SONAME
of the library and the corresponding name for the binary package
containing the runtime shared library should change.
https://docs.fedoraproject.org/en-US/packaging-guidelines/#_downstream_so_name_versioning
When new versions of the library are released, you should use an ABI
comparison tool to check for ABI differences in the built shared
libraries. If it detects any incompatibilities, bump the n number by
one.
Signed-off-by: Petr Lautrbach <plautrba@redhat.com>
These functions were converted to no-op by commit
c3f9492d7f ("selinux: Remove legacy local boolean and user code") and
left in libsepol/src/deprecated_functions.c to preserve API/ABI. As we
change libsepol ABI dropping duplicate symbols it's time to drop these
functions too.
Signed-off-by: Petr Lautrbach <plautrba@redhat.com>
Versioned duplicate symbols cause problems for LTO. These symbols were
introduced during the CIL integration several releases ago and were only
consumed by other SELinux userspace components.
Fixes: https://github.com/SELinuxProject/selinux/issues/245
Signed-off-by: Petr Lautrbach <plautrba@redhat.com>
When find_avtab_node() is called with key->specified & AVTAB_XPERMS and
xperms=NULL, xperms is being dereferenced. This is detected as a
"NULL pointer dereference issue" by static analyzers.
Even though it does not make much sense to call find_avtab_node() in a
way which triggers the NULL pointer dereference issue, static analyzers
have a hard time with calls such as:
node = find_avtab_node(handle, avtab, &avkey, cond, NULL);
... where xperms=NULL.
So, make the function report an error instead of crashing.
Here is an example of report from clang's static analyzer:
https://558-118970575-gh.circle-artifacts.com/0/output-scan-build/2020-10-02-065849-6375-1/report-d86a57.html#EndPath
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
Contrary to Linux kernel, BUG_ON() does not halt the execution, in
libsepol/src/services.c. Instead it displays an error message and
continues the execution.
This means that this code does not prevent an out-of-bound write from
happening:
case CEXPR_AND:
BUG_ON(sp < 1);
sp--;
s[sp] &= s[sp + 1];
Use if(...){BUG();rc=-EINVAL;goto out;} constructions instead, to make
sure that the array access is always in-bound.
This issue has been found using clang's static analyzer:
https://558-118970575-gh.circle-artifacts.com/0/output-scan-build/2020-10-02-065849-6375-1/report-50a861.html#EndPath
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
When compiling SELinux userspace tools with -ftrapv (this option
generates traps for signed overflow on addition, subtraction,
multiplication operations, instead of silently wrapping around),
semodule crashes when running the tests from
scripts/ci/fedora-test-runner.sh in a Fedora 32 virtual machine:
[root@localhost selinux-testsuite]# make test
make -C policy load
make[1]: Entering directory '/root/selinux-testsuite/policy'
# Test for "expand-check = 0" in /etc/selinux/semanage.conf
# General policy build
make[2]: Entering directory '/root/selinux-testsuite/policy/test_policy'
Compiling targeted test_policy module
Creating targeted test_policy.pp policy package
rm tmp/test_policy.mod.fc
make[2]: Leaving directory '/root/selinux-testsuite/policy/test_policy'
# General policy load
domain_fd_use --> off
/usr/sbin/semodule -i test_policy/test_policy.pp test_mlsconstrain.cil test_overlay_defaultrange.cil test_add_levels.cil test_glblub.cil
make[1]: *** [Makefile:174: load] Aborted (core dumped)
Using "coredumpctl gdb" leads to the following strack trace:
(gdb) bt
#0 0x00007f608fe4fa25 in raise () from /lib64/libc.so.6
#1 0x00007f608fe38895 in abort () from /lib64/libc.so.6
#2 0x00007f6090028aca in __addvsi3.cold () from /lib64/libsepol.so.1
#3 0x00007f6090096f59 in __avrule_xperm_setrangebits (low=30, high=30, xperms=0x8b9eea0)
at ../cil/src/cil_binary.c:1551
#4 0x00007f60900970dd in __cil_permx_bitmap_to_sepol_xperms_list (xperms=0xb650a30, xperms_list=0x7ffce2653b18)
at ../cil/src/cil_binary.c:1596
#5 0x00007f6090097286 in __cil_avrulex_ioctl_to_policydb (k=0xb8ec200 "@\023\214\022\006", datum=0xb650a30,
args=0x239a640) at ../cil/src/cil_binary.c:1649
#6 0x00007f609003f1e5 in hashtab_map (h=0x41f8710, apply=0x7f60900971da <__cil_avrulex_ioctl_to_policydb>,
args=0x239a640) at hashtab.c:234
#7 0x00007f609009ea19 in cil_binary_create_allocated_pdb (db=0x2394f10, policydb=0x239a640)
at ../cil/src/cil_binary.c:4969
#8 0x00007f609009d19d in cil_binary_create (db=0x2394f10, policydb=0x7ffce2653d30) at ../cil/src/cil_binary.c:4329
#9 0x00007f609008ec23 in cil_build_policydb_create_pdb (db=0x2394f10, sepol_db=0x7ffce2653d30)
at ../cil/src/cil.c:631
#10 0x00007f608fff4bf3 in semanage_direct_commit () from /lib64/libsemanage.so.1
#11 0x00007f608fff9fae in semanage_commit () from /lib64/libsemanage.so.1
#12 0x0000000000403e2b in main (argc=7, argv=0x7ffce2655058) at semodule.c:753
(gdb) f 3
#3 0x00007f6090096f59 in __avrule_xperm_setrangebits (low=30, high=30, xperms=0x8b9eea0)
at ../cil/src/cil_binary.c:1551
1551 xperms->perms[i] |= XPERM_SETBITS(h) - XPERM_SETBITS(low);
A signed integer overflow therefore occurs in XPERM_SETBITS(h):
#define XPERM_SETBITS(x) ((1 << (x & 0x1f)) - 1)
This macro is expanded with h=31, so "(1 << 31) - 1" is computed:
* (1 << 31) = -0x80000000 is the lowest signed 32-bit integer value
* (1 << 31) - 1 overflows the capacity of a signed 32-bit integer and
results in 0x7fffffff (which is unsigned)
Using unsigned integers (with "1U") fixes the crash, as
(1U << 31) = 0x80000000U has no overflowing issues.
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
Acked-by: Petr Lautrbach <plautrba@redhat.com>
When classmaps used in a neverallow were being expanded during CIL
neverallow checking, an empty classmapping in the list of
classmappings for a classmap would cause the classmap expansion to
stop and the rest of the classmapping of the classmap to be ignored.
This would mean that not all of the classes and permissions associated
with the classmap would be used to check for a neverallow violation.
Do not end the expansion of a classmap when one classmapping is empty.
Reported-by: Jonathan Hettwer <j2468h@gmail.com>
Signed-off-by: James Carter <jwcart2@gmail.com>
Acked-by: Stephen Smalley <stephen.smalley.work@gmail.com>
CIL was not correctly determining the depth of conditional expressions
which prevented it from giving an error when the max depth was exceeded.
This allowed invalid policy binaries to be created.
Validate the conditional expression using the same logic that is used
when evaluating a conditional expression. This includes checking the
depth of the expression.
Signed-off-by: James Carter <jwcart2@gmail.com>
Acked-by: Stephen Smalley <stephen.smalley.work@gmail.com>
CIL was not correctly determining the depth of constraint expressions
which prevented it from giving an error when the max depth was exceeded.
This allowed invalid policy binaries with constraint expressions exceeding
the max depth to be created.
Validate the constraint expression using the same logic that is used
when reading the binary policy. This includes checking the depth of the
the expression.
Reported-by: Jonathan Hettwer <j2468h@gmail.com>
Signed-off-by: James Carter <jwcart2@gmail.com>
Acked-by: Stephen Smalley <stephen.smalley.work@gmail.com>
Implement a new, more space-efficient form of storing filename
transitions in the binary policy. The internal structures have already
been converted to this new representation; this patch just implements
reading/writing an equivalent representation from/to the binary policy.
This new format reduces the size of Fedora policy from 7.6 MB to only
3.3 MB (with policy optimization enabled in both cases). With the
unconfined module disabled, the size is reduced from 3.3 MB to 2.4 MB.
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
In preparation to support a new policy format with a more optimal
representation of filename transition rules, this patch applies an
equivalent change from kernel commit c3a276111ea2 ("selinux: optimize
storage of filename transitions").
See the kernel commit's description [1] for the rationale behind this
representation. This change doesn't bring any measurable difference of
policy build performance (semodule -B) on Fedora.
[1] https://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git/commit/?id=c3a276111ea2572399281988b3129683e2a6b60b
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
The comparison function, portcon_data_cmp(), only made use of the
protocol to put tcp before udp, dccp, and sctp. Rules that have
the same port range, but with different protocols would be considered
equal unless one of the protocols was tcp. When generating a CIL or
conf source policy from a binary or using the "-S" option in
checkpolicy the non-tcp portcon rules with the same port range would
not be consistently sorted.
Changed portcon_data_cmp() to sort portcon rules like the CIL function
cil_post_portcon_compare().
Reported-by: Stephen Smalley <stephen.smalley.work@gmail.com>
Signed-off-by: James Carter <jwcart2@gmail.com>
Acked-by: Stephen Smalley <stephen.smalley.work@gmail.com>
Improves writing of CIL category rules when converting MLS kernel
policy to CIL. No changes to functionality, but eliminate useless
checks for category aliases when using the p_cat_val_to_name array,
find the actual number of aliases before allocating memory, and
skip the category alias rules if there are no aliases.
Signed-off-by: James Carter <jwcart2@gmail.com>
Acked-by: Stephen Smalley <stephen.smalley.work@gmail.com>
Improves writing of CIL sensitivity rules when converting MLS kernel
policy to CIL. No changes to functionality, but eliminate useless
checks for sensitivity aliases when using the p_sens_val_to_name
array, find the actual number of aliases before allocating memory,
and skip the sensitivity alias rules if there are no aliases.
Signed-off-by: James Carter <jwcart2@gmail.com>
When converting a non-MLS kernel binary policy to CIL, write the CIL
default MLS rules (since CIL requires at least one sensitivity,
and sensitivityorder statements) on separate lines.
This improves the readability of the resulting CIL policy.
Signed-off-by: James Carter <jwcart2@gmail.com>
Type alias rules are not written out when converting a binary kernel
policy to a policy.conf. The problem is that type aliases are not in
the type_val_to_struct array and that is what is being used to find
the aliases.
Since type aliases are only in the types hashtable, walk that to
find the type aliases.
Fixed the syntax of the typalias rule which requires "alias" to come
between the type and the aliases (ex/ typealias TYPE alias ALIAS;).
Fixes: 0a08fd1e69 ("libsepol: Add ability to convert binary
policy to policy.conf file")
Signed-off-by: James Carter <jwcart2@gmail.com>
Acked-by: Stephen Smalley <stephen.smalley.work@gmail.com>
Type alias rules are not written out when converting a binary kernel
policy to CIL. The problem is that type aliases are not in the
type_val_to_struct array and that is what is being used to find the
aliases.
Since type aliases are only in the types hashtable, walk that to
find the type aliases.
Fixes: 70a480bfcd ("libsepol: Add ability to convert binary
policy to CIL")
Signed-off-by: James Carter <jwcart2@gmail.com>
CIL allows a type to be redeclared when using the multiple declarations
option ("-m" or "--muliple-decls"), but make it an error for an identifier
to be declared as both a type and an attribute.
Change the error message so that it always gives the location and flavor
of both declarations. The flavors will be the same in all other cases,
but in this case they explain why there is an error even if multiple
declartions are allowed.
Fixes: Commit fafe4c212b ("libsepol: cil: Add ability to redeclare types[attributes]")
Reported-by: Topi Miettinen <toiwoton@gmail.com>
Signed-off-by: James Carter <jwcart2@gmail.com>
Acked-by: Stephen Smalley <stephen.smalley.work@gmail.com>
Initialize the multiple_decls field when intializing the structure
cil_db.
Fixes: fafe4c212b ("libsepol: cil: Add ability to redeclare types[attributes]")
Reported-by: Topi Miettinen <toiwoton@gmail.com
Acked-by: Stephen Smalley <stephen.smalley.work@gmail.com>
Signed-off-by: James Carter <jwcart2@gmail.com>
As per the issue below, libsepol segfaults on loading old kernel policies
that contain duplicate filename transition rules. The segfault is due to
the fact that the val_to_name arrays have not yet been populated at this
point in the policydb_read() processing. Since this warning apparently
never worked since it was first introduced, drop it and just silently
discard the duplicate like the kernel does. I was not able to produce a
policy with such duplicates using the current policy toolchain, either
via CIL or via binary modules with manual semodule_link/expand.
Fixes: https://github.com/SELinuxProject/selinux/issues/239
Fixes: 8fdb225521 ("libsepol,checkpolicy: convert rangetrans and filenametrans to hashtabs")
Signed-off-by: Stephen Smalley <stephen.smalley.work@gmail.com>
Acked-by: Ondrej Mosnacek <omosnace@redhat.com>
This reverts commit 692716fc5f.
Other parts of the SELinux userspace depend on certain attributes,
such as node_type, exisiting and this change breaks those parts.
Before this patch can be reapplied, we need to identify the attributes
that must never be expanded and create a CIL module with the needed
expandtypeattribute statements (or something similar).
Signed-off-by: James Carter <jwcarter@gmail.com>
Fix issues like:
<inline asm>:1:1: error: unknown directive
.symver cil_build_policydb_pdb, cil_build_policydb@LIBSEPOL_1.0
Which was caused by the DISABLE_SYMVER define not being defined
for static, Mac or Android builds.
Acked-by: Joshua Brindle <joshua.brindle@crunchydata.com>
Signed-off-by: William Roberts <william.c.roberts@intel.com>
Currently a constraint `t1 == t2` gets converted to the invalid cil syntax `(mlsconstrain (class_name (perm_name)) (eq t1 ))` and fails to be loaded into the kernel.
Fixes: 893851c0a1 ("policycoreutils: add a HLL compiler to convert policy packages (.pp) to CIL")
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
Acked-by: James Carter <jwcart2@gmail.com>
The iteration over the set ebitmap bits is not implemented very
efficiently in libsepol. It is slowing down the policy optimization
quite significantly, so convert the type_map from an array of ebitmaps
to an array of simple ordered vectors, which can be traveresed more
easily. The worse space efficiency of the vectors is less important than
the speed in this case.
After this change the duration of semodule -BN decreased from 6.4s to
5.5s on Fedora Rawhide x86_64 (and from 6.1s to 5.6s with the unconfined
module disabled).
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
Acked-by: Stephen Smalley <stephen.smalley.work@gmail.com>
Only attributes can be a superset of another attribute, so we can skip
non-attributes right away.
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
Acked-by: Stephen Smalley <stephen.smalley.work@gmail.com>
I copy-pasted it from a different part of the code, which had to deal
with policydb that isn't final yet. Since we only deal with the final
kernel policy here, we can skip the check for the type datum being NULL.
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
Acked-by: Stephen Smalley <stephen.smalley.work@gmail.com>
With the old hidden_def and hidden_proto DSO infrastructure removed,
correctness of the map file becomes paramount, as it is what filters out
public API. Because of this, the wild cards should not be used, as it
lets some functions through that should not be made public API. Thus
remove the wild cards, and sort the list.
Additionally, verify that nothing changed in external symbols as well:
This was checked by generating an old export map (from master):
nm --defined-only -g ./src/libsepol.so | cut -d' ' -f 3-3 | grep -v '^_' > old.map
Then creating a new one for this library after this patch is applied:
nm --defined-only -g ./src/libsepol.so | cut -d' ' -f 3-3 | grep -v '^_' > new.map
And diffing them:
diff old.map new.map
Fixes: #165Fixes: #204
Acked-by: Stephen Smalley <sds@tycho.nsa.gov>
Signed-off-by: William Roberts <william.c.roberts@intel.com>
Add -fno-semantic-interposition to CFLAGS. This will restore
the DSO infrastructures protections to insure internal callers
of exported symbols call into libselinux and not something loading first
in the library list.
Clang has this enabled by default.
Acked-by: Stephen Smalley <sds@tycho.nsa.gov>
Signed-off-by: William Roberts <william.c.roberts@intel.com>
libsepol already has a linker script controlling it's exports, so this
patch has a net 0 affect, with the exception that internal callers of
external routines, which there could be 0 of, could potentially call a
non-libsepol routine depending on library load order.
NOTE A FEW SYMBOLS ARE EXPORTED THAT NORMALLY WOULDN'T BE
- sepol_context_to_sid
- sepol_ibendport_sid
- sepol_ibpkey_sid
- sepol_msg_default_handler
- sepol_node_sid
- sepol_port_sid
A subsequent map update will follow.
This list was generated by generating an old export map (from master):
nm --defined-only -g ./src/libsepol.so | cut -d' ' -f 3-3 | grep -v '^_' > old.map
Then creating a new one for this library after this patch is applied:
nm --defined-only -g ./src/libsepol.so | cut -d' ' -f 3-3 | grep -v '^_' > new.map
And diffing them:
diff old.map new.map
Acked-by: Stephen Smalley <sds@tycho.nsa.gov>
Signed-off-by: William Roberts <william.c.roberts@intel.com>
libsepol carried its own (outdated) copy of flask.h with the generated
security class and initial SID values for use by the policy
compiler and the forked copy of the security server code
leveraged by tools such as audit2why. Convert libsepol and
checkpolicy entirely to looking up class values from the policy,
remove the SECCLASS_* definitions from its flask.h header, and move
the header with its remaining initial SID definitions private to
libsepol. While we are here, fix the sepol_compute_sid() logic to
properly support features long since added to the policy and kernel,
although there are no users of it other than checkpolicy -d (debug)
and it is not exported to users of the shared library. There
are still some residual differences between the kernel logic and
libsepol.
Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov>
Acked-by: Petr Lautrbach <plautrba@redhat.com>
The value attrs_expand_size == 1 removes all empty attributes, but it
also makes sense to expand all attributes that have only one type. This
removes some redundant rules (there is sometimes the same rule for the
type and the attribute) and reduces the number of attributes that the
kernel has to go through when looking up rules.
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
Acked-by: James Carter <jwcart2@gmail.com>
A parameter of a macro was only considered to be a duplicate if it
matched both the name and flavor of another parameter. While it is
true that CIL is able to differentiate between those two parameters,
there is no reason to use the same name for two macro parameters and
it is better to return an error for what is probably an error.
Remove the check of the flavors when checking for duplicate parameters.
Signed-off-by: James Carter <jwcart2@gmail.com>
Acked-by: Ondrej Mosnacek <omosnace@redhat.com>
Type transition file names are stored in a symbol table. Before the
name is added, the symbol table is searched to see if the name had
already been inserted. If it has, then the already existing datum is
returned. If it has not, then the name is added if either the
typetransition rule does not occur in a macro or the name is not one
of the macro parameters.
Checking for a previous insertion before checking if the name is a
macro parameter can cause a macro parameter to be treated as the
actual name if a previous type transition file name is the same as
the parameter.
Now check the name to see if it a macro paramter before checking for
its existence in the symbol table.
Signed-off-by: James Carter <jwcart2@gmail.com>
Acked-by: Ondrej Mosnacek <omosnace@redhat.com>
This reverts commit 542e878690.
After 6968ea9775 ("libsepol: make ebitmap_cardinality() of linear
complexity"), the caching only saves ~0.06 % of total semodule -BN
running time (on x86_64 without using the POPCNT instruction), so it's
no longer worth the added complexity.
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
Acked-by: Stephen Smalley <sds@tycho.nsa.gov>
Create the macro ebitmap_is_empty() to check if an ebitmap is empty.
Use ebitmap_is_empty(), instead of ebitmap_cardinality() or
ebitmap_length(), to check whether or not an ebitmap is empty.
Signed-off-by: James Carter <jwcart2@gmail.com>
Acked-by: Ondrej Mosnacek <omosnace@redhat.com>
As ebitmap_get_bit() complexity is linear in the size of the bitmap, the
complexity of ebitmap_cardinality() is quadratic. This can be optimized
by browsing the nodes of the bitmap directly in ebitmap_cardinality().
While at it, use built-in function __builtin_popcountll() to count the
ones in the 64-bit value n->map for each bitmap node. This seems better
suited than "count++". This seems to work on gcc and clang on x86,
x86_64, ARM and ARM64 but if it causes compatibility issues with some
compilers or architectures (or with older versions of gcc or clang),
the use of __builtin_popcountll() can be replaced by a C implementation
of a popcount algorithm.
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
Detect when the hashtab's load factor gets too high and try to grow it
and rehash it in such case. If the reallocation fails, just keep the
hashtab at its current size, since this is not a fatal error (it will
just be slower).
This speeds up semodule -BN on Fedora from ~8.9s to ~7.2s (1.7 seconds
saved).
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
hashtab_replace() and hashtab_map_remove_on_error() aren't used
anywhere, no need to keep them around...
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
Acked-by: Stephen Smalley <sds@tycho.nsa.gov>
According to profiling of semodule -BN, ebitmap_cardinality() is called
quite often and contributes a lot to the total runtime. Cache its result
in the ebitmap struct to reduce this overhead. The cached value is
invalidated on most modifying operations, but ebitmap_cardinality() is
usually called once the ebitmap doesn't change any more.
After this patch, the time to do 'semodule -BN' on Fedora Rawhide has
decreased from ~10.9s to ~8.9s (2s saved).
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
[sds@tycho.nsa.gov: correct times per follow-up on list]
Acked-by: Stephen Smalley <sds@tycho.nsa.gov>
Followed the following steps:
# In repo client
cd external/selinux
repo sync .
repo start mymerge .
git merge aosp/upstream-master --no-ff # resolve any conflicts
lunch && make -j
repo upload .
Test: compiles and boots
Change-Id: I45fd32fb9bfe2291ef32afcc86f26eb2b24e03e8
The filename_- and range_trans_table ancillary hash tables in
cil_binary.c just duplicate the final policydb content and can be simply
removed.
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
Use one of the policy config bits to tell the kernel to start using
the nlmsg_readpriv on RTM_GETLINK messages instead of the previous
hehavior of using nlmsg_read.
Bug: 141455849
Test: CtsSelinuxTargetSdkCurrentTestCases
Change-Id: I581af58247afe021d906f60153d5971cfaf14ab6
The classperms associated with each map class permission and with each
classpermissionset are verified in __cil_verify_classperms() which had
multiple problems with how it did the verification.
1) Verification was short-circuited when the first normal class is found.
The second classpermissionset statement below would not have been
verified.
(classpermission cp1)
(classpermissionset cp1 (CLASS (PERM)))
(classpermissionset cp1 cp2)
2) The classperms of a map class permission and classpermissionset were
not checked for being NULL before the function recursively called itself.
This would result in a segfault if the missing map or set was referred to
before the classmap or classpermission occured. This error was reported by
Dominick Grift (dominick.grift@defensec.nl).
These rules would cause a segfault.
(classmap cm1 (mp1))
(classmapping cm1 mp1 (cm2 (mp2)))
(classmap cm2 (mp2))
But an error would be produced for these rules.
(classmap cm1 (mp1))
(classmap cm2 (mp2))
(classmapping cm2 mp2 (cm1 (mp1)))
3) The loop detection logic was incomplete and could only detect a loop
with a certain statement ordering.
These rules would cause a stack overflow.
(classmap cm1 (mp1))
(classmapping cm1 mp1 (cm2 (mp2)))
(classmap cm2 (mp2))
(classmapping cm2 mp2 (cm3 (mp3)))
(classmap cm3 (mp3))
(classmapping cm3 mp3 (cm2 (mp2)))
Rewrote __cil_verify_classperms() to fix these errors.
Signed-off-by: James Carter <jwcart2@tycho.nsa.gov>
Add support for new SELinux policy capability genfs_seclabel_symlinks.
With this capability enabled symlinks on kernel filesystems will receive
contexts based on genfscon statements, like directories and files,
and not be restricted to the respective filesystem root sid.
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
The merge from upstream deleted and created some files.
Update the Android.bp file to incorporate those changes.
Test: compiles
Test: treehugger
Change-Id: I52d797cb43b96e5fa6e1237153d6000cc286405d
Incorporate the latest upstream SELinux changes into the Android
build tree.
% git merge aosp/upstream-master --no-ff
A followup commit will make Android specific changes to allow
this code to compile.
- Known issue: -
Multiple implementations of partial directory match digesting.
To fix bug 62302954 ("Investigate restorecon() / selabel_lookup()
performance"), Android implemented a number of patches, including
7d4a56f84d
The upstream SELinux community ended up writing their own variant of
Android's patch, which accomplishes the same thing. This patch is at
e016502c0a
This merge has the effect of having both patches in the tree at the same
time, which could be confusing. However, the new upstream code isn't
used by Android, so there's no impact (other than code bloat and
duplication).
Change-Id: I2bb804b71763c0c3ba77fe48d42787298f85a9d7
Test: treehugger
Remove restrictions in libsepol and checkpolicy that required all
declared initial SIDs to be assigned a context. With this patch,
it is possible to build and load a policy that drops the sid <sidname>
<context> declarations for the unused initial SIDs. It is still
required to retain the sid <sidname> declarations (in the flask
definitions) in order to preserve the initial SID ordering/values.
The unused initial SIDs can be renamed, e.g. to add an unused_
prefix or similar, if desired, since the names used in the policy
are not stored in the kernel binary policy.
In CIL policies, the (sid ...) and (sidorder (...)) statements
must be left intact for compatibility but the (sidcontext ...)
statements for the unused initial SIDs can be omitted after this change.
With current kernels, if one removes an unused initial SID context
from policy, builds policy with this change applied and loads the
policy into the kernel, cat /sys/fs/selinux/initial_contexts/<sidname>
will show the unlabeled context. With the kernel patch to remove unused
initial SIDs, the /sys/fs/selinux/initial_contexts/<sidname>
file will not be created for unused initial SIDs in the first place.
NB If an unused initial SID was assigned a context different from
the unlabeled context in existing policy, then it is not safe to
remove that initial SID context from policy and reload policy on
the running kernel that was booted with the original policy. This
is because that kernel may have assigned that SID to various kernel
objects already and those objects will then be treated as having
the unlabeled context after the removal. In refpolicy, examples
of such initial SIDs are the "fs" SID and the "sysctl" SID. Even
though these initial SIDs are not directly used (in code) by the current
kernel, their contexts are being applied to filesystems and sysctl files by
policy and therefore the SIDs are being assigned to objects.
NB The "sysctl" SID was in use by the kernel up until
commit 8e6c96935fcc1ed3dbebc96fddfef3f2f2395afc ("security/selinux:
fix /proc/sys/ labeling) circa v2.6.39. Removing its context from
policy will cause sysctl(2) or /proc/sys accesses to end up
performing permission checks against the unlabeled context and
likely encounter denials for kernels < 2.6.39.
Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov>
Commit 4459d635b8 ("libsepol: Remove cil_mem_error_handler() function
pointer") replaced cil_mem_error_handler usage with inline contents of
the default handler. However, it left over the header declaration and
two callers. Convert these as well and remove the header declaration.
This also fixes a build failure with -fno-common.
Fixes: 4459d635b8 ("libsepol: Remove cil_mem_error_handler() function pointer")
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
GCC 10 comes with -fno-common enabled by default - fix the CIL_KEY_*
global variables to be defined only once in cil.c and declared in the
header file correctly with the 'extern' keyword, so that other units
including the file don't generate duplicate definitions.
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
When copying an avrule with extended permissions (permx) in
cil_copy_avrule(), the check for a named permx checks the new permx
instead of the old one, so the check will always fail. This leads to a
segfault when trying to copy a named permx because there will be an
attempt to copy the nonexistent permx struct instead of the name of
the named permx.
Check whether the original is a named permx instead of the new one.
Signed-off-by: James Carter <jwcart2@tycho.nsa.gov>
Since failing to resolve a statement in an optional block is normal,
only display messages about the statement failing to resolve and the
optional block being disabled at the highest verbosity level.
These messages are now only at log level CIL_INFO instead of CIL_WARN.
Signed-off-by: James Carter <jwcart2@tycho.nsa.gov>
There's a typo in commit b8213acff8 ("libsepol: add a function to optimize
kernel policy") which added new function sepol_policydb_optimize(), but there's
sepol_optimize_policy in libsepol.map.
LIBSEPOL_3.0 is used to follow the next release version libsepol-3.0
Signed-off-by: Petr Lautrbach <plautrba@redhat.com>
Acked-by: Ondrej Mosnacek <omosnace@redhat.com>
Acked-by: Stephen Smalley <sds@tycho.nsa.gov>
This improves commit b8213acf (libsepol: add a function to optimize
kernel policy) by Ondrej Mosnacek <omosnace@redhat.com> by always
removing redundant conditional rules which have an identical rule
in the unconditional policy.
Add a flag called not_cond to is_avrule_redundant(). When checking
unconditional rules against the avtab (which stores the unconditional
rules) we need to skip the actual rule that we are checking (otherwise
a rule would be determined to be redundant with itself and bad things
would happen), but when checking a conditional rule against the avtab
we do not want to skip an identical rule (which is what currently
happens), we want to remove the redundant permissions in the conditional
rule.
A couple of examples to illustrate when redundant condtional rules
are not removed.
Example 1
allow t1 t2:class1 perm1;
if (bool1) {
allow t1 t2:class1 perm1;
}
The conditional rule is clearly redundant, but without this change it
will not be removed, because of the check for an identical rule.
Example 2
typeattribute t1 a1;
allow t1 t2:class1 perm1;
allow a1 t2:class1 perm1;
if (bool1) {
allow t1 t2:class1 perm1;
}
The conditional rule is again clearly redundant, but now the order of
processing during the optimization will determine whether or not the
rule is removed. Because a1 contains only t1, a1 and t1 are considered
to be supersets of each other. If the rule with the attribute is
processed first, then it will be determined to be redundant and
removed, so the conditional rule will not be removed. But if the rule
with the type is processed first, then it will be removed and the
conditional rule will be determined to be redundant with the rule with
the attribute and removed as well.
The change reduces the size of policy a bit more than the original
optimization. Looking at the change in number of allow rules, there is
about a 10% improvement over the old optimization.
orig old new
Refpolicy 113284 82467 78053
Fedora 106410 64015 60008
Signed-off-by: James Carter <jwcart2@tycho.nsa.gov>
Several static analyzers (clang's one, Facebook Infer, etc.) warn about
NULL pointer dereferences after a call to CU_ASSERT_PTR_NOT_NULL_FATAL()
in the test code written using CUnit framework. This is because this
CUnit macro is too complex for them to understand that the pointer
cannot be NULL: it is translated to a call to CU_assertImplementation()
with an argument as TRUE in order to mean that the call is fatal if the
asserted condition failed (cf.
http://cunit.sourceforge.net/doxdocs/group__Framework.html).
A possible solution could consist in replacing the
CU_ASSERT_..._FATAL() calls by assert() ones, as most static analyzers
know about assert(). Nevertheless this seems to go against CUnit's API.
An alternative solution consists in overriding CU_ASSERT_..._FATAL()
macros in order to expand to assert() after a call to the matching
CU_ASSERT_...() non-fatal macro. This appears to work fine and to remove
many false-positive warnings from various static analyzers.
As this substitution should only occur when using static analyzer, put
it under #ifdef __CHECKER__, which is the macro used by sparse when
analyzing the Linux kernel.
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
Use codespell (https://github.com/codespell-project/codespell) in order
to find many common misspellings that are present in English texts.
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
In test_attr_types, the pointer decl is allowed to be NULL in the
beginning, but is dereferenced to produce a helpful message right before
a CU_ASSERT_FATAL. Make this derefence not happen if the pointer is
NULL.
This issue has been found using clang's static analyzer.
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
semodule-utils/semodule_link/semodule_link.c contains:
static sepol_module_package_t *load_module(char *filename)
{
/* ... */
if (sepol_module_package_create(&p)) {
/* ... */
goto bad;
/* ... */
bad:
sepol_module_package_free(p);
When sepol_module_package_create() fails while having successfully
allocated p, it currently frees p without setting it back to NULL. This
causes a use-after-free in load_module().
Prevent this use-after-free by setting sepol_module_package_create's
argument back to NULL when an error happens.
This issue has been found using Infer static analyzer.
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
Doing this looks wrong:
len = scope->decl_ids_len;
if (scope == NULL) {
/* ... */
Move the dereferencing of scope after the NULL check.
This issue has been found using Infer static analyzer.
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
When strs_stack_init(&stack) fails to allocate memory and stack is still
NULL, it should not be dereferenced with strs_stack_pop(stack).
This issue has been found using Infer static analyzer.
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
As reported by Nicolas Iooss (nicolas.iooss@m4x.org), static analyzers
have problems understanding that the default memory error handler does
not return since it is called through the cil_mem_error_handler()
function pointer. This results in a number of false positive warnings
about null pointer dereferencing.
Since the ability to set the cil_mem_error_handler() is only through
the function cil_set_mem_error_handler() which is never used and whose
definition is not in any header file, remove that function, remove the
use of cil_mem_error_handler() and directly in-line the contents of
the default handler, cil_default_mem_error_handler().
Signed-off-by: James Carter <jwcart2@tycho.nsa.gov>
This patch is loosely based on a patch by Yuli Khodorkovskiy
<yuli@crunchydata.com> from June 13th, 2019.
Since any permission used in the policy should be defined, CIL
should return an error if it cannot resolve a permission used
in a policy. This was the original behavior of CIL.
The behavior was changed over three commits from July to November
2016 (See commits 46e157b47, da51020d6, and 2eefb20d8). The change
was motivated by Fedora trying to remove permissions from its
policy that were never upstreamed (ex/ process ptrace_child and
capability2 compromise_kernel). Local or third party modules
compiled with those permissions would break policy updates.
After three years it seems unlikely that we need to worry about
those local and third party modules and it is time for CIL to
give an error like it should.
Signed-off-by: James Carter <jwcart2@tycho.nsa.gov>
Policy developers can set a default_range default to glblub and
computed contexts will be the intersection of the ranges of the
source and target contexts. This can be used by MLS userspace
object managers to find the range of clearances that two contexts
have in common. An example usage is computing a transition between
the network context and the context of a user logging into an MLS
application.
For example, one can add a default with
this cil:
(defaultrange db_table glblub)
or in te (base module only):
default_range db_table glblub;
and then test using the compute_create utility:
$ ./compute_create system_u:system_r:kernel_t:s0:c1,c2,c5-s0:c1.c20 system_u:system_r:kernel_t:s0:c0.c20-s0:c0.c36 db_table
system_u:object_r:kernel_t:s0:c1,c2,c5-s0:c1.c20
Some example range transitions are:
User Permitted Range | Network Device Label | Computed Label
---------------------|----------------------|----------------
s0-s1:c0.c12 | s0 | s0
s0-s1:c0.c12 | s0-s1:c0.c1023 | s0-s1:c0.c12
s0-s4:c0.c512 | s1-s1:c0.c1023 | s1-s1:c0.c512
s0-s15:c0,c2 | s4-s6:c0.c128 | s4-s6:c0,c2
s0-s4 | s2-s6 | s2-s4
s0-s4 | s5-s8 | INVALID
s5-s8 | s0-s4 | INVALID
Signed-off-by: Joshua Brindle <joshua.brindle@crunchydata.com>
Remove legacy local boolean and user code, and to preserve API/ABI
compatibility the following functions int values should be set to '0'
as they are no longer used:
selinux_mkload_policy(int preservebools)
security_set_boolean_list(.... int permanent)
and the following are now no-op and return '-1':
security_load_booleans()
sepol_genusers()
sepol_set_delusers()
sepol_genbools()
sepol_genbools_array()
and these still return their paths for compatibility, however they are
marked as deprecated:
selinux_booleans_path()
selinux_users_path()
These have been removed as they are local functions only:
sepol_genusers_policydb()
sepol_genbools_policydb()
Also "SETLOCALDEFS" removed from SELinux config file and code.
Signed-off-by: Richard Haines <richard_c_haines@btinternet.com>
In module_to_cil.c, sepol_ppfile_to_module_package() calls functions
from module.c without including the internal header. This makes building
libsepol with "gcc -flto -fuse-ld=gold" fails when linking libsepol.so:
/tmp/ccHYAKVZ.ltrans21.ltrans.o:<artificial>:function
sepol_ppfile_to_module_package: error: undefined reference to
'sepol_module_package_free'
/tmp/ccHYAKVZ.ltrans21.ltrans.o:<artificial>:function
sepol_ppfile_to_module_package: error: undefined reference to
'sepol_module_package_create'
/tmp/ccHYAKVZ.ltrans21.ltrans.o:<artificial>:function
sepol_ppfile_to_module_package: error: undefined reference to
'sepol_module_package_create'
collect2: error: ld returned 1 exit status
Fixes: https://github.com/SELinuxProject/selinux/issues/165
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
Add sepol_policydb_optimize(), which checks a kernel policy for
redundant rules (i.e. those that are covered by an existing more general
rule) and removes them.
Results on Fedora 29 policy:
WITHOUT OPTIMIZATION:
# time semodule -B
real 0m21,280s
user 0m18,636s
sys 0m2,525s
$ wc -c /sys/fs/selinux/policy
8692158 /sys/fs/selinux/policy
$ seinfo (edited)
Allow: 113159
Dontaudit: 10297
Total: 123156
WITH OPTIMIZATION ENABLED:
# time semodule -B
real 0m22,825s
user 0m20,178s
sys 0m2,520s
$ wc -c /sys/fs/selinux/policy
8096158 /sys/fs/selinux/policy
$ seinfo (edited)
Allow: 66334
Dontaudit: 7480
Total: 73814
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
Installing a cil module with invalid mlsconstrain syntax currently
results in a segfault. In the following module, the right-hand side of
the second operand of the OR is a list (mlstrustedobject):
$ cat test.cil
(class test (foo) )
(classorder (unordered test))
(mlsconstrain (test (foo))
(or
(dom h1 h2)
(eq t2 (mlstrustedobject))
)
)
$ sudo semodule -i test.cil
zsh: segmentation fault sudo semodule -i test.cil
This syntax is invalid and should error accordingly, rather than
segfaulting. This patch provides this syntax error for the same module:
$ sudo semodule -i test.cil
t1, t2, r1, r2, u1, u2 cannot be used on the left side with a list on the right side
Bad expression tree for constraint
Bad constrain declaration at /var/lib/selinux/mls/tmp/modules/400/test/cil:4
semodule: Failed!
Signed-off-by: Mike Palmiotto <mike.palmiotto@crunchydata.com>
Followed the following steps:
# In repo client
cd external/selinux
repo sync .
repo start mymerge .
git merge aosp/upstream-master --no-ff # resolve any conflicts
lunch && make -j
repo upload .
Test: compiles and boots
Change-Id: Ia2dbf92e3127aa779bec3c46f171d3ef6c1cbfe5
When validatetrans rule is in CIL policy it errors with:
u3, r3, and t3 can only be used with mlsvalidatetrans rules
Will now resolve these examples:
(validatetrans binder (and (and (eq t1 t1_t) (eq t2 t2_t)) (eq t3 t3_t)))
(mlsvalidatetrans file (and (and (eq t1 t1_t) (eq t2 t2_t))
(and (eq t3 t3_t) (domby h1 h2))))
Signed-off-by: Richard Haines <richard_c_haines@btinternet.com>
Most of the users of ebitmap_for_each_bit() macro only care for the set
bits, so introduce a new ebitmap_for_each_positive_bit() macro that
skips the unset bits. Replace uses of ebitmap_for_each_bit() with the
new macro where appropriate.
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
Followed the following steps:
# In repo client
cd external/selinux
repo sync .
repo start mymerge .
git merge aosp/upstream-master --no-ff # resolve any conflicts
lunch && make -j
repo upload .
Test: compiles and boots
Change-Id: I75ccf5307012a2517c0fdf13bea806e10b8b8595
Additionally, resolve build time errors due to
c19395d722
libselinux: selinux_set_mapping: fix handling of unknown classes/perm
Followed the following steps:
# In repo client
cd external/selinux
repo sync .
repo start mymerge .
git merge aosp/upstream-master --no-ff # resolve any conflicts
lunch && make -j
repo upload .
Test: device boots and no obvious problems.
Change-Id: Ib3a6c086ceadaeaaaf35498d53b2b3e3ad5b8945
Fix compile time error in Android specific code.
../cil/src/cil_write_ast.c: In function ‘__cil_write_node_helper’:
../cil/src/cil_write_ast.c:1156:17: error: variable ‘db’ set but not used [-Werror=unused-but-set-variable]
struct cil_db *db = NULL;
^~
../cil/src/cil_write_ast.c: In function ‘__cil_write_last_child_helper’:
../cil/src/cil_write_ast.c:1465:17: error: variable ‘db’ set but not used [-Werror=unused-but-set-variable]
struct cil_db *db = NULL;
^~
Test: compiles
Change-Id: I0af0e896ba1591ef43726dbee9906d581390e1f3
Followed the following steps:
# In repo client
cd external/selinux
repo sync .
repo start mymerge .
git merge aosp/upstream-master --no-ff # resolve any conflicts
repo upload .
Test: device compiles and boots
Change-Id: If92a0b5e99e69ac0434197fa848b736b9cf0bf77
clang's static analyze reports a use-after-free in
__cil_expr_to_string(), when __cil_expr_to_string_helper() does not
modify its third parameter (variable s1 here) in this loop:
for (curr = curr->next; curr; curr = curr->next) {
__cil_expr_to_string_helper(curr, flavor, &s1);
cil_asprintf(&c2, "%s %s", c1, s1);
free(c1);
free(s1);
c1 = c2;
}
Silence this warning by making sure s1 is always NULL at the beginning
of every iteration of the loop.
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
clang's static analyzer reports a warning when low_bit is used without
having been initialized in statements such as:
low_value = low_bit << 8;
The warning is: "Result of operation is garbage or undefined".
This is caused by low_bit being only initialized when in_range is true.
This issue is not critical because low_value is only used in an
"if (in_range)" block. Silence this warning by moving low_value's
assignment inside this block.
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
These were reported by Petr Lautrbach (plautrba@redhat.com) and this
patch was based on his patch with only a few changes.
Signed-off-by: James Carter <jwcart2@tycho.nsa.gov>
Followed the following steps:
# In repo client
cd external/selinux
repo sync .
repo start mymerge .
git merge aosp/upstream-master --no-ff # resolve any conflicts
repo upload .
Test: device boots and no obvious problems.
Change-Id: I6beff804808e92d1002ead226c7d5c702f373cdc
Followed the following steps:
# In repo client
cd external/selinux
repo sync .
repo start mymerge .
git merge aosp/upstream-master --no-ff # resolve any conflicts
repo upload .
Test: android compiles and boots and no observed problems.
Change-Id: Ic353353e43d9f90921ea39087289fcd5b98521d6
Followed the following steps:
# In repo client
cd external/selinux
repo sync .
repo start mymerge .
git merge aosp/upstream-master --no-ff # resolve any conflicts
repo upload .
Test: android compiles and boots and no observed problems.
Change-Id: Ibdc3617c75af6ba0d93756cec1ad5ae229e3d5e2
Nicolas Iooss reports:
When using checkpolicy to read a binary policy, permissive types are not
written in the output file. In order to reproduce this issue, a test
policy can be written from minimal.cil with the following commands:
$ cd secilc/test/
$ cp minimum.cil my_policy.cil
$ echo '(typepermissive TYPE)' >> my_policy.cil
$ secilc my_policy.cil
$ checkpolicy -bC -o /dev/stdout policy.31
# There is no "(typepermissive TYPE)" in checkpolicy output.
This is because TYPE_FLAGS_PERMISSIVE is added to typdatum->flags only
when loading a module, which uses the permissive flag in the type
properties. A kernel policy defines permissive types in a dedicated
bitmap, which gets loaded as p->permissive_map before the types are
loaded.
The solution is to use the permissive_map bitmap instead of relying on
the flags field of the struct type_datum when writing out CIL or
policy.conf policy from a binary.
Reported-by: Nicolas Iooss <nicolas.iooss@m4x.org>
Signed-off-by: James Carter <jwcart2@tycho.nsa.gov>
In particular, pulls in
3f99b14939
which is needed for b/118685852
Followed the following steps:
# In repo client
cd external/selinux
repo sync .
repo start mymerge .
git merge aosp/upstream-master --no-ff # resolve any conflicts
repo upload .
Bug: 118685852
Test: Android compiles and no obvious problems.
Change-Id: Ic8f1bb2b573a1491b733732653afd7dbe494c85f
The kernel checks if the port is in the range 1-255 when loading an
ibenportcon rule. Add the same check to libsepol.
Fixes: 118c0cd103 ("libsepol: Add ibendport ocontext handling")
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
We need to convert from little-endian before dong range checks on the
ibpkey port numbers, otherwise we would be checking a wrong value on
big-endian systems.
Fixes: 9fbb311276 ("libsepol: Add ibpkey ocontext handling")
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
Followed the following steps:
# In repo client
cd external/selinux
repo sync .
repo start mymerge .
git merge aosp/upstream-master --no-ff # resolve any conflicts
repo upload .
Test: Android compiles and no obvious problems.
Change-Id: I526e8c09eb7cb7f73fe771fd1295bb406514589b
Xen uses the initial SIDs domU and domDM in its toolstack, so it makes
sense to add these to xen_sid_to_str[] in kernel_to_common.h
Signed-off-by: James Carter <jwcart2@tycho.nsa.gov>
When writing CIL from a policy module or when writing CIL or policy.conf
from a kernel binary policy, check that the initial sid index is within
the valid range of the selinux_sid_to_str[] array (or xen_sid_to_str[]
array for a XEN policy). If it is not, then create a unique name
("UNKNOWN"+index) for the initial sid.
Signed-off-by: James Carter <jwcart2@tycho.nsa.gov>
Since the initial sid strings are defined in kernel_to_common.h,
module_to_cil.c can use those and its initial sid string definitions
can be removed.
Signed-off-by: James Carter <jwcart2@tycho.nsa.gov>
Want to make use of selinux_sid_to_str[] and xen_sid_to_str[] from
kernel_to_common.h in module_to_cil.c, but stack functions with the
same names exist in module_to_cil.c and kernel_to_common.c (with
the function prototypes in kernel_to_common.h).
Since the stack functions in kernel_to_common.c are less general and
only work with strings, rename those functions from stack_* to
strs_stack_*.
Signed-off-by: James Carter <jwcart2@tycho.nsa.gov>
Create the function called policydb_sort_ocontexts() that calls
the internal function sort_ocontexts() to sort the ocontexts of
a policydb.
The function sort_ocontexts() is already used by
sepol_kernel_policydb_to_conf() and sepol_kernel_policydb_to_cil()
when converting a binary policy to cil or policy.conf format.
Signed-off-by: James Carter <jwcart2@tycho.nsa.gov>
Followed the following steps:
# In repo client
cd external/selinux
repo sync .
repo start mymerge .
git merge aosp/upstream-master --no-ff # resolve any conflicts
repo upload .
Test: Android compiles and no obvious problems.
Change-Id: I88fac5bcf4a6493672ba95daf8fd982c7b3548b5
Followed the following steps:
# In repo client
cd external/selinux
repo sync .
repo start mymerge .
git merge aosp/upstream-master --no-ff # resolve any conflicts
repo upload .
Test: compiles/boots no problems.
Change-Id: I4cd9f73fbbb818ef7fa07ff8dd183f8a7e892345
When write_binary_policy() fails to open the binary policy, it calls
sepol_handle_destroy(f.handle) but structure f has not been initialized
at this point. Use variable handle instead.
This issue has been found using clang's static analyzer.
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
f.handle is never set in read_binary_policy() so there is no need to
call sepol_handle_destroy() on it. Moreover clang's static analyzer
warns about an uninitialized argument value in the first call.
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
va_copy()'s manpage [1] states:
Each invocation of va_copy() must be matched by a corresponding
invocation of va_end() in the same function.
create_str_helper() is using va_copy() without va_end(). Add the missing
call.
[1] https://linux.die.net/man/3/va_copy
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
sepol_ppfile_to_module_package() does not use its variable "FILE *f =
NULL;" but to fclose() it. This variable has been unneeded since the
introduction of function ppfile_to_module_package() in commit
893851c0a1 ("policycoreutils: add a HLL compiler to convert policy
packages (.pp) to CIL").
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
clang's static analyzer reports that ebitmap_to_names() can call
malloc(0) when the bitmap is empty. If malloc() returns NULL, this
triggers a misleading "Out of memory" error.
Work around this by treating empty bitmaps as appropriate.
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
clang's static analyzer warns about dead assignments to local variables.
In module_to_cil.c, there are some which are quite straightforward to
review. Remove them.
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
When list_prepend() returns an error, it always means it failed to
allocate some memory and does not hold any reference to its argument
data. This argument needs to be freed by the caller in order to prevent
a memory leak.
While reviewing list_prepend() callers, I spend quite some time
understanding why typealiases_gather_map() does not need to strdup(key)
or free(key) when calling list_prepend(..., key) even though "key" comes
from pdb->p_types.table: because typealias_list_destroy() does not free
the inserted items. Add a comment to make this clearer in the code.
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
In cil_tree_print_expr(), "rc < 0" is equivalent to "rc != 0" but
clang's static analyzer does not know about this. Help it.
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
Fix the following build warning:
policydb.c: In function ‘get_symtab_destroy_func’:
policydb.c:1581:9: error: cast between incompatible function types from ‘int (*)(char *, void *, void *)’ to ‘void (*)(char *, void *, void *)’ [-Werror=cast-function-type]
return (hashtab_destroy_func_t) destroy_f[sym_num];
^
It turns out that this function and type are long unused in libsepol
and are not exported APIs for the shared library, so just remove them.
Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov>
When load_users() parses an invalid line with an empty level context
(ie. nothing between "level" and "range" keywords), it allocates memory
with malloc(0) and uses it. The behavior of malloc() in this case is
an unspecified behavior: it might return NULL, which would lead to a
segmentation fault.
Fix this issue by reporting the invalid entry instead. While at it,
ensure that the character before "range" is a space, and change the
logic slightly in order to avoid using "--p; ... p++;".
This issue is reported by clang's static analyzer with the following
message:
genusers.c:222:11: warning: Use of zero-allocated memory
*r++ = *s;
^
genusers.c:225:7: warning: Use of zero-allocated memory
*r = 0;
^
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
In cond_expr_to_cil(), when stack_init() fails to allocate a stack, the
function calls stack_pop() with stack = NULL. Then stack_pop()
dereferences the pointer ("if (stack->pos == -1) {"), which is NULL.
Fix this by moving the stack cleaning loop in a "if (stack != NULL)"
block.
This issue is reported by clang's static analyzer with the following
message:
module_to_cil.c:463:6: warning: Access to field 'pos' results in a
dereference of a null pointer (loaded from variable 'stack')
if (stack->pos == -1) {
^~~~~~~~~~
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
Improve the processing of netifcon, genfscon, ibpkeycon, ibendportcon,
portcon, nodecon, fsuse, filecon, iomemcon, ioportcon, pcidevicecon,
and devicetreecon rules.
If the multiple-decls option is not used then report errors if duplicate
context rules are found. If it is used then remove duplicate context rules
and report errors when two rules are identical except for the context.
This also changes the ordering of portcon and filecon rules. The protocol
of portcon rules will be compared if the port numbers are the same and the
path strings of filecon rules will be compared if the number of meta
characters, the stem length, string length and file types are the same.
Based on an initial patch by Pierre-Hugues Husson (phh@phh.me)
Signed-off-by: James Carter <jwcart2@tycho.nsa.gov>
This commit resolves conflicts in values of expandattribute statements
in policy language and expandtypeattribute in CIL.
For example, these statements resolve to false in policy language:
expandattribute hal_audio true;
expandattribute hal_audio false;
Similarly, in CIL these also resolve to false.
(expandtypeattribute (hal_audio) true)
(expandtypeattribute (hal_audio) false)
A warning will be issued on this conflict.
Motivation
When Android combines multiple .cil files from system.img and vendor.img
it's possible to have conflicting expandattribute statements.
This change deals with this scenario by resolving the value of the
corresponding expandtypeattribute to false. The rationale behind this
override is that true is used for reduce run-time lookups, while
false is used for tests which must pass.
Signed-off-by: Tri Vo <trong@android.com>
Acked-by: Jeff Vander Stoep <jeffv@google.com>
Acked-by: William Roberts <william.c.roberts@intel.com>
Acked-by: James Carter <jwcart2@tycho.nsa.gov>
Nicolas Iooss reports:
In sepol_ibendport_key_create(), if sepol_ibendport_alloc_ibdev_name()
fails to allocate tmp_key->ibdev_name, sepol_ibendport_key_free() is
called to free the memory associated with tmp_key, which results in
free() being called on uninitialized tmp_key->ibdev_name.
This issue is reported by clang's static analyzer with the following
message:
ibendport_record.c:115:2: warning: 1st function call argument is an
uninitialized value
free(key->ibdev_name);
^~~~~~~~~~~~~~~~~~~~~
Signed-off-by: James Carter <jwcart2@tycho.nsa.gov>
Export the sepol_polcap_getnum/name() functions to users of
the shared library. This will enable SETools to stop depending
on the static library.
Note that we may want to move polcaps.h up one level since
the convention is that headers directly under include/sepol are
shared library APIs while headers under include/sepol/policydb
are limited to static users. However, this will unnecessarily
break the build for existing static users so it is deferred.
Suggested-by: Chris PeBenito <pebenito@ieee.org>
Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov>
cil_tree_print_expr() calls cil_expr_to_string() in order to compute a
string expression into expr_str. If this function fails, expr_str is
left unitialized but its value is dereferenced with:
cil_log(CIL_INFO, "%s)", expr_str);
Prevent such an issue by checking cil_expr_to_string()'s return value
before using expr_str.
This issue has been found with clang's static analyzer.
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
This patch solves the following issues:
- The pkg-config files generates odd paths when using DESTDIR without PREFIX
- DESTDIR is needed during compile time to compute library and header paths which it should not.
- Installing with both DESTDIR and PREFIX set gives us odd paths
- Make usage of DESTDIR and PREFIX more standard
Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
Daniel Cashman <dcashman@android.com> discovered the following:
When using cil_db multiple_decls, the different cil_attribute nodes
all point to the same underlying cil_attribute struct. This leads
to problems, though, when modifying the used value in the struct.
__cil_post_db_attr() changes the value of the field to based on
the output of cil_typeattribute_used(), for use later in
cil_typeattribute_to_policydb and cil_typeattribute_to_bitmap, but
due to the multiple declarations, cil_typeattribute_used() could be
called again by a second node. In this second call, the value used
is the modifed value of CIL_TRUE or CIL_FALSE, not the flags actually
needed. This could result in the field being reset again, to an
incorrect CIL_FALSE value.
Add the field "keep" to struct cil_typeattributeset, set its value
using cil_typeattribute_used(), and use it when determining whether
the attribute is to be kept or if it should be expanded.
Signed-off-by: James Carter <jwcart2@tycho.nsa.gov>
When reading policy, ibendport device names are allocated in
ocontext_read_selinux() but they are not freed when calling
sepol_policydb_free();
Fix this by freeing them in ocontext_selinux_free().
Signed-off-by: Jan Zarsky <jzarsky@redhat.com>
cil_android_attributize() uses a source policy to identify types and
attributes to be versioned according to the given version number, and
then proceeds to modify the provided target policy to provide this
versioning. Previously, the versioned policy relied on a base policy
for the declarations of these type[attribute]s, but with multiple_decl
support added, the declarations can be made in both places. This will
make the policy more robust and allow for greater flexibility and the
types of base policy used.
Bug: 37915794
Test: Builds 'n' boots. non_plat_sepolicy.cil now contains the public
type[attribute] declarations.
Change-Id: Iaa084211a9e2774a7ed391888fb21c4c0cf955db
Followed the following steps:
# In repo client
cd external/selinux
repo sync .
repo start mymerge .
git merge aosp/upstream-master --no-ff # resolve any conflicts
repo upload .
Test: compiles/boots no problems.
Change-Id: I4e7ee3fddfe021d128ebd70a663374367914b825
Public types get versioned for Treble compatibility. The versioned
attribute is declared in vendor policy. Then a mapping between the
newer system policy occurs on the system partition. This causes
on-device compilation to fail in AOSP because the vendor image stays
static and thus the mapping file references undeclared typeattributes.
This can be solved by declaring, or redeclaring all versioned types
in the mapping file. Support for this was added to secilc in commit
fafe4c212b 'libsepol: cil: Add ability
to redeclare types[attributes]'
Bug: 67092827
Bug: 37915794
Test: build and boot aosp_sailfish with new type declared in public
policy
Change-Id: I04a884db6d0eaf42ee189b0669833dd0860b626f
* Use -Wno-error= to keep existing warnings to fix later.
Bug: 66996870
Test: build with WITH_TIDY=1
Change-Id: I0e84d5fb2ae2ae68a687a0b6b81eb64a983db57c
Cutting down on the number of attributes associated with each type
speeds up policy lookup times when there is an access vector cache
miss.
This commit reduces the number of attributes in the policy binary from
607 to 159.
(cherry-pick of commit: 574926fe67)
Bug: 36508258
Test: build and boot Marlin
Change-Id: I42c778fe9e760abf62d4ec1f7b7e748e28ac5497
When sepol_bool_query() returns NULL response, variable name is not
freed. Fix this by calling free() before returning.
Signed-off-by: Jan Zarsky <jzarsky@redhat.com>
cil_gen_node() has been using its argument "db" since commit
fafe4c212b ("libsepol: cil: Add ability to redeclare
types[attributes]"). Drop attribute "unused" on this argument.
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
cil_defaults_to_policy() defines its third argument as non-const "char
*kind" even though it is called with literal strings. This makes gcc
report the following warning when compiling with -Wwrite-strings:
../cil/src/cil_policy.c: In function ‘cil_gen_policy’:
../cil/src/cil_policy.c:1931:60: error: passing argument 3 of
‘cil_defaults_to_policy’ discards ‘const’ qualifier from pointer
target type [-Werror=discarded-qualifiers]
cil_defaults_to_policy(out, lists[CIL_LIST_DEFAULT_USER],
"default_user");
^~~~~~~~~~~~~~
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
Since commit 67b410e80f ("libsepol/cil: Keep attributes used by
generated attributes in neverallow rules") gcc reports the following
warning when building libsepol:
../cil/src/cil_post.c: In function
‘__cil_post_db_neverallow_attr_helper’:
../cil/src/cil_post.c:1322:17: error: unused variable ‘db’
[-Werror=unused-variable]
struct cil_db *db = extra_args;
^~
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
When compiling libsepol with clang and some warning flags, the compiler
complains about the way IPv6 addresses are initialized:
kernel_to_cil.c:2795:35: error: suggest braces around initialization
of subobject [-Werror,-Wmissing-braces]
struct in6_addr subnet_prefix = {0};
^
{}
When replacing the initializer as suggested, gcc 4.8.4 complains:
kernel_to_cil.c: In function ‘write_selinux_ibpkey_rules_to_cil’:
kernel_to_cil.c:2795:9: error: missing initializer for field
‘__in6_u’ of ‘struct in6_addr’ [-Werror=missing-field-initializers]
struct in6_addr subnet_prefix = {};
^
Thankfully netinet/in.h provides a macro to initialize struct in6_addr
variables:
#define IN6ADDR_ANY_INIT { { { 0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0 } } }
Both clang and gcc no longer report warnings when using this macro.
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
libsepol is entirely C, so it doesn't need libc++. On the host,
libsepolwrap loads libsepol into the system python, which does not have
the sanitizer runtime.
Test: SANITIZE_HOST=true m treble_sepolicy_tests
Test: mmma external/selinux
Change-Id: If22a6b3d492170871bdb70e71bba8148c607af01
In order to reduce policy size, CIL removes attributes that are not used
by a policy rule in the generated binary policy. However, CIL keeps
attributes used by neverallow rules (which are checked at compile time
and not in the binary policy) even if the attribute is not used anywhere
else in the policy. This behavior is useful to Google who pulls neverallow
rules out of the original policy.conf for compatibility testing, but
converts the policy.conf to CIL and uses the CIL compiler to generate
policy. Without this behavior, the generated binary policy might not have
an attribute referred to by one of the neverallow rules used for testing.
The one exception to this behavior is for attributes generated in
module_to_cil (these have an "_typeattr_" in the middle of their name).
Since these attributes are only created because CIL does not allow a
type expression in an AV rule, they are removed if they only appear in
a neverallow rule (which is the case for most of them) or if the
option to expand generated attributes (-G or --expand-generated) is
specified for secilc when compiling the policy.
Removing generated attributes causes a problem, however, if the type
expression that the generated attribute is replacing uses an attribute
that is removed. In this case, the original neverallow rule will refer
to an attribute that does not exist in the generated binary policy.
Now any non-generated attribute used in a typeattributeset rule for a
generated attribute which is used in a neverallow rule will be treated
like it was used in a neverallow rule.
This does not change the behavior of an expandtypeattribute rule for
the attribute. That rule, if it exists, will take precedence.
Signed-off-by: James Carter <jwcart2@tycho.nsa.gov>
Modify cil_gen_node() to check to see if the cil_db supports multiple
declarations, and if so, to check whether or not the
repeated symbol is eligible to share the existing, already-stored datum. The
only types considered so far are CIL_TYPE and CIL_TYPEATTRIBUTE, both of
which intall empty datums during AST building, so they automatically return
true.
Test: Build policy with multilpe type and attribute declarations, and
without. Policies are binary-identical.
Signed-off-by: Dan Cashman <dcashman@android.com>
Signed-off-by: James Carter <jwcart2@tycho.nsa.gov>
In cil_strpool_destroy(), cil_strpool_tab is freed but it is not reset to NULL.
When cil_strpool_init() is called again it assumes that cil_strpool_tab was
already initialized. Other functions then work with invalid data.
Signed-off-by: Jan Zarsky <jzarsky@redhat.com>
Define the nnp_nosuid_transition policy capability used to enable
SELinux domain transitions under NNP or nosuid if the
nnp_transition permission or nosuid_transition permission is
allowed between the old and new contexts. When this capability is not
enabled, such transitions remain limited to bounded transitions as they
were prior to the introduction of this capability.
Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov>
A build toolchain may override CPPFLAGS on the command line of "make".
Doing so currently breaks libsepol/tests compilation, as it requires
"-I../include/ -I$(CHECKPOLICY)" to be provided in gcc's arguments.
This completes commit 15f2740733 ("Makefiles: override *FLAGS and
*LIBS").
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
There were several places in the makefiles where LDLIBS or CFLAGS were
supposed to include options to build. They were missing the override
keyword so would be skipped if these vars were set on the make cmdline.
Add the override directive to fix this.
Signed-off-by: Jason Zaman <jason@perfinion.com>
