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

[Alec Brown]

On Sat, Oct 22, 2022 at 12:52:02AM +1100, Daniel Axtens wrote:
> 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.
>

I took a look at the code again and realized I made a mistake. There is already
a fix for this issue. Turns out the branch I was using to develop my code was
outdated and this patch is redundant. My bad.

Alec Brown

> 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://urldefense.com/v3/__https://lists.gnu.org/mailman/listinfo/grub-devel__;!!ACWV5N9M2RV99hQ!IlmVbvmGcbJZqlfowZKaM9HBO2MzpJDPrCjfmlLWmsN8tlaAmt5iNq5JZTiU3rQor7YXKdCwTYpAAQ$
> >