diff mbox

[FFmpeg-devel] avformat/matroskadec: Check parents remaining length

Message ID CAPUDrwdgFMebytMhKfUbCok7Yk1wd2h0=vfkGTy97Q2ta_DGJQ@mail.gmail.com
State New
Headers show

Commit Message

Dale Curtis Feb. 21, 2019, 10:30 p.m. UTC 
One of our test clips is behaving differently after this patch:
https://cs.chromium.org/chromium/src/media/test/data/bear-320x240-live.webm

The printed log message is:
[matroska,webm @ 0x1516c84f4e00] Invalid length 0xffffffffffffff >
0x10000000000002f in parent

Looking through the code I'm unsure which of the mixed usage "(uint64_t)-1"
and "2**56-1" as marker values is correct. Changing the code to:
                        "Invalid length 0x%"PRIx64" > 0x%"PRIx64" in
parent\n",

Resolves our issues. Should all other length tests be done the same way?

- dale

On Sun, Feb 17, 2019 at 12:45 AM Michael Niedermayer <michaelni@gmx.at>
wrote:

> On Wed, Feb 13, 2019 at 01:41:31PM +0100, Michael Niedermayer wrote:
> > Reported-by: Steve Lhomme
> > This was found through the Hacker One program on VLC but is not a
> security issue in libavformat
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  libavformat/matroskadec.c | 21 +++++++++++++++++++++
> >  1 file changed, 21 insertions(+)
>
> will apply an equivalent variant from steve
>
> [...]
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> Asymptotically faster algorithms should always be preferred if you have
> asymptotical amounts of data
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>

Comments

Dale Curtis Feb. 22, 2019, 8:31 p.m. UTC | #1 
+steve who submitted the original patch.

- dale


On Thu, Feb 21, 2019 at 2:30 PM Dale Curtis <dalecurtis@chromium.org> wrote:

