Internally class values are stored in multiple placed in a 16-bit wide
integer. Reject class values exceeding the maximum representable value.
This avoids truncations in the helper
policydb_string_to_security_class(), which gets called before validation
of the policy:
policydb.c:4082:9: runtime error: implicit conversion from type 'uint32_t' (aka 'unsigned int') of value 2113929220 (32-bit, unsigned) to type 'sepol_security_class_t' (aka 'unsigned short') changed the value to 4 (16-bit, unsigned)
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
Acked-by: James Carter <jwcart2@gmail.com>
Avoid duplicate policydb initialization when reading a kernel policy.
One caller, main(), already performs the initialization. The other one,
link_module(), needs to do it also for the module policy case.
Also set the target platform to enable module linking.
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
Acked-by: James Carter <jwcart2@gmail.com>
Avoid truncations of the read 32 bit unsigned integer:
conditional.c:764:8: runtime error: implicit conversion from type 'uint32_t' (aka 'unsigned int') of value 3758096384 (32-bit, unsigned) to type 'int' changed the value to -536870912 (32-bit, signed)
conditional.c:831:8: runtime error: implicit conversion from type 'uint32_t' (aka 'unsigned int') of value 4280295456 (32-bit, unsigned) to type 'int' changed the value to -14671840 (32-bit, signed)
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
Acked-by: James Carter <jwcart2@gmail.com>
Similar to unconditional avtab keys check the default type of type av
rules are a simple type, not an attribute.
Since extended permission rules are not allowed in conditional policies
this check does not need to be performed.
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
Acked-by: James Carter <jwcart2@gmail.com>
yum module is not available since RHEL 7.
Drop -systemd related code as it's obsoleted these days - only 2
packages ship their .service in -systemd subpackage
Signed-off-by: Petr Lautrbach <lautrbach@redhat.com>
Acked-by: James Carter <jwcart2@gmail.com>
Acked-by: Ondrej Mosnacek <omosnace@redhat.com>
An extended access vector rule can consist of many individual ranges of
permissions. Use a dynamically growing sized buffer for formatting such
rules instead of a static buffer to avoid write failures due to
truncations.
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
Acked-by: James Carter <jwcart2@gmail.com>
An extended access vector rule can consist of many individual ranges of
permissions. Use a dynamically growing sized buffer for formatting such
rules instead of a static buffer to avoid write failures due to
truncations.
Reported-by: oss-fuzz (issue 64316)
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
Acked-by: James Carter <jwcart2@gmail.com>
In mls_semantic_level_expand(), there is a explicitly determine
whether category is 0, which may cause an potential integer
overflow in error branch.
Signed-off-by: Huaxin Lu <luhuaxin1@huawei.com>
Acked-by: James Carter <jwcart2@gmail.com>
Macros can use classpermission arguments. These are used in two
different ways. Either a named classpermission is passed (which is
declared using a classpermisison rule) or an anonymous classpermission
is passed (something like "(CLASS (PERM))").
Usually this will look like either of the following:
Ex1/
(classpermission cp1)
(classpermisisonset cp1 (CLASS (PERM)))
(macro m1 ((classpermisison ARG1))
(allow t1 self ARG1)
)
(call m1 (cp1))
or
Ex2/
(macro m2 ((classpermission ARG2))
(allow t2 self ARG2)
)
(call m2 ((CLASS (PERM))))
The following would also be valid:
Ex3/
(classpermission cp3)
(macro m3 ((classpermission ARG3))
(classpermissionset ARG3 (CLASS (PERM)))
(allow t3 self ARG3)
)
(call m3 (cp3))
The oss-fuzzer did the equivalent of the following:
(classpermission cp4)
(macro m4 ((classpermission ARG4))
(classpermissionset ARG4 (CLASS (PERM1)))
(allow t4 self ARG4)
)
(call m4 (CLASS (PERM2)))
It passed an anonymous classpermission into a macro where there
was a classpermissionset rule. Suprisingly, everything worked well
until it was time to destroy the AST. There is no way to distinguish
between the anonymous classpermission being passed in which needs
to be destroyed and the classpermission in the classpermissionset
rule which is destroyed when the classpermissionset rule is
destroyed. This led to CIL trying to destroy the classpermission
in the classpermissionset rule twice.
To fix this, when resolving the classpermission name in the
classpermissionset rule, check if the datum returned is for
an anonymous classpermission (it has no name) and return an
error if it is.
This fixes oss-fuzz issue 60670.
Signed-off-by: James Carter <jwcart2@gmail.com>
While still giving an error if there is a declaration with the
same flavor and name as a macro parameter, now give a warning in
the case where there is a declaration with the same name as a
macro parameter, but with a different flavor.
Example/
(macro m1 ((string ARG1))
(type ARG1)
(allow ARG1 ARG1 (CLASS (PERM)))
(typetransition t1a t1b CLASS ARG1 t1c)
)
(call m1 (foo))
This will result in the following equivalent code:
(type ARG1)
(allow ARG1 ARG1 (CLASS (PERM)))
(typetransition t1a t1b CLASS "foo" t1c)
With the warning (if using "-v"), "Declaration of type ARG1 has
same name as a macro parameter with a different flavor"
Signed-off-by: James Carter <jwcart2@gmail.com>
There are many rules in CIL that do not declare an object but
reference a datum or relate two or more datums together. In the
struct for these rules, strings are stored so that the appropriate
datums can be looked up when the rule is resolved. One example is
classcommon, which relates a kernel class and a common class. Often
the datums referenced in these rules will not be needed again, so
there are no pointers to datums in the struct for these rules.
When these rules are in a macro and make use of one of the arguments,
then we do not know the actual value to use when writing out the
AST for the resolve phase or later. Re-resolving the strings to
find the corresponding datums would be complex. If the structs for
these rules had pointers to the datums, then we could use the datums
to write out the correct values.
Add pointers to the datums in the data structures for these rules
and then use the actual datum values when writing out the AST.
Signed-off-by: James Carter <jwcart2@gmail.com>
Remove references to "typealias", "categoryalias", and
sensitivityalias" as valid parameter kinds, because they are not.
Add "string" as a valid parameter kind.
Add a note that "categoryset", "level", "levelrange",
"classpermission", and "ipaddr" can be named or anonymous.
Add a note that "type", "role", and "user" can be used for attributes.
Add a note that "type", "sensitivity" and "category" can be used for
aliases.
Add a note that "string" and "name" can be used for filenames in
typetransition rules and paths in filecon rules.
Signed-off-by: James Carter <jwcart2@gmail.com>
Allow paths in filecon rules to be passed as arguments in macro calls
just like filenames can be passed for named type transition rules.
The paths are handled just like the filenames in named type transition
rules.
Example/
(macro m1 ((string ARG1))
(filecon ARG1 dir (USER ROLE TYPE ((SENS)(SENS))))
)
(call m1 ("/usr/bin"))
Results in the following equivalent rule:
(filecon "/usr/bin" dir (USER ROLE TYPE ((SENS)(SENS))))
Signed-off-by: James Carter <jwcart2@gmail.com>
To support passing a filename as an argument in a macro call that
is to be used in a named type transition, the filename is considered
to be declared when it is used in a named type transition or passed
as an argument with the name flavor. In the struct for a named
type transition, there are fields for a pointer to the filename
string and the filename datum pointer.
When writing out the filename after the resolve phase AST, it is not
possible to determine whether the filename in a named type transition
is an argument name or an actual filename. If it is an actual filename,
then it should be enclosed in double quotes, otherwise, it should
not. Currently, it is always double quoted.
Rework how filenames are declared and handled, so that if the datum
pointer for the name is not NULL, then that is an actual filename
that should be double quoted. Otherwise, the value pointed to by
the string pointer is used and not double quoted.
Move the declaration of the filename to the build phase. Any named
type transition that is not in a macro or is not using a macro
argument is an actual filename, so create a datum and store that in
the struct for the named type transition. Otherwise, store the
string in the named type transition. During the resolve phase,
filename strings can be looked up to find the actual filename that
is being passed into the macro call.
Since the name parameter was never used, just get rid of the
cil_name struct and use datums directly.
Allow either "name" or "string" to be used as the parameter flavor.
Internally, it will be a CIL_DECLARED_STRING and "string" will be
used to write out the AST.
Signed-off-by: James Carter <jwcart2@gmail.com>
For nodecon rules, IP Addresses may be declared without a previous
declaration by enclosing them within parentheses.
Like this: (127.0.0.1) or (::1)
Allow them to also be declared by writing them directly.
Like this: 127.0.0.11 or ::1
This can be done without causing problems with the use of named
IP addresses because identifiers cannot start with a number or
contain a ":".
Signed-off-by: James Carter <jwcart2@gmail.com>
The nodecon statement requires that the IP address and mask values be
enclosed in parentheses so that these values can be distinguished from
named IP addresses. But since an identifier in CIL cannot start with a
number or contain colons, the parentheses are not really required.
Allow IP address and mask values to be written directly and do not
require (but still allow) parentheses around them. Distinguish
between an address or mask and an identifier by checking if the
first character is a number or if the string contains a colon.
Both of these are now valid:
(nodecon (10.0.0.1) (255.255.255.0) (USER ROLE TYPE ((SENS) (SENS))))
(nodecon 10.0.0.1 255.255.255.0 (USER ROLE TYPE ((SENS) (SENS))))
Signed-off-by: James Carter <jwcart2@gmail.com>
Use the same common structure for the ordering rules (classorder,
sidorder, sensitivityorder, and categoryorder). This removes code
duplication and makes it easier to write out the CIL AST.
Simplify the merging of multiple order rules.
Add a verification that checks that the final merged ordering is
fully specified and without ambiguity.
Signed-off-by: James Carter <jwcart2@gmail.com>
In the CIL AST resolve phase, the functions all take a void *
and struct cil_args_resolve * is passed in to them. But in almost
all cases, only the cil_db is needed.
Modify the functions to take struct cil_db * and pass in extra
arguments in the few cases where something more is needed.
Signed-off-by: James Carter <jwcart2@gmail.com>
Reviewed-by: Daniel Burgener <dburgener@linux.microsoft.com>
Acked-by: Petr Lautrbach <lautrbach@redhat.com>
The patch set to change to the hash string function used to DJB2a
caused the ordering of the reported neverallow failures to change
in the libsepol tests.
Changed the expected test results to reflect the new ordering.
See the DJB2a patch series here:
https://lore.kernel.org/all/20230816123845.80171-1-cgzones@googlemail.com/
Signed-off-by: James Carter <jwcart2@gmail.com>
The hash table implementation uses `& (h->size - 1)` to truncate
generated hashes to the number of buckets. This operation is equal to
`% h->size` if and only if the size is a power of two (which seems to be
always the case). One property of the binary and with a power of two
(and probably a small one <=2048) is all higher bits are discarded.
Thus a hash function is needed with a good avalanche effect, which the
current one is not.
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
Acked-by: James Carter <jwcart2@gmail.com>
The hash table implementation uses `& (SIDTAB_SIZE - 1)` to truncate
generated hashes to the number of buckets. This operation is equal to
`% SIDTAB_SIZE` if and only if the size is a power of two (which seems
to be always the case). One property of the binary and with a power of
two (and probably a small one <=2048) is all higher bits are discarded.
Thus a hash function is needed with a good avalanche effect, which the
current one is not.
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
Acked-by: James Carter <jwcart2@gmail.com>
The hash table implementation uses `& (h->size - 1)` to truncate
generated hashes to the number of buckets. This operation is equal to
`% h->size` if and only if the size is a power of two (which seems to be
always the case). One property of the binary and with a power of two
(and probably a small one <=2048) is all higher bits are discarded.
Thus a hash function is needed with a good avalanche effect, which the
current one is not.
Benchmark of building dssp5:
# Current
Time (mean ± σ): 1.347 s ± 0.065 s [User: 1.207 s, System: 0.138 s]
Range (min … max): 1.274 s … 1.436 s 10 runs
# Patch
Time (mean ± σ): 1.336 s ± 0.029 s [User: 1.195 s, System: 0.140 s]
Range (min … max): 1.303 s … 1.376 s 10 runs
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
Acked-by: James Carter <jwcart2@gmail.com>
The hash table implementation uses `& (h->size - 1)` to truncate
generated hashes to the number of buckets. This operation is equal to
`% h->size` if and only if the size is a power of two (which seems to be
always the case). One property of the binary and with a power of two
(and probably a small one <=2048) is all higher bits are discarded.
Thus a hash function is needed with a good avalanche effect, which the
current one is not.
Benchmark of building Reference Policy:
# Current
Benchmark 1: /tmp/destdir/usr/bin/checkpolicy -c 33 -U deny -S -O -E policy.conf -o policy.33
Time (mean ± σ): 2.521 s ± 0.025 s [User: 2.442 s, System: 0.076 s]
Range (min … max): 2.467 s … 2.550 s 10 runs
# Patch
Benchmark 1: /tmp/destdir/usr/bin/checkpolicy -c 33 -U deny -S -O -E policy.conf -o policy.33
Time (mean ± σ): 2.385 s ± 0.031 s [User: 2.303 s, System: 0.081 s]
Range (min … max): 2.353 s … 2.446 s 10 runs
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
Acked-by: James Carter <jwcart2@gmail.com>
Include the chain length squared sum as metric in the debug function
hashtab_hash_eval(), adopted from the kernel avtab.
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
Acked-by: James Carter <jwcart2@gmail.com>
The traditional language and CIL permit common classes only to be
defined with at least one permission. Thus writing a common class
without one will fail.
Reported-by: oss-fuzz (issue 64059)
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
Acked-by: James Carter <jwcart2@gmail.com>
Use their enum values as indices to clarify their relationships.
Specify array size to verify it at compile time.
Remove unnecessary trailing entry, since all access is controlled by a
check against POLICYDB_CAP_MAX.
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
Acked-by: James Carter <jwcart2@gmail.com>
Use a single pointer variable for the realloc(3) result to not
immediately override the source pointer.
Also don't unnecessarily copy the first character.
Reported by Clang Analyzer:
services.c:810:14: warning: Assigned value is garbage or undefined [core.uninitialized.Assign]
810 | **r_buf = **new_buf;
| ^ ~~~~~~~~~
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
Acked-by: James Carter <jwcart2@gmail.com>
In case the member sid_key failed to allocate, free the parent struct.
Reported by Clang Analyzer:
module_to_cil.c:2607:9: warning: Potential leak of memory pointed to by 'item' [unix.Malloc]
2607 | return rc;
| ^~
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
Acked-by: James Carter <jwcart2@gmail.com>
Reported by Clang Analyzer:
is_customizable_type.c:36:3: warning: Potential leak of memory pointed to by 'buf' [unix.Malloc]
36 | fclose(fp);
| ^~~~~~
Fixes: 9911f2ac6f ("libselinux: check for stream rewind failures")
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
Acked-by: James Carter <jwcart2@gmail.com>
Set the number of target names to 0 instead of leaving it uninitialized.
The number is always 0 since CIL does not support non-trivial not-self
neverallow rules yet.
Reported by Clang Analyzer:
module_to_cil.c:1211:18: warning: The right operand of '<' is a garbage value [core.UndefinedBinaryOperatorResult]
1211 | for (t = 0; t < num_tnames; t++) {
| ^ ~~~~~~~~~~
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
Acked-by: James Carter <jwcart2@gmail.com>
In case the initial calloc(3) call fails the variable mods is still NULL
while its size hint num_mods is set.
Reported by Clang Analyzer:
semodule_link.c:182:29: warning: Array access (from variable 'mods') results in a null pointer dereference [core.NullDereference]
182 | sepol_module_package_free(mods[i]);
| ^~~~~~~
Fixes: 63e798a203 ("semodule_link: update")
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
Acked-by: James Carter <jwcart2@gmail.com>
* Do not build test target
Building the test target breaks the whole build since the tests for
libsepol require checkpolicy to be build already:
make[2]: *** No rule to make target '../../checkpolicy/y.tab.o', needed by 'libsepol-tests'. Stop.
make[2]: *** Waiting for unfinished jobs....
Since issues in the test suites are not critical do not build them.
* Update build status reporting
Since the script sets the option -e scan-build will immediately exit
on failure and the informative message "++ Build failed" is not
printed.
* Bump to fortify level 3
* Fix typo
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
Acked-by: James Carter <jwcart2@gmail.com>
Between Debian 11 and 12 the way to install Python packages into the
system location under /usr, and not /usr/local, changed[1]. The
previous setup argument --install-layout=deb is now unsupported and the
environment variable DEB_PYTHON_INSTALL_LAYOUT needs to be set instead.
See also [2].
[1]: https://lists.debian.org/debian-devel/2023/07/msg00307.html
[2]: cbfb31a092
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
Acked-by: James Carter <jwcart2@gmail.com>
Update for commit 494eb683f3 ("libselinux: add getpidprevcon").
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
Acked-by: James Carter <jwcart2@gmail.com>
Currently the GitHub Action vm_testsuite fails:
The requested URL returned error: 404
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
Acked-by: James Carter <jwcart2@gmail.com>
Skip running and setting environment variables for unavailable
interpreters in the env_use_destdir wrapper script to avoid output
like:
$ ./scripts/env_use_destdir $DESTDIR/usr/sbin/getenforce
./scripts/env_use_destdir: 59: ruby: not found
./scripts/env_use_destdir: 59: ruby: not found
Enforcing
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
Acked-by: James Carter <jwcart2@gmail.com>
Check the identifier for initial SIDs is less than the maximum known ID.
The kernel will ignore all unknown IDs, see
security/selinux/ss/policydb.c:policydb_load_isids().
Without checking huge IDs result in OOM events, while writing policies,
e.g. in write_sids_to_conf() or write_sids_to_cil(), due to allocation
of large (continuous) string lists.
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
Acked-by: James Carter <jwcart2@gmail.com>
Several values while parsing kernel policies, like symtab sizes or
string lengths, are checked for saturation. They may not be set to the
maximum value, to avoid overflows or occupying a reserved value, and
many of those sizes must not be 0. This is currently handled via the
two macros is_saturated() and zero_or_saturated().
Both macros are tweaked for the fuzzer, because the fuzzer can create
input with huge sizes. While there is no subsequent data to provide
the announced sizes, which will be caught later, memory of the requested
size is allocated, which would lead to OOM reports. Thus the sizes for
the fuzzer are limited to 2^16. This has the drawback of the fuzzer
not checking the complete input space.
Check the sizes in question for actual enough bytes available in the
input. This is (only) possible for mapped memory, which the fuzzer
uses.
Application like setools do currently not benefit from this change,
since they load the policy via a stream. There are currently multiple
interfaces to load a policy, so reworking them to use mapped memory by
default might be subject for future work.
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
Acked-by: James Carter <jwcart2@gmail.com>
Change the type for the number of primary names in a symtab to uint32_t,
which conforms to the bytes read and the type used in the symtab.
The type is important for the saturation check via is_saturated(), since
it checks against -1 casted to the specific type.
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
Acked-by: James Carter <jwcart2@gmail.com>
Use the internal helper str_read() in more places while reading strings
from a binary policy. This improves readability and helps adjusting
future sanity checks on inputs in fewer places.
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
Acked-by: James Carter <jwcart2@gmail.com>
Kernel policies with unsupported policy capabilities enabled can
currently be parsed, since they result just in a bit set inside an
ebitmap. Writing such a loaded policy into the traditional language or
CIL will fail however, since the unsupported policy capabilities can not
be converted into a name.
Reject kernel policies with invalid policy capabilities.
Reported-by: oss-fuzz (issue 60573)
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
Acked-by: James Carter <jwcart2@gmail.com>
Ensure the ibendport port is not 0 (similar to the kernel).
More general depth test for boolean expressions.
Ensure the boolean id is not set for logic operators.
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
Acked-by: James Carter <jwcart2@gmail.com>
Ensure constraint expressions are complete and do not exceed the
supported depth limit.
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
Acked-by: James Carter <jwcart2@gmail.com>
The default type of a type transition must be a regular type, not an
attribute.
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
Acked-by: James Carter <jwcart2@gmail.com>
