[FFmpeg-devel] [PATCH 1/2] avcodec/vp9: Check in decode_tiles() if there is data remaining

Michael Niedermayer michael at niedermayer.cc
Fri Aug 10 03:49:31 EEST 2018


On Wed, Aug 08, 2018 at 10:00:42PM -0300, James Almer wrote:
> On 8/8/2018 9:30 PM, Michael Niedermayer wrote:
> > On Tue, Aug 07, 2018 at 09:07:11PM -0400, Ronald S. Bultje wrote:
> >> Hi,
> >>
> >> On Tue, Aug 7, 2018 at 7:15 PM, Michael Niedermayer <michael at niedermayer.cc>
> >> wrote:
> >>
> >>> On Mon, Aug 06, 2018 at 09:50:57PM -0400, Ronald S. Bultje wrote:
> >>>> Hi,
> >>>>
> >>>> On Mon, Aug 6, 2018 at 3:00 PM, Michael Niedermayer
> >>> <michael at niedermayer.cc>
> >>>> wrote:
> >>>>
> >>>>> On Tue, Aug 07, 2018 at 01:05:51AM +0800, Ronald S. Bultje wrote:
> >>>>>> Hi,
> >>>>>>
> >>>>>> On Sun, Aug 5, 2018, 9:17 AM Michael Niedermayer
> >>> <michael at niedermayer.cc
> >>>>>>
> >>>>>> wrote:
> >>>>>>
> >>>>>>> Fixes: Timeout
> >>>>>>> Fixes:
> >>>>>>> 9330/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_
> >>>>> VP9_fuzzer-5707345857347584
> >>>>>>>
> >>>>>>> Found-by: continuous fuzzing process
> >>>>>>> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> >>>>>>> Signed-off-by
> >>>>>>> <https://github.com/google/oss-fuzz/tree/master/projects/
> >>>>> ffmpegSigned-off-by>:
> >>>>>>> Michael Niedermayer <michael at niedermayer.cc>
> >>>>>>> ---
> >>>>>>>  libavcodec/vp9.c | 3 +++
> >>>>>>>  1 file changed, 3 insertions(+)
> >>>>>>>
> >>>>>>> diff --git a/libavcodec/vp9.c b/libavcodec/vp9.c
> >>>>>>> index b1178c9c0c..4ca51ec108 100644
> >>>>>>> --- a/libavcodec/vp9.c
> >>>>>>> +++ b/libavcodec/vp9.c
> >>>>>>> @@ -1302,6 +1302,9 @@ static int decode_tiles(AVCodecContext
> >>> *avctx,
> >>>>>>>                          memset(lflvl_ptr->mask, 0,
> >>>>>>> sizeof(lflvl_ptr->mask));
> >>>>>>>                      }
> >>>>>>>
> >>>>>>> +                    if (td->c->end <= td->c->buffer &&
> >>> td->c->bits >=
> >>>>> 0) {
> >>>>>>> +                        return AVERROR_INVALIDDATA;
> >>>>>>> +                    }
> >>>>>>>                      if (s->pass == 2) {
> >>>>>>>                          decode_sb_mem(td, row, col, lflvl_ptr,
> >>>>>>>                                        yoff2, uvoff2, BL_64X64);
> >>>>>>>
> >>>>>>
> >>>>>> I don't think this matches spec. Implementations could use this to
> >>> store
> >>>>>> auxiliary data.
> >>>>>
> >>>>> This checks, or rather is intended to check for a premature end of the
> >>>>> input.
> >>>>> Am i missing something? because a premature end of input seems not to
> >>> lead
> >>>>> to more (auxiliary or other) data in the input.
> >>>>
> >>>>
> >>>> Hm, I misread it, sorry about that, my bad. Please ignore my first
> >>> review.
> >>>>
> >>>
> >>>> Is end == buffer && bits == 0 something that can happen on valid input if
> >>>> things just align properly? Otherwise looks OK.
> >>>
> >>> The same condition is used in vp5/6/8.
> >>> I think this condition only occurs if the input ends. The part i do not
> >>> know
> >>> is if such premature ends might be a "valid compression"
> >>>
> >>> Either way, if this check misbehaves for a valid file then it should be
> >>> reverted/removed unless its improv-able and improved.
> >>
> >>
> > 
> >> <sarcasm> Yes, that's how production grade software works. </sarcasm> 
> > 
> > yes ;)
> > but seriously, It only needs a single user hitting a bug and reporting it
> > for us to know its a issue and revert. This is not in a release just git
> > master.
> > Its my oppinion that this is wiser than to never attempt to fix the issue.
> > Which ultimatly is the alternative. That is unless you know that this is
> > correct or incorrect for all cases.
> > Maybe we have very differnt views of how to do software development.
> > My goal is to minimize the amount of issues in the code per developer time
> > spend. Time is a limited resource. Doing anything else simply leads to worse code.
> > If i spend a week testing code that i know is 99.8% right to make it
> > 99.9% right just to save a single user from once hitting and reporting a
> > non security bug. That is a loss as i could have spend that week fixing
> > many other issues.
> > Its even worse if the 99.9% still hits one user and gets reverted, in this
> > case there is no improvment in user experience at all for the time spend.
> > In either the 99.8 and 99.9% cases one user ultimatly hits a file that breaks
> > and teh change gets reverted. but in one case we would have spend alot more
> > time. 
> > 
> > So yes, it is my philosophy to use our users as beta testers. Not as a
> > default no absolutely not, but in the rare cases where the extra work
> > we cause our users is comparably tiny compared to how much work we would
> > have to do to avoid it. Or if we can never really match user testing anyway
> > And we also did and do this all over the place. For example, if we do not
> > know how to handle some case and have no sample, we print a message and ask
> > the user for a sample. 
> > This is comparable to the case here. "Is there a sample out there for which this
> > check misbehaves" vs. "Is there a file out there that uses a special feature"
> > Either is a question about global file existence and search
> 

