[FFmpeg-devel] [PATCH 1/2] avformat/mov: add validate_box_size

[FFmpeg-devel] [PATCH 1/2] avformat/mov: add validate_box_size

[Gyan Doshi]

Helper function to check if stored box size is correct and looks
to be fully available.
---
 libavformat/mov.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/libavformat/mov.c b/libavformat/mov.c
index 2aed6e80ef..2cc9e699de 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -80,6 +80,38 @@ static int mov_read_mfra(MOVContext *c, AVIOContext *f);
 static int64_t add_ctts_entry(MOVCtts** ctts_data, unsigned int* ctts_count, unsigned int* allocated_size,
                               int count, int duration);
 
+/**  Check if the box size meets the requirements passed in limit and constraint_type.
+ *   If input avio_size is valid, it checks if box size appears to be available.
+ *
+ *   constraint_type may be
+ *   0  if the box size has to be exactly equal to limit
+ *  -1  if the box size has to be at most limit
+ *   1  if the box size has to be at least limit
+ *
+ *   Returns 0 if size meets requirements.
+ */
+static int validate_box_size(MOVContext *c, MOVAtom atom, AVIOContext *pb,
+                             int64_t pos, int64_t limit, int constraint_type)
+{
+    int size_fit;
+    int64_t input_size = avio_size(pb);
+
+    if (input_size > 0 &&
+        input_size - pos < atom.size) {
+        av_log(c->fc, AV_LOG_ERROR, "Box %s is truncated\n", av_fourcc2str(atom.type));
+        return AVERROR_INVALIDDATA;
+    }
+
+    switch(constraint_type) {
+    case  0: size_fit = atom.size == limit; break;
+    case -1: size_fit = atom.size <= limit; break;
+    case  1: size_fit = atom.size >= limit; break;
+    default: return AVERROR_INVALIDDATA;
+    }
+
+   return !size_fit;
+}
+
 static int mov_metadata_track_or_disc_number(MOVContext *c, AVIOContext *pb,
                                              unsigned len, const char *key)
 {
-- 
2.33.0

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 2/2] avformat/mov: validate box size for stts

[Gyan Doshi]

---
 libavformat/mov.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/libavformat/mov.c b/libavformat/mov.c
index 2cc9e699de..59ee7ed2a9 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -2967,6 +2967,12 @@ static int mov_read_stts(MOVContext *c, AVIOContext *pb, MOVAtom atom)
     avio_rb24(pb); /* flags */
     entries = avio_rb32(pb);
 
+    if (validate_box_size(c, atom, pb, avio_tell(pb)-8, 8+(int64_t)entries*8, 0)) {
+        av_log(c->fc, AV_LOG_ERROR, "Invalid or incomplete %s box in stream %d\n",
+               av_fourcc2str(atom.type), c->fc->nb_streams-1);
+       return AVERROR_INVALIDDATA;
+    }
+
     av_log(c->fc, AV_LOG_TRACE, "track[%u].stts.entries = %u\n",
             c->fc->nb_streams-1, entries);
 
-- 
2.33.0

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/2] avformat/mov: add validate_box_size

["zhilizhao(赵志立)"]

> On Dec 22, 2021, at 7:30 PM, Gyan Doshi <ffmpeg@gyani.pro> wrote:
> 
> Helper function to check if stored box size is correct and looks
> to be fully available.
> ---
> libavformat/mov.c | 32 ++++++++++++++++++++++++++++++++
> 1 file changed, 32 insertions(+)
> 
> diff --git a/libavformat/mov.c b/libavformat/mov.c
> index 2aed6e80ef..2cc9e699de 100644
> --- a/libavformat/mov.c
> +++ b/libavformat/mov.c
> @@ -80,6 +80,38 @@ static int mov_read_mfra(MOVContext *c, AVIOContext *f);
> static int64_t add_ctts_entry(MOVCtts** ctts_data, unsigned int* ctts_count, unsigned int* allocated_size,
>                               int count, int duration);
> 
> +/**  Check if the box size meets the requirements passed in limit and constraint_type.
> + *   If input avio_size is valid, it checks if box size appears to be available.
> + *
> + *   constraint_type may be
> + *   0  if the box size has to be exactly equal to limit
> + *  -1  if the box size has to be at most limit
> + *   1  if the box size has to be at least limit
> + *
> + *   Returns 0 if size meets requirements.
> + */
> +static int validate_box_size(MOVContext *c, MOVAtom atom, AVIOContext *pb,
> +                             int64_t pos, int64_t limit, int constraint_type)
> +{
> +    int size_fit;
> +    int64_t input_size = avio_size(pb);
> +
> +    if (input_size > 0 &&
> +        input_size - pos < atom.size) {
> +        av_log(c->fc, AV_LOG_ERROR, "Box %s is truncated\n", av_fourcc2str(atom.type));
> +        return AVERROR_INVALIDDATA;
> +    }
> +
> +    switch(constraint_type) {
> +    case  0: size_fit = atom.size == limit; break;
> +    case -1: size_fit = atom.size <= limit; break;
> +    case  1: size_fit = atom.size >= limit; break;
> +    default: return AVERROR_INVALIDDATA;

The default case doesn’t come from invalid data, I prefer assert and
return AVERROR(EINVAL) or AVERROR_BUG.

> +    }
> +
> +   return !size_fit;
> +}
> +
> static int mov_metadata_track_or_disc_number(MOVContext *c, AVIOContext *pb,
>                                              unsigned len, const char *key)
> {
> -- 
> 2.33.0
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH v2 1/2] avformat/mov: add validate_box_size

