From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/1485 Path: news.gmane.org!not-for-mail From: Szabolcs Nagy Newsgroups: gmane.linux.lib.musl.general Subject: Re: crypt* files in crypt directory Date: Thu, 9 Aug 2012 13:58:12 +0200 Message-ID: <20120809115811.GA32316@port70.net> References: <20120808022421.GE27715@brightrain.aerifal.cx> <20120808044235.GA22470@openwall.com> <20120808052844.GF27715@brightrain.aerifal.cx> <20120808062706.GA23135@openwall.com> <20120808214855.GL27715@brightrain.aerifal.cx> <20120809033613.GA24926@openwall.com> <20120809072940.GA26288@openwall.com> <20120809105348.GA27361@openwall.com> Reply-To: musl@lists.openwall.com NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-Trace: dough.gmane.org 1344513505 2364 80.91.229.3 (9 Aug 2012 11:58:25 GMT) X-Complaints-To: usenet@dough.gmane.org NNTP-Posting-Date: Thu, 9 Aug 2012 11:58:25 +0000 (UTC) To: musl@lists.openwall.com Original-X-From: musl-return-1486-gllmg-musl=m.gmane.org@lists.openwall.com Thu Aug 09 13:58:26 2012 Return-path: Envelope-to: gllmg-musl@plane.gmane.org Original-Received: from mother.openwall.net ([195.42.179.200]) by plane.gmane.org with smtp (Exim 4.69) (envelope-from ) id 1SzRNh-0002V1-T3 for gllmg-musl@plane.gmane.org; Thu, 09 Aug 2012 13:58:26 +0200 Original-Received: (qmail 32057 invoked by uid 550); 9 Aug 2012 11:58:24 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: Original-Received: (qmail 32048 invoked from network); 9 Aug 2012 11:58:24 -0000 Content-Disposition: inline In-Reply-To: <20120809105348.GA27361@openwall.com> User-Agent: Mutt/1.5.21 (2010-09-15) Xref: news.gmane.org gmane.linux.lib.musl.general:1485 Archived-At: 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 [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