[FFmpeg-devel] avformat/mov: validate chunk_count vs stsc_data

Submitted by chcunningham@chromium.org on Feb. 2, 2019, 2:25 a.m.

Details

Message ID 20190202022519.24932-1-chcunningham@chromium.org
State New
Headers show

Commit Message

chcunningham@chromium.org Feb. 2, 2019, 2:25 a.m.
Bad content may contain stsc boxes with a first_chunk index that
exceeds stco.entries (chunk_count).

mov_get_stsc_samples now checks for this and returns 0 when
values are invalid.
---
 libavformat/mov.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Michael Niedermayer Feb. 2, 2019, 11:54 a.m.
On Fri, Feb 01, 2019 at 06:25:19PM -0800, chcunningham wrote:
> Bad content may contain stsc boxes with a first_chunk index that
> exceeds stco.entries (chunk_count).
> 
> mov_get_stsc_samples now checks for this and returns 0 when
> values are invalid.
> ---
>  libavformat/mov.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/libavformat/mov.c b/libavformat/mov.c
> index 9b9739f788..dcf4ee8dc1 100644
> --- a/libavformat/mov.c
> +++ b/libavformat/mov.c
> @@ -2690,11 +2690,11 @@ static inline int mov_stsc_index_valid(unsigned int index, unsigned int count)
>  /* Compute the samples value for the stsc entry at the given index. */
>  static inline int64_t mov_get_stsc_samples(MOVStreamContext *sc, unsigned int index)
>  {
> -    int chunk_count;
> +    unsigned int chunk_count = 0;
>  
>      if (mov_stsc_index_valid(index, sc->stsc_count))
>          chunk_count = sc->stsc_data[index + 1].first - sc->stsc_data[index].first;
> -    else
> +    else if (sc->chunk_count >= sc->stsc_data[index].first)
>          chunk_count = sc->chunk_count - (sc->stsc_data[index].first - 1);

This construct occurs a 2nd time (in mov_build_index()) is this not affected?

mov_read_trak() contains a check for chunk_count and the first index(es)
(obviously this is not catching this one but)
is there a reason not to eliminate the inconsistancy at that or some other
"early" point?

Thanks

[...]
chcunningham@chromium.org Feb. 5, 2019, 12:29 a.m.
On Sat, Feb 2, 2019 at 3:55 AM Michael Niedermayer <michael@niedermayer.cc>
wrote:

> >  static inline int64_t mov_get_stsc_samples(MOVStreamContext *sc,
> unsigned int index)
> >  {
> > -    int chunk_count;
> > +    unsigned int chunk_count = 0;
> >
> >      if (mov_stsc_index_valid(index, sc->stsc_count))
> >          chunk_count = sc->stsc_data[index + 1].first -
> sc->stsc_data[index].first;
> > -    else
> > +    else if (sc->chunk_count >= sc->stsc_data[index].first)
> >          chunk_count = sc->chunk_count - (sc->stsc_data[index].first -
> 1);
>
> This construct occurs a 2nd time (in mov_build_index()) is this not
> affected?
>

Didn't notice it, but I think it would be affected. I'll leave this alone
for now, but I'm open to adding a mov_get_chunk_count helper to call from
both mov_build_index and mov_get_stsc_samples.


> mov_read_trak() contains a check for chunk_count and the first index(es)
> (obviously this is not catching this one but)
> is there a reason not to eliminate the inconsistancy at that or some other
> "early" point?
>

Agree this sounds better. Stand by for patch.

Patch hide | download patch | download mbox

diff --git a/libavformat/mov.c b/libavformat/mov.c
index 9b9739f788..dcf4ee8dc1 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -2690,11 +2690,11 @@  static inline int mov_stsc_index_valid(unsigned int index, unsigned int count)
 /* Compute the samples value for the stsc entry at the given index. */
 static inline int64_t mov_get_stsc_samples(MOVStreamContext *sc, unsigned int index)
 {
-    int chunk_count;
+    unsigned int chunk_count = 0;
 
     if (mov_stsc_index_valid(index, sc->stsc_count))
         chunk_count = sc->stsc_data[index + 1].first - sc->stsc_data[index].first;
-    else
+    else if (sc->chunk_count >= sc->stsc_data[index].first)
         chunk_count = sc->chunk_count - (sc->stsc_data[index].first - 1);
 
     return sc->stsc_data[index].count * (int64_t)chunk_count;