Re: [FFmpeg-devel] [PATCH 1/2] avcodec/qtrle: Do not output duplicated frames on insufficient input

[Michael Niedermayer]

On Tue, May 15, 2018 at 11:48:46PM +0200, Carl Eugen Hoyos wrote:
> 2018-05-14 1:07 GMT+02:00, Michael Niedermayer :
> > Fixes: Timeout
> > Fixes:
> > 6383/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_QTRLE_fuzzer-6199846902956032
> 
> Looking at the fate change, my suspicion is that the change
> is a (very) good idea but the commit message could explain
> better that duplicating frames is generally not what
> libavcodec does, it should be left to the calling application.

will repost with a more verbose commit message

[...]
-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Avoid a single point of failure, be that a person or equipment.


signature.asc
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/2] avcodec/qtrle: Do not output duplicated frames on insufficient input

[Michael Niedermayer]

On Thu, May 17, 2018 at 12:22:01PM +0200, Hendrik Leppkes wrote:
> On Thu, May 17, 2018 at 11:49 AM, Michael Niedermayer
>  wrote:
> > On Wed, May 16, 2018 at 12:53:46AM +0200, Carl Eugen Hoyos wrote:
> >> 2018-05-16 0:29 GMT+02:00, Hendrik Leppkes :
> >>
> >> > It makes no real difference if its less efficient or whatever -
> >> > if a codec specification asks for this behavior, then our
> >> > decoders should act accordingly.
> >>
> >> I wonder where this suddenly comes from?
> >> (I was away from my mail client when a similar argument
> >> was used a few weeks ago and I forgot later.)
> >> Luckily, many of our codecs do smarter things
> >> than the specifications asks for...
> >>
> >> Do we have a specification for qtrle?
> >
> > Iam only aware of the one "we", that is more correctly mike melanson
> > wrote: https://multimedia.cx/qtrle.txt
> >
> > I think this invalidates the argument, so if i hear no objections then
> > ill apply this patch in a few days
> >
> 
> How does that invalidate the argument that you take a compressed CRF
> stream and suddenly decide to make VFR out of it?
> Because it does not.

We have always droped cloned fields and frames even where the specification
unquestionably wants something different, when this is inefficient

For example mpeg2 has its field and frame duplication information, we
update timestamps according to this information but never duplicate
fields/frames.

If we imagine an alterantive reality where we did clone then
the decoder would be 20% slower in cases where it differs maybe, and we
would output many progressive videos as interlaced which could not be 
displayed nicely that way on the majority of display devices.
So what we do for mpeg2 makes sense. And this too changes constant field
rate mpeg2 into vfr

now qtrle, is a niche codec, why should it be handled in a way that is
1. inefficient & slower
2. easier to mount a DOS attack against
3. inconsistent with the other decoders we have

?

Maybe i just dont understand your concern
Is it that theres some need in some application for something like regular
heartbeat frames ? 
If so this is a issue for most VFR and especially subtitles, changes to qtrle
wont really help or hurt this. It would require injecting regular frames
or a flag that for every decoder clones frames if nothing occured beyond a
threshold



[...]
-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

"Nothing to hide" only works if the folks in power share the values of
you and everyone you know entirely and always will -- Tom Scott



signature.asc
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/2] avcodec/qtrle: Do not output duplicated frames on insufficient input

[Carl Eugen Hoyos]

2018-05-17 12:22 GMT+02:00, Hendrik Leppkes :
> On Thu, May 17, 2018 at 11:49 AM, Michael Niedermayer
>  wrote:
>> On Wed, May 16, 2018 at 12:53:46AM +0200, Carl Eugen Hoyos wrote:
>>> 2018-05-16 0:29 GMT+02:00, Hendrik Leppkes :
>>>
>>> > It makes no real difference if its less efficient or whatever -
>>> > if a codec specification asks for this behavior, then our
>>> > decoders should act accordingly.
>>>
>>> I wonder where this suddenly comes from?
>>> (I was away from my mail client when a similar argument
>>> was used a few weeks ago and I forgot later.)
>>> Luckily, many of our codecs do smarter things
>>> than the specifications asks for...
>>>
>>> Do we have a specification for qtrle?
>>
>> Iam only aware of the one "we", that is more correctly mike
>> melanson wrote: https://multimedia.cx/qtrle.txt
>>
>> I think this invalidates the argument, so if i hear no objections
>> then ill apply this patch in a few days
>
> How does that invalidate the argument that you take a compressed
> CRF stream and suddenly decide to make VFR out of it?