> One of our test clips is behaving differently after this patch:
> https://cs.chromium.org/chromium/src/media/test/data/bear-320x240-live.webm
>
> The printed log message is:
> [matroska,webm @ 0x1516c84f4e00] Invalid length 0xffffffffffffff >
> 0x10000000000002f in parent
>
> Looking through the code I'm unsure which of the mixed usage
> "(uint64_t)-1" and "2**56-1" as marker values is correct. Changing the code
> to:
> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
> index 9b706ab4e0..3015a0b230 100644
> --- a/libavformat/matroskadec.c
> +++ b/libavformat/matroskadec.c
> @@ -1205,7 +1205,7 @@ static int ebml_parse_elem(MatroskaDemuxContext
> *matroska,
>              MatroskaLevel *level = &matroska->levels[matroska->num_levels
> - 1];
>              AVIOContext *pb = matroska->ctx->pb;
>              int64_t pos = avio_tell(pb);
> -            if (level->length != (uint64_t) -1 &&
> +            if (level->length != 0xffffffffffffffULL &&
>                  (pos + length) > (level->start + level->length)) {
>                  av_log(matroska->ctx, AV_LOG_ERROR,
>                         "Invalid length 0x%"PRIx64" > 0x%"PRIx64" in
> parent\n",
>
> Resolves our issues. Should all other length tests be done the same way?
>
> - dale
>
> On Sun, Feb 17, 2019 at 12:45 AM Michael Niedermayer <michaelni@gmx.at>
> wrote:
>
>> On Wed, Feb 13, 2019 at 01:41:31PM +0100, Michael Niedermayer wrote:
>> > Reported-by: Steve Lhomme
>> > This was found through the Hacker One program on VLC but is not a
>> security issue in libavformat
>> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>> > ---
>> >  libavformat/matroskadec.c | 21 +++++++++++++++++++++
>> >  1 file changed, 21 insertions(+)
>>
>> will apply an equivalent variant from steve
>>
>> [...]
>> --
>> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>>
>> Asymptotically faster algorithms should always be preferred if you have
>> asymptotical amounts of data
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>
Dale Curtis Feb. 22, 2019, 11:42 p.m. UTC | #2 
Sent http://ffmpeg.org/pipermail/ffmpeg-devel/2019-February/240418.html -
which passes fate and fixes the issue with our test clip.

- dale

On Fri, Feb 22, 2019 at 12:31 PM Dale Curtis <dalecurtis@chromium.org>
wrote:

> +steve who submitted the original patch.
>
> - dale
>
>
> On Thu, Feb 21, 2019 at 2:30 PM Dale Curtis <dalecurtis@chromium.org>
> wrote:
>
>> One of our test clips is behaving differently after this patch:
>>
>> https://cs.chromium.org/chromium/src/media/test/data/bear-320x240-live.webm
>>
>> The printed log message is:
>> [matroska,webm @ 0x1516c84f4e00] Invalid length 0xffffffffffffff >
>> 0x10000000000002f in parent
>>
>> Looking through the code I'm unsure which of the mixed usage
>> "(uint64_t)-1" and "2**56-1" as marker values is correct. Changing the code
>> to:
>> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
>> index 9b706ab4e0..3015a0b230 100644
>> --- a/libavformat/matroskadec.c
>> +++ b/libavformat/matroskadec.c
>> @@ -1205,7 +1205,7 @@ static int ebml_parse_elem(MatroskaDemuxContext
>> *matroska,
>>              MatroskaLevel *level =
>> &matroska->levels[matroska->num_levels - 1];
>>              AVIOContext *pb = matroska->ctx->pb;
>>              int64_t pos = avio_tell(pb);
>> -            if (level->length != (uint64_t) -1 &&
>> +            if (level->length != 0xffffffffffffffULL &&
>>                  (pos + length) > (level->start + level->length)) {
>>                  av_log(matroska->ctx, AV_LOG_ERROR,
>>                         "Invalid length 0x%"PRIx64" > 0x%"PRIx64" in
>> parent\n",
>>
>> Resolves our issues. Should all other length tests be done the same way?
>>
>> - dale
>>
>> On Sun, Feb 17, 2019 at 12:45 AM Michael Niedermayer <michaelni@gmx.at>
>> wrote:
>>
>>> On Wed, Feb 13, 2019 at 01:41:31PM +0100, Michael Niedermayer wrote:
>>> > Reported-by: Steve Lhomme
>>> > This was found through the Hacker One program on VLC but is not a
>>> security issue in libavformat
>>> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>>> > ---
>>> >  libavformat/matroskadec.c | 21 +++++++++++++++++++++
>>> >  1 file changed, 21 insertions(+)
>>>
>>> will apply an equivalent variant from steve
>>>
>>> [...]
>>> --
>>> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>>>
>>> Asymptotically faster algorithms should always be preferred if you have
>>> asymptotical amounts of data
>>> _______________________________________________
>>> ffmpeg-devel mailing list
>>> ffmpeg-devel@ffmpeg.org
>>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>>
>>
Steve Lhomme Feb. 23, 2019, 9:05 a.m. UTC | #3 
Hi,

It's possible that it fixes the issue. When I did the patch I noticed some places used a similar value and some are using -1. I think this should be a define (probably set to UINT64_MAX) that is used everywhere. It's just a marker to say the length is unknown.


> On February 22, 2019 at 9:31 PM Dale Curtis <dalecurtis@chromium.org> wrote:
> 
>     +steve who submitted the original patch.
> 
>     - dale
> 
> 
>     On Thu, Feb 21, 2019 at 2:30 PM Dale Curtis < dalecurtis@chromium.org mailto:dalecurtis@chromium.org > wrote:
> 
>         > >         One of our test clips is behaving differently after this patch:
> >         https://cs.chromium.org/chromium/src/media/test/data/bear-320x240-live.webm
> > 
> >         The printed log message is:
> >         [matroska,webm @ 0x1516c84f4e00] Invalid length 0xffffffffffffff > 0x10000000000002f in parent
> > 
> >         Looking through the code I'm unsure which of the mixed usage "(uint64_t)-1" and "2**56-1" as marker values is correct. Changing the code to:
> >         diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
> >         index 9b706ab4e0..3015a0b230 100644
> >         --- a/libavformat/matroskadec.c
> >         +++ b/libavformat/matroskadec.c
> >         @@ -1205,7 +1205,7 @@ static int ebml_parse_elem(MatroskaDemuxContext *matroska,
> >                      MatroskaLevel *level = &matroska->levels[matroska->num_levels - 1];
> >                      AVIOContext *pb = matroska->ctx->pb;
> >                      int64_t pos = avio_tell(pb);
> >         -            if (level->length != (uint64_t) -1 &&
> >         +            if (level->length != 0xffffffffffffffULL &&
> >                          (pos + length) > (level->start + level->length)) {
> >                          av_log(matroska->ctx, AV_LOG_ERROR,
> >                                 "Invalid length 0x%"PRIx64" > 0x%"PRIx64" in parent\n",
> > 
> >         Resolves our issues. Should all other length tests be done the same way?
> > 
> >         - dale
> > 
> >         On Sun, Feb 17, 2019 at 12:45 AM Michael Niedermayer < michaelni@gmx.at mailto:michaelni@gmx.at > wrote:
> > 
> >             > > > On Wed, Feb 13, 2019 at 01:41:31PM +0100, Michael Niedermayer wrote:
> > >             > Reported-by: Steve Lhomme
> > >             > This was found through the Hacker One program on VLC but is not a security issue in libavformat
> > >             > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > >             > ---
> > >             >  libavformat/matroskadec.c | 21 +++++++++++++++++++++
> > >             >  1 file changed, 21 insertions(+)
> > > 
> > >             will apply an equivalent variant from steve
> > > 
> > >             [...]
> > >             --
> > >             Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
> > > 
> > >             Asymptotically faster algorithms should always be preferred if you have
> > >             asymptotical amounts of data
> > >             _______________________________________________
> > >             ffmpeg-devel mailing list
> > >             ffmpeg-devel@ffmpeg.org mailto:ffmpeg-devel@ffmpeg.org
> > >             http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > > 
> > >         > > 
> >     >
diff mbox

Patch

diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
index 9b706ab4e0..3015a0b230 100644
--- a/libavformat/matroskadec.c
+++ b/libavformat/matroskadec.c
@@ -1205,7 +1205,7 @@  static int ebml_parse_elem(MatroskaDemuxContext
*matroska,
             MatroskaLevel *level = &matroska->levels[matroska->num_levels
- 1];
             AVIOContext *pb = matroska->ctx->pb;
             int64_t pos = avio_tell(pb);
-            if (level->length != (uint64_t) -1 &&
+            if (level->length != 0xffffffffffffffULL &&
                 (pos + length) > (level->start + level->length)) {
                 av_log(matroska->ctx, AV_LOG_ERROR,