Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

RFE: policydb read validation #72

Draft
wants to merge 22 commits into
base: dev
Choose a base branch
from
Draft

RFE: policydb read validation #72

wants to merge 22 commits into from
+940 −362

Conversation

cgzones
Copy link
Contributor

@cgzones cgzones commented Nov 25, 2024

From https://lore.kernel.org/selinux/20241115133619.114393-23-cgoettsche@seltendoof.de/:

With the SELinux namespace feature on the horizon it becomes important
to identify and reject malformed policies at load time. Otherwise
memory corruptions can compromise the kernel or NULL-pointer dereferences
and BUG() encounters can bring systems down. Currently this is not a
security relevant issue since loading a policy requires root privileges
and permission of the current loaded SELinux policy, making it one of the
most privileged operation.

The first 9 patches are cleanup commits with overseeable diffs.

Patch 10 unifies the underlying type used for security class identifiers.

Patch 11 to 21 add various checks at policy load time to reject malformed
policies.

Patch 22 needs some discussion:
It limits the valid set of characters and the length for strings defined
by policies. Currently there are no restrictions, so control characters
are accepted, e.g. Esc as part of a type name, and their length can be
arbitrary. Human formatted security contexts however must not be
arbitrarily long, one example is they must fit in a page size for
selinuxfs interaction and network associations.
Thus the patch introduces the following restrictions:

  • Disallow control characters
  • Limit characters of identifiers to alphanumeric, underscore, dash,
    and dot
  • Limit identifiers in length to 128, expect types to 1024 and
    categories to 32, characters (excluding NUL-terminator)

p.s.:
On a related note to patch 10, the underlying type for types (and type-
attributes) is also not consistent:
In role, range and filename transitions, and the actual datum u32 is
used, while avtables use u16, practically limiting the number of
available types to 65534 (= U16_MAX - 2 (0 and U16_MAX are invalid)).

@pcmoore pcmoore changed the title Dev wip policadb read RFE: policydb read validation Nov 25, 2024
@pcmoore
Copy link
Member

pcmoore commented Nov 25, 2024

It's fine if you want to post this as a draft PR here, but technical discussions and formal patch postings must be done on the selinux@vger.kernel.org mailing list.

cgzones added 10 commits December 16, 2024 15:47
@cgzones
selinux: supply missing field initializers
7dec3b8 
Please clang by supplying the missing field initializes:

    security/selinux/selinuxfs.c:2004:21: warning: missing field 'ops' initializer [-Wmissing-field-initializers]
     2004 |                 /* last one */ {""}
          |                                   ^
    ./security/selinux/include/classmap.h:182:9: warning: missing field 'perms' initializer [-Wmissing-field-initializers]
      182 |         { NULL }
          |                ^

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
@cgzones
selinux: avoid using types indicating user space interaction
aa7eed7 
Integer types starting with a double underscore, like __u32, are
intended for usage of variables interacting with user-space.

Just use the plain variant.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
@cgzones
selinux: align and constify functions
700fbb4 
Align the parameter names between declarations and definitions, and
constify read-only parameters.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
@cgzones
selinux: rework match_ipv6_addrmask()
4244746 
Constify parameters, add size hints, and simplify control flow.

According to godbolt the same assembly is generated.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
@cgzones
selinux: avoid nontransitive comparison
77ff838 
Avoid using nontransitive comparison to prevent unexpected sorting
results due to (well-defined) overflows.
See https://www.qualys.com/2024/01/30/qsort.txt for a related issue in
glibc's qsort(3).

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
@cgzones
selinux: rename comparison functions for clarity
4dd8d99 
The functions context_cmp(), mls_context_cmp() and ebitmap_cmp() are not
traditional C style compare functions returning -1, 0, and 1 for less
than, equal, and greater than; they only return whether their arguments
are equal.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
v2: also convert ebitmap_cmp() as suggested by Daniel
@cgzones
selinux: use known type instead of void pointer
006cadd 
Improve type safety and readability by using the known type.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
v2:
 - move one little diff from subsequent patch to this one
 - reorder struct definition in policydb.h as suggested by Daniel
