On Wed, Nov 1, 2023 at 12:39 PM Christian Göttsche
<cgzones@xxxxxxxxxxxxxx> wrote:
>
> 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.
>
> Rework the saturation checks to actually check if there is enough data
> available for the announced size before allocating actual memory.
> This only works for input via mapped memory, since the remaining size
> for streams is not easily available.
>
> 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@xxxxxxxxxxxxxx>
> ---
> libsepol/src/avtab.c | 2 +-
> libsepol/src/context.c | 2 +-
> libsepol/src/module_to_cil.c | 2 +-
> libsepol/src/policydb.c | 16 ++++++++--------
> libsepol/src/private.h | 22 ++++++++++++++++------
> libsepol/src/services.c | 2 +-
> 6 files changed, 28 insertions(+), 18 deletions(-)
>
> diff --git a/libsepol/src/avtab.c b/libsepol/src/avtab.c
> index 6ab49c5e..f379d8d8 100644
> --- a/libsepol/src/avtab.c
> +++ b/libsepol/src/avtab.c
> @@ -601,7 +601,7 @@ int avtab_read(avtab_t * a, struct policy_file *fp, uint32_t vers)
> goto bad;
> }
> nel = le32_to_cpu(buf[0]);
> - if (!nel) {
> + if (zero_or_saturated(nel, fp, sizeof(uint32_t) * 3)) {
I think that I would prefer zero_or_staturated() and is_saturated() to
be left alone and just add the additional check where it can be used.
Like:
if (zero_or_saturated(nel) || exceeds_available_bytes(nel, fp,
sizeof(unit32_t)*3))) {
> ERR(fp->handle, "table is empty");
> goto bad;
> }
> diff --git a/libsepol/src/context.c b/libsepol/src/context.c
> index 5cc90afb..5ee21724 100644
> --- a/libsepol/src/context.c
> +++ b/libsepol/src/context.c
> @@ -297,7 +297,7 @@ int context_from_string(sepol_handle_t * handle,
> char *con_cpy = NULL;
> sepol_context_t *ctx_record = NULL;
>
> - if (zero_or_saturated(con_str_len)) {
> + if (con_str_len == 0 || con_str_len == SIZE_MAX) {
This doesn't need to change if my suggestion above is followed.
> ERR(handle, "Invalid context length");
> goto err;
> }
> diff --git a/libsepol/src/module_to_cil.c b/libsepol/src/module_to_cil.c
> index d2868019..1d0a507c 100644
> --- a/libsepol/src/module_to_cil.c
> +++ b/libsepol/src/module_to_cil.c
> @@ -122,7 +122,7 @@ static int get_line(char **start, char *end, char **line)
>
> for (len = 0; p < end && *p != '\n' && *p != '\0'; p++, len++);
>
> - if (zero_or_saturated(len)) {
> + if (len == 0 || len == SIZE_MAX) {
This wouldn't have to change either.
> rc = 0;
> goto exit;
> }
> diff --git a/libsepol/src/policydb.c b/libsepol/src/policydb.c
> index f608aba4..00a986e8 100644
> --- a/libsepol/src/policydb.c
> +++ b/libsepol/src/policydb.c
> @@ -2854,7 +2854,7 @@ static int ocontext_read_xen(const struct policydb_compat_info *info,
> if (rc < 0)
> return -1;
> c->sid[0] = le32_to_cpu(buf[0]);
> - if (is_saturated(c->sid[0]))
> + if (c->sid[0] > 127)
Where does the 127 come from? Practically, 127 is way more than we
would probably ever have, but I don't think there is a limit.
There might be a case to be made that we should set some reasonable
limits for things like initial sids.
> return -1;
> if (context_read_and_validate
> (&c->context[0], p, fp))
> @@ -2960,7 +2960,7 @@ static int ocontext_read_selinux(const struct policydb_compat_info *info,
> if (rc < 0)
> return -1;
> c->sid[0] = le32_to_cpu(buf[0]);
> - if (is_saturated(c->sid[0]))
> + if (c->sid[0] > 127)
Same comment here.
> return -1;
> if (context_read_and_validate
> (&c->context[0], p, fp))
> @@ -3857,7 +3857,7 @@ static int scope_index_read(scope_index_t * scope_index,
> if (rc < 0)
> return -1;
> scope_index->class_perms_len = le32_to_cpu(buf[0]);
> - if (is_saturated(scope_index->class_perms_len))
> + if (is_saturated(scope_index->class_perms_len, fp, sizeof(uint32_t) * 3))
So:
if (is_saturated(scope_index->class_perms_len) ||
exceeds_available_bytes(scope_index->class_perms_len, fp,
sizeof(uint32_t) * 3)) {
> return -1;
> if (scope_index->class_perms_len == 0) {
> scope_index->class_perms_map = NULL;
> @@ -3913,7 +3913,7 @@ static int avrule_decl_read(policydb_t * p, avrule_decl_t * decl,
> if (rc < 0)
> return -1;
> nprim = le32_to_cpu(buf[0]);
> - if (is_saturated(nprim))
> + if (nprim == UINT32_MAX)
This can then be unchanged.
> return -1;
> nel = le32_to_cpu(buf[1]);
> for (j = 0; j < nel; j++) {
> @@ -4036,7 +4036,7 @@ static int scope_read(policydb_t * p, int symnum, struct policy_file *fp)
> goto cleanup;
> scope->scope = le32_to_cpu(buf[0]);
> scope->decl_ids_len = le32_to_cpu(buf[1]);
> - if (zero_or_saturated(scope->decl_ids_len)) {
> + if (zero_or_saturated(scope->decl_ids_len, fp, sizeof(uint32_t))) {
Again:
if (zero_or_staturated(...) || exceeds_available_bytes()) {
And so on below.
Thanks,
Jim
> ERR(fp->handle, "invalid scope with no declaration");
> goto cleanup;
> }
> @@ -4120,8 +4120,8 @@ int policydb_read(policydb_t * p, struct policy_file *fp, unsigned verbose)
> {
>
> unsigned int i, j, r_policyvers;
> - uint32_t buf[5];
> - size_t len, nprim, nel;
> + uint32_t buf[5], nprim;
> + size_t len, nel;
> char *policydb_str;
> const struct policydb_compat_info *info;
> unsigned int policy_type, bufindex;
> @@ -4315,7 +4315,7 @@ int policydb_read(policydb_t * p, struct policy_file *fp, unsigned verbose)
> if (rc < 0)
> goto bad;
> nprim = le32_to_cpu(buf[0]);
> - if (is_saturated(nprim))
> + if (is_saturated(nprim, fp, sizeof(uint32_t) * 3))
> goto bad;
> nel = le32_to_cpu(buf[1]);
> if (nel && !nprim) {
> diff --git a/libsepol/src/private.h b/libsepol/src/private.h
> index 1833b497..d1fe66b6 100644
> --- a/libsepol/src/private.h
> +++ b/libsepol/src/private.h
> @@ -44,13 +44,23 @@
>
> #define ARRAY_SIZE(x) (sizeof(x)/sizeof((x)[0]))
>
> -#ifdef FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION
> -# define is_saturated(x) (x == (typeof(x))-1 || (x) > (1U << 16))
> -#else
> -# define is_saturated(x) (x == (typeof(x))-1)
> -#endif
> +static inline int exceeds_available_bytes(size_t x, const struct policy_file *fp, size_t req_elem_size)
> +{
> + size_t req_size;
> +
> + /* Remaining input size is only available for mmap'ed memory */
> + if (fp->type != PF_USE_MEMORY)
> + return 0;
> +
> + if (__builtin_mul_overflow(x, req_elem_size, &req_size))
> + return 1;
> +
> + return req_size > fp->len;
> +}
> +
> +#define is_saturated(x, fp, req_elem_size) ((x) == (typeof(x))-1 || exceeds_available_bytes(x, fp, req_elem_size))
>
> -#define zero_or_saturated(x) ((x == 0) || is_saturated(x))
> +#define zero_or_saturated(x, fp, req_elem_size) ((x) == 0 || is_saturated(x, fp, req_elem_size))
>
> #define spaceship_cmp(a, b) (((a) > (b)) - ((a) < (b)))
>
> diff --git a/libsepol/src/services.c b/libsepol/src/services.c
> index 51bd56a0..f9280d89 100644
> --- a/libsepol/src/services.c
> +++ b/libsepol/src/services.c
> @@ -1748,7 +1748,7 @@ int str_read(char **strp, struct policy_file *fp, size_t len)
> int rc;
> char *str;
>
> - if (zero_or_saturated(len)) {
> + if (zero_or_saturated(len, fp, sizeof(char))) {
> errno = EINVAL;
> return -1;
> }
> --
> 2.42.0
>
