diff mbox series

[FFmpeg-devel,1/7] avformat/mov_chan: Check for FF_SANE_NB_CHANNELS

Message ID 20240912233337.2444412-1-michael@niedermayer.cc
State New
Headers show
Series [FFmpeg-devel,1/7] avformat/mov_chan: Check for FF_SANE_NB_CHANNELS | expand

Checks

Context Check Description
yinshiyou/make_loongarch64 success Make finished
yinshiyou/make_fate_loongarch64 success Make fate finished
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished

Commit Message

Michael Niedermayer Sept. 12, 2024, 11:33 p.m. UTC
We do not support more channels. For example avcodec_open2() limits channels this way too

The example file contains multiple chunks with over 16 million channels

Fixes: Timeout / DOS
Fixes: 67143/clusterfuzz-testcase-minimized-ffmpeg_dem_CAF_fuzzer-4858720481771520

Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavformat/mov_chan.c | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Anton Khirnov Sept. 13, 2024, 10:08 a.m. UTC | #1
Quoting Michael Niedermayer (2024-09-13 01:33:31)
> We do not support more channels. For example avcodec_open2() limits channels this way too
> 
> The example file contains multiple chunks with over 16 million channels

We had this discussion already. Ad-hoc checks like this are only
addressing a symptom (probably one of many), and hide the actual bug.

> +#include "libavcodec/internal.h"

I dislike this as well.
Michael Niedermayer Sept. 13, 2024, 5:48 p.m. UTC | #2
On Fri, Sep 13, 2024 at 12:08:45PM +0200, Anton Khirnov wrote:
> Quoting Michael Niedermayer (2024-09-13 01:33:31)
> > We do not support more channels. For example avcodec_open2() limits channels this way too
> > 
> > The example file contains multiple chunks with over 16 million channels
> 
> We had this discussion already.

I remembered something too, but couldnt find the thread within teh time i was looking for it


> Ad-hoc checks like this are only
> addressing a symptom (probably one of many), and hide the actual bug.

If you have a better fix, submit it.
If you want me to implement this differently, the first step is to describe
what you have in mind, that the implementation should look like.

But if one
1. allocates an attacker specified amount of memory
2. iterate over it by an attacker specified number of times
3. the case is never supported for numbers over 512
4. doing that 512 check leads to rejected patches

Then theres a problem

Also if the suggestion is to add a user specified limit. This can
be done for git master, for previous release branches thats not an
option and as we only backport from master in general we still need
this kind of fix before a user specified limit.


> 
> > +#include "libavcodec/internal.h"
> 
> I dislike this as well.

I am fine with it.

But if you dont, then maybe you can suggest another way to check
for the number that we support.

Thx

[...]
Anton Khirnov Sept. 15, 2024, 7:09 p.m. UTC | #3
Quoting Michael Niedermayer (2024-09-13 19:48:46)
> On Fri, Sep 13, 2024 at 12:08:45PM +0200, Anton Khirnov wrote:
> > Quoting Michael Niedermayer (2024-09-13 01:33:31)
> > > We do not support more channels. For example avcodec_open2() limits channels this way too
> > > 
> > > The example file contains multiple chunks with over 16 million channels
> > 
> > We had this discussion already.
> 
> I remembered something too, but couldnt find the thread within teh time i was looking for it
> 
> 
> > Ad-hoc checks like this are only
> > addressing a symptom (probably one of many), and hide the actual bug.
> 
> If you have a better fix, submit it.
> If you want me to implement this differently, the first step is to describe
> what you have in mind, that the implementation should look like.
> 
> But if one
> 1. allocates an attacker specified amount of memory
> 2. iterate over it by an attacker specified number of times

The root of my objection is exactly this - the patch does not make it
at all clear what code is allocating excessive amounts of memory.

> > 
> > > +#include "libavcodec/internal.h"
> > 
> > I dislike this as well.
> 
> I am fine with it.
> 
> But if you dont, then maybe you can suggest another way to check
> for the number that we support.

My second objection, in case it is not clear, is to libavformat
including libavcodec internal headers that define libavcodec internal
structures. That is a slippery slope that easily leads to people
actually accessing those internal structures.

But also the number 512 is entirely arbitrary, and if we decide to
arbitrarily restrict the channel count everywhere then it should be done
at the level of AVChannelLayout. E.g. by making nb_channels and uint8 or
uint16, and/or adding the check to av_channel_layout_check().
James Almer Sept. 16, 2024, 2:30 p.m. UTC | #4
On 9/13/2024 2:48 PM, Michael Niedermayer wrote:
> On Fri, Sep 13, 2024 at 12:08:45PM +0200, Anton Khirnov wrote:
>> Quoting Michael Niedermayer (2024-09-13 01:33:31)
>>> We do not support more channels. For example avcodec_open2() limits channels this way too
>>>
>>> The example file contains multiple chunks with over 16 million channels
>>
>> We had this discussion already.
> 
> I remembered something too, but couldnt find the thread within teh time i was looking for it
> 
> 
>> Ad-hoc checks like this are only
>> addressing a symptom (probably one of many), and hide the actual bug.
> 
> If you have a better fix, submit it.
Does the following help with this sample?

