Re: [PATCH 2/2] video/readers/jpeg: Check next_marker is within file size

[Daniel Axtens]

Alec Brown <alec.r.brown@oracle.com> writes:

> In grub-core/video/readers/jpeg.c, the function grub_jpeg_decode_huff_table()
> has the variable next_marker which reads data from grub_jpeg_get_word() and
> then uses it as an upper limit in a while loop. However, the function isn't
> checking that next_marker is within the file size, so this check is being 
> added
> to the function.
>
> Signed-off-by: Alec Brown <alec.r.brown@oracle.com>
> ---
>  grub-core/video/readers/jpeg.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/grub-core/video/readers/jpeg.c b/grub-core/video/readers/jpeg.c
> index 0eeea0e63..c0f95fbf9 100644
> --- a/grub-core/video/readers/jpeg.c
> +++ b/grub-core/video/readers/jpeg.c
> @@ -199,6 +199,12 @@ grub_jpeg_decode_huff_table (struct grub_jpeg_data *data)
>    next_marker = data->file->offset;
>    next_marker += grub_jpeg_get_word (data);
>  
> +  if (next_marker > data->file->size)
> +    {
> +      /* Should never be set beyond the size of the file. */
> +      return grub_error (GRUB_ERR_BAD_FILE_TYPE, "jpeg: invalid next 
> reference");
> +    }
> +

This is a good idea. I briefly wondered why this didn't lead to issues
detected in fuzzing. It'll be because the code reads byte by byte until
we get an EOF and then the loop will bail. Your change is still good, I
just wanted to double check that you hadn't accidentally found and fixed
a more serious bug.

The fuller context:

  while (data->file->offset + sizeof (count) + 1 <= next_marker)
    {
      id = grub_jpeg_get_byte (data);
      if (grub_errno != GRUB_ERR_NONE)
        return grub_errno;


>    while (data->file->offset + sizeof (count) + 1 <= next_marker)
>      {
>        id = grub_jpeg_get_byte (data);
> -- 
> 2.27.0
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel