diff mbox series

[FFmpeg-devel] avformat/mov: Fix crash with too big STSZ atoms

Message ID AM7PR03MB6660E96FCB508BFD1C1A04558FE69@AM7PR03MB6660.eurprd03.prod.outlook.com
State Accepted
Commit c2d853c1aae22bbc7d9905c43a9f16cb2ba3ba33
Headers show
Series [FFmpeg-devel] avformat/mov: Fix crash with too big STSZ atoms
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

Andreas Rheinhardt July 24, 2021, 4:09 a.m. UTC
mov_read_stsz() did not ensure that every bit of a buffer is addressable
by an int as is required by the get_bits API, leading to a crash in
ticket #9344. Fix this by restricting the size more thoroughly.

The file from said ticket will then be considered invalid; in the
future, we might read and process the data in chunks to actually support
such files.

Fixes ticket #9344.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
The commit message is written as if it were certain that this
indeed fixes the ticket, despite me not knowing it yet (as the sample
in question is not public).
The above is intended as a quick fix that is easy to backport;
supporting such files can be done later.

 libavformat/mov.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Paul B Mahol Sept. 3, 2021, 9:09 p.m. UTC | #1
On Sat, Jul 24, 2021 at 6:09 AM Andreas Rheinhardt <
andreas.rheinhardt@outlook.com> wrote:

> mov_read_stsz() did not ensure that every bit of a buffer is addressable
> by an int as is required by the get_bits API, leading to a crash in
> ticket #9344. Fix this by restricting the size more thoroughly.
>
> The file from said ticket will then be considered invalid; in the
> future, we might read and process the data in chunks to actually support
> such files.
>
> Fixes ticket #9344.
>
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
> The commit message is written as if it were certain that this
> indeed fixes the ticket, despite me not knowing it yet (as the sample
> in question is not public).
> The above is intended as a quick fix that is easy to backport;
> supporting such files can be done later.
>
>  libavformat/mov.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/libavformat/mov.c b/libavformat/mov.c
> index 3fc5a1e8ab..e0d805b07b 100644
> --- a/libavformat/mov.c
> +++ b/libavformat/mov.c
> @@ -2856,7 +2856,7 @@ static int mov_read_stsz(MOVContext *c, AVIOContext
> *pb, MOVAtom atom)
>
>      if (!entries)
>          return 0;
> -    if (entries >= (UINT_MAX - 4) / field_size)
> +    if (entries >= (INT_MAX - 4 - 8 * AV_INPUT_BUFFER_PADDING_SIZE) /
> field_size)
>          return AVERROR_INVALIDDATA;
>      if (sc->sample_sizes)
>          av_log(c->fc, AV_LOG_WARNING, "Duplicated STSZ atom\n");
> --
> 2.30.2
>

Is so big bit buffer really needed?

Why not check use init_get_bits8 directly and thus depends on its
implementation directly?


>
> _______________________________________________
> 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".
>
Andreas Rheinhardt Sept. 3, 2021, 9:16 p.m. UTC | #2
Paul B Mahol:
> On Sat, Jul 24, 2021 at 6:09 AM Andreas Rheinhardt <
> andreas.rheinhardt@outlook.com> wrote:
> 
>> mov_read_stsz() did not ensure that every bit of a buffer is addressable
>> by an int as is required by the get_bits API, leading to a crash in
>> ticket #9344. Fix this by restricting the size more thoroughly.
>>
>> The file from said ticket will then be considered invalid; in the
>> future, we might read and process the data in chunks to actually support
>> such files.
>>
>> Fixes ticket #9344.
>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
>> ---
>> The commit message is written as if it were certain that this
>> indeed fixes the ticket, despite me not knowing it yet (as the sample
>> in question is not public).
>> The above is intended as a quick fix that is easy to backport;
>> supporting such files can be done later.
>>
>>  libavformat/mov.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/libavformat/mov.c b/libavformat/mov.c
>> index 3fc5a1e8ab..e0d805b07b 100644
>> --- a/libavformat/mov.c
>> +++ b/libavformat/mov.c
>> @@ -2856,7 +2856,7 @@ static int mov_read_stsz(MOVContext *c, AVIOContext
>> *pb, MOVAtom atom)
>>
>>      if (!entries)
>>          return 0;
>> -    if (entries >= (UINT_MAX - 4) / field_size)
>> +    if (entries >= (INT_MAX - 4 - 8 * AV_INPUT_BUFFER_PADDING_SIZE) /
>> field_size)
>>          return AVERROR_INVALIDDATA;
>>      if (sc->sample_sizes)
>>          av_log(c->fc, AV_LOG_WARNING, "Duplicated STSZ atom\n");
>> --
>> 2.30.2
>>
> 
> Is so big bit buffer really needed?
> 

No, such a big buffer is never needed, as one can read in smaller chunks
(this would actually support the files that #9344 is about). I wanted to
implement this, but I never got around to investigate what a sane chunk
size is (one that avoids reading multiple times for ordinary files). Is
it 1MB? 4MB?

> Why not check use init_get_bits8 directly and thus depends on its
> implementation directly?
> 
Because then we have already allocated a buffer. Checking before the
allocation avoids that.

- Andreas
diff mbox series

Patch

diff --git a/libavformat/mov.c b/libavformat/mov.c
index 3fc5a1e8ab..e0d805b07b 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -2856,7 +2856,7 @@  static int mov_read_stsz(MOVContext *c, AVIOContext *pb, MOVAtom atom)
 
     if (!entries)
         return 0;
-    if (entries >= (UINT_MAX - 4) / field_size)
+    if (entries >= (INT_MAX - 4 - 8 * AV_INPUT_BUFFER_PADDING_SIZE) / field_size)
         return AVERROR_INVALIDDATA;
     if (sc->sample_sizes)
         av_log(c->fc, AV_LOG_WARNING, "Duplicated STSZ atom\n");