[Gyan Doshi]

Helper function to check if stored box size is correct and looks
to be fully available.
---
 libavformat/mov.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/libavformat/mov.c b/libavformat/mov.c
index 2aed6e80ef..7de95b7ab0 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -80,6 +80,40 @@ static int mov_read_mfra(MOVContext *c, AVIOContext *f);
 static int64_t add_ctts_entry(MOVCtts** ctts_data, unsigned int* ctts_count, unsigned int* allocated_size,
                               int count, int duration);
 
+/**  Check if the box size meets the requirements passed in limit and constraint_type.
+ *   If input avio_size is valid, it checks if box size appears to be available.
+ *
+ *   constraint_type may be
+ *   0  if the box size has to be exactly equal to limit
+ *  -1  if the box size has to be at most limit
+ *   1  if the box size has to be at least limit
+ *
+ *   Returns 0 if size meets requirements.
+ */
+static int validate_box_size(MOVContext *c, MOVAtom atom, AVIOContext *pb,
+                             int64_t pos, int64_t limit, int constraint_type)
+{
+    int size_fit;
+    int64_t input_size = avio_size(pb);
+
+    if (input_size > 0 &&
+        input_size - pos < atom.size) {
+        av_log(c->fc, AV_LOG_ERROR, "Box %s is truncated\n", av_fourcc2str(atom.type));
+        return AVERROR_INVALIDDATA;
+    }
+
+    if (FFABS(constraint_type) > 1)
+        return AVERROR_BUG;
+
+    switch(constraint_type) {
+    case  0: size_fit = atom.size == limit; break;
+    case -1: size_fit = atom.size <= limit; break;
+    case  1: size_fit = atom.size >= limit; break;
+    }
+
+   return !size_fit;
+}
+
 static int mov_metadata_track_or_disc_number(MOVContext *c, AVIOContext *pb,
                                              unsigned len, const char *key)
 {
-- 
2.33.0

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH v2 2/2] avformat/mov: validate box size for stts

[Gyan Doshi]

---
 libavformat/mov.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/libavformat/mov.c b/libavformat/mov.c
index 7de95b7ab0..1e44c74944 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -2969,6 +2969,12 @@ static int mov_read_stts(MOVContext *c, AVIOContext *pb, MOVAtom atom)
     avio_rb24(pb); /* flags */
     entries = avio_rb32(pb);
 
+    if (validate_box_size(c, atom, pb, avio_tell(pb)-8, 8+(int64_t)entries*8, 0)) {
+        av_log(c->fc, AV_LOG_ERROR, "Invalid or incomplete %s box in stream %d\n",
+               av_fourcc2str(atom.type), c->fc->nb_streams-1);
+       return AVERROR_INVALIDDATA;
+    }
+
     av_log(c->fc, AV_LOG_TRACE, "track[%u].stts.entries = %u\n",
             c->fc->nb_streams-1, entries);
 
-- 
2.33.0

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH v2 1/2] avformat/mov: add validate_box_size

[Nicolas George]