Apart from the fact that your argument was that there is a
specification that we have to follow (an argument that should
be used very carefully imo), the argument that we should do
something less efficient instead of improving the efficiency
does not seem very strong to me.

Carl Eugen
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/2] avcodec/qtrle: Do not output duplicated frames on insufficient input

[Hendrik Leppkes]

On Thu, May 17, 2018 at 11:49 AM, Michael Niedermayer
 wrote:
> On Wed, May 16, 2018 at 12:53:46AM +0200, Carl Eugen Hoyos wrote:
>> 2018-05-16 0:29 GMT+02:00, Hendrik Leppkes :
>>
>> > It makes no real difference if its less efficient or whatever -
>> > if a codec specification asks for this behavior, then our
>> > decoders should act accordingly.
>>
>> I wonder where this suddenly comes from?
>> (I was away from my mail client when a similar argument
>> was used a few weeks ago and I forgot later.)
>> Luckily, many of our codecs do smarter things
>> than the specifications asks for...
>>
>> Do we have a specification for qtrle?
>
> Iam only aware of the one "we", that is more correctly mike melanson
> wrote: https://multimedia.cx/qtrle.txt
>
> I think this invalidates the argument, so if i hear no objections then
> ill apply this patch in a few days
>

How does that invalidate the argument that you take a compressed CRF
stream and suddenly decide to make VFR out of it?
Because it does not.

- Hendrik
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/2] avcodec/qtrle: Do not output duplicated frames on insufficient input

[Michael Niedermayer]

On Wed, May 16, 2018 at 12:53:46AM +0200, Carl Eugen Hoyos wrote:
> 2018-05-16 0:29 GMT+02:00, Hendrik Leppkes :
> 
> > It makes no real difference if its less efficient or whatever -
> > if a codec specification asks for this behavior, then our
> > decoders should act accordingly.
> 
> I wonder where this suddenly comes from?
> (I was away from my mail client when a similar argument
> was used a few weeks ago and I forgot later.)
> Luckily, many of our codecs do smarter things
> than the specifications asks for...
> 
> Do we have a specification for qtrle?

Iam only aware of the one "we", that is more correctly mike melanson
wrote: https://multimedia.cx/qtrle.txt

I think this invalidates the argument, so if i hear no objections then
ill apply this patch in a few days

thanks

[...]
-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Elect your leaders based on what they did after the last election, not
based on what they say before an election.



signature.asc
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/2] avcodec/qtrle: Do not output duplicated frames on insufficient input

[Carl Eugen Hoyos]

2018-05-16 0:29 GMT+02:00, Hendrik Leppkes :

> It makes no real difference if its less efficient or whatever -
> if a codec specification asks for this behavior, then our
> decoders should act accordingly.

I wonder where this suddenly comes from?
(I was away from my mail client when a similar argument
was used a few weeks ago and I forgot later.)
Luckily, many of our codecs do smarter things
than the specifications asks for...

Do we have a specification for qtrle?

Carl Eugen
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/2] avcodec/qtrle: Do not output duplicated frames on insufficient input

[Hendrik Leppkes]

On Tue, May 15, 2018 at 11:02 PM, Michael Niedermayer
 wrote:
> On Mon, May 14, 2018 at 01:31:42AM +0200, Hendrik Leppkes wrote:
>> On Mon, May 14, 2018 at 1:07 AM, Michael Niedermayer
>>  wrote:
>> > Fixes: Timeout
>> > Fixes: 
>> > 6383/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_QTRLE_fuzzer-6199846902956032
>> >
>> > Found-by: continuous fuzzing process 
>> > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>> > Signed-off-by: Michael Niedermayer 
>> >
>>
>> This does not seem like an issue to fix, but a fundamental change in
>> the behavior of the codec. It currently properly honors all frame
>> types, including "empty" frames, which results in proper constant
>> frame rate output.
>> After the change, it just flat out ignores those and discards them
>> without any further consideration.
>>
>> Does not sound like a fuzzing or security related change to me, and
>> possibly also quite the wrong thing to do.
>
> having a small code that outputs empty frames is a denial of service
> issue (which is a security issue). The attacker can with almost no bandwidth
> requirement cause the decode side to spend almost arbitrary amounts of
> CPU doing nothing usefull.
>
> This problem is reduced when the copy case requires bandwidth proportional
> to the resolution. As in a code per macroblock to skip it.
> not one bit to make the player copy 1gb of data
>
> said differently this increases the cost to an attacker per CPU cycle
> she causes on the player.
>
> In this particular case it also looks like that this is not
> just a skip code but the decoder actually outputs empty frames on errors.
>
> besides security its also more efficient not to send cloned frames
> though the whole chain (which forces every component to do duplicated work
> on each). And we avoid this in other decoders as well.
>

It makes no real difference if its less efficient or whatever - if a
codec specification asks for this behavior, then our decoders should
act accordingly. Generating vfr decoder output from a cfr input stream
is not in any way a good idea.

- Hendrik
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/2] avcodec/qtrle: Do not output duplicated frames on insufficient input

[Carl Eugen Hoyos]

2018-05-14 1:07 GMT+02:00, Michael Niedermayer :
> Fixes: Timeout
> Fixes:
> 6383/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_QTRLE_fuzzer-6199846902956032

Looking at the fate change, my suspicion is that the change
is a (very) good idea but the commit message could explain
better that duplicating frames is generally not what
libavcodec does, it should be left to the calling application.

Carl Eugen
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/2] avcodec/qtrle: Do not output duplicated frames on insufficient input

[Michael Niedermayer]

On Mon, May 14, 2018 at 01:31:42AM +0200, Hendrik Leppkes wrote:
> On Mon, May 14, 2018 at 1:07 AM, Michael Niedermayer
>  wrote:
> > Fixes: Timeout
> > Fixes: 
> > 6383/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_QTRLE_fuzzer-6199846902956032
> >
> > Found-by: continuous fuzzing process 
> > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > Signed-off-by: Michael Niedermayer 
> >
> 
> This does not seem like an issue to fix, but a fundamental change in
> the behavior of the codec. It currently properly honors all frame
> types, including "empty" frames, which results in proper constant
> frame rate output.
> After the change, it just flat out ignores those and discards them
> without any further consideration.
> 
> Does not sound like a fuzzing or security related change to me, and
> possibly also quite the wrong thing to do.

having a small code that outputs empty frames is a denial of service
issue (which is a security issue). The attacker can with almost no bandwidth
requirement cause the decode side to spend almost arbitrary amounts of
CPU doing nothing usefull.

This problem is reduced when the copy case requires bandwidth proportional
to the resolution. As in a code per macroblock to skip it.
not one bit to make the player copy 1gb of data

said differently this increases the cost to an attacker per CPU cycle
she causes on the player.

In this particular case it also looks like that this is not
just a skip code but the decoder actually outputs empty frames on errors.

besides security its also more efficient not to send cloned frames
though the whole chain (which forces every component to do duplicated work
on each). And we avoid this in other decoders as well.

[...]
-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

If a bugfix only changes things apparently unrelated to the bug with no
further explanation, that is a good sign that the bugfix is wrong.


signature.asc
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/2] avcodec/qtrle: Do not output duplicated frames on insufficient input

[Hendrik Leppkes]

On Mon, May 14, 2018 at 1:07 AM, Michael Niedermayer
 wrote:
> Fixes: Timeout
> Fixes: 
> 6383/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_QTRLE_fuzzer-6199846902956032
>
> Found-by: continuous fuzzing process 
> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer 
>

This does not seem like an issue to fix, but a fundamental change in
the behavior of the codec. It currently properly honors all frame
types, including "empty" frames, which results in proper constant
frame rate output.
After the change, it just flat out ignores those and discards them
without any further consideration.

Does not sound like a fuzzing or security related change to me, and
possibly also quite the wrong thing to do.

- Hendrik
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel