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

Submitted by chcunningham@chromium.org on Feb. 1, 2019, 1:18 a.m.

Details

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

Commit Message

chcunningham@chromium.org Feb. 1, 2019, 1:18 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.

Also updates MOVStsc to use unsigned ints, per spec.
---
 libavformat/isom.h | 6 +++---
 libavformat/mov.c  | 4 ++--
 2 files changed, 5 insertions(+), 5 deletions(-)

Comments

chcunningham@chromium.org Feb. 1, 2019, 1:32 a.m.
Some extra context: this remedies some bad math that otherwise triggers an
abort near the bottom of mov_seek_stream().

My first thought on seeing this was we should probably be
returning AVERROR_INVALIDDATA somewhere. The stsc and stco boxes aren't
agreeing (stco says no chunks, stsc.first points to chunk 135). stsc and
stco aren't gauranteed to come in a particular order, but we could try to
validate that they agree whenever both arrive. One challenge is
that mov_read_stco is already pretty relaxed about setting sc->chunk_count
(it sets to some lower number if we hit EOF during parsing).

Feedback welcome.

On Thu, Jan 31, 2019 at 5:26 PM chcunningham <chcunningham@chromium.org>
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.
>
> Also updates MOVStsc to use unsigned ints, per spec.
> ---
>  libavformat/isom.h | 6 +++---
>  libavformat/mov.c  | 4 ++--
>  2 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/libavformat/isom.h b/libavformat/isom.h
> index e629663949..8e0d8355b3 100644
> --- a/libavformat/isom.h
> +++ b/libavformat/isom.h
> @@ -59,9 +59,9 @@ typedef struct MOVStts {
>  } MOVStts;
>
>  typedef struct MOVStsc {
> -    int first;
> -    int count;
> -    int id;
> +    unsigned int first;
> +    unsigned int count;
> +    unsigned int id;
>  } MOVStsc;
>
>  typedef struct MOVElst {
> 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;
> --
> 2.20.1.611.gfbb209baf1-goog
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
Carl Eugen Hoyos Feb. 1, 2019, 7:04 p.m.
2019-02-01 2:18 GMT+01:00, chcunningham <chcunningham@chromium.org>:
> 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.

> Also updates MOVStsc to use unsigned ints, per spec.

Could be split...

Carl Eugen

Patch hide | download patch | download mbox

diff --git a/libavformat/isom.h b/libavformat/isom.h
index e629663949..8e0d8355b3 100644
--- a/libavformat/isom.h
+++ b/libavformat/isom.h
@@ -59,9 +59,9 @@  typedef struct MOVStts {
 } MOVStts;
 
 typedef struct MOVStsc {
-    int first;
-    int count;
-    int id;
+    unsigned int first;
+    unsigned int count;
+    unsigned int id;
 } MOVStsc;
 
 typedef struct MOVElst {
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;