> diff --git a/libavformat/mov_chan.c b/libavformat/mov_chan.c
> index cc5b333129..4484a22a10 100644
> --- a/libavformat/mov_chan.c
> +++ b/libavformat/mov_chan.c
> @@ -543,10 +543,22 @@ int ff_mov_read_chan(AVFormatContext *s, AVIOContext *pb, AVStream *st,
>          return 0;
> 
>      if (layout_tag == MOV_CH_LAYOUT_USE_DESCRIPTIONS) {
> -        int nb_channels = ch_layout->nb_channels ? ch_layout->nb_channels : num_descr;
> +        int nb_channels = ch_layout->nb_channels;
> +
> +        if (!num_descr || num_descr < nb_channels) {
> +            av_log(s, AV_LOG_ERROR, "got %d channel descriptions when at least %d were needed\n",
> +                   num_descr, nb_channels);
> +            return AVERROR_INVALIDDATA;
> +        }
> +
>          if (num_descr > nb_channels) {
> -            av_log(s, AV_LOG_WARNING, "got %d channel descriptions, capping to the number of channels %d\n",
> +            int strict = s->strict_std_compliance >= FF_COMPLIANCE_STRICT;
> +            av_log(s, strict ? AV_LOG_ERROR : AV_LOG_WARNING,
> +                   "got %d channel descriptions when number of channels is %d\n",
>                     num_descr, nb_channels);
> +            if (strict)
> +                return AVERROR_INVALIDDATA;
> +            av_log(s, AV_LOG_WARNING, "capping channel descriptions to the number of channels\n");
>              num_descr = nb_channels;
>          }
Michael Niedermayer Sept. 17, 2024, 5:45 p.m. UTC | #5
On Mon, Sep 16, 2024 at 11:30:34AM -0300, James Almer wrote:
> On 9/13/2024 2:48 PM, Michael Niedermayer wrote:
> > On Fri, Sep 13, 2024 at 12:08:45PM +0200, Anton Khirnov wrote:
> > > Quoting Michael Niedermayer (2024-09-13 01:33:31)
> > > > We do not support more channels. For example avcodec_open2() limits channels this way too
> > > > 
> > > > The example file contains multiple chunks with over 16 million channels
> > > 
> > > We had this discussion already.
> > 
> > I remembered something too, but couldnt find the thread within teh time i was looking for it
> > 
> > 
> > > Ad-hoc checks like this are only
> > > addressing a symptom (probably one of many), and hide the actual bug.
> > 
> > If you have a better fix, submit it.
> Does the following help with this sample?

yes, please apply

thx

[...]
Michael Niedermayer Sept. 17, 2024, 5:59 p.m. UTC | #6
On Sun, Sep 15, 2024 at 09:09:39PM +0200, Anton Khirnov wrote:
> Quoting Michael Niedermayer (2024-09-13 19:48:46)
[...]
> > > 
> > > > +#include "libavcodec/internal.h"
> > > 
> > > I dislike this as well.
> > 
> > I am fine with it.
> > 
> > But if you dont, then maybe you can suggest another way to check
> > for the number that we support.
> 
> My second objection, in case it is not clear, is to libavformat
> including libavcodec internal headers that define libavcodec internal
> structures. That is a slippery slope that easily leads to people
> actually accessing those internal structures.
> 

> But also the number 512 is entirely arbitrary, and if we decide to
> arbitrarily restrict the channel count everywhere then it should be done
> at the level of AVChannelLayout. E.g. by making nb_channels and uint8 or
> uint16, and/or adding the check to av_channel_layout_check().

Now that this specific issue is fixed by james patch.
Would you agree that the more general issue of "something" allocating
and or processing a large number of channels can be checked for ?
And that the constant would then of course be moved to libavutil

thx

[...]
Anton Khirnov Sept. 20, 2024, 8:52 a.m. UTC | #7
Quoting Michael Niedermayer (2024-09-17 19:59:53)
> On Sun, Sep 15, 2024 at 09:09:39PM +0200, Anton Khirnov wrote:
> > Quoting Michael Niedermayer (2024-09-13 19:48:46)
> [...]
> > > > 
> > > > > +#include "libavcodec/internal.h"
> > > > 
> > > > I dislike this as well.
> > > 
> > > I am fine with it.
> > > 
> > > But if you dont, then maybe you can suggest another way to check
> > > for the number that we support.
> > 
> > My second objection, in case it is not clear, is to libavformat
> > including libavcodec internal headers that define libavcodec internal
> > structures. That is a slippery slope that easily leads to people
> > actually accessing those internal structures.
> > 
> 
> > But also the number 512 is entirely arbitrary, and if we decide to
> > arbitrarily restrict the channel count everywhere then it should be done
> > at the level of AVChannelLayout. E.g. by making nb_channels and uint8 or
> > uint16, and/or adding the check to av_channel_layout_check().
> 
> Now that this specific issue is fixed by james patch.
> Would you agree that the more general issue of "something" allocating
> and or processing a large number of channels can be checked for ?

Depends on how it is done. I do NOT agree with ad-hoc checks scattered
across random demuxers.
diff mbox series

Patch

diff --git a/libavformat/mov_chan.c b/libavformat/mov_chan.c
index cc5b3331290..2cc6b2a7797 100644
--- a/libavformat/mov_chan.c
+++ b/libavformat/mov_chan.c
@@ -30,6 +30,7 @@ 
 #include "libavutil/channel_layout.h"
 #include "libavutil/mem.h"
 #include "libavcodec/codec_id.h"
+#include "libavcodec/internal.h"
 #include "mov_chan.h"
 
 enum {
@@ -549,6 +550,10 @@  int ff_mov_read_chan(AVFormatContext *s, AVIOContext *pb, AVStream *st,
                    num_descr, nb_channels);
             num_descr = nb_channels;
         }
+        if (nb_channels > FF_SANE_NB_CHANNELS) {
+            ret = AVERROR(ENOTSUP);
+            goto out;
+        }
 
         av_channel_layout_uninit(ch_layout);
         ret = av_channel_layout_custom_init(ch_layout, nb_channels);