[-- Attachment #1.1: Type: text/plain, Size: 2227 bytes --]

Gyan Doshi (12021-12-22):
> Helper function to check if stored box size is correct and looks
> to be fully available.
> ---
>  libavformat/mov.c | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/libavformat/mov.c b/libavformat/mov.c
> index 2aed6e80ef..7de95b7ab0 100644
> --- a/libavformat/mov.c
> +++ b/libavformat/mov.c
> @@ -80,6 +80,40 @@ static int mov_read_mfra(MOVContext *c, AVIOContext *f);
>  static int64_t add_ctts_entry(MOVCtts** ctts_data, unsigned int* ctts_count, unsigned int* allocated_size,
>                                int count, int duration);
>  
> +/**  Check if the box size meets the requirements passed in limit and constraint_type.
> + *   If input avio_size is valid, it checks if box size appears to be available.
> + *
> + *   constraint_type may be
> + *   0  if the box size has to be exactly equal to limit
> + *  -1  if the box size has to be at most limit
> + *   1  if the box size has to be at least limit
> + *
> + *   Returns 0 if size meets requirements.
> + */
> +static int validate_box_size(MOVContext *c, MOVAtom atom, AVIOContext *pb,
> +                             int64_t pos, int64_t limit, int constraint_type)
> +{
> +    int size_fit;
> +    int64_t input_size = avio_size(pb);
> +
> +    if (input_size > 0 &&
> +        input_size - pos < atom.size) {
> +        av_log(c->fc, AV_LOG_ERROR, "Box %s is truncated\n", av_fourcc2str(atom.type));
> +        return AVERROR_INVALIDDATA;
> +    }
> +

> +    if (FFABS(constraint_type) > 1)
> +        return AVERROR_BUG;

av_assert() whould have been the right choice here.

> +
> +    switch(constraint_type) {
> +    case  0: size_fit = atom.size == limit; break;

> +    case -1: size_fit = atom.size <= limit; break;
> +    case  1: size_fit = atom.size >= limit; break;

This code is unused, AFAICS. Not a good idea.

> +    }
> +
> +   return !size_fit;
> +}
> +
>  static int mov_metadata_track_or_disc_number(MOVContext *c, AVIOContext *pb,
>                                               unsigned len, const char *key)
>  {

I think the changes belong in a single patch.

Regards,

-- 
  Nicolas George

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH v2 1/2] avformat/mov: add validate_box_size

[Gyan Doshi]

On 2021-12-22 06:43 pm, Nicolas George wrote:
> Gyan Doshi (12021-12-22):
>> Helper function to check if stored box size is correct and looks
>> to be fully available.
>> ---
>>   libavformat/mov.c | 34 ++++++++++++++++++++++++++++++++++
>>   1 file changed, 34 insertions(+)
>>
>> diff --git a/libavformat/mov.c b/libavformat/mov.c
>> index 2aed6e80ef..7de95b7ab0 100644
>> --- a/libavformat/mov.c
>> +++ b/libavformat/mov.c
>> @@ -80,6 +80,40 @@ static int mov_read_mfra(MOVContext *c, AVIOContext *f);
>>   static int64_t add_ctts_entry(MOVCtts** ctts_data, unsigned int* ctts_count, unsigned int* allocated_size,
>>                                 int count, int duration);
>>   
>> +/**  Check if the box size meets the requirements passed in limit and constraint_type.
>> + *   If input avio_size is valid, it checks if box size appears to be available.
>> + *
>> + *   constraint_type may be
>> + *   0  if the box size has to be exactly equal to limit
>> + *  -1  if the box size has to be at most limit
>> + *   1  if the box size has to be at least limit
>> + *
>> + *   Returns 0 if size meets requirements.
>> + */
>> +static int validate_box_size(MOVContext *c, MOVAtom atom, AVIOContext *pb,
>> +                             int64_t pos, int64_t limit, int constraint_type)
>> +{
>> +    int size_fit;
>> +    int64_t input_size = avio_size(pb);
>> +
>> +    if (input_size > 0 &&
>> +        input_size - pos < atom.size) {
>> +        av_log(c->fc, AV_LOG_ERROR, "Box %s is truncated\n", av_fourcc2str(atom.type));
>> +        return AVERROR_INVALIDDATA;
>> +    }
>> +
>> +    if (FFABS(constraint_type) > 1)
>> +        return AVERROR_BUG;
> av_assert() whould have been the right choice here.

Will change.

>
>> +
>> +    switch(constraint_type) {
>> +    case  0: size_fit = atom.size == limit; break;
>> +    case -1: size_fit = atom.size <= limit; break;
>> +    case  1: size_fit = atom.size >= limit; break;
> This code is unused, AFAICS. Not a good idea.

I'll call this function in other box readers. Their requirements are 
different.
That's why I made the check a separate function.

Regards,
Gyan
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH v2 2/2] avformat/mov: validate box size for stts

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 1195 bytes --]

On Wed, Dec 22, 2021 at 06:17:28PM +0530, Gyan Doshi wrote:
> ---
>  libavformat/mov.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/libavformat/mov.c b/libavformat/mov.c
> index 7de95b7ab0..1e44c74944 100644
> --- a/libavformat/mov.c
> +++ b/libavformat/mov.c
> @@ -2969,6 +2969,12 @@ static int mov_read_stts(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>      avio_rb24(pb); /* flags */
>      entries = avio_rb32(pb);
>  
> +    if (validate_box_size(c, atom, pb, avio_tell(pb)-8, 8+(int64_t)entries*8, 0)) {
> +        av_log(c->fc, AV_LOG_ERROR, "Invalid or incomplete %s box in stream %d\n",
> +               av_fourcc2str(atom.type), c->fc->nb_streams-1);
> +       return AVERROR_INVALIDDATA;
> +    }
> +
>      av_log(c->fc, AV_LOG_TRACE, "track[%u].stts.entries = %u\n",
>              c->fc->nb_streams-1, entries);
>  

this breaks playback of

./ffplay H263_NM_f.mp4

i presume its this file here:
https://samples.ffmpeg.org/F4V/H263_NM_f.mp4

thx

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Nations do behave wisely once they have exhausted all other alternatives. 
-- Abba Eban

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH v2 2/2] avformat/mov: validate box size for stts

[Gyan Doshi]

On 2021-12-23 03:04 am, Michael Niedermayer wrote:
> On Wed, Dec 22, 2021 at 06:17:28PM +0530, Gyan Doshi wrote:
>> ---
>>   libavformat/mov.c | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/libavformat/mov.c b/libavformat/mov.c
>> index 7de95b7ab0..1e44c74944 100644
>> --- a/libavformat/mov.c
>> +++ b/libavformat/mov.c
>> @@ -2969,6 +2969,12 @@ static int mov_read_stts(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>>       avio_rb24(pb); /* flags */
>>       entries = avio_rb32(pb);
>>   
>> +    if (validate_box_size(c, atom, pb, avio_tell(pb)-8, 8+(int64_t)entries*8, 0)) {
>> +        av_log(c->fc, AV_LOG_ERROR, "Invalid or incomplete %s box in stream %d\n",
>> +               av_fourcc2str(atom.type), c->fc->nb_streams-1);
>> +       return AVERROR_INVALIDDATA;
>> +    }
>> +
>>       av_log(c->fc, AV_LOG_TRACE, "track[%u].stts.entries = %u\n",
>>               c->fc->nb_streams-1, entries);
>>   
> this breaks playback of
>
> ./ffplay H263_NM_f.mp4

Sent revised set.

However, do we need to allow this?

The file has multiple invalid boxes. gpac refuses to import it. vlc 
using its default mp4 demuxer does not open it, neither does wmp or firefox.
Seems only avformat users can open the file.

Regards,
Gyan
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH v2 2/2] avformat/mov: validate box size for stts

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 1434 bytes --]

