From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/1328 Path: news.gmane.org!not-for-mail From: Solar Designer Newsgroups: gmane.linux.lib.musl.general Subject: Re: crypt* files in crypt directory Date: Sat, 21 Jul 2012 21:11:02 +0400 Message-ID: <20120721171102.GA16724@openwall.com> References: 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 1342890673 30600 80.91.229.3 (21 Jul 2012 17:11:13 GMT) X-Complaints-To: usenet@dough.gmane.org NNTP-Posting-Date: Sat, 21 Jul 2012 17:11:13 +0000 (UTC) Cc: musl@lists.openwall.com To: Lukasz Sowa Original-X-From: musl-return-1329-gllmg-musl=m.gmane.org@lists.openwall.com Sat Jul 21 19:11:13 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 1SsdCw-0002ta-0U for gllmg-musl@plane.gmane.org; Sat, 21 Jul 2012 19:11:10 +0200 Original-Received: (qmail 15440 invoked by uid 550); 21 Jul 2012 17:11:09 -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 15390 invoked from network); 21 Jul 2012 17:11:09 -0000 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.4.2.3i Xref: news.gmane.org gmane.linux.lib.musl.general:1328 Archived-At: 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