Re: crypt* files in crypt directory

[Szabolcs Nagy <nsz@port70.net>]

it's scary how these crypto codes mix various
signed and unsigned integer types

i thought these algorithms were designed and
implemented with the c type system in mind..


i added review comments
the style comments are subjective of course

* Solar Designer <solar@openwall.com> [2012-08-09 14:53:48 +0400]:
> typedef unsigned int BF_word;
> typedef signed int BF_word_signed;
> 
> /* Number of Blowfish rounds, this is also hardcoded into a few places */
> #define BF_N				16
> 
> typedef BF_word BF_key[BF_N + 2];

i don't like these typedefs
it seems the code assumes 32bit unsigned BF_word
(eg. the way L>>24 is used below)

> #define BF_safe_atoi64(dst, src) \
> { \
> 	tmp = (unsigned char)(src); \
> 	if ((unsigned int)(tmp -= 0x20) >= 0x60) return -1; \

tmp is already unsigned int

> #define BF_ROUND(L, R, N) \
> 	tmp1 = L & 0xFF; \
> 	tmp2 = L >> 8; \
> 	tmp2 &= 0xFF; \
> 	tmp3 = L >> 16; \
> 	tmp3 &= 0xFF; \
> 	tmp4 = L >> 24; \
> 	tmp1 = ctx->s.S[3][tmp1]; \
> 	tmp2 = ctx->s.S[2][tmp2]; \
> 	tmp3 = ctx->s.S[1][tmp3]; \
> 	tmp3 += ctx->s.S[0][tmp4]; \
> 	tmp3 ^= tmp2; \
> 	R ^= ctx->s.P[N + 1]; \
> 	tmp3 += tmp1; \
> 	R ^= tmp3;

i guess this is performance critical, but
i wouldn't spread those expressions over
several lines

tmp1 = ctx->S[3][L & 0xff];
tmp2 = ctx->S[2][L>>8 & 0xff];
tmp3 = ctx->S[1][L>>16 & 0xff];
tmp4 = ctx->S[0][L>>24 & 0xff];
R ^= ctx->P[N+1];
R ^= ((tmp3 + tmp4) ^ tmp2) + tmp1;

> 	do {
> 		ptr += 2;
> 		L ^= ctx->s.P[0];
> 		BF_ROUND(L, R, 0);
> 		BF_ROUND(R, L, 1);
> 		BF_ROUND(L, R, 2);
> 		BF_ROUND(R, L, 3);
> 		BF_ROUND(L, R, 4);
> 		BF_ROUND(R, L, 5);
> 		BF_ROUND(L, R, 6);
> 		BF_ROUND(R, L, 7);
> 		BF_ROUND(L, R, 8);
> 		BF_ROUND(R, L, 9);
> 		BF_ROUND(L, R, 10);
> 		BF_ROUND(R, L, 11);
> 		BF_ROUND(L, R, 12);
> 		BF_ROUND(R, L, 13);
> 		BF_ROUND(L, R, 14);
> 		BF_ROUND(R, L, 15);
> 		tmp4 = R;
> 		R = L;
> 		L = tmp4 ^ ctx->s.P[BF_N + 1];
> 		*(ptr - 1) = R;
> 		*(ptr - 2) = L;
> 	} while (ptr < end);

why increase ptr at the begining?
it seems the idiomatic way would be

 *ptr++ = L;
 *ptr++ = R;

> 			tmp[1] |= (BF_word_signed)(signed char)*ptr; /* bug */

i don't think the BF_word_signed cast helps here
signed char is promoted to int and then
it will be converted to BF_word

> 	if (setting[0] != '$' ||
> 	    setting[1] != '2' ||
> 	    setting[2] < 'a' || setting[2] > 'z' ||
> 	    !flags_by_subtype[(unsigned int)(unsigned char)setting[2] - 'a'] ||

setting[2] is already range checked so
the casts are not necessary
(assuming 'a' < 'z')

> 	BF_set_key(key, data.expanded_key, data.ctx.s.P,
> 	    flags_by_subtype[(unsigned int)(unsigned char)setting[2] - 'a']);

ditto

> 		do {
> 			L = BF_encrypt(&data.ctx,
> 			    L ^ data.binary.salt[0], R ^ data.binary.salt[1],
> 			    ptr, ptr);
> 			R = *(ptr + 1);
> 			ptr += 2;
> 
> 			if (ptr >= &data.ctx.PS[BF_N + 2 + 4 * 0x100])
> 				break;
> 
> 			L = BF_encrypt(&data.ctx,
> 			    L ^ data.binary.salt[2], R ^ data.binary.salt[3],
> 			    ptr, ptr);
> 			R = *(ptr + 1);
> 			ptr += 2;
> 		} while (1);

i'd use for (;;) for infinite loops

eventhough most of the loops in the code are do{}while

> 	do {
> 		int done;
> 
> 		for (i = 0; i < BF_N + 2; i += 2) {
> 			data.ctx.s.P[i] ^= data.expanded_key[i];
> 			data.ctx.s.P[i + 1] ^= data.expanded_key[i + 1];
> 		}
> 
> 		done = 0;
> 		do {
> 			BF_encrypt(&data.ctx, 0, 0,
> 			    &data.ctx.PS[0],
> 			    &data.ctx.PS[BF_N + 2 + 4 * 0x100]);
> 
> 			if (done)
> 				break;
> 			done = 1;
> 
> 			{
> 				BF_word tmp1, tmp2, tmp3, tmp4;
> 
> 				tmp1 = data.binary.salt[0];
> 				tmp2 = data.binary.salt[1];
> 				tmp3 = data.binary.salt[2];
> 				tmp4 = data.binary.salt[3];
> 				for (i = 0; i < BF_N; i += 4) {
> 					data.ctx.s.P[i] ^= tmp1;
> 					data.ctx.s.P[i + 1] ^= tmp2;
> 					data.ctx.s.P[i + 2] ^= tmp3;
> 					data.ctx.s.P[i + 3] ^= tmp4;
> 				}
> 				data.ctx.s.P[16] ^= tmp1;
> 				data.ctx.s.P[17] ^= tmp2;
> 			}
> 		} while (1);

i like for better

for (done = 0; ; done++) {
	...
	if (done)
		break;
	...
}

> 		count = 64;
> 		do {
> 			L = BF_encrypt(&data.ctx, L, LR[1],
> 			    &LR[0], &LR[0]);
> 		} while (--count);

for (count = 0; count < 64; count++)

i assume it will be optimized to the same thing
(count is not used later)

> 	output[7 + 22 - 1] = BF_itoa64[(int)
> 		BF_atoi64[(int)setting[7 + 22 - 1] - 0x20] & 0x30];

is setting[28] range checked at this point?

the int casts do not help

> 	_crypt_output_magic(setting, output, size);
> 	retval = BF_crypt(key, setting, output, size, 16);
> 	save_errno = errno;

why save errno before the self test?

if the self test fails then errno can be anything anyway
if it does not fail then errno is unchanged i guess

> 	memcpy(buf.s, test_setting, sizeof(buf.s));

sizeof buf.s
without ()

> 	memset(buf.o, 0x55, sizeof(buf.o));
> 	buf.o[sizeof(buf.o) - 1] = 0;
> 	p = BF_crypt(test_key, buf.s, buf.o, sizeof(buf.o) - (1 + 1), 1);

ditto

i'd write (1 + 1) as 2

> 	    test_hash[(unsigned int)(unsigned char)buf.s[2] & 1],

the extra (unsigned int) cast is unnecessary