Some invalid policies might have p->p_types.nprim = 0. When parsing
such a policy, "i > p->p_types.nprim - 1" is always false even though
reading p->type_val_to_struct[i] triggers a segmentation fault.
Make type_set_expand() return an error when parsing such a policy by
handling correctly when p->p_types.nprim is zero.
This issue has been found while fuzzing semodule_package with the
American Fuzzy Lop.
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
Some functions assumes that p->global is not NULL. For example
range_read() contains:
p->global->enabled->range_tr_rules = rtr;
However p->global may currently be NULL when loading a policy module
with no avrule block. Avoid a NULL pointer dereference by making such a
policy invalid.
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
If exclude_non_seclabel_mounts() ever gets run on a kernel where
/proc/mounts only contains three columns, mount_info[3] will be used
"without being initialized in "strtok(mount_info[3], ",")" because
variable index would be 3 at the end of this loop:
index = 0;
item = strtok(buf, " ");
while (item != NULL) {
mount_info[index] = item;
if (index == 3)
break;
index++;
item = strtok(NULL, " ");
}
Swap the condition on index and its increment so that it gets to 4 only
when there are at least four columns.
This issue has been found using clang's static analyzer.
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
When security_load_booleans() calls process_boolean() to parse a boolean
definition, process_boolean() returns a successful value when it fails
to use strtok_r() (e.g. when there is no "=" in the parsed line). This
leads security_load_booleans() to use uninitialized name and/or val when
setting the boolean into the policy.
This issue has been found using clang's static analyzer and is similar
to the one which has been fixed in libsepol with commit 76f8c04c19
("libsepol: make process_boolean() fail on invalid lines"). Fix it in
the same way.
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
semanage_module_info_destroy() always returns 0. Nevertheless
semanage_direct_list_all() uses its return value in a surprising way:
cleanup:
if (priorities != NULL) {
/* ... */
free(priorities);
}
/* ... */
ret = semanage_module_info_destroy(sh, modinfo_tmp);
if (ret != 0) {
status = -1;
goto cleanup;
}
The last "goto cleanup;" leads clang's static analyzer to believe a
double free is possible. Even though this is a false positive, the
body of condition "if (ret != 0)" contains dead code. Remove it.
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
selabel_is_digest_set() contains the following code:
digest = calloc(1, sizeof(*digest));
if (!digest)
goto err;
/* ... */
err:
free(digest->digest);
If calloc() failed, digest is NULL but is dereferenced when the
execution jumps to label err.
Check that digest is not NULL before freeing its fields.
This issue has been found using clang's static analyzer.
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
This check is a remnant of the libselinux <2.5 era, back when
is_selinux_enabled() checked whether a policy had been loaded. Nowadays
it only checks whether selinuxfs is mounted, and "load_policy -i"
therefore incorrectly refuses operation when selinuxfs is mounted, but
no policy has been loaded yet.
While it doesn't make much sense to call selinux_init_load_policy()
twice, there's no harm in doing so either, so let's just drop this
safeguard instead of fixing it.
Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov>
commit e5aaa01f81 ("Skip policy module
re-link when only setting booleans.") can lead to duplicate entries
(e.g. portcon entries) being added into the kernel policy because the
existing linked policy already includes the local customizations.
Revert this commit until we can come up with an approach that handles
this properly. This means that setsebool -P triggers a full policy
rebuild.
From the original bug report:
I've noticed a strange interaction with custom ports and booleans.
After setting a boolean, the list of ports for a particular type
(which has been customized) shows duplicate entries.
Example:
$ semanage port -a -t http_port_t -p tcp 12345
$ semanage port -l | grep http_port_t
http_port_t tcp 12345, 80, 81, ...
$ setsebool -P zebra_write_config false
$ semanage port -l | grep http_port_t
http_port_t tcp 12345, 12345, 80, 81, ...
$ setsebool -P zebra_write_config false
$ semanage port -l | grep http_port_t
http_port_t tcp 12345, 12345, 12345, 80, 81, ...
As can be seen, each time a boolean is set persistently (it doesn't
matter which boolean or which state), the custom port 12345 is
duplicated. Running "semodule -B" clears the duplicates.
However, if only the local customizations are listed, the port is
always listed only once:
$ semanage port -l -C
SELinux Port Type Proto Port Number
http_port_t tcp 12345
Resolves: https://github.com/SELinuxProject/selinux/issues/50
Reported-by: Carlos Rodrigues <cefrodrigues@gmail.com>
Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov>
sepol_*_key_free(NULL) should just be a no-op just like
free(NULL). Fix several instances that did not handle this
correctly and would seg fault if called with NULL.
Test: setsebool -P zebra_write_config=1 while non-root
Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov>
CIL does not allow type or role sets in certain rules (such as allow
rules). It does, however, allow sets in typeattributeset and
roleattributeset statements. Because of this, when module_to_cil
translates a policy into CIL, it creates a new attribute for each
set that it encounters. But often the same set is used multiple times
which means that more attributes are created then necessary. As the
number of attributes increases the time required for the kernel to
make each policy decision increases which can be a problem.
To help reduce the number of attributes in a kernel policy,
when module_to_cil encounters a role or type set search to see if the
set was encountered already and, if it was, use the previously
generated attribute instead of creating a new one.
Testing on Android and Refpolicy policies show that this reduces the
number of attributes generated by about 40%.
Signed-off-by: James Carter <jwcart2@tycho.nsa.gov>
Use the same option "-C" used to ouput CIL from a policy.conf, but now
generate CIL from a binary policy instead of giving an error.i
Use the option "-F" to generate a policy.conf file from a binary policy.
Signed-off-by: James Carter <jwcart2@tycho.nsa.gov>
It would sometimes be helpful for debugging or verification purposes
to be able to convert a binary policy to a human-readable form.
Create new function, sepol_kernel_policydb_to_conf(), that takes a
policydb created from a binary policy and writes a policy.conf file
to the provided FILE pointer.
Signed-off-by: James Carter <jwcart2@tycho.nsa.gov>
It would sometimes be helpful for debugging or verification purposes
to be able to convert a binary policy to a human-readable form.
Create new function, sepol_kernel_policydb_to_cil(), that takes a
policydb created from a binary policy and writes CIL policy to the
provided FILE pointer.
Signed-off-by: James Carter <jwcart2@tycho.nsa.gov>
When write_contexts() frees variables context and new_context_str after
a line has been successfully emitted, these variables are not reset to
NULL. This leads the function to free them again if an error occurs when
processing the next line. Fix this by always resetting these variables
at the beginning of the loop.
This issue has been found using clang's static analyzer.
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
If "names = calloc(num_modinfos, sizeof(*names))" fails in
semanage_get_cil_paths(), the function tries to frees items in array
"names" even though it is NULL. Avoid this by returning directly.
This issue has been found using clang's static analyzer.
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
When pipe() fails in semanage_pipe_data(), this function closes all file
descriptors in variables output_fd, err_fd and input_fd even when they
have not been initialized. Fix this by initializing the file descriptors
to -1.
This issue has been found using clang's static analyzer.
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
When sepol_user_add_role() fails to allocate memory for role_cp but
succeeds in reallocating user->roles memory, it frees this reallocated
memory, thus leaving user->roles referencing a free memory block. When
sepol_user_clone() calls sepol_user_free(new_user) because the
allocation failure made sepol_user_add_role() fail, the following code
is executed:
for (i = 0; i < user->num_roles; i++)
free(user->roles[i]);
free(user->roles);
As user->roles has been freed, this code frees pointers which may be
invalid and then tries to free user->roles again.
Fix this flaw by returning right after strdup() failed in
sepol_user_add_role().
This issue has been found using clang's static analyzer.
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
When load_booleans() calls process_boolean() to parse a boolean
definition, process_boolean() returns a successful value when it fails
to use strtok_r() (e.g. when there is no "=" in the parsed line). This
leads load_booleans() to use uninitialized name and/or val when setting
the boolean into the policy.
Rework process_boolean() in order to report errors when a boolean
definition is incorrect.
This issue has been found using clang's static analyzer.
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
In cond_expr_to_cil() when stack_init(&stack) fails, stack is set to
NULL and the execution flow jumps to label "exit". This triggers a call
to stack_pop(stack) which dereferences a NULL pointer in "if (stack->pos
== -1)".
This issue has been found using clang's static analyzer.
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
Don't force output through a pipe - let them access the TTY.
When run interactively, this acts as a workaround for
"Output of fixfiles gets garbled?"
https://bugzilla.redhat.com/show_bug.cgi?id=1435894
E.g. it would also be useful if restorecon ever decides it doesn't want to
output backspace characters on non-TTY outputs.
Signed-off-by: Alan Jenkins <alan.christopher.jenkins@gmail.com>
I suggested that if you run a command for its informational output (by
passing `-v`), you don't expect it to be prefixed with the program name.
Prefixing is used for error messages, so you can tell where your shell
script blew up :). If a script is running a command for its informational
output, it's usually the script's responsibility to make sure it's in
context, e.g. providing headers if there are multiple sections of output.
Removing the program name from setfiles/restorecon output is particularly
useful because it generates very long lines. But also, it actually helps
highlight where there are error messages - the prefix will make them
stand out visually.
Signed-off-by: Alan Jenkins <alan.christopher.jenkins@gmail.com>
I accidently ran `fixfiles "a b"` during testing. Let's fix this too.
Before:
/sbin/fixfiles: line 394: [: a: binary operator expected
Usage: ...
After:
Usage: ...
Signed-off-by: Alan Jenkins <alan.christopher.jenkins@gmail.com>
E.g. `fixfiles restore -v /usr` - before:
Warning: Skipping the following R/O filesystems:
/sys/fs/cgroup
Progress and Verbose mutually exclusive
usage: /sbin/restorecon [-iFnprRv0] [-e excludedir] pathname...
usage: /sbin/restorecon [-iFnprRv0] [-e excludedir] -f filename
Warning: Skipping the following R/O filesystems:
/sys/fs/cgroup
229k
after:
Warning: Skipping the following R/O filesystems:
/sys/fs/cgroup
/sbin/restorecon: lstat(-v) failed: No such file or directory
Warning: Skipping the following R/O filesystems:
/sys/fs/cgroup
229k
This matches the usage shown in the manual page. While we're in there,
we should handle spaces as well e.g `fixfiles restore "a b"`. Before:
Warning: Skipping the following R/O filesystems:
/sys/fs/cgroup
/sbin/restorecon: lstat(b) failed: No such file or directory
After:
Warning: Skipping the following R/O filesystems:
/sys/fs/cgroup
/sbin/restorecon: lstat(a b) failed: No such file or directory
Signed-off-by: Alan Jenkins <alan.christopher.jenkins@gmail.com>
When list_init() fails to allocate a list with calloc(), it calls
list_destroy(&l) with l = NULL. This functions starts by dereferencing
its argument ("(*list)->head"), which does not work well when it is
NULL.
This bug can be fixed by returning directly in list_init() when calloc()
fails. Doing so allows making list_init() implementation shorter by
removing label "exit" and local variable "rc".
This issue has been found using clang's static analyzer.
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
When writing a policy.conf file from CIL source, use hexadecimal
numbers in ioportcon, iomemcon, and pcidevicecon rules.
Signed-off-by: James Carter <jwcart2@tycho.nsa.gov>
Allow the use of hexadecimal numbers in iomemcon, ioportcon, and
pcidevicecon statements. The use of hexadecimal numbers is often
the natural choice for these rules.
A zero base is now passed to strtol() and strtoull() which will
assume base 16 if the string has a prefix of "0x", base 8 if the
string starts with "0", and base 10 otherwise.
Signed-off-by: James Carter <jwcart2@tycho.nsa.gov>
sepol_set_sidtab() is called without calling sepol_sidtab_destroy().
This is not a big deal, since checkpolicy does not run for long, but
it does add noise when checking for other, more important, leaks.
Call sepol_sidtab_destroy() before exiting if not in debug mode.
Signed-off-by: James Carter <jwcart2@tycho.nsa.gov>
cil_resolve_ast() begins by checking whether one of its parameters is
NULL and "goto exit;" when it is the case. As extra_args has not been
initialized there, this leads to calling cil_destroy_tree_node_stack(),
__cil_ordered_lists_destroy()... on garbage values.
In practise this cannot happen because cil_resolve_ast() is only called
by cil_compile() after cil_build_ast() succeeded. As the if condition
exists nonetheless, fix the body of the if block in order to silence a
warning reported by clang Static Analyzer.
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
When compiling a CIL policy which defines conflicting type transitions,
secilc crashes when trying to format an error message with uninitialized
values. This is caused by __cil_typetransition_to_avtab() not
initializing the ..._str fields of its local variable "struct
cil_type_rule trans" before calling __cil_type_rule_to_avtab().
While at it, make the error report clearer about what is wrong by
showing the types and classes which got expanded in
__cil_type_rule_to_avtab(). Here is an example of the result:
Conflicting type rules (scontext=testuser_emacs.subj
tcontext=fs.tmpfs.fs tclass=dir
result=users.generic_tmpfs.user_tmpfs_file),
existing=emacs.tmpfs.user_tmpfs_file
Expanded from type rule (scontext=ARG1 tcontext=fs tclass=ARG3
result=ARG2)
Reported-By: Dominick Grift <dac.override@gmail.com>
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
https://docs.python.org/3/whatsnew/3.6.html#deprecated-python-behavior
A backslash-character pair that is not a valid escape sequence now
generates a DeprecationWarning. Although this will eventually become a
SyntaxError, that will not be for several Python releases.
The problem appears when you use '-W error':
$ python3 -W error -c 'import re; re.findall("[^a-zA-Z0-9_\-\.]", " *%$")'
File "<string>", line 1
SyntaxError: invalid escape sequence \-
Signed-off-by: Ville Skyttä <ville.skytta@iki.fi>
[ Edited commit message as per suggestion from Petr Lautrbach ]
Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov>
HTMLManPages got domain name by splitting name of selinux manpage
on "_selinux" which doesn't work properly when domain name contains
"_selinux".
Signed-off-by: Vit Mojzis <vmojzis@redhat.com>
This makes it possible for static analyzers such as clang's one to
understand that strings_list_add() cannot dereference a NULL pointer in
the following code:
if (!newptr)
exitApp("Out of Memory");
newptr->string = strdup(string);
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
Define the new cgroup_seclabel policy capability used to
enable userspace setting of security labels on cgroup files
via setfscreatecon() aka /proc/self/attr/fscreate and/or
setfilecon() aka setxattr().
Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov>
Currently this Python program triggers a segmentation fault in
libselinux SWIG wrapper:
import selinux
selinux.get_ordered_context_list()
gdb shows that the segmentation fault occurs when freeing some memory:
Reading symbols from python...(no debugging symbols found)...done.
Starting program: /usr/bin/python -c import\
selinux\;selinux.get_ordered_context_list\(\)
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/usr/lib/libthread_db.so.1".
Program received signal SIGSEGV, Segmentation fault.
0x00007ffff789a304 in free () from /usr/lib/libc.so.6
(gdb) bt
#0 0x00007ffff789a304 in free () from /usr/lib/libc.so.6
#1 0x00007ffff6011499 in freeconary (con=0x7ffff6ac5d00) at
freeconary.c:14
#2 0x00007ffff6296899 in _wrap_get_ordered_context_list
(self=<optimized out>, args=<optimized out>) at
selinuxswig_wrap.c:6185
#3 0x00007ffff741891f in _PyCFunction_FastCallDict () from
/usr/lib/libpython3.6m.so.1.0
...
SWIG generated the following code for _wrap_get_ordered_context_list():
char ***arg3 = (char ***) 0 ;
char **temp3 ;
arg3 = &temp3;
if (!PyArg_ParseTuple(args, "OO:get_ordered_context_list",&obj0,&obj1))
SWIG_fail;
/* ... */
fail:
if (*arg3) freeconary(*arg3);
If PyArg_ParseTuple fails, freeconary() is called on the value of
"temp3", which has not been initialized. Fix this by initializing temp
to NULL in the SWIG template.
A similar issue exists with security_get_boolean_names(). Fix it too.
This issue has been found using clang's static analyzer, on a system
which uses SWIG 3.0.12.
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
When building and running tests on a system without SELinux with a
command similar to "make DESTDIR=/tmp/destdir install test", libsemanage
tests fail to build with the following error:
In file included from utilities.h:20:0,
from utilities.c:24:
../src/handle.h:29:26: fatal error: sepol/handle.h: No such file or
directory
#include <sepol/handle.h>
^
Fix this by adding the newly-installed directory under $DESTDIR (using
variable $PREFIX) in the search paths of the compiler.
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
clang's static analyzer reports "Argument with 'nonnull' attribute
passed null" in append_str(), because argument t may be NULL but is used
in a call to memcpy().
Make append_str() do nothing when called with t=NULL.
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
