Follow @Openwall on Twitter for new release announcements and other news
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Sat, 21 Jul 2012 16:17:03 -0400
From: Rich Felker <>
Cc: Lukasz Sowa <>
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.


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.