Make it more obvious which parameters are read-only and not being
modified and allow callers to pass const pointers.
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
checkpolicy.c: In function ‘main’:
checkpolicy.c:1000:25: error: ‘tsid’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
1000 | printf("if_sid %d default_msg_sid %d\n", ssid, tsid);
| ^
checkpolicy.c: In function ‘main’:
checkpolicy.c:971:25: error: ‘tsid’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
971 | printf("fs_sid %d default_file_sid %d\n", ssid, tsid);
| ^
Found by GCC 11 with LTO enabled.
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
policy_define.c: In function ‘define_te_avtab_extended_perms’:
policy_define.c:1946:17: error: potential null pointer dereference [-Werror=null-dereference]
1946 | r->omit = omit;
| ^
In the case of `r` being NULL, avrule_read_ioctls() would return
with its parameter `rangehead` being a pointer to NULL, which is
considered a failure in its caller `avrule_ioctl_ranges`.
So it is not necessary to alter the return value.
Found by GCC 11 with LTO enabled.
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
The variable `cladatum` is otherwise always assigned before used, so
these two assignments without a follow up usages are not needed.
Found by clang-analyzer.
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
Follow the project style of no declaration after statement.
Found by the GCC warning -Wdeclaration-after-statement.
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
test/dispol.c:288:4: warning: %d in format string (no. 1) requires 'int' but the argument type is 'unsigned int'. [invalidPrintfArgType_sint]
snprintf(buf, sizeof(buf), "unknown (%d)", i);
^
test/dismod.c:830:4: warning: %d in format string (no. 1) requires 'int' but the argument type is 'unsigned int'. [invalidPrintfArgType_sint]
snprintf(buf, sizeof(buf), "unknown (%d)", i);
^
Found by Cppcheck.
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
The variable `id` is guaranteed to be non-NULL due to the preceding
while condition.
policy_define.c:1171:7: style: Condition '!id' is always false [knownConditionTrueFalse]
if (!id) {
^
policy_define.c:1170:13: note: Assuming that condition 'id=queue_remove(id_queue)' is not redundant
while ((id = queue_remove(id_queue))) {
^
policy_define.c:1171:7: note: Condition '!id' is always false
if (!id) {
^
Found by Cppcheck.
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
checkpolicy.c:504:20: style: The statement 'if (policyvers!=n) policyvers=n' is logically equivalent to 'policyvers=n'. [duplicateConditionalAssign]
if (policyvers != n)
^
checkpolicy.c:505:17: note: Assignment 'policyvers=n'
policyvers = n;
^
checkpolicy.c:504:20: note: Condition 'policyvers!=n' is redundant
if (policyvers != n)
^
Found by Cppcheck
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
The compiler option -pipe does not affect the generated code; it affects
whether the compiler uses temporary files or pipes. As the benefit might
vary from system to system usually its up to the packager or build
framework to set it.
Also these are the only places where the flag is used.
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
Pass CFLAGS when invoking CC at link time, it might contain optimization
or sanitizer flags required for linking.
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
Clang complains:
ibendport_record.c: In function ‘sepol_ibendport_get_ibdev_name’:
ibendport_record.c:169:2: error: ‘strncpy’ specified bound 64 equals destination size [-Werror=stringop-truncation]
169 | strncpy(tmp_ibdev_name, ibendport->ibdev_name, IB_DEVICE_NAME_MAX);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
ibendport_record.c: In function ‘sepol_ibendport_set_ibdev_name’:
ibendport_record.c:189:2: error: ‘strncpy’ specified bound 64 equals destination size [-Werror=stringop-truncation]
189 | strncpy(tmp, ibdev_name, IB_DEVICE_NAME_MAX);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
strncpy(3) does not NUL-terminate the destination if the source is of
the same length or longer then the specified size.
The source of these copies are retrieved from
sepol_ibendport_alloc_ibdev_name(), which allocates a fixed amount of
IB_DEVICE_NAME_MAX bytes.
Reduce the size to copy by 1 of all memory regions allocated by
sepol_ibendport_alloc_ibdev_name().
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
Avoid implicit conversions from signed to unsigned values, found by
UB sanitizers, by using unsigned values in the first place.
expand.c:1644:18: runtime error: implicit conversion from type 'int' of value -1 (32-bit, signed) to type 'uint32_t' (aka 'unsigned int') changed the value to 4294967295 (32-bit, unsigned)
expand.c:2892:24: runtime error: implicit conversion from type 'int' of value -2 (32-bit, signed) to type 'unsigned int' changed the value to 4294967294 (32-bit, unsigned)
policy_define.c:2344:4: runtime error: implicit conversion from type 'int' of value -1048577 (32-bit, signed) to type 'unsigned int' changed the value to 4293918719 (32-bit, unsigned)
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
Unsigned integer overflow is well-defined and not undefined behavior.
But it is still useful to enable undefined behavior sanitizer checks on
unsigned arithmetic to detect possible issues on counters or variables
with similar purpose.
Annotate functions, in which unsigned overflows are expected to happen,
with the respective Clang function attribute[1].
GCC does not support sanitizing unsigned integer arithmetic[2].
avtab.c:76:2: runtime error: unsigned integer overflow: 6 * 3432918353 cannot be represented in type 'unsigned int'
policydb.c:795:42: runtime error: unsigned integer overflow: 8160943042179512010 * 11 cannot be represented in type 'unsigned long'
symtab.c:25:12: runtime error: left shift of 1766601759 by 4 places cannot be represented in type 'unsigned int'
[1]: https://clang.llvm.org/docs/AttributeReference.html#no-sanitize
[2]: https://gcc.gnu.org/onlinedocs/gcc/Instrumentation-Options.html
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
Unsigned integer overflow is well-defined and not undefined behavior.
It is commonly used for hashing or pseudo random number generation.
But it is still useful to enable undefined behavior sanitizer checks on
unsigned arithmetic to detect possible issues on counters or variables
with similar purpose or missed overflow checks on user input.
Use a spaceship operator like comparison instead of subtraction.
policydb.c:851:24: runtime error: unsigned integer overflow: 801 - 929 cannot be represented in type 'unsigned int'
Follow-up of: 1537ea8412 ("libsepol: avoid unsigned integer overflow")
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
commits 37863b0b14 (libsepol/cil:
Improve degenerate inheritance check) and
74d00a8dec (libsepol/cil: Detect
degenerate inheritance and exit with an error) attempted to detect
and exit with an error when compiling policies that have degenerate
inheritances. These policies result in the exponential growth of memory
usage while copying the blocks that are inherited.
There were two problems with the previous attempts to detect this
bad inheritance problem. The first is that the quick check using
cil_possible_degenerate_inheritance() did not detect all patterns
of degenerate inheritance. The second problem is that the detection
of inheritance loops during the CIL_PASS_BLKIN_LINK pass did not
detect all inheritance loops which made it possible for the full
degenerate inheritance checking done with
cil_check_for_degenerate_inheritance() to have a stack overflow
when encountering the inheritance loops. Both the degenerate and
loop inheritance checks need to be done at the same time and done
after the CIL_PASS_BLKIN_LINK pass. Otherwise, if loops are being
detected first, then a degenerate policy can cause the consumption
of all system memory and if degenerate policy is being detected
first, then an inheritance loop can cause a stack overflow.
With the new approach, the quick check is eliminated and the full
check is always done after the CIL_PASS_BLKIN_LINK pass. Because
of this the "inheritance_check" field in struct cil_resolve_args
is not needed and removed and the functions
cil_print_recursive_blockinherit(), cil_check_recursive_blockinherit(),
and cil_possible_degenerate_inheritance() have been deleted. The
function cil_count_potential() is renamed cil_check_inheritances()
and has checks for both degenerate inheritance and inheritance loops.
The inheritance checking is improved and uses an approach similar
to commit c28525a26f (libsepol/cil:
Properly check for loops in sets).
As has been the case with these degenerate inheritance patches,
these issues were discovered by the secilc-fuzzer.
Signed-off-by: James Carter <jwcart2@gmail.com>
On Ubuntu 20.04, when building with clang -Werror -Wextra-semi-stmt
(which is not the default build configuration), the compiler reports:
mcstransd.c:72:35: error: empty expression statement has no effect;
remove unnecessary ';' to silence this warning
[-Werror,-Wextra-semi-stmt]
log_debug("%s\n", "cleanup_exit");
^
Replace the empty log_debug substitution with a do { ... } while (0)
construction to silence this warning.
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
On Ubuntu 20.04, when building with clang -Werror -Wextra-semi-stmt
(which is not the default build configuration), the compiler reports:
secon.c:686:3: error: empty expression statement has no effect;
remove unnecessary ';' to silence this warning
[-Werror,-Wextra-semi-stmt]
};
^
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
On Ubuntu 20.04, when building with clang -Werror -Wextra-semi-stmt
(which is not the default build configuration), the compiler reports:
checkpolicy.c:740:33: error: empty expression statement has no
effect; remove unnecessary ';' to silence this warning
[-Werror,-Wextra-semi-stmt]
FGETS(ans, sizeof(ans), stdin);
^
Introduce "do { } while (0)" blocks to silence such warnings.
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
On Ubuntu 20.04, when building with clang -Werror -Wextra-semi-stmt
(which is not the default build configuration), the compiler reports:
genhomedircon.c:742:67: error: empty expression statement has no
effect; remove unnecessary ';' to silence this warning
[-Werror,-Wextra-semi-stmt]
const semanage_seuser_t **u2 = (const semanage_seuser_t **) arg2;;
^
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
On Ubuntu 20.04, when building with clang -Werror -Wextra-semi-stmt
(which is not the default build configuration), the compiler reports:
sha1.c:90:21: error: empty expression statement has no effect;
remove unnecessary ';' to silence this warning
[-Werror,-Wextra-semi-stmt]
R0(a,b,c,d,e, 0); R0(e,a,b,c,d, 1); R0(d,e,a,b,c, 2); R0(c,d,e,a,b, 3);
^
In file included from selinux_restorecon.c:39:
./label_file.h:458:15: error: empty expression statement has no
effect; remove unnecessary ';' to silence this warning
[-Werror,-Wextra-semi-stmt]
lineno);
^
Introduce "do { } while (0)" blocks to silence such warnings.
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
On Ubuntu 20.04, when building with clang -Werror -Wextra-semi-stmt
(which is not the default build configuration), the compiler reports:
../cil/src/cil_binary.c:4293:22: error: empty expression statement
has no effect; remove unnecessary ';' to silence this warning
[-Werror,-Wextra-semi-stmt]
mix(k->target_class);
^
../cil/src/cil_binary.c:4294:21: error: empty expression statement
has no effect; remove unnecessary ';' to silence this warning
[-Werror,-Wextra-semi-stmt]
mix(k->target_type);
^
../cil/src/cil_binary.c:4295:21: error: empty expression statement
has no effect; remove unnecessary ';' to silence this warning
[-Werror,-Wextra-semi-stmt]
mix(k->source_type);
^
../cil/src/cil_binary.c:4296:19: error: empty expression statement
has no effect; remove unnecessary ';' to silence this warning
[-Werror,-Wextra-semi-stmt]
mix(k->specified);
^
Use a do { ... } while (0) construction to silence this warning.
Moreover the same warning appears when using two semicolons to end a
statement. Remove such occurrences, like what was already done in commit
811185648a ("libsepol: drop repeated semicolons").
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
When __cil_verify_map_class() verifies a classpermission, it calls
__verify_map_perm_classperms() on each item. If the first item reports a
failure and the next one succeeds, the failure is overwritten in
map_args->rc. This is a bug which causes a NULL pointer dereference in
the CIL compiler when compiling the following policy:
(sid SID)
(sidorder (SID))
(class CLASS (PERM1))
(classorder (CLASS))
(classpermission CLSPERM)
(classpermissionset CLSPERM (CLASS (PERM1)))
(classmap files (CLAMAPxx x))
(classmapping files CLAMAPxx CLSPERM)
Fixes: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=30286
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
Fix the following build failure with gcc 4.8 which is raised since
version 3.2 and
156dd0de5c
getseuser.c:53:2: error: 'for' loop initial declarations are only allowed in C99 mode
for (int i = 0; i < n; i++)
^
Fixes:
- http://autobuild.buildroot.org/results/37eb0952a763256fbf6ef3c668f6c95fbdf2dd35
Signed-off-by: Fabrice Fontaine <fontaine.fabrice@gmail.com>
Provide the option "-Q" or "--qualified-names" to indicate that the
policy is using qualified names.
Using qualified names means that declaration names can have "dots"
in them, but blocks, blockinherits, blockabstracts, and in-statements
are not allowed in the policy.
The libsepol function cil_set_qualified_names() is called with the
desired value for the CIL db's "qualified_names" field.
Signed-off-by: James Carter <jwcart2@gmail.com>
Provide the option "-Q" or "--qualified-names" to indicate that the
policy is using qualified names.
Using qualified names means that declaration names can have "dots"
in them, but blocks, blockinherits, blockabstracts, and in-statements
are not allowed in the policy.
The libsepol function cil_set_qualified_names() is called with the
desired value for the CIL db's "qualified_names" field.
Signed-off-by: James Carter <jwcart2@gmail.com>
Provide the option "-Q" or "--qualified-names" to indicate that the
policy is using qualified names.
Using qualified names means that declaration names can have "dots"
in them, but blocks, blockinherits, blockabstracts, and in-statements
are not allowed in the policy.
The libsepol function cil_set_qualified_names() is called with the
desired value for the CIL db's "qualified_names" field.
Signed-off-by: James Carter <jwcart2@gmail.com>
Qualified names have "dots" in them. They are generated when a CIL
policy is compiled and come from declarations in blocks. If a kernel
policy is decompiled into a CIL policy, the resulting policy could
have declarations that use qualified names. Compiling this policy would
result in an error because "dots" in declarations are not allowed.
Qualified names in a policy are normally used to refer to the name of
identifiers, blocks, macros, or optionals that are declared in a
different block (that is not a parent). Name resolution is based on
splitting a name based on the "dots", searching the parents up to the
global namespace for the first block using the first part of the name,
using the second part of the name to lookup the next block using the
first block's symbol tables, looking up the third block in the second's
symbol tables, and so on.
To allow the option of using qualified names in declarations:
1) Create a field in the struct cil_db called "qualified_names" which
is set to CIL_TRUE when qualified names are to be used. This field is
checked in cil_verify_name() and "dots" are allowed if qualified names
are being allowed.
2) Only allow the direct lookup of the whole name in the global symbol
table. This means that blocks, blockinherits, blockabstracts, and in-
statements cannot be allowed. Use the "qualified_names" field of the
cil_db to know when using one of these should result in an error.
3) Create the function cil_set_qualified_names() that is used to set
the "qualified_names" field. Export the function in libsepol.
Signed-off-by: James Carter <jwcart2@gmail.com>
When disabling optionals, the AST needs to be reset only if one
of the optional blocks being disabled contains a declaration.
Call the function cil_tree_subtree_has_decl() for each optional
block being disabled and only reset the AST if one of them has
a declaration in it.
Signed-off-by: James Carter <jwcart2@gmail.com>
Create the function cil_tree_subtree_has_decl() that returns CIL_TRUE
if the subtree has a declaration in it and CIL_FALSE otherwise.
Signed-off-by: James Carter <jwcart2@gmail.com>
The commit 74d00a8dec (libsepol/cil:
Detect degenerate inheritance and exit with an error) detects the
use of inheritance (mostly by the secilc-fuzzer and not in any real
policies) that results in the exponential growth of the policy through
the copying of blocks that takes place with inheritance in CIL.
Unfortunately, the check takes place during the pass when all the
blocks are being copied, so it is possible to consume all of a system's
memory before an error is produced.
The new check happens in two parts. First, a check is made while the
block inheritance is being linked to the block it will inherit. In
this check, all of the parent nodes of the inheritance rule up to the
root node are checked and if enough of these blocks are being inherited
(>= CIL_DEGENERATE_INHERITANCE_DEPTH), then a flag is set for a more
in-depth check after the pass. This in-depth check will determine the
number of potential inheritances that will occur when resolving the
all of the inheritance rules. If this value is greater than
CIL_DEGENERATE_INHERITANCE_GROWTH * the original number of inheritance
rules and greater than CIL_DEGENERATE_INHERITANCE_MINIMUM (which is
set to 0x1 << CIL_DEGENERATE_INHERITANCE_DEPTH), then degenerate
inheritance is determined to have occurred and an error result will
be returned.
Since the potential number of inheritances can quickly be an extremely
large number, the count of potential inheritances is aborted as soon
as the threshold for degenerate inheritance has been exceeded.
Normal policies should rarely, if ever, have the in-depth check occur.
Signed-off-by: James Carter <jwcart2@gmail.com>
Acked-by: Nicolas Iooss <nicolas.iooss@m4x.org>
It is possible to create bad behaving policy that can consume all
of a system's memory (one way is through the use of inheritance).
Analyzing these policies shows that most of the memory usage is for
the block symtabs.
Most of the nineteen symtabs will most likely never be used, so give
these symtabs an initial size of 1. The others are given more
appropriate sizes.
Signed-off-by: James Carter <jwcart2@gmail.com>
Acked-by: Nicolas Iooss <nicolas.iooss@m4x.org>
When marking a type attribute as used in a neverallow (to help determine
whether or not it should be expanded), check if the attribute's expression
list is empty (no attributes are associated with it) before iterating
over the list.
Signed-off-by: James Carter <jwcart2@gmail.com>
Acked-by: Nicolas Iooss <nicolas.iooss@m4x.org>
When "glblub" was added as a default for the defaultrange rule, the
syntax array was updated because the "glblub" default does not need
to specify a range of "low", "high", or "low-high". Unfortunately,
additional checking was not added for the "source" and "target"
defaults to make sure they specified a range. This means that using
the "source" or "target" defaults without specifying the range will
result in a segfault.
When the "source" or "target" defaults are used, check that the rule
specifies a range as well.
This bug was found by the secilc-fuzzer.
Signed-off-by: James Carter <jwcart2@gmail.com>
Acked-by: Nicolas Iooss <nicolas.iooss@m4x.org>
Commit 61fbdce666 (ibsepol/cil: Check
for self-referential loops in sets) added checks for self-referential
loops in user, role, type, and category sets. Unfortunately, this
check ends up in an infinite loop if the set with the self-referential
loop is used in a different set that is checked before the bad set.
The problem with the old check is that only the initial datum is used
for the check. Instead, use a stack to track all of the set datums
that are currently involved as the check is made. A self-referential
loop occurs if a duplicate datum is found for any of the datums in the
stack.
Signed-off-by: James Carter <jwcart2@gmail.com>
Acked-by: Nicolas Iooss <nicolas.iooss@m4x.org>
The commit d155b410d4 (libsepol/cil:
Check for duplicate blocks, optionals, and macros) fixed a bug
that allowed duplicate blocks, optionals, and macros with the same
name in the same namespace. For blocks and macros, a duplicate
is always a problem, but optional block names are only used for
in-statement resolution. If no in-statement refers to an optional
block, then it does not matter if more than one with same name
exists.
One easy way to generate multiple optional blocks with the same
name declaration is to call a macro with an optional block multiple
times in the same namespace.
As an example, here is a portion of CIL policy
(macro m1 ((type t))
(optional op1
(allow t self (CLASS (PERM)))
)
)
(type t1)
(call m1 (t1))
(type t2)
(call m1 (t2))
This will result in two optional blocks with the name op1.
There are three parts to allowing multiple optional blocks with
the same name declaration.
1) Track an optional block's enabled status in a different way.
One hinderance to allowing multiple optional blocks with the same
name declaration is that they cannot share the same datum. This is
because the datum is used to get the struct cil_optional which has
the enabled field and each block's enabled status is independent of
the others.
Remove the enabled field from struct cil_optional, so it only contains
the datum. Use a stack to track which optional blocks are being
disabled, so they can be deleted in the right order.
2) Allow multiple declarations of optional blocks.
Update cil_allow_multiple_decls() so that a flavor of CIL_OPTIONAL
will return CIL_TRUE. Also remove the check in cil_copy_optional().
3) Check if an in-statement refers to an optional with multiple
declarations and exit with an error if it does.
Signed-off-by: James Carter <jwcart2@gmail.com>
Acked-by: Nicolas Iooss <nicolas.iooss@m4x.org>
Make it more apparent that those data does not change and enforce it.
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
Acked-by: James Carter <jwcart2@gmail.com>
Clang issues:
module_to_cil.c:65:7: warning: no previous extern declaration for non-static variable 'out_file' [-Wmissing-variable-declarations]
FILE *out_file;
^
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
Acked-by: James Carter <jwcart2@gmail.com>
`hashtab_search()` does take `const_hashtab_key_t` as second parameter,
which is a typedef for `const char *`.
Drop the unnecessary and const-violating cast.
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
Acked-by: James Carter <jwcart2@gmail.com>
Initialize variables, as they are set after goto statements, which jump
to cleanup code using them.
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
Acked-by: James Carter <jwcart2@gmail.com>
`const_hashtab_key_t` is a typedef of `const char *`, so these casts are
not needed.
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
Acked-by: James Carter <jwcart2@gmail.com>
../cil/src/cil_binary.c:2230:24: warning: Value stored to 'cb_node' during its initialization is never read [deadcode.DeadStores]
struct cil_tree_node *cb_node = node->cl_head;
^~~~~~~ ~~~~~~~~~~~~~
Found by clang-analyzer
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
Acked-by: James Carter <jwcart2@gmail.com>
../cil/src/cil_write_ast.c:86:32: error: cast to smaller integer type 'enum cil_flavor' from 'void *' [-Werror,-Wvoid-pointer-to-enum-cast]
enum cil_flavor op_flavor = (enum cil_flavor)curr->data;
^~~~~~~~~~~~~~~~~~~~~~~~~~~
../cil/src/cil_write_ast.c:130:37: error: cast to smaller integer type 'enum cil_flavor' from 'void *' [-Werror,-Wvoid-pointer-to-enum-cast]
enum cil_flavor operand_flavor = (enum cil_flavor)curr->data;
^~~~~~~~~~~~~~~~~~~~~~~~~~~
Silence this warning by casting the pointer to an integer the cast to
enum cil_flavor.
See 32f8ed3d6b ("libsepol/cil: introduce intermediate cast to silence -Wvoid-pointer-to-enum-cast")
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
Acked-by: James Carter <jwcart2@gmail.com>
