Re: FreeSec crypt()

[Rich Felker <dalias@aerifal.cx>]

On Wed, Jun 13, 2012 at 08:10:32AM +0200, Szabolcs Nagy wrote:
> > char *
> > _crypt_extended_r(const char *key, const char *setting, char *output)
> > {
> ....
> > 	while (q - (u_char *) keybuf < sizeof(keybuf)) {
> > 		*q++ = *key << 1;
> 
> implementation-defined signed shift

Undefined. Right shifts are implementation-defined. Left-shifts are
defined only for positive operands that don't overflow.

Note that even if the behavior were defined, this code seems to have
different behavior for high bytes depending on the signedness of char.
That looks like a major bug; I thought that bug in crypt
implementations was fixed years ago.

> > 		for (i = 1, count = 0; i < 5; i++) {
> > 			int value = ascii_to_bin(setting[i]);
> > 			if (ascii64[value] != setting[i])
> > 				return NULL;
> > 			count |= value << (i - 1) * 6;
> > 		}
> 
> signed shift (harmless)

Yes this one seems to be harmless unless it can overflow, and from the
loop bounds it seems impossible to overflow.

> > 			while (q - (u_char *) keybuf < sizeof(keybuf) && *key)
> > 				*q++ ^= *key++ << 1;
> 
> signed shift

This one looks like it has the same bug as the first.

Rich