On Thu, Dec 23, 2021 at 12:54:58PM +0530, Gyan Doshi wrote:
> 
> 
> On 2021-12-23 03:04 am, Michael Niedermayer wrote:
> > On Wed, Dec 22, 2021 at 06:17:28PM +0530, Gyan Doshi wrote:
> > > ---
> > >   libavformat/mov.c | 6 ++++++
> > >   1 file changed, 6 insertions(+)
> > > 
> > > diff --git a/libavformat/mov.c b/libavformat/mov.c
> > > index 7de95b7ab0..1e44c74944 100644
> > > --- a/libavformat/mov.c
> > > +++ b/libavformat/mov.c
> > > @@ -2969,6 +2969,12 @@ static int mov_read_stts(MOVContext *c, AVIOContext *pb, MOVAtom atom)
> > >       avio_rb24(pb); /* flags */
> > >       entries = avio_rb32(pb);
> > > +    if (validate_box_size(c, atom, pb, avio_tell(pb)-8, 8+(int64_t)entries*8, 0)) {
> > > +        av_log(c->fc, AV_LOG_ERROR, "Invalid or incomplete %s box in stream %d\n",
> > > +               av_fourcc2str(atom.type), c->fc->nb_streams-1);
> > > +       return AVERROR_INVALIDDATA;
> > > +    }
> > > +
> > >       av_log(c->fc, AV_LOG_TRACE, "track[%u].stts.entries = %u\n",
> > >               c->fc->nb_streams-1, entries);
> > this breaks playback of
> > 
> > ./ffplay H263_NM_f.mp4
> 
> Sent revised set.
> 
> However, do we need to allow this?

yes
its a real world file its not crafted to be funky 

thx

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

Observe your enemies, for they first find out your faults. -- Antisthenes

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".