Date: Sat, 21 Jul 2012 16:17:03 -0400 From: Rich Felker <dalias@...ifal.cx> To: musl@...ts.openwall.com Cc: Lukasz Sowa <kontakt@...aszsowa.pl> Subject: Re: crypt* files in crypt directory 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 <<= 8; > > + tmp |= (unsigned char)*ptr; /* correct */ > > + tmp <<= 8; > > + tmp |= (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
Powered by blists - more mailing lists
Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.