@cgzones
selinux: avoid unnecessary indirection in struct level_datum
2269778 
Store the owned member of type struct mls_level directly in the parent
struct instead of an extra heap allocation.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
@cgzones
selinux: make use of str_read()
3c9b6ba 
Simplify the call sites, and enable future string validation in a single
place.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
@cgzones
selinux: use u16 for security classes
6af4054 
Security class identifiers are limited to 2^16, thus use the appropriate
type u16 consistently.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
@cgzones cgzones force-pushed the dev_wip_policadb_read branch 3 times, most recently from ed79d0a to ff0a443 Compare December 16, 2024 16:25
@cgzones
selinux: more strict policy parsing
cf94452 
Be more strict during parsing of policies and reject invalid values.

Add some error messages in the case of policy parse failures, to
enhance debugging, either on a malformed policy or a too strict check.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
v2:
  accept unknown xperm specifiers to support backwards compatibility for
  future ones, suggested by Thiébaud
@cgzones cgzones force-pushed the dev_wip_policadb_read branch from ff0a443 to 658ac7d Compare December 16, 2024 16:31
cgzones added 11 commits January 7, 2025 14:52
@cgzones
selinux: check length fields in policies
147b5ef 
In multiple places the binary policy announces how many items of some
kind are to be expected next.  Before reading them the kernel already
allocates enough memory for that announced size.  Validate that the
remaining input size can actually fit the announced items, to avoid OOM
issues on malformed binary policies.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
v3:
  fix error branch by returning directly instead of jumping to goto
  label, see
    https://lore.kernel.org/all/202412241405.LK8YTZqp-lkp@intel.com/
@cgzones
selinux: validate constraints
68c7ee9 
Validate constraint expressions during reading the policy.
Avoid the usage of BUG() on constraint evaluation, to mitigate malformed
policies halting the system.

Closes: SELinuxProject/selinux-testsuite#76

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
@cgzones
selinux: pre-validate conditional expressions
db38021 
Validate conditional expressions while reading the policy, to avoid
unexpected access decisions on malformed policies.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
@cgzones
selinux: introduce ebitmap_highest_set_bit()
49208dc 
Introduce an ebitmap function to calculate the highest set bit.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
@cgzones
selinux: check type attr map overflows
47ed2d5 
Validate that no types with an invalid too high ID are present in the
attribute map.  Gaps are still not checked.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
@cgzones
selinux: reorder policydb_index()
c7d0d82 
Index as soon as possible to enable isvalid() checks to fail on gaps.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
@cgzones
selinux: beef up isvalid checks
c8fa296 
Check that an ID does not refer to a gap in the global array of
definitions.

Constify parameters of isvalid() function and change return type to
bool.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
@cgzones
selinux: validate symbols
21f6dfd 
Some symbol tables need to be validated after indexing, since during
indexing their referenced entries might not yet have been indexed.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
@cgzones
selinux: more strict bounds check
5caf311 
Validate the types used in bounds checks.
Replace the usage of BUG(), to avoid halting the system on malformed
polices.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
@cgzones
selinux: check for simple types
a1c6ee0 
Validate that the target of AVTAB_TYPE rules and file transitions are
simple types and not attributes.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
@cgzones
selinux: restrict policy strings
6a550ad 
Validate the characters and the lengths of strings parsed from binary
policies.

  * Disallow control characters
  * Limit characters of identifiers to alphanumeric, underscore, dash,
    and dot
  * Limit identifiers in length to 128, expect types to 1024 and
    categories to 32, characters (excluding NUL-terminator)

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
v2:
  - add wrappers for str_read() to minimize the usage of magic numbers
  - limit sensitivities to a length of 32, to match categories,
    suggested by Daniel
@cgzones cgzones force-pushed the dev_wip_policadb_read branch from 658ac7d to 6a550ad Compare January 7, 2025 13:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@cgzones @pcmoore