diff mbox

[FFmpeg-devel] avformat/mov: Fix reading saio/saiz for clear content.

Message ID CAO7y9i8U-V2d0k1ALvgXNEmBFcBsKi_cWq6Se9Knq5OwS+StbA@mail.gmail.com
State New
Headers show

Commit Message

Jacob Trimble June 7, 2018, 8:42 p.m. UTC 
On Thu, Jun 7, 2018 at 10:38 AM Jacob Trimble <modmaker@google.com> wrote:
>
> Found by Chrome's ClusterFuzz: http://crbug.com/850389
>
> Signed-off-by: Jacob Trimble <modmaker@google.com>
> ---
>  libavformat/mov.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/libavformat/mov.c b/libavformat/mov.c
> index 4ad19122b3..d07171b3f4 100644
> --- a/libavformat/mov.c
> +++ b/libavformat/mov.c
> @@ -6041,6 +6041,11 @@ static int mov_read_saiz(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>      if (ret != 1)
>          return ret;
>
> +    if (!sc->cenc.default_encrypted_sample) {
> +        // Didn't see a 'schm' or 'tenc' atom, so it isn't encrypted.
> +        return 0;
> +    }
> +
>      if (encryption_index->nb_encrypted_samples) {
>          // This can happen if we have both saio/saiz and senc atoms.
>          av_log(c->fc, AV_LOG_DEBUG, "Ignoring duplicate encryption info in saiz\n");
> @@ -6095,6 +6100,11 @@ static int mov_read_saio(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>      if (ret != 1)
>          return ret;
>
> +    if (!sc->cenc.default_encrypted_sample) {
> +        // Didn't see a 'schm' or 'tenc' atom, so it isn't encrypted.
> +        return 0;
> +    }
> +
>      if (encryption_index->nb_encrypted_samples) {
>          // This can happen if we have both saio/saiz and senc atoms.
>          av_log(c->fc, AV_LOG_DEBUG, "Ignoring duplicate encryption info in saio\n");
> --
> 2.17.1.1185.g55be947832-goog
>

Based on comments downstream, I've added error checks for the
encrypted type of saio/saiz atoms.

Comments

Michael Niedermayer June 9, 2018, 6:23 p.m. UTC | #1 
On Thu, Jun 07, 2018 at 01:42:51PM -0700, Jacob Trimble wrote:
> On Thu, Jun 7, 2018 at 10:38 AM Jacob Trimble <modmaker@google.com> wrote:
> >
> > Found by Chrome's ClusterFuzz: http://crbug.com/850389
> >
> > Signed-off-by: Jacob Trimble <modmaker@google.com>
> > ---
> >  libavformat/mov.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> >
> > diff --git a/libavformat/mov.c b/libavformat/mov.c
> > index 4ad19122b3..d07171b3f4 100644
> > --- a/libavformat/mov.c
> > +++ b/libavformat/mov.c
> > @@ -6041,6 +6041,11 @@ static int mov_read_saiz(MOVContext *c, AVIOContext *pb, MOVAtom atom)
> >      if (ret != 1)
> >          return ret;
> >
> > +    if (!sc->cenc.default_encrypted_sample) {
> > +        // Didn't see a 'schm' or 'tenc' atom, so it isn't encrypted.
> > +        return 0;
> > +    }
> > +
> >      if (encryption_index->nb_encrypted_samples) {
> >          // This can happen if we have both saio/saiz and senc atoms.
> >          av_log(c->fc, AV_LOG_DEBUG, "Ignoring duplicate encryption info in saiz\n");
> > @@ -6095,6 +6100,11 @@ static int mov_read_saio(MOVContext *c, AVIOContext *pb, MOVAtom atom)
> >      if (ret != 1)
> >          return ret;
> >
> > +    if (!sc->cenc.default_encrypted_sample) {
> > +        // Didn't see a 'schm' or 'tenc' atom, so it isn't encrypted.
> > +        return 0;
> > +    }
> > +
> >      if (encryption_index->nb_encrypted_samples) {
> >          // This can happen if we have both saio/saiz and senc atoms.
> >          av_log(c->fc, AV_LOG_DEBUG, "Ignoring duplicate encryption info in saio\n");
> > --
> > 2.17.1.1185.g55be947832-goog
> >
> 
> Based on comments downstream, I've added error checks for the
> encrypted type of saio/saiz atoms.

>  mov.c |   71 +++++++++++++++++++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 55 insertions(+), 16 deletions(-)
> f0cb531170be23bb7f754c99edab172f00e79d6e  0001-avformat-mov-Fix-reading-saio-saiz-for-clear-content-v2.patch
> From e4185c0fd08a1baedcf81935ff0f5ac9a97eba4e Mon Sep 17 00:00:00 2001
> From: Jacob Trimble <modmaker@google.com>
> Date: Thu, 7 Jun 2018 10:29:33 -0700
> Subject: [PATCH] avformat/mov: Fix reading saio/saiz for clear content.
> 
> This validates that the common encryption saio/saiz atoms only appear
> when the data is actually encrypted.  This also ignores those atoms
> in clear content.
> 
> Found by Chrome's ClusterFuzz: http://crbug.com/850389
> 
> Signed-off-by: Jacob Trimble <modmaker@google.com>

will apply

btw, the 2 changed code pieces look a bit as if they could be factored into a
single function

thx

[...]
diff mbox

Patch

From e4185c0fd08a1baedcf81935ff0f5ac9a97eba4e Mon Sep 17 00:00:00 2001
From: Jacob Trimble <modmaker@google.com>
Date: Thu, 7 Jun 2018 10:29:33 -0700
Subject: [PATCH] avformat/mov: Fix reading saio/saiz for clear content.

This validates that the common encryption saio/saiz atoms only appear
when the data is actually encrypted.  This also ignores those atoms
in clear content.

Found by Chrome's ClusterFuzz: http://crbug.com/850389

Signed-off-by: Jacob Trimble <modmaker@google.com>
---
 libavformat/mov.c | 71 ++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 55 insertions(+), 16 deletions(-)

diff --git a/libavformat/mov.c b/libavformat/mov.c
index 4ad19122b3..2fca025889 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -6035,7 +6035,7 @@  static int mov_read_saiz(MOVContext *c, AVIOContext *pb, MOVAtom atom)
     MOVEncryptionIndex *encryption_index;
     MOVStreamContext *sc;
     int ret;
-    unsigned int sample_count;
+    unsigned int sample_count, aux_info_type, aux_info_param;
 
     ret = get_current_encryption_info(c, &encryption_index, &sc);
     if (ret != 1)
@@ -6054,14 +6054,33 @@  static int mov_read_saiz(MOVContext *c, AVIOContext *pb, MOVAtom atom)
 
     avio_r8(pb); /* version */
     if (avio_rb24(pb) & 0x01) {  /* flags */
-        if (avio_rb32(pb) != sc->cenc.default_encrypted_sample->scheme) {
-            av_log(c->fc, AV_LOG_DEBUG, "Ignoring saiz box with non-zero aux_info_type\n");
-            return 0;
-        }
-        if (avio_rb32(pb) != 0) {
-            av_log(c->fc, AV_LOG_DEBUG, "Ignoring saiz box with non-zero aux_info_type_parameter\n");
-            return 0;
+        aux_info_type = avio_rb32(pb);
+        aux_info_param = avio_rb32(pb);
+        if (sc->cenc.default_encrypted_sample) {
+            if (aux_info_type != sc->cenc.default_encrypted_sample->scheme) {
+                av_log(c->fc, AV_LOG_DEBUG, "Ignoring saiz box with non-zero aux_info_type\n");
+                return 0;
+            }
+            if (aux_info_param != 0) {
+                av_log(c->fc, AV_LOG_DEBUG, "Ignoring saiz box with non-zero aux_info_type_parameter\n");
+                return 0;
+            }
+        } else {
+            // Didn't see 'schm' or 'tenc', so this isn't encrypted.
+            if ((aux_info_type == MKBETAG('c','e','n','c') ||
+                 aux_info_type == MKBETAG('c','e','n','s') ||
+                 aux_info_type == MKBETAG('c','b','c','1') ||
+                 aux_info_type == MKBETAG('c','b','c','s')) &&
+                aux_info_param == 0) {
+                av_log(c->fc, AV_LOG_ERROR, "Saw encrypted saiz without schm/tenc\n");
+                return AVERROR_INVALIDDATA;
+            } else {
+                return 0;
+            }
         }
+    } else if (!sc->cenc.default_encrypted_sample) {
+        // Didn't see 'schm' or 'tenc', so this isn't encrypted.
+        return 0;
     }
 
     encryption_index->auxiliary_info_default_size = avio_r8(pb);
@@ -6089,7 +6108,8 @@  static int mov_read_saio(MOVContext *c, AVIOContext *pb, MOVAtom atom)
     MOVEncryptionIndex *encryption_index;
     MOVStreamContext *sc;
     int i, ret;
-    unsigned int version, entry_count, alloc_size = 0;
+    unsigned int version, entry_count, aux_info_type, aux_info_param;
+    unsigned int alloc_size = 0;
 
     ret = get_current_encryption_info(c, &encryption_index, &sc);
     if (ret != 1)
@@ -6108,14 +6128,33 @@  static int mov_read_saio(MOVContext *c, AVIOContext *pb, MOVAtom atom)
 
     version = avio_r8(pb); /* version */
     if (avio_rb24(pb) & 0x01) {  /* flags */
-        if (avio_rb32(pb) != sc->cenc.default_encrypted_sample->scheme) {
-            av_log(c->fc, AV_LOG_DEBUG, "Ignoring saio box with non-zero aux_info_type\n");
-            return 0;
-        }
-        if (avio_rb32(pb) != 0) {
-            av_log(c->fc, AV_LOG_DEBUG, "Ignoring saio box with non-zero aux_info_type_parameter\n");
-            return 0;
+        aux_info_type = avio_rb32(pb);
+        aux_info_param = avio_rb32(pb);
+        if (sc->cenc.default_encrypted_sample) {
+            if (aux_info_type != sc->cenc.default_encrypted_sample->scheme) {
+                av_log(c->fc, AV_LOG_DEBUG, "Ignoring saio box with non-zero aux_info_type\n");
+                return 0;
+            }
+            if (aux_info_param != 0) {
+                av_log(c->fc, AV_LOG_DEBUG, "Ignoring saio box with non-zero aux_info_type_parameter\n");
+                return 0;
+            }
+        } else {
+            // Didn't see 'schm' or 'tenc', so this isn't encrypted.
+            if ((aux_info_type == MKBETAG('c','e','n','c') ||
+                 aux_info_type == MKBETAG('c','e','n','s') ||
+                 aux_info_type == MKBETAG('c','b','c','1') ||
+                 aux_info_type == MKBETAG('c','b','c','s')) &&
+                aux_info_param == 0) {
+                av_log(c->fc, AV_LOG_ERROR, "Saw encrypted saio without schm/tenc\n");
+                return AVERROR_INVALIDDATA;
+            } else {
+                return 0;
+            }
         }
+    } else if (!sc->cenc.default_encrypted_sample) {
+        // Didn't see 'schm' or 'tenc', so this isn't encrypted.
+        return 0;
     }
 
     entry_count = avio_rb32(pb);
-- 
2.17.1.1185.g55be947832-goog