On Wed, 29 Jul 2020 19:00:18 +0200
Daniel Kiper <daniel.ki...@oracle.com> wrote:
> From: Peter Jones <pjo...@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
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
