Re: ftpd: the advisory version

[scut () NB IN-BERLIN DE (Sebastian)]

Hi.

On Tue, Jun 27, 2000 at 03:41:59PM -0700, Dan Harkless wrote:

> > void
> > func_proper (unsigned char *domain)
> > {
> >     int             len = domain[0];
> >     unsigned char   buff[64];
> >
> >
> >     if (len >= 64)
> >             return;
> >
> >     strncpy (buff, &domain[1], len);
> >     buff[63] = '\x00';
> > }

> Uh, no, the strncpy() prototype is:
>     char *strncpy(char *dst, const char *src, size_t n);

> len should be a size_t (which is typedef'd to be some kind of unsigned int),
> which would avoid the problem (without having to mess with explicitly
> unsigned chars, which will cause warnings on platforms where chars are
> signed, for one thing).

Yes and no.

The problem with type conversion always arise from the signedness of the
source type, the type of the destination type is not important, except
for later comparisons.

So this is still unsafe:

void
func_weak (char *domain)
{
        unsigned char   buff[2000];
        size_t          len = domain[0];

        strncpy (buff, &domain[1], len);
        buff[1999] = '\x00';
}

In this case, len can very well get very large, hence nullifying the
sense of the len parameter in the strncpy statement. I removed the
len comparison because it would catch that case, but one may assume
that code like the above may look secure to some people.

But I agree that the usage of size_t is good, I used it in the above
code for clarification.

ciao,
scut

--
- scut () nb in-berlin de - http://nb.in-berlin.de/scut/ --- you don't need a --
-- lot of people to be great, you need a few great to be the best ------------
http://3261000594/scut/pgp - 5453 AC95 1E02 FDA7 50D2 A42D 427E 6DEF 745A 8E07
-- data in VK/USA Mayfly experienced, awaiting transfer location, hi echelon -