> The end user is not a beta tester. A user knowingly and willingly acting
> as one is a beta tester.

i did not mean "beta tester" in the litteral sense of the word.
You are correct of course if you read it litterally


> Firefox doesn't use git master, it uses the latest release, and that
> means that a sizable bulk of users testing a wide array of vp9 streams
> in order to find out if this change breaks any of them will not happen
> until it's deployed to the general public.
> We commit this change, we branch ffmpeg 4.1 with it, Firefox implements
> it, deploys a release, and only then it will truly get tested. With luck
> any such stream is detected in the beta channel by the aforementioned
> beta testers, but the percentage of users testing vp9 streams there is
> minimal. Chances are nothing would be found until it's effectively
> deployed in the release channel and the complains that Youtube or some
> other streaming service is misbehaving start pouring by the hundreds.

This is a valid concern but without this fix theres one more way with which
to do a denial of service attack against firefox.


> 
> > 
> > 
> >> Can
> >> you just make it not error out on the end == buffer && bits == 0 condition?
> >> Or does that somehow not fix your timeout?
> > 
> > If the code continues processing "nothing" and does alot of computations on it,
> > it will need a long time to complete that. Thats the issue.
> > Its like a network router deciding to spend an hour processing a 100byte
> > packet that says its 4gb large in a header without there anywhere being 4gb
> > of input data. simple denial of service.
> 
> If continuing when the "end == buffer && bits == 0" condition is met is
> the intended behavior as Ronald mentioned, then the theoretical case
> where it could end up in a lot of time processing nothing is irrelevant.
> It is intended behavior, and breaking it may have consequences with
> real, valid files.
> 

> Apply this patch with changes to allow that specific condition and lets
> not waste more time on this.

this is the only change the patch does. Without it there is no patch.

Either we stop when the input ends -> that might break decoding a file
that was designed so as to expect a decoder not to stop.
or we do not stop then that allows doing denial of service

Of course we could do X iterations beyond the end. But not only has noone
suggested this, we then would have to decide on how many and why.

Its not the Implementations that is in dispute here. At least not in a
way from which i could write a different working Implementation


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

It is dangerous to be right in matters on which the established authorities
are wrong. -- Voltaire
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20180810/b7b8d8b3/attachment.sig>


More information about the ffmpeg-devel mailing list