| 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 |
| 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 |
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.
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 [...]
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().
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; > }
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 [...]
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 [...]
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);
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(+)