diff mbox series

[FFmpeg-devel,1/3] avformat/matroskadec: Sanitize SeekHead entries

Message ID 20200501005522.16402-1-andreas.rheinhardt@gmail.com
State Accepted
Headers show
Series [FFmpeg-devel,1/3] avformat/matroskadec: Sanitize SeekHead entries
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Andreas Rheinhardt May 1, 2020, 12:55 a.m. UTC
A Seek element in a Matroska SeekHead should contain a SeekID and a
SeekPosition element and upon reading, they should be sanitized:

Given that IDs are restricted to 32 bit, longer SeekIDs should be treated
as invalid. Instead currently the lower 32 bits have been used.

For SeekPosition, no checks were performed for the element to be
present and if present, whether it was excessively large (i.e. the
absolute file position described by it exceeding INT64_MAX). The
SeekPosition element had a default value of -1 which means that a check
seems to have been intended; but it was not implemented. This commit adds
a check for overflow to the calculation of the absolute file position of
the referenced level 1 elements.
Using -1 (i.e. UINT64_MAX) as default value for SeekPosition implies that
a Seek element without SeekPosition will run afoul of this check.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
---
 libavformat/matroskadec.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Comments

Andreas Rheinhardt May 7, 2020, 4:58 a.m. UTC | #1
Andreas Rheinhardt:
> A Seek element in a Matroska SeekHead should contain a SeekID and a
> SeekPosition element and upon reading, they should be sanitized:
> 
> Given that IDs are restricted to 32 bit, longer SeekIDs should be treated
> as invalid. Instead currently the lower 32 bits have been used.
> 
> For SeekPosition, no checks were performed for the element to be
> present and if present, whether it was excessively large (i.e. the
> absolute file position described by it exceeding INT64_MAX). The
> SeekPosition element had a default value of -1 which means that a check
> seems to have been intended; but it was not implemented. This commit adds
> a check for overflow to the calculation of the absolute file position of
> the referenced level 1 elements.
> Using -1 (i.e. UINT64_MAX) as default value for SeekPosition implies that
> a Seek element without SeekPosition will run afoul of this check.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> ---
>  libavformat/matroskadec.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
> index 8e1326abf6..dea8f14f9e 100644
> --- a/libavformat/matroskadec.c
> +++ b/libavformat/matroskadec.c
> @@ -1865,8 +1865,12 @@ static void matroska_execute_seekhead(MatroskaDemuxContext *matroska)
>          MatroskaSeekhead *seekheads = seekhead_list->elem;
>          uint32_t id = seekheads[i].id;
>          int64_t pos = seekheads[i].pos + matroska->segment_start;
> +        MatroskaLevel1Element *elem;
>  
> -        MatroskaLevel1Element *elem = matroska_find_level1_elem(matroska, id);
> +        if (id != seekheads[i].id || pos < matroska->segment_start)
> +            continue;
> +
> +        elem = matroska_find_level1_elem(matroska, id);
>          if (!elem || elem->parsed)
>              continue;
>  
Will apply this patchset tomorrow if there are no objections.

- Andreas
Andreas Rheinhardt May 8, 2020, 12:05 p.m. UTC | #2
Andreas Rheinhardt:
> Andreas Rheinhardt:
>> A Seek element in a Matroska SeekHead should contain a SeekID and a
>> SeekPosition element and upon reading, they should be sanitized:
>>
>> Given that IDs are restricted to 32 bit, longer SeekIDs should be treated
>> as invalid. Instead currently the lower 32 bits have been used.
>>
>> For SeekPosition, no checks were performed for the element to be
>> present and if present, whether it was excessively large (i.e. the
>> absolute file position described by it exceeding INT64_MAX). The
>> SeekPosition element had a default value of -1 which means that a check
>> seems to have been intended; but it was not implemented. This commit adds
>> a check for overflow to the calculation of the absolute file position of
>> the referenced level 1 elements.
>> Using -1 (i.e. UINT64_MAX) as default value for SeekPosition implies that
>> a Seek element without SeekPosition will run afoul of this check.
>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
>> ---
>>  libavformat/matroskadec.c | 6 +++++-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
>> index 8e1326abf6..dea8f14f9e 100644
>> --- a/libavformat/matroskadec.c
>> +++ b/libavformat/matroskadec.c
>> @@ -1865,8 +1865,12 @@ static void matroska_execute_seekhead(MatroskaDemuxContext *matroska)
>>          MatroskaSeekhead *seekheads = seekhead_list->elem;
>>          uint32_t id = seekheads[i].id;
>>          int64_t pos = seekheads[i].pos + matroska->segment_start;
>> +        MatroskaLevel1Element *elem;
>>  
>> -        MatroskaLevel1Element *elem = matroska_find_level1_elem(matroska, id);
>> +        if (id != seekheads[i].id || pos < matroska->segment_start)
>> +            continue;
>> +
>> +        elem = matroska_find_level1_elem(matroska, id);
>>          if (!elem || elem->parsed)
>>              continue;
>>  
> Will apply this patchset tomorrow if there are no objections.
> 
> - Andreas
> 
Applied.

- Andreas
diff mbox series

Patch

diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
index 8e1326abf6..dea8f14f9e 100644
--- a/libavformat/matroskadec.c
+++ b/libavformat/matroskadec.c
@@ -1865,8 +1865,12 @@  static void matroska_execute_seekhead(MatroskaDemuxContext *matroska)
         MatroskaSeekhead *seekheads = seekhead_list->elem;
         uint32_t id = seekheads[i].id;
         int64_t pos = seekheads[i].pos + matroska->segment_start;
+        MatroskaLevel1Element *elem;
 
-        MatroskaLevel1Element *elem = matroska_find_level1_elem(matroska, id);
+        if (id != seekheads[i].id || pos < matroska->segment_start)
+            continue;
+
+        elem = matroska_find_level1_elem(matroska, id);
         if (!elem || elem->parsed)
             continue;