[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
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[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
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.
