grub-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [SECURITY PATCH 05/28] malloc: Use overflow checking primitives wher


From: Glenn Washburn
Subject: Re: [SECURITY PATCH 05/28] malloc: Use overflow checking primitives where we do complex allocations
Date: Fri, 10 Sep 2021 16:10:33 +0000

On Wed, 29 Jul 2020 19:00:18 +0200
Daniel Kiper <daniel.kiper@oracle.com> wrote:

> From: Peter Jones <pjones@redhat.com>
> 
> This attempts to fix the places where we do the following where
> arithmetic_expr may include unvalidated data:
> 
>   X = grub_malloc(arithmetic_expr);
> 
> It accomplishes this by doing the arithmetic ahead of time using grub_add(),
> grub_sub(), grub_mul() and testing for overflow before proceeding.

...

> diff --git a/grub-core/fs/udf.c b/grub-core/fs/udf.c
> index a83761674..21ac7f446 100644
> --- a/grub-core/fs/udf.c
> +++ b/grub-core/fs/udf.c
> @@ -28,6 +28,7 @@
>  #include <grub/charset.h>
>  #include <grub/datetime.h>
>  #include <grub/udf.h>
> +#include <grub/safemath.h>
>  
>  GRUB_MOD_LICENSE ("GPLv3+");
>  
> @@ -890,9 +891,19 @@ read_string (const grub_uint8_t *raw, grub_size_t sz, 
> char *outbuf)
>       utf16[i] = (raw[2 * i + 1] << 8) | raw[2*i + 2];
>      }
>    if (!outbuf)
> -    outbuf = grub_malloc (utf16len * GRUB_MAX_UTF8_PER_UTF16 + 1);
> +    {
> +      grub_size_t size;
> +
> +      if (grub_mul (utf16len, GRUB_MAX_UTF8_PER_UTF16, &size) ||
> +       grub_add (size, 1, &size))
> +     goto fail;
> +
> +      outbuf = grub_malloc (size);
> +    }
>    if (outbuf)
>      *grub_utf16_to_utf8 ((grub_uint8_t *) outbuf, utf16, utf16len) = '\0';
> +
> + fail:
>    grub_free (utf16);
>    return outbuf;
>  }
> @@ -1005,7 +1016,7 @@ grub_udf_read_symlink (grub_fshelp_node_t node)
>    grub_size_t sz = U64 (node->block.fe.file_size);
>    grub_uint8_t *raw;
>    const grub_uint8_t *ptr;
> -  char *out, *optr;
> +  char *out = NULL, *optr;
>  
>    if (sz < 4)
>      return NULL;
> @@ -1013,14 +1024,16 @@ grub_udf_read_symlink (grub_fshelp_node_t node)
>    if (!raw)
>      return NULL;
>    if (grub_udf_read_file (node, NULL, NULL, 0, sz, (char *) raw) < 0)
> -    {
> -      grub_free (raw);
> -      return NULL;
> -    }
> +    goto fail_1;
>  
> -  out = grub_malloc (sz * 2 + 1);
> +  if (grub_mul (sz, 2, &sz) ||
> +      grub_add (sz, 1, &sz))

The old code did not change the value of sz, but the new code does.
This is a problem because sz is used further down. This is causing udf
symlinks to not be accessible via grub and is causing the udf tests to
fail on the symlink test.

> +    goto fail_0;
> +
> +  out = grub_malloc (sz);
>    if (!out)
>      {
> + fail_0:
>        grub_free (raw);
>        return NULL;
>      }
> @@ -1031,17 +1044,17 @@ grub_udf_read_symlink (grub_fshelp_node_t node)
>      {
>        grub_size_t s;
>        if ((grub_size_t) (ptr - raw + 4) > sz)
> -     goto fail;
> +     goto fail_1;
>        if (!(ptr[2] == 0 && ptr[3] == 0))
> -     goto fail;
> +     goto fail_1;
>        s = 4 + ptr[1];
>        if ((grub_size_t) (ptr - raw + s) > sz)
> -     goto fail;
> +     goto fail_1;
>        switch (*ptr)
>       {
>       case 1:
>         if (ptr[1])
> -         goto fail;
> +         goto fail_1;
>         /* Fallthrough.  */
>       case 2:
>         /* in 4 bytes. out: 1 byte.  */
> @@ -1066,11 +1079,11 @@ grub_udf_read_symlink (grub_fshelp_node_t node)
>         if (optr != out)
>           *optr++ = '/';
>         if (!read_string (ptr + 4, s - 4, optr))
> -         goto fail;
> +         goto fail_1;
>         optr += grub_strlen (optr);
>         break;
>       default:
> -       goto fail;
> +       goto fail_1;
>       }
>        ptr += s;
>      }
> @@ -1078,7 +1091,7 @@ grub_udf_read_symlink (grub_fshelp_node_t node)
>    grub_free (raw);
>    return out;
>  
> - fail:
> + fail_1:
>    grub_free (raw);
>    grub_free (out);
>    grub_error (GRUB_ERR_BAD_FS, "invalid symlink");

I actually noticed in my CI tests that the UDF tests were failing before
the 2.06 release, but I assumed it was something long standing and
didn't look into what was causing it. Here's another example of how my
CI and testing improvements could've made a more solid release.

Glenn



reply via email to

[Prev in Thread] Current Thread [Next in Thread]