From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/1329 Path: news.gmane.org!not-for-mail From: Rich Felker Newsgroups: gmane.linux.lib.musl.general Subject: Re: crypt* files in crypt directory Date: Sat, 21 Jul 2012 16:17:03 -0400 Message-ID: <20120721201703.GZ544@brightrain.aerifal.cx> References: <20120721171102.GA16724@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 1342901860 8539 80.91.229.3 (21 Jul 2012 20:17:40 GMT) X-Complaints-To: usenet@dough.gmane.org NNTP-Posting-Date: Sat, 21 Jul 2012 20:17:40 +0000 (UTC) Cc: Lukasz Sowa To: musl@lists.openwall.com Original-X-From: musl-return-1330-gllmg-musl=m.gmane.org@lists.openwall.com Sat Jul 21 22:17:40 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 1Ssg7M-0001qt-Rg for gllmg-musl@plane.gmane.org; Sat, 21 Jul 2012 22:17:36 +0200 Original-Received: (qmail 26215 invoked by uid 550); 21 Jul 2012 20:17:35 -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 26206 invoked from network); 21 Jul 2012 20:17:35 -0000 Content-Disposition: inline In-Reply-To: <20120721171102.GA16724@openwall.com> User-Agent: Mutt/1.5.21 (2010-09-15) Xref: news.gmane.org gmane.linux.lib.musl.general:1329 Archived-At: On Sat, Jul 21, 2012 at 09:11:02PM +0400, Solar Designer wrote: > > However, there are some consts arrays used inside functions which may > > clutter > > stack like flags_by_subtype from BF_crypt(), test_key, test_setting, > > test_hash > > from _crypt_blowfish_rn(). I think that they can be pulled up to global > > static > > consts but we haven't done that yet. What do you think about this? > > I think that they are in .rodata as long as you have "const" on them, > and thus there's no reason to move them to global scope. They don't > clutter the stack. These arrays have static storage duration so they do not use stack space. If they were not static, then a naive compiler would be forced to put them on the stack and generate bloated code to initialize them. A very smart compiler could determine that the address does not leak and thus optimize them to a single static copy. > crypt_r() wrappers. You may also add similarly hash type agnostic > crypt_rn(), crypt_ra(), crypt_gensalt(), crypt_gensalt_rn(), and > crypt_gensalt_ra(). The crypt.3 man page included with crypt_blowfish > documents them - perhaps it can also become the man page for musl's. This isn't a decision yet, but I really question (1) the value of the _rn/_ra interfaces, and (2) whether any of these belong in libc. There's no historical precedent (except perhaps openwall-patched glibc) for having these functions in libc, and as far as I can tell, the only program which will have any use for them on a typical system is the passwd utility. As for why I question _rn/_ra, the only historical reason for having large amounts of data in the crypt_data structure is to store internal state, which should not exist for the crypt_r interface - keeping state is at best useless and at worst an information-leak security vuln. The only time state would be needed is for the encrypt_r and setkey_r functions (analogues of XSI legacy encrypt and setkey functions) which have no place in modern cryptographic programming. As such, a small crypt_data structure (or even a legacy large one) should be fine for storing the only thing it's actually needed to store: the resulting hash from crypt_r. If we do include the crypt_rn/_ra functions, I'd prefer them be the trivial wrappers for the plain crypt_r function. > > +typedef unsigned int BF_word; > > +typedef signed int BF_word_signed; Using types whose range could vary between systems (even though they won't vary on musl systems) seems like a bigger portability issue than the char stuff below... Should these be uint32_t/int32_t? > [...] > > + const char *ptr = key; > [...] > > + tmp[0] <<= 8; > > + tmp[0] |= (unsigned char)*ptr; /* correct */ > > + tmp[1] <<= 8; > > + tmp[1] |= (BF_word_signed)(signed char)*ptr; /* bug */ > > I think I have an instance of undefined behavior on the "bug" line here: > I am casting a potentially unsigned char *ptr to (signed char), thus > causing signed overflow (value may not fit in the signed type's data > range on systems where char is unsigned by default). While reproducing There's no signed overflow here; it's a conversion to a signed type, whose result is implementation-defined if the value does not fit. We require the standard conversion (modular reduction) in musl, so I have no objection to the code as-is, but if you want it to be "more portable" you can write this some other way like the way we did it in the DES crypt module. Rich