[09/11] wtv: Add more sanity checks for a length read from the file

Message ID 1379599756-27062-9-git-send-email-martin@martin.st
State Committed
Headers show

Commit Message

Martin Storsjö Sept. 19, 2013, 2:09 p.m.
Also make sure the existing length check can't overflow.

Reported-by: Mateusz "j00ru" Jurczyk and Gynvael Coldwind
CC: libav-stable@libav.org
---
 libavformat/wtv.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Comments

Kostya Shishkov Sept. 19, 2013, 2:24 p.m. | #1
On Thu, Sep 19, 2013 at 05:09:14PM +0300, Martin Storsjö wrote:
> Also make sure the existing length check can't overflow.
> 
> Reported-by: Mateusz "j00ru" Jurczyk and Gynvael Coldwind
> CC: libav-stable@libav.org
> ---
>  libavformat/wtv.c |    7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/libavformat/wtv.c b/libavformat/wtv.c
> index b003905..093f795 100644
> --- a/libavformat/wtv.c
> +++ b/libavformat/wtv.c
> @@ -269,10 +269,15 @@ static AVIOContext * wtvfile_open2(AVFormatContext *s, const uint8_t *buf, int b
>          dir_length  = AV_RL16(buf + 16);
>          file_length = AV_RL64(buf + 24);
>          name_size   = 2 * AV_RL32(buf + 32);
> -        if (buf + 48 + name_size > buf_end) {
> +        if (48 + name_size > buf_end - buf) {
>              av_log(s, AV_LOG_ERROR, "filename exceeds buffer size; remaining directory entries ignored\n");
>              break;
>          }
> +        if (name_size < 0) {
> +            av_log(s, AV_LOG_ERROR,
> +                   "bad filename length, remaining directory entries ignored\n");
> +            break;
> +        }
>          first_sector = AV_RL32(buf + 40 + name_size);
>          depth        = AV_RL32(buf + 44 + name_size);
>  
> -- 

It makes sense to reorder the checks IMO
but LGTM
Luca Barbato Sept. 19, 2013, 2:24 p.m. | #2
On 19/09/13 16:09, Martin Storsjö wrote:
> Also make sure the existing length check can't overflow.
> 

Ok.
Martin Storsjö Sept. 19, 2013, 3:39 p.m. | #3
On Thu, 19 Sep 2013, Kostya Shishkov wrote:

> On Thu, Sep 19, 2013 at 05:09:14PM +0300, Martin Storsjö wrote:
>> Also make sure the existing length check can't overflow.
>>
>> Reported-by: Mateusz "j00ru" Jurczyk and Gynvael Coldwind
>> CC: libav-stable@libav.org
>> ---
>>  libavformat/wtv.c |    7 ++++++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/libavformat/wtv.c b/libavformat/wtv.c
>> index b003905..093f795 100644
>> --- a/libavformat/wtv.c
>> +++ b/libavformat/wtv.c
>> @@ -269,10 +269,15 @@ static AVIOContext * wtvfile_open2(AVFormatContext *s, const uint8_t *buf, int b
>>          dir_length  = AV_RL16(buf + 16);
>>          file_length = AV_RL64(buf + 24);
>>          name_size   = 2 * AV_RL32(buf + 32);
>> -        if (buf + 48 + name_size > buf_end) {
>> +        if (48 + name_size > buf_end - buf) {
>>              av_log(s, AV_LOG_ERROR, "filename exceeds buffer size; remaining directory entries ignored\n");
>>              break;
>>          }
>> +        if (name_size < 0) {
>> +            av_log(s, AV_LOG_ERROR,
>> +                   "bad filename length, remaining directory entries ignored\n");
>> +            break;
>> +        }
>>          first_sector = AV_RL32(buf + 40 + name_size);
>>          depth        = AV_RL32(buf + 44 + name_size);
>>
>> --
>
> It makes sense to reorder the checks IMO

That is, having the < 0 check first?

// Martin
Kostya Shishkov Sept. 19, 2013, 5:34 p.m. | #4
On Thu, Sep 19, 2013 at 06:39:08PM +0300, Martin Storsjö wrote:
> On Thu, 19 Sep 2013, Kostya Shishkov wrote:
> 
> >On Thu, Sep 19, 2013 at 05:09:14PM +0300, Martin Storsjö wrote:
> >>Also make sure the existing length check can't overflow.
> >>
> >>Reported-by: Mateusz "j00ru" Jurczyk and Gynvael Coldwind
> >>CC: libav-stable@libav.org
> >>---
> >> libavformat/wtv.c |    7 ++++++-
> >> 1 file changed, 6 insertions(+), 1 deletion(-)
> >>
> >>diff --git a/libavformat/wtv.c b/libavformat/wtv.c
> >>index b003905..093f795 100644
> >>--- a/libavformat/wtv.c
> >>+++ b/libavformat/wtv.c
> >>@@ -269,10 +269,15 @@ static AVIOContext * wtvfile_open2(AVFormatContext *s, const uint8_t *buf, int b
> >>         dir_length  = AV_RL16(buf + 16);
> >>         file_length = AV_RL64(buf + 24);
> >>         name_size   = 2 * AV_RL32(buf + 32);
> >>-        if (buf + 48 + name_size > buf_end) {
> >>+        if (48 + name_size > buf_end - buf) {
> >>             av_log(s, AV_LOG_ERROR, "filename exceeds buffer size; remaining directory entries ignored\n");
> >>             break;
> >>         }
> >>+        if (name_size < 0) {
> >>+            av_log(s, AV_LOG_ERROR,
> >>+                   "bad filename length, remaining directory entries ignored\n");
> >>+            break;
> >>+        }
> >>         first_sector = AV_RL32(buf + 40 + name_size);
> >>         depth        = AV_RL32(buf + 44 + name_size);
> >>
> >>--
> >
> >It makes sense to reorder the checks IMO
> 
> That is, having the < 0 check first?

Yes - it just feels safer than falsely passing current first check when
having negative name_size.

Patch

diff --git a/libavformat/wtv.c b/libavformat/wtv.c
index b003905..093f795 100644
--- a/libavformat/wtv.c
+++ b/libavformat/wtv.c
@@ -269,10 +269,15 @@  static AVIOContext * wtvfile_open2(AVFormatContext *s, const uint8_t *buf, int b
         dir_length  = AV_RL16(buf + 16);
         file_length = AV_RL64(buf + 24);
         name_size   = 2 * AV_RL32(buf + 32);
-        if (buf + 48 + name_size > buf_end) {
+        if (48 + name_size > buf_end - buf) {
             av_log(s, AV_LOG_ERROR, "filename exceeds buffer size; remaining directory entries ignored\n");
             break;
         }
+        if (name_size < 0) {
+            av_log(s, AV_LOG_ERROR,
+                   "bad filename length, remaining directory entries ignored\n");
+            break;
+        }
         first_sector = AV_RL32(buf + 40 + name_size);
         depth        = AV_RL32(buf + 44 + name_size);