In commit b61922f727 ("libsemanage: revert
"Skip policy module re-link when only setting booleans"), we reverted
an optimization for setting booleans since it produced incorrect behavior.
This incorrect behavior was due to operating on the policy with local
changes already merged. However, reverting this change leaves us with
undesirable overhead for setsebool -P. We also have long wanted
to support the same optimization for making other changes that do
not truly require module re-compilation/re-linking.
If we save the linked policy prior to merging local changes, we
can skip re-linking the policy modules in most cases, thereby
significantly improvement the performance and memory overhead of
semanage and setsebool -P commands. Save the linked policy in the
policy sandbox and use it when we are not making a change that requires
recompilation of the CIL modules. With this change, a re-link
is not performed when setting booleans or when adding, deleting, or
modifying port, node, interface, user, login (seusers) or fcontext
mappings. We save linked versions of the kernel policy, seusers,
and users_extra produced from the CIL modules before any local
changes are merged. This has an associated storage cost, primarily
storing an extra copy of the kernel policy file.
Before:
$ time setsebool -P zebra_write_config=1
real 0m8.714s
user 0m7.937s
sys 0m0.748s
After:
$ time setsebool -P zebra_write_config=1
real 0m1.070s
user 0m0.343s
sys 0m0.703s
Resolves: https://github.com/SELinuxProject/selinux/issues/50
Reported-by: Carlos Rodrigues <cefrodrigues@gmail.com>
Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov>
Makes libselinux safer and less likely to leak file descriptors when
used as part of a multithreaded program.
Signed-off-by: Nick Kralevich <nnk@google.com>
In extract_pw_data(), if "getpwuid(uid)" fails, the function returns an
error value without initializing main's pw.pw_name. This leads main() to
call "free(pw.pw_name)" on an uninitialized value.
Use memset() to initialize structure pw in main().
This issue has been found using clang's static analyzer.
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
In main(), if "extract_pw_data(&pw)" returns a failed value, it has
already freed pw.pw_name, pw.pw_dir and pw.pw_shell. These fields are
freed a second time in main's err_free label, which is incorrect. Work
around this by setting them to NULL after they are freed.
This issue has been found using clang's static analyzer.
While at it, make extract_pw_data() static.
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
When getsebool's main() fails to allocate memory for the boolean names,
it returns without freeing variables first, even though other errors do
this (with label "out").
This silences a warning reported by clang's static analyzer.
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
When curcon is NULL, calling strcmp(curcon, newcon) produces an undefined
behavior. Avoid this by checking whether curcon is NULL beforehand.
This issue has been found using clang's static analyzer.
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
cil_gen_default() and cil_gen_defaultrange() call cil_fill_list()
without checking its return value. If it failed, propagate the return
value to the caller.
This issue has been found using clang's static analyzer. It reported
"warning: Value stored to 'rc' is never read" four times.
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
Added "-G, --expand_generated" option to specify that all automatically
generated attributes should be expanded and removed.
Added "-X, --expand_size <SIZE>" option to specify which attributes
are expanded when building a kernel policy. All attributes that have
less types assigned to it than SIZE will be expanded when writing AV
rules.
Signed-off-by: James Carter <jwcart2@tycho.nsa.gov>
Originally, all type attributes were expanded when building a binary
policy. As the policy grew, binary policy sizes became too large, so
changes were made to keep attributes in the binary policy to minimize
policy size.
Keeping attributes works well as long as each type does not have too
many attributes. If an access check fails for types t1 and t2, then
additional checks must be made for every attribute that t1 is a member
of against t2 and all the attributes that t2 is a member of. This is
O(n*m) behavior and there are cases now where this is becoming a
performance issue.
Attributes are more aggressively removed than before. An attribute
will now be removed if it only appears in rules where attributes are
always expanded (typetransition, typechange, typemember, roletransition,
rangetransition, roletype, and AV Rules with self).
Attributes that are used in constraints are always kept because the
attribute name is stored for debugging purposes in the binary policy.
Attributes that are used in neverallow rules, but not in other AV rules,
will be kept unless the attribute is auto-generated.
Attributes that are only used in AV rules other than neverallow rules
are kept unless the number of types assigned to them is less than the
value of attrs_expand_size in the CIL db. The default is 1, which means
that any attribute that has no types assigned to it will be expanded (and
the rule removed from the policy), which is CIL's current behavior. The
value can be set using the function cil_set_attrs_expand_size().
Auto-generated attributes that are used only in neverallow rules are
always expanded. The rest are kept by default, but if the value of
attrs_expand_generated in the CIL db is set to true, they will be
expanded. The function cil_set_attrs_expand_generated() can be used
to set the value.
When creating the binary policy, CIL will expand all attributes that
are being removed and it will expand all attributes with less members
than the value specified by attrs_expand_size. So even if an attribute
is used in a constraint or neverallow and the attribute itself will be
included in the binary policy, it will be expanded when writing AV
rules if it has less members than attrs_expand_size.
Signed-off-by: James Carter <jwcart2@tycho.nsa.gov>
name_list_to_string() and constraint_expr_to_string() both define an
exit label to clean-up dynamically-allocated memory when an error
occurs, but they miss some variables. Free the missing ones too.
This issue has been found using clang's static analyzer.
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
When set_to_names() fails to allocate *names, it frees variable
attr_name even though it either came from attr_list or was newly created
and added to attr_list. By doing so, the name is freed a second time
when attr_list is destroyed (with "attr_list_destroy(&attr_list)").
Avoid this double free by not freeing attr_name when it belongs to
attr_list.
This issue has been found using clang's static analyzer.
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
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>
