FreeTDS Development Group

[Claudio Cicali <c.cicali AT mclink.it>]

Hi Steve,

First of all this considerations are only *potentially* dangerous, imho.
I've not walked throw the code of freetds to see if this really can happens.
I'll make an example.

Let's try this :

char dest[255];  // I suppose that if I call a function which accepts
                 // the length of the output buffer, that function never
                 // write beyond this limit...

TDS_INT tds_convert_text(srctype, src, 300, SYBCHAR, dest, sizeof(dest));

// 300 is used just to have a > 255 srclen

> >        case SYBCHAR:
> >           cplen = srclen;

cplen == 300

> >           if (destlen<cplen) cplen = destlen;

cplen == 255

> >           if (cplen>255) cplen=255;

cplen == 255

> >           memcpy(dest, src, cplen);

This is ok; dest is 255 bytes long, cplen is 255 too.

> >           dest[cplen]='\0';

This is wrong. You are writing the 256th byte.

In data 05:46 PM 1/23/01 -0600, Steve Langasek ha scritto:
> Hi Claudio,
>
> I'm not sure I understand what you're saying.  How can this buffer overflow
> happen?  From what I see, cplen is never more than 255, and it's also never
> more than the value of destlen.  If cplen == 255, then the code will write
> dest[255] = '\0', which should be ok if our buffer is 256 chars in size.  Can
> you tell us where this is a problem?  Is there a place in the code where
> tds_convert_text() is being called with the wrong value for 'destlen'?
>
> Regards,
> Steve Langasek
> postmodern programmer
>
> > In convert.c
>
> > TDS_INT tds_convert_text(int srctype,unsigned char *src,TDS_UINT srclen,
> >          int desttype,unsigned char *dest,TDS_UINT destlen)
> > {
> > int cplen;
> >
> >     switch(desttype) {
> >        case SYBTEXT:
> >           cplen = srclen > destlen ? destlen : srclen;
> >           memcpy(dest, src, cplen);
> >           return cplen;
> >        case SYBCHAR:
> >           cplen = srclen;
> >           if (destlen<cplen) cplen = destlen;
> >           if (cplen>255) cplen=255;
> >           memcpy(dest, src, cplen);
> >           dest[cplen]='\0';        <----------- if dest was dest[255] this
> > writes "dest[256]"
> >           return cplen;                         It should be dest[cplen-1]
> > = '\0'
> >     }
> > }


+-------------------+----------------------+
|  Claudio Cicali   | http://www.flexer.it |
|c.cicali AT mclink.it |    Nerd Made Good    |
+-------------------+----------------------+