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!
--
Best regards,
Ruikai Liu
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.xiph.org/pipermail/speex-dev/attachments/20180209/9db2091b/attachment.html>
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 On 02/09/2018 04:42 AM, Ruikai Liu wrote:> Hi, > > I came into a crash when using 32-bit `speexdec` and found that there's > an address overflow in function `print_comments()`: > > 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. > > 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! > > -- > Best regards, > > Ruikai Liu > > > _______________________________________________ > Speex-dev mailing list > Speex-dev at xiph.org > http://lists.xiph.org/mailman/listinfo/speex-dev >
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.
On Fri, Feb 9, 2018 at 11:56 PM, Jean-Marc Valin <jmvalin at jmvalin.ca>
wrote:
> 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
>
> On 02/09/2018 04:42 AM, Ruikai Liu wrote:
> > Hi,
> >
> > I came into a crash when using 32-bit `speexdec` and found that
there's
> > an address overflow in function `print_comments()`:
> >
> > 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.
> >
> > 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!
> >
> > --
> > Best regards,
> >
> > Ruikai Liu
> >
> >
> > _______________________________________________
> > Speex-dev mailing list
> > Speex-dev at xiph.org
> > http://lists.xiph.org/mailman/listinfo/speex-dev
> >
>
--
Best regards,
Ruikai Liu
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.xiph.org/pipermail/speex-dev/attachments/20180211/d112226a/attachment.html>
On 09 February 2018 15:56 Jean-Marc Valin wrote:> 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