[FFmpeg-devel] [PATCH] [mov] Avoid OOM for invalid STCO / CO64 constructions.

[FFmpeg-devel] [PATCH] [mov] Avoid OOM for invalid STCO / CO64 constructions.

[Dale Curtis]

[-- Attachment #1: Type: text/plain, Size: 355 bytes --]

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(+)

[-- Attachment #2: stco-clamp-entries.patch --]
[-- Type: application/octet-stream, Size: 1402 bytes --]

[-- Attachment #3: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] [mov] Avoid OOM for invalid STCO / CO64 constructions.

[Andreas Rheinhardt]

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-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] [mov] Avoid OOM for invalid STCO / CO64 constructions.

[Dale Curtis]

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.


>
> - 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-request@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-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] [mov] Avoid OOM for invalid STCO / CO64 constructions.

[Dale Curtis]

[-- Attachment #1: Type: text/plain, Size: 985 bytes --]

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.

[-- Attachment #2: stco-clamp-entries-v2.patch --]
[-- Type: application/octet-stream, Size: 1366 bytes --]

[-- Attachment #3: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] [mov] Avoid OOM for invalid STCO / CO64 constructions.

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 3155 bytes --]

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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] [mov] Avoid OOM for invalid STCO / CO64 constructions.

[Dale Curtis]

[-- Attachment #1: Type: text/plain, Size: 664 bytes --]

On Mon, Feb 5, 2024 at 12:07 PM Michael Niedermayer <michael@niedermayer.cc>
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

[-- Attachment #2: stco-clamp-entries-v3.patch --]
[-- Type: application/x-patch, Size: 1644 bytes --]

[-- Attachment #3: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] [mov] Avoid OOM for invalid STCO / CO64 constructions.

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 1368 bytes --]

On Thu, Feb 15, 2024 at 12:07:05PM -0800, Dale Curtis wrote:
> On Mon, Feb 5, 2024 at 12:07 PM Michael Niedermayer <michael@niedermayer.cc>
> 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 ..."

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] [mov] Avoid OOM for invalid STCO / CO64 constructions.

[Dale Curtis]

[-- Attachment #1: Type: text/plain, Size: 231 bytes --]

On Thu, Feb 15, 2024 at 2:35 PM Michael Niedermayer <michael@niedermayer.cc>
wrote:

> FFMIN/MAX can evaluate their arguments multiple times so avio_rb32() might
> be executed more than once
>

Thanks. Good catch. Fixed.

[-- Attachment #2: stco-clamp-entries-v4.patch --]
[-- Type: application/octet-stream, Size: 1622 bytes --]

[-- Attachment #3: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] [mov] Avoid OOM for invalid STCO / CO64 constructions.

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 1284 bytes --]

On Fri, Feb 16, 2024 at 01:41:35PM -0800, Dale Curtis wrote:
> On Thu, Feb 15, 2024 at 2:35 PM Michael Niedermayer <michael@niedermayer.cc>
> 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 <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(+)

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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".