[Speex-dev] [PATCH]Add address overflow check

[Ruikai Liu]

Hi,

I came into a crash when using 32-bit `speexdec` and found that there's an
address overflow in function `print_comments()`:

static void print_comments(char *comments, int length)

{

char *c=comments;

int len, i, nb_fields;

char *end;


if (length<8)

{

fprintf (stderr, "Invalid/corrupted comments\n");

return;

}

end = c+length;

len=readint(c, 0);

c+=4;

// 'c+len' MAY OVERFLOW

if (len < 0 || c+len>end)

{

fprintf (stderr, "Invalid/corrupted comments\n");

return;

}


The pointer `c` happened to be greater than `0x80000000` and the sum
overflowed, even though `length` is positive.

Here's the patch code:

*diff --git a/src/speexdec.c b/src/speexdec.c*

*index 4721dc1..18786f1 100644*

*--- a/src/speexdec.c*

*+++ b/src/speexdec.c*

@@ -105,7 +105,7 @@ static void print_comments(char *comments, int length)

end = c+length;

len=readint(c, 0);

c+=4;

- if (len < 0 || c+len>end)

+ if (len < 0 || c+len>end || c+len<c)

{

fprintf (stderr, "Invalid/corrupted comments\n");

return;

@@ -129,7 +129,7 @@ static void print_comments(char *comments, int length)

}

len=readint(c, 0);

c+=4;

- if (len < 0 || c+len>end)

+ if (len < 0 || c+len>end || c+len<c)

{

fprintf (stderr, "Invalid/corrupted comments\n");

return;

Thanks!

[Jean-Marc Valin]

Pointers are unsigned so this shouldn't be an issue. I suspect you're
being hit by something else. That or your compiler is really broken.

Cheers,

Jean-Marc

Post by Ruikai Liu
Hi,
I came into a crash when using 32-bit `speexdec` and found that there's
staticvoidprint_comments(char*comments, intlength)
{
   char*c=comments;
   intlen, i, nb_fields;
   char*end;
   if(length<8)
   {   
      fprintf (stderr, "Invalid/corrupted comments\n");
      return;
   }   
   end = c+length;
   len=readint(c, 0); 
   c+=4;
// 'c+len' MAY OVERFLOW
   if(len < 0|| c+len>end)
   {   
      fprintf (stderr, "Invalid/corrupted comments\n");
      return;
   }
The pointer `c` happened to be greater than `0x80000000` and the sum
overflowed, even though `length` is positive.
*diff --git a/src/speexdec.c b/src/speexdec.c*
*index 4721dc1..18786f1 100644*
*--- a/src/speexdec.c*
*+++ b/src/speexdec.c*
@@ -105,7 +105,7 @@static void print_comments(char *comments, int length)
    end = c+length;
    len=readint(c, 0);
    c+=4;
-   if (len < 0 || c+len>end)
+   if (len < 0 || c+len>end || c+len<c)
    {
       fprintf (stderr, "Invalid/corrupted comments\n");
       return;
@@ -129,7 +129,7 @@static void print_comments(char *comments, int length)
       }
       len=readint(c, 0);
       c+=4;
-      if (len < 0 || c+len>end)
+      if (len < 0 || c+len>end || c+len<c)
       {
          fprintf (stderr, "Invalid/corrupted comments\n");
          return;
Thanks!
--
Best regards,
Ruikai Liu
_______________________________________________
Speex-dev mailing list
http://lists.xiph.org/mailman/listinfo/speex-dev

[Ruikai Liu]

I'm not familiar with the code, but it seems that the new address is
accessed for `nb_fields`:

end = c+length;

len=readint(c, 0);

c+=4;

if (len < 0 || c+len>end)

{

fprintf (stderr, "Invalid/corrupted comments\n");

return;

}

fwrite(c, 1, len, stderr);

c+=len;

fprintf (stderr, "\n");

if (c+4>end)

{

fprintf (stderr, "Invalid/corrupted comments\n");

return;

}

nb_fields=readint(c, 0);

So if `c+len` overflows and becomes some really small value, say `NULL`,
then `readint()` would result in segfault.

Post by Jean-Marc Valin
Pointers are unsigned so this shouldn't be an issue. I suspect you're
being hit by something else. That or your compiler is really broken.
Cheers,
Jean-Marc

Post by Ruikai Liu
Hi,
I came into a crash when using 32-bit `speexdec` and found that there's
staticvoidprint_comments(char*comments, intlength)
{
char*c=comments;
intlen, i, nb_fields;
char*end;
if(length<8)
{
fprintf (stderr, "Invalid/corrupted comments\n");
return;
}
end = c+length;
len=readint(c, 0);
c+=4;
// 'c+len' MAY OVERFLOW
if(len < 0|| c+len>end)
{
fprintf (stderr, "Invalid/corrupted comments\n");
return;
}
The pointer `c` happened to be greater than `0x80000000` and the sum
overflowed, even though `length` is positive.
*diff --git a/src/speexdec.c b/src/speexdec.c*
*index 4721dc1..18786f1 100644*
*--- a/src/speexdec.c*
*+++ b/src/speexdec.c*
@@ -105,7 +105,7 @@static void print_comments(char *comments, int length)
end = c+length;
len=readint(c, 0);
c+=4;
- if (len < 0 || c+len>end)
+ if (len < 0 || c+len>end || c+len<c)
{
fprintf (stderr, "Invalid/corrupted comments\n");
return;
@@ -129,7 +129,7 @@static void print_comments(char *comments, int length)
}
len=readint(c, 0);
c+=4;
- if (len < 0 || c+len>end)
+ if (len < 0 || c+len>end || c+len<c)
{
fprintf (stderr, "Invalid/corrupted comments\n");
return;
Thanks!
--
Best regards,
Ruikai Liu
_______________________________________________
Speex-dev mailing list
http://lists.xiph.org/mailman/listinfo/speex-dev

[Jean-Marc Valin]

Yes, I agree that buf_end - buf_start < len_to_check is better. It's the
0xFFFFFFFF overflow that's a cause of concern, not the 0x80000000. That
being said, I believe that the length argument in this case can be
trusted since it comes from the application and not from the user.

Cheers,

Jean-Marc

Post by Jean-Marc Valin
Pointers are unsigned so this shouldn't be an issue. I suspect you're
being hit by something else. That or your compiler is really broken.

I don't know how important it is in this case (probably pretty minor) but in general Ruikai's right.
It doesn't matter that pointers are unsigned; that fact that a pointer could have a "large" value like 0xffff_ff00 means that they can wrap if you do length checks the wrong way. The behaviour is completely defined - it just causes the code not to work as intended.
The "bad" way of doing a length check is
char* buf_start, buf_end;
unsigned len_to_check;
if (buf_start + len_to_check > buf_end)
fail()
Because the length is to-be-checked, it could have an unsafe large value, causing an (unsigned) overflow. For example, with buf_start = 0xffff_ff00 and buf_end = 0xffff_ff10, the maximum allowed length is 0x10, but a length of 0x100 will cause an overflow and bypass the check.
The safe way of doing a length check is
if (buf_end - buf_start < len_to_check)
fail()
The buffer bounds are known safe, so the arithmetic is OK to do that way round.
Nick

[Jean-Marc Valin]

The "bad" way of doing a length check is
char* buf_start, buf_end;
unsigned len_to_check;
if (buf_start + len_to_check > buf_end)
fail()
Because the length is to-be-checked, it could have an unsafe large value, causing an (unsigned) overflow. For example, with buf_start = 0xffff_ff00 and buf_end = 0xffff_ff10, the maximum allowed length is 0x10, but a length of 0x100 will cause an overflow and bypass the check.

Right. And in fact the C standard says that merely computing the
expression ptr+large_value is undefined (when the result points beyond
the buffer) even if you don't even use the pointer.

The safe way of doing a length check is
if (buf_end - buf_start < len_to_check)
fail()
The buffer bounds are known safe, so the arithmetic is OK to do that way round.

Yes, I agree. That would be the right way to make the check.

Cheers,

Jean-Marc