Re: [FFmpeg-devel] [PATCH 1/3] avformat/cafdec: sanity check channels and bps
On Mon, Jul 01, 2024 at 09:01:28PM -0300, James Almer wrote:
> On 7/1/2024 8:42 PM, Michael Niedermayer wrote:
> > On Sun, Jun 30, 2024 at 08:07:28PM -0300, James Almer wrote:
> > > On 6/29/2024 8:37 PM, Michael Niedermayer wrote:
> > > > On Wed, Jun 26, 2024 at 09:52:44PM -0300, James Almer wrote:
> > > > > On 3/22/2024 8:08 PM, Michael Niedermayer wrote:
> > > > > > Fixes: Timeout
> > > > > > Fixes:
> > > > > > 67044/clusterfuzz-testcase-minimized-ffmpeg_dem_CAF_fuzzer-5791144363491328
> > > > > >
> > > > > > Found-by: continuous fuzzing process
> > > > > > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > > > > > Signed-off-by: Michael Niedermayer
> > > > > > ---
> > > > > > libavformat/cafdec.c | 5 +
> > > > > > 1 file changed, 5 insertions(+)
> > > > > >
> > > > > > diff --git a/libavformat/cafdec.c b/libavformat/cafdec.c
> > > > > > index 426c56b9bd..334077efb5 100644
> > > > > > --- a/libavformat/cafdec.c
> > > > > > +++ b/libavformat/cafdec.c
> > > > > > @@ -33,6 +33,7 @@
> > > > > > #include "isom.h"
> > > > > > #include "mov_chan.h"
> > > > > > #include "libavcodec/flac.h"
> > > > > > +#include "libavcodec/internal.h"
> > > > > > #include "libavutil/intreadwrite.h"
> > > > > > #include "libavutil/intfloat.h"
> > > > > > #include "libavutil/dict.h"
> > > > > > @@ -87,6 +88,10 @@ static int read_desc_chunk(AVFormatContext *s)
> > > > > > st->codecpar->ch_layout.nb_channels = avio_rb32(pb);
> > > > > > st->codecpar->bits_per_coded_sample = avio_rb32(pb);
> > > > > > +if (st->codecpar->ch_layout.nb_channels > FF_SANE_NB_CHANNELS
> > > > > > ||
> > > > > > +st->codecpar->bits_per_coded_sample > 64)
> > > > >
> > > > > Where does the process take so long that oss-fuzz gets a timeout if
> > > > > these
> > > > > are unreasonably high? I don't see nb_channels used anywhere in here
> > > > > where
> > > > > that matters.
> > > > > Is it in the PCM decoder? Because that decoder is meant to handle any
> > > > > arbitrary amount of channels, so limiting it to whatever
> > > > > FF_SANE_NB_CHANNELS
> > > > > is set to is not ok.
> > > >
> > > > libavutil/channel_layout.c:627:17
> > > > or it will OOM before depending on how much memory is available
> > >
> > > Does this fix the timeout?
> > >
> > > > diff --git a/libavutil/channel_layout.c b/libavutil/channel_layout.c
> > > > index 2d6963b6df..a4fcd199e1 100644
> > > > --- a/libavutil/channel_layout.c
> > > > +++ b/libavutil/channel_layout.c
> > > > @@ -623,6 +623,8 @@ int av_channel_layout_describe_bprint(const
> > > > AVChannelLayout *channel_layout,
> > > > for (i = 0; i < channel_layout->nb_channels; i++) {
> > > > enum AVChannel ch =
> > > > av_channel_layout_channel_from_index(channel_layout, i);
> > > >
> > > > +if (!av_bprint_is_complete(bp))
> > > > +break;
> > > > if (i)
> > > > av_bprintf(bp, "+");
> > > > av_channel_name_bprint(bp, ch);
> > >
> > > But this is not ok as it will affect the buffer len value
> > > av_channel_layout_describe() returns on success when truncation took
> > > place,
> > > so something else would have to be done.
> >
> > This makes the testcase which is 108 bytes long take a bit more than 7
> > seconds
> > which is below the threshold for the timeout, but i would guess 30x on the
> > channel
> > number would bring it well above
>
> If you try to process that file without the fuzzer, does it also take 7
> seconds before it stops?
real0m3.232s
user0m1.064s
sys 0m2.156s
for a 108 byte file with your speed up patch, without that patch i didnt try as
i have not
enough patience ATM
with 66 instead of 33 million channels
real0m6.181s
user0m1.774s
sys 0m4.385s
> If not, then the fuzzer is having Valgrind levels
> of penalty hit, and i don't think adding dubious checks in our codebase just
> to appease it is correct.
Please think again
The fuzzer is slower than non instrumented code, but its purpose is to detect
issues
First i dont see how that leads to "dubious checks" or "appease" these are
loaded
words not appropriate for a technical discusison
Second, if the fuzzer spends the majority of its time in a busy loop copying
millions
of channels. Thats time it will not find any issues.
So both the fuzzing process itself as well as DOS of users are affected
Third if it goes over its threshold, for its purpose it found an issue.
If thats not the case the fuzzers configuration does not match the definition
of a
DOS issue. Thats a problem in itself. (this isnt applying as the 7 sec is below)
Fourth, we should try to investigate and fix issues (truth we often dont have
the
resources to fully investigate everything). Here the issue is clearly 33 million
channels being worked on as valid data. It would take time, alot of time to
process each
sample times 33 million. The user does not have 33 million
Re: [FFmpeg-devel] [PATCH 1/3] avformat/cafdec: sanity check channels and bps
On 7/1/2024 8:42 PM, Michael Niedermayer wrote:
On Sun, Jun 30, 2024 at 08:07:28PM -0300, James Almer wrote:
On 6/29/2024 8:37 PM, Michael Niedermayer wrote:
On Wed, Jun 26, 2024 at 09:52:44PM -0300, James Almer wrote:
On 3/22/2024 8:08 PM, Michael Niedermayer wrote:
Fixes: Timeout
Fixes:
67044/clusterfuzz-testcase-minimized-ffmpeg_dem_CAF_fuzzer-5791144363491328
Found-by: continuous fuzzing process
https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
Signed-off-by: Michael Niedermayer
---
libavformat/cafdec.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/libavformat/cafdec.c b/libavformat/cafdec.c
index 426c56b9bd..334077efb5 100644
--- a/libavformat/cafdec.c
+++ b/libavformat/cafdec.c
@@ -33,6 +33,7 @@
#include "isom.h"
#include "mov_chan.h"
#include "libavcodec/flac.h"
+#include "libavcodec/internal.h"
#include "libavutil/intreadwrite.h"
#include "libavutil/intfloat.h"
#include "libavutil/dict.h"
@@ -87,6 +88,10 @@ static int read_desc_chunk(AVFormatContext *s)
st->codecpar->ch_layout.nb_channels = avio_rb32(pb);
st->codecpar->bits_per_coded_sample = avio_rb32(pb);
+if (st->codecpar->ch_layout.nb_channels > FF_SANE_NB_CHANNELS ||
+st->codecpar->bits_per_coded_sample > 64)
Where does the process take so long that oss-fuzz gets a timeout if these
are unreasonably high? I don't see nb_channels used anywhere in here where
that matters.
Is it in the PCM decoder? Because that decoder is meant to handle any
arbitrary amount of channels, so limiting it to whatever FF_SANE_NB_CHANNELS
is set to is not ok.
libavutil/channel_layout.c:627:17
or it will OOM before depending on how much memory is available
Does this fix the timeout?
diff --git a/libavutil/channel_layout.c b/libavutil/channel_layout.c
index 2d6963b6df..a4fcd199e1 100644
--- a/libavutil/channel_layout.c
+++ b/libavutil/channel_layout.c
@@ -623,6 +623,8 @@ int av_channel_layout_describe_bprint(const AVChannelLayout
*channel_layout,
for (i = 0; i < channel_layout->nb_channels; i++) {
enum AVChannel ch =
av_channel_layout_channel_from_index(channel_layout, i);
+if (!av_bprint_is_complete(bp))
+break;
if (i)
av_bprintf(bp, "+");
av_channel_name_bprint(bp, ch);
But this is not ok as it will affect the buffer len value
av_channel_layout_describe() returns on success when truncation took place,
so something else would have to be done.
This makes the testcase which is 108 bytes long take a bit more than 7 seconds
which is below the threshold for the timeout, but i would guess 30x on the
channel
number would bring it well above
If you try to process that file without the fuzzer, does it also take 7
seconds before it stops? If not, then the fuzzer is having Valgrind
levels of penalty hit, and i don't think adding dubious checks in our
codebase just to appease it is correct.
The next location it gets stuck on if the timeout threshold is reduced:
#0 0x4a5b41 in __sanitizer_print_stack_trace
/b/swarming/w/ir/cache/builder/src/third_party/llvm/compiler-rt/lib/asan/asan_stack.cc:86:3
#1 0x2a30aae in fuzzer::Fuzzer::AlarmCallback()
Fuzzer/build/../FuzzerLoop.cpp:248:7
#2 0x7fbe0815e41f (/lib/x86_64-linux-gnu/libpthread.so.0+0x1441f)
#3 0x7fbe07c73e08 (/lib/x86_64-linux-gnu/libc.so.6+0xbbe08)
#4 0x49c747 in __asan_memcpy
/b/swarming/w/ir/cache/builder/src/third_party/llvm/compiler-rt/lib/asan/asan_interceptors_memintrinsics.cc:22:3
#5 0x2873f59 in av_channel_layout_copy
ffmpeg/libavutil/channel_layout.c:452:9
#6 0x1895763 in avcodec_parameters_to_context
ffmpeg/libavcodec/codec_par.c:235:15
#7 0x71923e in avformat_find_stream_info ffmpeg/libavformat/demux.c:2579:15
#8 0x4cd17b in LLVMFuzzerTestOneInput
ffmpeg/tools/target_dem_fuzzer.c:207:11
#9 0x2a31b1d in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*,
unsigned long) Fuzzer/build/../FuzzerLoop.cpp:495:13
#10 0x2a266f2 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned
long) Fuzzer/build/../FuzzerDriver.cpp:273:6
#11 0x2a2b8f1 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char
const*, unsigned long)) Fuzzer/build/../FuzzerDriver.cpp:690:9
#12 0x2a263d0 in main Fuzzer/build/../FuzzerMain.cpp:20:10
#13 0x7fbe07bdc082 in __libc_start_main
(/lib/x86_64-linux-gnu/libc.so.6+0x24082)
#14 0x42529d in _start (ffmpeg/tools/target_dem_caf_fuzzer+0x42529d)
And is the bits_per_coded_sample > 64 check to prevent codec_id being
AV_CODEC_ID_NONE? if so, how does that affect demuxing time?
AV_CODEC_ID_NONE for that matter could happen for valid files with a codec
we don't currently support.
We generally check read values for validity at the earliest,
bits_per_coded_sample of more than 64 seem not valid to me.
I agree the check is fine, but my point is that, assuming this is affecting
demuxing time,
Re: [FFmpeg-devel] [PATCH 1/3] avformat/cafdec: sanity check channels and bps
On Sun, Jun 30, 2024 at 08:07:28PM -0300, James Almer wrote:
> On 6/29/2024 8:37 PM, Michael Niedermayer wrote:
> > On Wed, Jun 26, 2024 at 09:52:44PM -0300, James Almer wrote:
> > > On 3/22/2024 8:08 PM, Michael Niedermayer wrote:
> > > > Fixes: Timeout
> > > > Fixes:
> > > > 67044/clusterfuzz-testcase-minimized-ffmpeg_dem_CAF_fuzzer-5791144363491328
> > > >
> > > > Found-by: continuous fuzzing process
> > > > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > > > Signed-off-by: Michael Niedermayer
> > > > ---
> > > >libavformat/cafdec.c | 5 +
> > > >1 file changed, 5 insertions(+)
> > > >
> > > > diff --git a/libavformat/cafdec.c b/libavformat/cafdec.c
> > > > index 426c56b9bd..334077efb5 100644
> > > > --- a/libavformat/cafdec.c
> > > > +++ b/libavformat/cafdec.c
> > > > @@ -33,6 +33,7 @@
> > > >#include "isom.h"
> > > >#include "mov_chan.h"
> > > >#include "libavcodec/flac.h"
> > > > +#include "libavcodec/internal.h"
> > > >#include "libavutil/intreadwrite.h"
> > > >#include "libavutil/intfloat.h"
> > > >#include "libavutil/dict.h"
> > > > @@ -87,6 +88,10 @@ static int read_desc_chunk(AVFormatContext *s)
> > > >st->codecpar->ch_layout.nb_channels = avio_rb32(pb);
> > > >st->codecpar->bits_per_coded_sample = avio_rb32(pb);
> > > > +if (st->codecpar->ch_layout.nb_channels > FF_SANE_NB_CHANNELS ||
> > > > +st->codecpar->bits_per_coded_sample > 64)
> > >
> > > Where does the process take so long that oss-fuzz gets a timeout if these
> > > are unreasonably high? I don't see nb_channels used anywhere in here where
> > > that matters.
> > > Is it in the PCM decoder? Because that decoder is meant to handle any
> > > arbitrary amount of channels, so limiting it to whatever
> > > FF_SANE_NB_CHANNELS
> > > is set to is not ok.
> >
> > libavutil/channel_layout.c:627:17
> > or it will OOM before depending on how much memory is available
>
> Does this fix the timeout?
>
> > diff --git a/libavutil/channel_layout.c b/libavutil/channel_layout.c
> > index 2d6963b6df..a4fcd199e1 100644
> > --- a/libavutil/channel_layout.c
> > +++ b/libavutil/channel_layout.c
> > @@ -623,6 +623,8 @@ int av_channel_layout_describe_bprint(const
> > AVChannelLayout *channel_layout,
> > for (i = 0; i < channel_layout->nb_channels; i++) {
> > enum AVChannel ch =
> > av_channel_layout_channel_from_index(channel_layout, i);
> >
> > +if (!av_bprint_is_complete(bp))
> > +break;
> > if (i)
> > av_bprintf(bp, "+");
> > av_channel_name_bprint(bp, ch);
>
> But this is not ok as it will affect the buffer len value
> av_channel_layout_describe() returns on success when truncation took place,
> so something else would have to be done.
This makes the testcase which is 108 bytes long take a bit more than 7 seconds
which is below the threshold for the timeout, but i would guess 30x on the
channel
number would bring it well above
The next location it gets stuck on if the timeout threshold is reduced:
#0 0x4a5b41 in __sanitizer_print_stack_trace
/b/swarming/w/ir/cache/builder/src/third_party/llvm/compiler-rt/lib/asan/asan_stack.cc:86:3
#1 0x2a30aae in fuzzer::Fuzzer::AlarmCallback()
Fuzzer/build/../FuzzerLoop.cpp:248:7
#2 0x7fbe0815e41f (/lib/x86_64-linux-gnu/libpthread.so.0+0x1441f)
#3 0x7fbe07c73e08 (/lib/x86_64-linux-gnu/libc.so.6+0xbbe08)
#4 0x49c747 in __asan_memcpy
/b/swarming/w/ir/cache/builder/src/third_party/llvm/compiler-rt/lib/asan/asan_interceptors_memintrinsics.cc:22:3
#5 0x2873f59 in av_channel_layout_copy
ffmpeg/libavutil/channel_layout.c:452:9
#6 0x1895763 in avcodec_parameters_to_context
ffmpeg/libavcodec/codec_par.c:235:15
#7 0x71923e in avformat_find_stream_info ffmpeg/libavformat/demux.c:2579:15
#8 0x4cd17b in LLVMFuzzerTestOneInput
ffmpeg/tools/target_dem_fuzzer.c:207:11
#9 0x2a31b1d in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*,
unsigned long) Fuzzer/build/../FuzzerLoop.cpp:495:13
#10 0x2a266f2 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned
long) Fuzzer/build/../FuzzerDriver.cpp:273:6
#11 0x2a2b8f1 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char
const*, unsigned long)) Fuzzer/build/../FuzzerDriver.cpp:690:9
#12 0x2a263d0 in main Fuzzer/build/../FuzzerMain.cpp:20:10
#13 0x7fbe07bdc082 in __libc_start_main
(/lib/x86_64-linux-gnu/libc.so.6+0x24082)
#14 0x42529d in _start (ffmpeg/tools/target_dem_caf_fuzzer+0x42529d)
>
> >
> >
> > >
> > > And is the bits_per_coded_sample > 64 check to prevent codec_id being
> > > AV_CODEC_ID_NONE? if so, how does that affect demuxing time?
> > > AV_CODEC_ID_NONE for that matter could happen for valid files with a codec
> > > we don't currently support.
> >
> > We generally check read values for validity at the earliest,
> > bits_per_coded_sample of more than 64 seem not valid to me.
>
> I agree the check
Re: [FFmpeg-devel] [PATCH 1/3] avformat/cafdec: sanity check channels and bps
On 6/29/2024 8:37 PM, Michael Niedermayer wrote:
On Wed, Jun 26, 2024 at 09:52:44PM -0300, James Almer wrote:
On 3/22/2024 8:08 PM, Michael Niedermayer wrote:
Fixes: Timeout
Fixes:
67044/clusterfuzz-testcase-minimized-ffmpeg_dem_CAF_fuzzer-5791144363491328
Found-by: continuous fuzzing process
https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
Signed-off-by: Michael Niedermayer
---
libavformat/cafdec.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/libavformat/cafdec.c b/libavformat/cafdec.c
index 426c56b9bd..334077efb5 100644
--- a/libavformat/cafdec.c
+++ b/libavformat/cafdec.c
@@ -33,6 +33,7 @@
#include "isom.h"
#include "mov_chan.h"
#include "libavcodec/flac.h"
+#include "libavcodec/internal.h"
#include "libavutil/intreadwrite.h"
#include "libavutil/intfloat.h"
#include "libavutil/dict.h"
@@ -87,6 +88,10 @@ static int read_desc_chunk(AVFormatContext *s)
st->codecpar->ch_layout.nb_channels = avio_rb32(pb);
st->codecpar->bits_per_coded_sample = avio_rb32(pb);
+if (st->codecpar->ch_layout.nb_channels > FF_SANE_NB_CHANNELS ||
+st->codecpar->bits_per_coded_sample > 64)
Where does the process take so long that oss-fuzz gets a timeout if these
are unreasonably high? I don't see nb_channels used anywhere in here where
that matters.
Is it in the PCM decoder? Because that decoder is meant to handle any
arbitrary amount of channels, so limiting it to whatever FF_SANE_NB_CHANNELS
is set to is not ok.
libavutil/channel_layout.c:627:17
or it will OOM before depending on how much memory is available
Does this fix the timeout?
diff --git a/libavutil/channel_layout.c b/libavutil/channel_layout.c
index 2d6963b6df..a4fcd199e1 100644
--- a/libavutil/channel_layout.c
+++ b/libavutil/channel_layout.c
@@ -623,6 +623,8 @@ int av_channel_layout_describe_bprint(const AVChannelLayout
*channel_layout,
for (i = 0; i < channel_layout->nb_channels; i++) {
enum AVChannel ch =
av_channel_layout_channel_from_index(channel_layout, i);
+if (!av_bprint_is_complete(bp))
+break;
if (i)
av_bprintf(bp, "+");
av_channel_name_bprint(bp, ch);
But this is not ok as it will affect the buffer len value
av_channel_layout_describe() returns on success when truncation took
place, so something else would have to be done.
And is the bits_per_coded_sample > 64 check to prevent codec_id being
AV_CODEC_ID_NONE? if so, how does that affect demuxing time?
AV_CODEC_ID_NONE for that matter could happen for valid files with a codec
we don't currently support.
We generally check read values for validity at the earliest,
bits_per_coded_sample of more than 64 seem not valid to me.
I agree the check is fine, but my point is that, assuming this is
affecting demuxing time, this check as is may be hiding an issue related
to codec_id being set to AV_CODEC_ID_NONE here (the result of
ff_get_pcm_codec_id() with an unsupported bits_per_coded_sample value),
so it should be looked at more closely because said codec_id could
happen with valid files using codecs the demuxer does not know about.
If it does not affect demuxing time and is a "just in case" check, then
it doesn't belong in a patch that says "Fixes: Timeout" and mentions an
ossfuzz issue.
thx
[...]
___
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 1/3] avformat/cafdec: sanity check channels and bps
On Thu, Jun 27, 2024 at 08:40:00AM +0200, Paul B Mahol wrote: > On Thu, Jun 27, 2024 at 1:50 AM Michael Niedermayer > wrote: > > > On Tue, Jun 25, 2024 at 09:27:55PM +0200, Anton Khirnov wrote: > > > Quoting Michael Niedermayer (2024-06-25 21:25:46) > > > > On Thu, Mar 28, 2024 at 12:27:02AM +0100, Michael Niedermayer wrote: > > > > > On Wed, Mar 27, 2024 at 08:39:17AM +0100, Anton Khirnov wrote: > > > > > > Quoting Michael Niedermayer (2024-03-23 00:08:16) > > > > > > > Fixes: Timeout > > > > > > > Fixes: > > 67044/clusterfuzz-testcase-minimized-ffmpeg_dem_CAF_fuzzer-5791144363491328 > > > > > > > > > > > > > > Found-by: continuous fuzzing process > > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > > > > > > Signed-off-by: Michael Niedermayer > > > > > > > --- > > > > > > > libavformat/cafdec.c | 5 + > > > > > > > 1 file changed, 5 insertions(+) > > > > > > > > > > > > > > diff --git a/libavformat/cafdec.c b/libavformat/cafdec.c > > > > > > > index 426c56b9bd..334077efb5 100644 > > > > > > > --- a/libavformat/cafdec.c > > > > > > > +++ b/libavformat/cafdec.c > > > > > > > @@ -33,6 +33,7 @@ > > > > > > > #include "isom.h" > > > > > > > #include "mov_chan.h" > > > > > > > #include "libavcodec/flac.h" > > > > > > > +#include "libavcodec/internal.h" > > > > > > > #include "libavutil/intreadwrite.h" > > > > > > > #include "libavutil/intfloat.h" > > > > > > > #include "libavutil/dict.h" > > > > > > > @@ -87,6 +88,10 @@ static int read_desc_chunk(AVFormatContext *s) > > > > > > > st->codecpar->ch_layout.nb_channels = avio_rb32(pb); > > > > > > > st->codecpar->bits_per_coded_sample = avio_rb32(pb); > > > > > > > > > > > > > > +if (st->codecpar->ch_layout.nb_channels > > > FF_SANE_NB_CHANNELS || > > > > > > > > > > > > I dislike this. > > > > > > > > > > I dislike it too > > > > > > > > so what do we do about this ? > > > > > > About what? What is the actual problem that needs addressed? > > > > 67044/clusterfuzz-testcase-minimized-ffmpeg_dem_CAF_fuzzer-5791144363491328 > > > > > > > > > > > any objections to apply this ? > > > > > > yes, FF_SANE_NB_CHANNELS is a hack that should be removed, not spread > > > > a maximum number for each theoretically unlimited parameter is desirable > > This can be a user setable value or a compile time value when such is > > preferred. > > > > Cant you check if so many channels are actually available in stream? Iam not sure how. Maybe there is a way ... > Or there is some silly loop that goes unchecked up to INT32_MAX ? there are multiple loops one is in av_channel_layout_custom_init() another is in av_channel_layout_describe_bprint() thx [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Freedom in capitalist society always remains about the same as it was in ancient Greek republics: Freedom for slave owners. -- Vladimir Lenin 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 1/3] avformat/cafdec: sanity check channels and bps
On Wed, Jun 26, 2024 at 09:52:44PM -0300, James Almer wrote: > On 3/22/2024 8:08 PM, Michael Niedermayer wrote: > > Fixes: Timeout > > Fixes: > > 67044/clusterfuzz-testcase-minimized-ffmpeg_dem_CAF_fuzzer-5791144363491328 > > > > Found-by: continuous fuzzing process > > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > Signed-off-by: Michael Niedermayer > > --- > > libavformat/cafdec.c | 5 + > > 1 file changed, 5 insertions(+) > > > > diff --git a/libavformat/cafdec.c b/libavformat/cafdec.c > > index 426c56b9bd..334077efb5 100644 > > --- a/libavformat/cafdec.c > > +++ b/libavformat/cafdec.c > > @@ -33,6 +33,7 @@ > > #include "isom.h" > > #include "mov_chan.h" > > #include "libavcodec/flac.h" > > +#include "libavcodec/internal.h" > > #include "libavutil/intreadwrite.h" > > #include "libavutil/intfloat.h" > > #include "libavutil/dict.h" > > @@ -87,6 +88,10 @@ static int read_desc_chunk(AVFormatContext *s) > > st->codecpar->ch_layout.nb_channels = avio_rb32(pb); > > st->codecpar->bits_per_coded_sample = avio_rb32(pb); > > +if (st->codecpar->ch_layout.nb_channels > FF_SANE_NB_CHANNELS || > > +st->codecpar->bits_per_coded_sample > 64) > > Where does the process take so long that oss-fuzz gets a timeout if these > are unreasonably high? I don't see nb_channels used anywhere in here where > that matters. > Is it in the PCM decoder? Because that decoder is meant to handle any > arbitrary amount of channels, so limiting it to whatever FF_SANE_NB_CHANNELS > is set to is not ok. libavutil/channel_layout.c:627:17 or it will OOM before depending on how much memory is available > > And is the bits_per_coded_sample > 64 check to prevent codec_id being > AV_CODEC_ID_NONE? if so, how does that affect demuxing time? > AV_CODEC_ID_NONE for that matter could happen for valid files with a codec > we don't currently support. We generally check read values for validity at the earliest, bits_per_coded_sample of more than 64 seem not valid to me. thx [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB I have never wished to cater to the crowd; for what I know they do not approve, and what they approve I do not know. -- Epicurus 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 1/3] avformat/cafdec: sanity check channels and bps
On Thu, Jun 27, 2024 at 08:53:09AM +0200, Anton Khirnov wrote: > Quoting Michael Niedermayer (2024-06-27 01:50:12) > > On Tue, Jun 25, 2024 at 09:27:55PM +0200, Anton Khirnov wrote: > > > Quoting Michael Niedermayer (2024-06-25 21:25:46) > > > > On Thu, Mar 28, 2024 at 12:27:02AM +0100, Michael Niedermayer wrote: > > > > > On Wed, Mar 27, 2024 at 08:39:17AM +0100, Anton Khirnov wrote: > > > > > > Quoting Michael Niedermayer (2024-03-23 00:08:16) > > > > > > > Fixes: Timeout > > > > > > > Fixes: > > > > > > > 67044/clusterfuzz-testcase-minimized-ffmpeg_dem_CAF_fuzzer-5791144363491328 > > > > > > > > > > > > > > Found-by: continuous fuzzing process > > > > > > > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > > > > > > Signed-off-by: Michael Niedermayer > > > > > > > --- > > > > > > > libavformat/cafdec.c | 5 + > > > > > > > 1 file changed, 5 insertions(+) > > > > > > > > > > > > > > diff --git a/libavformat/cafdec.c b/libavformat/cafdec.c > > > > > > > index 426c56b9bd..334077efb5 100644 > > > > > > > --- a/libavformat/cafdec.c > > > > > > > +++ b/libavformat/cafdec.c > > > > > > > @@ -33,6 +33,7 @@ > > > > > > > #include "isom.h" > > > > > > > #include "mov_chan.h" > > > > > > > #include "libavcodec/flac.h" > > > > > > > +#include "libavcodec/internal.h" > > > > > > > #include "libavutil/intreadwrite.h" > > > > > > > #include "libavutil/intfloat.h" > > > > > > > #include "libavutil/dict.h" > > > > > > > @@ -87,6 +88,10 @@ static int read_desc_chunk(AVFormatContext *s) > > > > > > > st->codecpar->ch_layout.nb_channels = avio_rb32(pb); > > > > > > > st->codecpar->bits_per_coded_sample = avio_rb32(pb); > > > > > > > > > > > > > > +if (st->codecpar->ch_layout.nb_channels > > > > > > > > FF_SANE_NB_CHANNELS || > > > > > > > > > > > > I dislike this. > > > > > > > > > > I dislike it too > > > > > > > > so what do we do about this ? > > > > > > About what? What is the actual problem that needs addressed? > > > > 67044/clusterfuzz-testcase-minimized-ffmpeg_dem_CAF_fuzzer-5791144363491328 > > > > > > > > > > > any objections to apply this ? > > > > > > yes, FF_SANE_NB_CHANNELS is a hack that should be removed, not spread > > > > a maximum number for each theoretically unlimited parameter is desirable > > I disagree. > > > This can be a user setable value or a compile time value when such is > > preferred. > > > > We can take this to the TC if you want. > > > > The same way as the number of files or number of bytes used by some cache > > needs limits, > > Those limits are implemented at the OS level, not by individual > programs. random pick, man ccache --max-files=N --max-size=SIZE > And they are runtime-configurable. I would favor runtime-configurable as well > > > so do channels, and number of pixels. > > Does not follow. For a file with length n "bits" it is understood and expected that there will be O(n) processing needed. having O(2^n) is not "expected" if you have a loop going over channels, pixels, or another "unlimited" property you can create very small files that require the maximum memory and processing allowed by the OS for the process Thats a DOS attack > > > One can remove them but users and companies concious about security and > > efficiency with untrusted input in an (semi-) automated environment will > > likely > > choose the codebase providing such features. > > [citation needed] > > My main objection to this approach is that you're addressing a symptom > (fuzzer timeout) rather than the actual problem (some code scaling > inappropriately with input size), and it's completely unclear where that > actual problem even is, as cafdec does not seem to be doing anything > with the channel count. That suggests the problem is really in some > other code and you're just papering over it. ive rerun the testcase and depending on settings it either hits OOM in av_channel_layout_custom_init() or it will timeout in av_channel_layout_describe_bprint libavutil/channel_layout.c:627:17 The file has 33 million channels. Now if for some reason none of the above would hit a timeout and instead the decoder would decode 33 million channels of silence maybe because it had a 1 byte packet and error concealment produced a silent packet to fill the space. Still the same issue exists only a limit on the channel number avoids this. Surely improving all the code that is slow with millions of channels is a good idea but even the most efficient code to produce a frame with arbitrary # million channels in it will lead to a DOS attack opertunity thx [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Whats the most studid thing your enemy could do ? Blow himself up Whats the most studid thing you could do ? Give up your rights and freedom because your enemy blew himself up. signature.asc Description: PGP signature ___ ffmpeg-devel mailing list
Re: [FFmpeg-devel] [PATCH 1/3] avformat/cafdec: sanity check channels and bps
Quoting Michael Niedermayer (2024-06-27 01:50:12) > On Tue, Jun 25, 2024 at 09:27:55PM +0200, Anton Khirnov wrote: > > Quoting Michael Niedermayer (2024-06-25 21:25:46) > > > On Thu, Mar 28, 2024 at 12:27:02AM +0100, Michael Niedermayer wrote: > > > > On Wed, Mar 27, 2024 at 08:39:17AM +0100, Anton Khirnov wrote: > > > > > Quoting Michael Niedermayer (2024-03-23 00:08:16) > > > > > > Fixes: Timeout > > > > > > Fixes: > > > > > > 67044/clusterfuzz-testcase-minimized-ffmpeg_dem_CAF_fuzzer-5791144363491328 > > > > > > > > > > > > Found-by: continuous fuzzing process > > > > > > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > > > > > Signed-off-by: Michael Niedermayer > > > > > > --- > > > > > > libavformat/cafdec.c | 5 + > > > > > > 1 file changed, 5 insertions(+) > > > > > > > > > > > > diff --git a/libavformat/cafdec.c b/libavformat/cafdec.c > > > > > > index 426c56b9bd..334077efb5 100644 > > > > > > --- a/libavformat/cafdec.c > > > > > > +++ b/libavformat/cafdec.c > > > > > > @@ -33,6 +33,7 @@ > > > > > > #include "isom.h" > > > > > > #include "mov_chan.h" > > > > > > #include "libavcodec/flac.h" > > > > > > +#include "libavcodec/internal.h" > > > > > > #include "libavutil/intreadwrite.h" > > > > > > #include "libavutil/intfloat.h" > > > > > > #include "libavutil/dict.h" > > > > > > @@ -87,6 +88,10 @@ static int read_desc_chunk(AVFormatContext *s) > > > > > > st->codecpar->ch_layout.nb_channels = avio_rb32(pb); > > > > > > st->codecpar->bits_per_coded_sample = avio_rb32(pb); > > > > > > > > > > > > +if (st->codecpar->ch_layout.nb_channels > FF_SANE_NB_CHANNELS > > > > > > || > > > > > > > > > > I dislike this. > > > > > > > > I dislike it too > > > > > > so what do we do about this ? > > > > About what? What is the actual problem that needs addressed? > > 67044/clusterfuzz-testcase-minimized-ffmpeg_dem_CAF_fuzzer-5791144363491328 > > > > > > > any objections to apply this ? > > > > yes, FF_SANE_NB_CHANNELS is a hack that should be removed, not spread > > a maximum number for each theoretically unlimited parameter is desirable I disagree. > This can be a user setable value or a compile time value when such is > preferred. > > We can take this to the TC if you want. > > The same way as the number of files or number of bytes used by some cache > needs limits, Those limits are implemented at the OS level, not by individual programs. And they are runtime-configurable. > so do channels, and number of pixels. Does not follow. > One can remove them but users and companies concious about security and > efficiency with untrusted input in an (semi-) automated environment will > likely > choose the codebase providing such features. [citation needed] My main objection to this approach is that you're addressing a symptom (fuzzer timeout) rather than the actual problem (some code scaling inappropriately with input size), and it's completely unclear where that actual problem even is, as cafdec does not seem to be doing anything with the channel count. That suggests the problem is really in some other code and you're just papering over it. -- Anton Khirnov ___ 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 1/3] avformat/cafdec: sanity check channels and bps
On Thu, Jun 27, 2024 at 1:50 AM Michael Niedermayer wrote: > On Tue, Jun 25, 2024 at 09:27:55PM +0200, Anton Khirnov wrote: > > Quoting Michael Niedermayer (2024-06-25 21:25:46) > > > On Thu, Mar 28, 2024 at 12:27:02AM +0100, Michael Niedermayer wrote: > > > > On Wed, Mar 27, 2024 at 08:39:17AM +0100, Anton Khirnov wrote: > > > > > Quoting Michael Niedermayer (2024-03-23 00:08:16) > > > > > > Fixes: Timeout > > > > > > Fixes: > 67044/clusterfuzz-testcase-minimized-ffmpeg_dem_CAF_fuzzer-5791144363491328 > > > > > > > > > > > > Found-by: continuous fuzzing process > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > > > > > Signed-off-by: Michael Niedermayer > > > > > > --- > > > > > > libavformat/cafdec.c | 5 + > > > > > > 1 file changed, 5 insertions(+) > > > > > > > > > > > > diff --git a/libavformat/cafdec.c b/libavformat/cafdec.c > > > > > > index 426c56b9bd..334077efb5 100644 > > > > > > --- a/libavformat/cafdec.c > > > > > > +++ b/libavformat/cafdec.c > > > > > > @@ -33,6 +33,7 @@ > > > > > > #include "isom.h" > > > > > > #include "mov_chan.h" > > > > > > #include "libavcodec/flac.h" > > > > > > +#include "libavcodec/internal.h" > > > > > > #include "libavutil/intreadwrite.h" > > > > > > #include "libavutil/intfloat.h" > > > > > > #include "libavutil/dict.h" > > > > > > @@ -87,6 +88,10 @@ static int read_desc_chunk(AVFormatContext *s) > > > > > > st->codecpar->ch_layout.nb_channels = avio_rb32(pb); > > > > > > st->codecpar->bits_per_coded_sample = avio_rb32(pb); > > > > > > > > > > > > +if (st->codecpar->ch_layout.nb_channels > > FF_SANE_NB_CHANNELS || > > > > > > > > > > I dislike this. > > > > > > > > I dislike it too > > > > > > so what do we do about this ? > > > > About what? What is the actual problem that needs addressed? > > 67044/clusterfuzz-testcase-minimized-ffmpeg_dem_CAF_fuzzer-5791144363491328 > > > > > > > any objections to apply this ? > > > > yes, FF_SANE_NB_CHANNELS is a hack that should be removed, not spread > > a maximum number for each theoretically unlimited parameter is desirable > This can be a user setable value or a compile time value when such is > preferred. > Cant you check if so many channels are actually available in stream? Or there is some silly loop that goes unchecked up to INT32_MAX ? > > We can take this to the TC if you want. > > The same way as the number of files or number of bytes used by some cache > needs limits, so do channels, and number of pixels. > > One can remove them but users and companies concious about security and > efficiency with untrusted input in an (semi-) automated environment will > likely > choose the codebase providing such features. > "Sure." > > thx > > [...] > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > The day soldiers stop bringing you their problems is the day you have > stopped > leading them. They have either lost confidence that you can help or > concluded > you do not care. Either case is a failure of leadership. - Colin Powell > ___ > 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 1/3] avformat/cafdec: sanity check channels and bps
On 3/22/2024 8:08 PM, Michael Niedermayer wrote: Fixes: Timeout Fixes: 67044/clusterfuzz-testcase-minimized-ffmpeg_dem_CAF_fuzzer-5791144363491328 Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg Signed-off-by: Michael Niedermayer --- libavformat/cafdec.c | 5 + 1 file changed, 5 insertions(+) diff --git a/libavformat/cafdec.c b/libavformat/cafdec.c index 426c56b9bd..334077efb5 100644 --- a/libavformat/cafdec.c +++ b/libavformat/cafdec.c @@ -33,6 +33,7 @@ #include "isom.h" #include "mov_chan.h" #include "libavcodec/flac.h" +#include "libavcodec/internal.h" #include "libavutil/intreadwrite.h" #include "libavutil/intfloat.h" #include "libavutil/dict.h" @@ -87,6 +88,10 @@ static int read_desc_chunk(AVFormatContext *s) st->codecpar->ch_layout.nb_channels = avio_rb32(pb); st->codecpar->bits_per_coded_sample = avio_rb32(pb); +if (st->codecpar->ch_layout.nb_channels > FF_SANE_NB_CHANNELS || +st->codecpar->bits_per_coded_sample > 64) Where does the process take so long that oss-fuzz gets a timeout if these are unreasonably high? I don't see nb_channels used anywhere in here where that matters. Is it in the PCM decoder? Because that decoder is meant to handle any arbitrary amount of channels, so limiting it to whatever FF_SANE_NB_CHANNELS is set to is not ok. And is the bits_per_coded_sample > 64 check to prevent codec_id being AV_CODEC_ID_NONE? if so, how does that affect demuxing time? AV_CODEC_ID_NONE for that matter could happen for valid files with a codec we don't currently support. +return AVERROR_INVALIDDATA; + if (caf->bytes_per_packet < 0 || caf->frames_per_packet < 0 || st->codecpar->ch_layout.nb_channels < 0) return AVERROR_INVALIDDATA; ___ 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 1/3] avformat/cafdec: sanity check channels and bps
On Tue, Jun 25, 2024 at 09:27:55PM +0200, Anton Khirnov wrote: > Quoting Michael Niedermayer (2024-06-25 21:25:46) > > On Thu, Mar 28, 2024 at 12:27:02AM +0100, Michael Niedermayer wrote: > > > On Wed, Mar 27, 2024 at 08:39:17AM +0100, Anton Khirnov wrote: > > > > Quoting Michael Niedermayer (2024-03-23 00:08:16) > > > > > Fixes: Timeout > > > > > Fixes: > > > > > 67044/clusterfuzz-testcase-minimized-ffmpeg_dem_CAF_fuzzer-5791144363491328 > > > > > > > > > > Found-by: continuous fuzzing process > > > > > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > > > > Signed-off-by: Michael Niedermayer > > > > > --- > > > > > libavformat/cafdec.c | 5 + > > > > > 1 file changed, 5 insertions(+) > > > > > > > > > > diff --git a/libavformat/cafdec.c b/libavformat/cafdec.c > > > > > index 426c56b9bd..334077efb5 100644 > > > > > --- a/libavformat/cafdec.c > > > > > +++ b/libavformat/cafdec.c > > > > > @@ -33,6 +33,7 @@ > > > > > #include "isom.h" > > > > > #include "mov_chan.h" > > > > > #include "libavcodec/flac.h" > > > > > +#include "libavcodec/internal.h" > > > > > #include "libavutil/intreadwrite.h" > > > > > #include "libavutil/intfloat.h" > > > > > #include "libavutil/dict.h" > > > > > @@ -87,6 +88,10 @@ static int read_desc_chunk(AVFormatContext *s) > > > > > st->codecpar->ch_layout.nb_channels = avio_rb32(pb); > > > > > st->codecpar->bits_per_coded_sample = avio_rb32(pb); > > > > > > > > > > +if (st->codecpar->ch_layout.nb_channels > FF_SANE_NB_CHANNELS || > > > > > > > > I dislike this. > > > > > > I dislike it too > > > > so what do we do about this ? > > About what? What is the actual problem that needs addressed? 67044/clusterfuzz-testcase-minimized-ffmpeg_dem_CAF_fuzzer-5791144363491328 > > > any objections to apply this ? > > yes, FF_SANE_NB_CHANNELS is a hack that should be removed, not spread a maximum number for each theoretically unlimited parameter is desirable This can be a user setable value or a compile time value when such is preferred. We can take this to the TC if you want. The same way as the number of files or number of bytes used by some cache needs limits, so do channels, and number of pixels. One can remove them but users and companies concious about security and efficiency with untrusted input in an (semi-) automated environment will likely choose the codebase providing such features. thx [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB The day soldiers stop bringing you their problems is the day you have stopped leading them. They have either lost confidence that you can help or concluded you do not care. Either case is a failure of leadership. - Colin Powell 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 1/3] avformat/cafdec: sanity check channels and bps
Quoting Michael Niedermayer (2024-06-25 21:25:46) > On Thu, Mar 28, 2024 at 12:27:02AM +0100, Michael Niedermayer wrote: > > On Wed, Mar 27, 2024 at 08:39:17AM +0100, Anton Khirnov wrote: > > > Quoting Michael Niedermayer (2024-03-23 00:08:16) > > > > Fixes: Timeout > > > > Fixes: > > > > 67044/clusterfuzz-testcase-minimized-ffmpeg_dem_CAF_fuzzer-5791144363491328 > > > > > > > > Found-by: continuous fuzzing process > > > > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > > > Signed-off-by: Michael Niedermayer > > > > --- > > > > libavformat/cafdec.c | 5 + > > > > 1 file changed, 5 insertions(+) > > > > > > > > diff --git a/libavformat/cafdec.c b/libavformat/cafdec.c > > > > index 426c56b9bd..334077efb5 100644 > > > > --- a/libavformat/cafdec.c > > > > +++ b/libavformat/cafdec.c > > > > @@ -33,6 +33,7 @@ > > > > #include "isom.h" > > > > #include "mov_chan.h" > > > > #include "libavcodec/flac.h" > > > > +#include "libavcodec/internal.h" > > > > #include "libavutil/intreadwrite.h" > > > > #include "libavutil/intfloat.h" > > > > #include "libavutil/dict.h" > > > > @@ -87,6 +88,10 @@ static int read_desc_chunk(AVFormatContext *s) > > > > st->codecpar->ch_layout.nb_channels = avio_rb32(pb); > > > > st->codecpar->bits_per_coded_sample = avio_rb32(pb); > > > > > > > > +if (st->codecpar->ch_layout.nb_channels > FF_SANE_NB_CHANNELS || > > > > > > I dislike this. > > > > I dislike it too > > so what do we do about this ? About what? What is the actual problem that needs addressed? > any objections to apply this ? yes, FF_SANE_NB_CHANNELS is a hack that should be removed, not spread -- Anton Khirnov ___ 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 1/3] avformat/cafdec: sanity check channels and bps
On Thu, Mar 28, 2024 at 12:27:02AM +0100, Michael Niedermayer wrote: > On Wed, Mar 27, 2024 at 08:39:17AM +0100, Anton Khirnov wrote: > > Quoting Michael Niedermayer (2024-03-23 00:08:16) > > > Fixes: Timeout > > > Fixes: > > > 67044/clusterfuzz-testcase-minimized-ffmpeg_dem_CAF_fuzzer-5791144363491328 > > > > > > Found-by: continuous fuzzing process > > > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > > Signed-off-by: Michael Niedermayer > > > --- > > > libavformat/cafdec.c | 5 + > > > 1 file changed, 5 insertions(+) > > > > > > diff --git a/libavformat/cafdec.c b/libavformat/cafdec.c > > > index 426c56b9bd..334077efb5 100644 > > > --- a/libavformat/cafdec.c > > > +++ b/libavformat/cafdec.c > > > @@ -33,6 +33,7 @@ > > > #include "isom.h" > > > #include "mov_chan.h" > > > #include "libavcodec/flac.h" > > > +#include "libavcodec/internal.h" > > > #include "libavutil/intreadwrite.h" > > > #include "libavutil/intfloat.h" > > > #include "libavutil/dict.h" > > > @@ -87,6 +88,10 @@ static int read_desc_chunk(AVFormatContext *s) > > > st->codecpar->ch_layout.nb_channels = avio_rb32(pb); > > > st->codecpar->bits_per_coded_sample = avio_rb32(pb); > > > > > > +if (st->codecpar->ch_layout.nb_channels > FF_SANE_NB_CHANNELS || > > > > I dislike this. > > I dislike it too so what do we do about this ? any objections to apply this ? thx [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB If the United States is serious about tackling the national security threats related to an insecure 5G network, it needs to rethink the extent to which it values corporate profits and government espionage over security.-Bruce Schneier 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 1/3] avformat/cafdec: sanity check channels and bps
On Wed, Mar 27, 2024 at 08:39:17AM +0100, Anton Khirnov wrote: > Quoting Michael Niedermayer (2024-03-23 00:08:16) > > Fixes: Timeout > > Fixes: > > 67044/clusterfuzz-testcase-minimized-ffmpeg_dem_CAF_fuzzer-5791144363491328 > > > > Found-by: continuous fuzzing process > > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > Signed-off-by: Michael Niedermayer > > --- > > libavformat/cafdec.c | 5 + > > 1 file changed, 5 insertions(+) > > > > diff --git a/libavformat/cafdec.c b/libavformat/cafdec.c > > index 426c56b9bd..334077efb5 100644 > > --- a/libavformat/cafdec.c > > +++ b/libavformat/cafdec.c > > @@ -33,6 +33,7 @@ > > #include "isom.h" > > #include "mov_chan.h" > > #include "libavcodec/flac.h" > > +#include "libavcodec/internal.h" > > #include "libavutil/intreadwrite.h" > > #include "libavutil/intfloat.h" > > #include "libavutil/dict.h" > > @@ -87,6 +88,10 @@ static int read_desc_chunk(AVFormatContext *s) > > st->codecpar->ch_layout.nb_channels = avio_rb32(pb); > > st->codecpar->bits_per_coded_sample = avio_rb32(pb); > > > > +if (st->codecpar->ch_layout.nb_channels > FF_SANE_NB_CHANNELS || > > I dislike this. I dislike it too thx [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Republics decline into democracies and democracies degenerate into despotisms. -- Aristotle 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 1/3] avformat/cafdec: sanity check channels and bps
Quoting Michael Niedermayer (2024-03-23 00:08:16) > Fixes: Timeout > Fixes: > 67044/clusterfuzz-testcase-minimized-ffmpeg_dem_CAF_fuzzer-5791144363491328 > > Found-by: continuous fuzzing process > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > Signed-off-by: Michael Niedermayer > --- > libavformat/cafdec.c | 5 + > 1 file changed, 5 insertions(+) > > diff --git a/libavformat/cafdec.c b/libavformat/cafdec.c > index 426c56b9bd..334077efb5 100644 > --- a/libavformat/cafdec.c > +++ b/libavformat/cafdec.c > @@ -33,6 +33,7 @@ > #include "isom.h" > #include "mov_chan.h" > #include "libavcodec/flac.h" > +#include "libavcodec/internal.h" > #include "libavutil/intreadwrite.h" > #include "libavutil/intfloat.h" > #include "libavutil/dict.h" > @@ -87,6 +88,10 @@ static int read_desc_chunk(AVFormatContext *s) > st->codecpar->ch_layout.nb_channels = avio_rb32(pb); > st->codecpar->bits_per_coded_sample = avio_rb32(pb); > > +if (st->codecpar->ch_layout.nb_channels > FF_SANE_NB_CHANNELS || I dislike this. -- Anton Khirnov ___ 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 1/3] avformat/cafdec: sanity check channels and bps
Fixes: Timeout Fixes: 67044/clusterfuzz-testcase-minimized-ffmpeg_dem_CAF_fuzzer-5791144363491328 Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg Signed-off-by: Michael Niedermayer --- libavformat/cafdec.c | 5 + 1 file changed, 5 insertions(+) diff --git a/libavformat/cafdec.c b/libavformat/cafdec.c index 426c56b9bd..334077efb5 100644 --- a/libavformat/cafdec.c +++ b/libavformat/cafdec.c @@ -33,6 +33,7 @@ #include "isom.h" #include "mov_chan.h" #include "libavcodec/flac.h" +#include "libavcodec/internal.h" #include "libavutil/intreadwrite.h" #include "libavutil/intfloat.h" #include "libavutil/dict.h" @@ -87,6 +88,10 @@ static int read_desc_chunk(AVFormatContext *s) st->codecpar->ch_layout.nb_channels = avio_rb32(pb); st->codecpar->bits_per_coded_sample = avio_rb32(pb); +if (st->codecpar->ch_layout.nb_channels > FF_SANE_NB_CHANNELS || +st->codecpar->bits_per_coded_sample > 64) +return AVERROR_INVALIDDATA; + if (caf->bytes_per_packet < 0 || caf->frames_per_packet < 0 || st->codecpar->ch_layout.nb_channels < 0) return AVERROR_INVALIDDATA; -- 2.17.1 ___ 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".
