Re: crypt* files in crypt directory

[Solar Designer <solar@openwall.com>]

Hi,

I suggest that you subscribe to the list, so that if someone does not CC
you on a message yet you want to reply, you don't happen to start a new
thread (again).

On Sat, Jul 21, 2012 at 05:23:24PM +0200, ?ukasz Sowa wrote:
> Together with Daniel we've prepared initial patch for inclusion 
> crypt_blowfish
> and crypt_gensalt in musl. Things to change were rather cosmetic. Both 
> blowfish
> and gensalt implementations don't use global state, only const statics 
> (which
> needed simply 'const' to meet musl standard).

I will likely add those const's to my "upstream" code for crypt_blowfish.

> 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.

> +++ b/include/crypt.h
> @@ -13,6 +13,19 @@ struct crypt_data {
>  char *crypt(const char *, const char *);
>  char *crypt_r(const char *, const char *, struct crypt_data *);
>  
> +char *_crypt_gensalt_traditional_rn(const char *prefix, unsigned long count,
> +	const char *input, int size, char *output, int output_size);
> +char *_crypt_gensalt_extended_rn(const char *prefix, unsigned long count,
> +	const char *input, int size, char *output, int output_size);
> +char *_crypt_gensalt_md5_rn(const char *prefix, unsigned long count,
> +	const char *input, int size, char *output, int output_size);
> +
> +int _crypt_output_magic(const char *setting, char *output, int size);
> +char *_crypt_blowfish_rn(const char *key, const char *setting,
> +	char *output, int size);
> +char *_crypt_gensalt_blowfish_rn(const char *prefix, unsigned long count,
> +	const char *input, int size, char *output, int output_size);
> +
>  #ifdef __cplusplus
>  }
>  #endif

None of the interfaces you've added above were supposed to be exported
by a libc.  They're not in Owl's glibc, for example, although it does
include crypt_blowfish.

Instead, you need to roll crypt_blowfish support into crypt() and
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.

See crypt_blowfish's wrapper.c and modify it for use in musl or at least
reuse code from it.

> +typedef unsigned int BF_word;
> +typedef signed int BF_word_signed;
[...]
> +	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
the old bug here is on purpose, the new behavior should better be
precisely defined (just _the_ bug the way it happened to be compiled
before, not some other misbehavior).  I'd appreciate suggestions for a
clean and not too verbose fix for this.

Thanks,

Alexander