Re: [FFmpeg-devel] [PATCH] [mov] Avoid OOM for invalid STCO / CO64 constructions.
On Fri, Feb 16, 2024 at 01:41:35PM -0800, Dale Curtis wrote: > On Thu, Feb 15, 2024 at 2:35 PM Michael Niedermayer > wrote: > > > FFMIN/MAX can evaluate their arguments multiple times so avio_rb32() might > > be executed more than once > > > > Thanks. Good catch. Fixed. > mov.c |7 +++ > 1 file changed, 7 insertions(+) > 08ba396380cc14f3df2bdd4a638c43c1c521b8fc stco-clamp-entries-v4.patch > From b94e542582e375025c59862cee58ec45d39c9cd6 Mon Sep 17 00:00:00 2001 > From: Dale Curtis > Date: Fri, 2 Feb 2024 20:49:44 + > 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 > --- > libavformat/mov.c | 7 +++ > 1 file changed, 7 insertions(+) will apply thx [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB It is a danger to trust the dream we wish for rather than the science we have, -- Dr. Kenneth Brown signature.asc Description: PGP signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] [mov] Avoid OOM for invalid STCO / CO64 constructions.
On Thu, Feb 15, 2024 at 2:35 PM Michael Niedermayer wrote: > FFMIN/MAX can evaluate their arguments multiple times so avio_rb32() might > be executed more than once > Thanks. Good catch. Fixed. stco-clamp-entries-v4.patch Description: Binary data ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] [mov] Avoid OOM for invalid STCO / CO64 constructions.
On Thu, Feb 15, 2024 at 12:07:05PM -0800, Dale Curtis wrote:
> On Mon, Feb 5, 2024 at 12:07 PM Michael Niedermayer
> wrote:
>
> > 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
>
>
> Thanks. I've moved the clamp up to before the zero check. The only way a
> bad 64-bit value could get in is if atom.size < 8, which I didn't think was
> possible, but I've added a FFMAX(0,) there too.
[...]
> +FFMIN(avio_rb32(pb),
> + FFMAX(0, (atom.size - 8) /
> + (atom.type == MKTAG('s', 't', 'c', 'o') ? 4 :
> 8)));
FFMIN/MAX can evaluate their arguments multiple times so avio_rb32() might
be executed more than once
thx
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
The real ebay dictionary, page 1
"Used only once"- "Some unspecified defect prevented a second use"
"In good condition" - "Can be repaird by experienced expert"
"As is" - "You wouldnt want it even if you were payed for it, if you knew ..."
signature.asc
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] [mov] Avoid OOM for invalid STCO / CO64 constructions.
On Mon, Feb 5, 2024 at 12:07 PM Michael Niedermayer
wrote:
> 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
Thanks. I've moved the clamp up to before the zero check. The only way a
bad 64-bit value could get in is if atom.size < 8, which I didn't think was
possible, but I've added a FFMAX(0,) there too.
- dale
From db3e9ffc364cc94cb3a72696d4d4858af6abcc42 Mon Sep 17 00:00:00 2001
From: Dale Curtis
Date: Fri, 2 Feb 2024 20:49:44 +
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
---
libavformat/mov.c | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/libavformat/mov.c b/libavformat/mov.c
index af95e1f662..1e4850fe9f 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -2228,7 +2228,12 @@ static int mov_read_stco(MOVContext *c, AVIOContext *pb, MOVAtom atom)
avio_r8(pb); /* version */
avio_rb24(pb); /* flags */
-entries = avio_rb32(pb);
+// 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(avio_rb32(pb),
+ FFMAX(0, (atom.size - 8) /
+ (atom.type == MKTAG('s', 't', 'c', 'o') ? 4 : 8)));
if (!entries)
return 0;
@@ -2237,6 +2242,7 @@ 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;
}
+
av_free(sc->chunk_offsets);
sc->chunk_count = 0;
sc->chunk_offsets = av_malloc_array(entries, sizeof(*sc->chunk_offsets));
--
2.44.0.rc0.258.g7320e95886-goog
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] [mov] Avoid OOM for invalid STCO / CO64 constructions.
On Fri, Feb 02, 2024 at 03:45:24PM -0800, Dale Curtis wrote:
> On Fri, Feb 2, 2024 at 3:42 PM Dale Curtis wrote:
>
> > On Fri, Feb 2, 2024 at 3:20 PM Andreas Rheinhardt <
> > andreas.rheinha...@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
> Date: Fri, 2 Feb 2024 20:49:44 +
> 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
> ---
> 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
signature.asc
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] [mov] Avoid OOM for invalid STCO / CO64 constructions.
On Fri, Feb 2, 2024 at 3:42 PM Dale Curtis wrote:
> On Fri, Feb 2, 2024 at 3:20 PM Andreas Rheinhardt <
> andreas.rheinha...@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.
stco-clamp-entries-v2.patch
Description: Binary data
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] [mov] Avoid OOM for invalid STCO / CO64 constructions.
On Fri, Feb 2, 2024 at 3:20 PM Andreas Rheinhardt <
andreas.rheinha...@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.
>
> - Andreas
>
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
>
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] [mov] Avoid OOM for invalid STCO / CO64 constructions.
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?
- Andreas
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
[FFmpeg-devel] [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 --- libavformat/mov.c | 7 +++ 1 file changed, 7 insertions(+) stco-clamp-entries.patch Description: Binary data ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
