Re: ftpd: the advisory version

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

Hi.

On Sun, Jun 25, 2000 at 09:43:19AM +0200, Bernd Luevelsmeyer wrote:

> According to the C standard, <ctype.h> functions operate on values that
> are representable as a 'unsigned char' or EOF; hence, if the compiler's
> 'char' is signed then negative character values in the string 'cmd' are
> possible and might cause unpredictable results in isspace(), isupper()
> and tolower(). Because sanitizing shouldn't stop, and the test with
> isupper() is unnecessary anyway (tolower() returns the unchanged value
> if the parameter isn't an uppercase letter), I suggest:
>      for (t = cmd; *t; t++)
>              *t = tolower((unsigned char)*t);

As a general advice from my own experience in auditing programs, it is
good to always prefer the (unsigned char) type over the (char) type when
dealing with possible non-ASCII (ie >= 0x80) data, especially when user
supplied data is used.

A lot of security issues arise from code where the programmer is not
fully aware of what a type cast does, or how to properly ensure no value-
overflow does occur.

This code is insecure:

void
func (char *domain)
{
        int     len = domain[0];
        char    buff[64];

        if (len >= 64)
                return;

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

len can become negative when domain[0] is >= 0x80. The len check will be
passed because of it's type, and the strncpy may result in arbitrary length
NUL-terminated data being copied into buff.

This is known, and there is a small paper about it (written by mixter I
think), and is obvious in the code above. But there are more complicated
source codes where such vulnerabilities can easily arise, and it may slip
through even the careful programmers hands (hi l0pht).

So please, use unsigned char pointers, buffers and casts. For example the
proper code for the above function would be:

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';
}

Also, with the recent vulnerabilities being made public the security
community notices a strong trend towards more wicked vulnerabilities,
drifing away from the simple "unchecked-strcpy" vulnerabilities, to
format-string-supply, type-casting, heap-overflow and one-byte-overflows
and yet unknown vulnerabilities.

There are many common mistakes still being made especially in C code,
like misuse of sizeof constants in strncat, snprintf and other functions,
falling for a "I use str*n* functions, so I'm safe" mentality. That's
where the future exploits will arise from.

ciao,
scut / teso


--
- 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 -