On Fri, Feb 02, 2024 at 03:45:24PM -0800, Dale Curtis wrote:
> On Fri, Feb 2, 2024 at 3:42 PM Dale Curtis <dalecurtis@chromium.org> wrote:
> 
> > On Fri, Feb 2, 2024 at 3:20 PM Andreas Rheinhardt <
> > andreas.rheinhardt@outlook.com> wrote:
> >
> >> Dale Curtis:
> >> > +    // Clamp allocation size for `chunk_offsets` -- don't throw an
> >> error for an
> >> > +    // invalid count since the EOF path doesn't throw either.
> >> > +    entries =
> >> > +        FFMIN(entries, FFMIN(atom.size - 8, avio_size(pb) -
> >> avio_tell(pb)) /
> >> > +                           (atom.type == MKTAG('s', 't', 'c', 'o') ? 4
> >> : 8));
> >> > +
> >>
> >> This may call avio_size() and avio_tell() multiple times. Furthermore,
> >> is it even certain that avio_size() returns a sane value?
> >>
> >
> > I hope so since there are other usages of avio_size() throughout the file
> > in a similar manner. I guess you're saying it may be invalid when
> > !AVIO_SEEKABLE_NORMAL? Sticking to just atom.size is also fine.
> >
> 
> Here's a version of the patch which does just that.

>  mov.c |    7 +++++++
>  1 file changed, 7 insertions(+)
> d5ba3836202adc762f38f03cbb5e30921e6073b4  stco-clamp-entries-v2.patch
> From b76f526a01788a11e625eb1d7d7005a1959df75c Mon Sep 17 00:00:00 2001
> From: Dale Curtis <dalecurtis@chromium.org>
> Date: Fri, 2 Feb 2024 20:49:44 +0000
> Subject: [PATCH] [mov] Avoid OOM for invalid STCO / CO64 constructions.
> 
> The `entries` value is read directly from the stream and used to
> allocate memory. This change clamps `entries` to however many are
> possible in the remaining atom or file size (whichever is smallest).
> 
> Fixes https://crbug.com/1429357
> 
> Signed-off-by: Dale Curtis <dalecurtis@chromium.org>
> ---
>  libavformat/mov.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/libavformat/mov.c b/libavformat/mov.c
> index af95e1f662..25e5beadcf 100644
> --- a/libavformat/mov.c
> +++ b/libavformat/mov.c
> @@ -2237,6 +2237,13 @@ static int mov_read_stco(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>          av_log(c->fc, AV_LOG_WARNING, "Ignoring duplicated STCO atom\n");
>          return 0;
>      }
> +
> +    // Clamp allocation size for `chunk_offsets` -- don't throw an error for an
> +    // invalid count since the EOF path doesn't throw either.
> +    entries =
> +        FFMIN(entries, (atom.size - 8) /
> +                           (atom.type == MKTAG('s', 't', 'c', 'o') ? 4 : 8));

assuming atom.size is an arbitrary 64bit value
then the value of FFMIN() is also 64bit but entries is unsigned 32bit, this truncation
would allow setting entries to values outside whats expected from FFMIN()
also we seem to disalllow entries == 0 before this
and its maybe possible to set entries = 0 here, bypassing the == 0 check before

thx


[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Concerning the gods, I have no means of knowing whether they exist or not
or of what sort they may be, because of the obscurity of the subject, and
the brevity of human life -- Protagoras