Hi, On 11/21/2014 06:37 AM, Ted Unangst wrote: > Sneaking the change in was easier than I thought, and upon further > review, some of the extra login_cap handling was unnecessary for most > callers. Updated to use a string parameter (arguments regarding its > defintion postponed til next week :)). Thanks for the quick feedback.
Sounds good. :) I've been looking through the code more closely and I see some more problems: First, crypt_checkpass makes a call to crypt() to be compatible with old hashes. However it doesn't check the return value, which is NULL if the hash is invalid, in which case the next code will segfault. Secondly, the call to crypt isn't thread-safe. The crypt call only happens if the hash isn't blowfish (thus wasn't made with crypt_newhash). However, since the flak blog post criticizes crypt for being thread unsafe, and the manual page for crypt_checkpass(3) doesn't mention threading issues and looks safe, one would not expect older inputs to be dangerous in threaded programs. This can be easily solved if crypt_checkpass checks the hash doesn't start with '$' and then calls the utility crypt_hashpass directly (will need to be visible from outside its current translation unit), and storing the temporary hash in a little stack buffer (21 bytes is sufficient). The DES tables must also be safely initialized with a once lock, or precomputed at compile time. Alternatively, support for checking older hashes could be dropped altogether. Third, the purpose of the hashlen parameter to bcrypt_newhash(3) and crypt_newhash(3) is undocumented, but judging from its name and uses across the tree, it obviously looks like the size of the hash buffer for bounds-checking purposes. bcrypt_hashpass(3) never uses the size of the buffer, despite printing and base64 encoding into it! The function should verify the buffer is large enough (rather than assuming the caller uses _PASSWORD_LAN) before doing any work and failing with ERANGE, so the caller can use a bigger buffer. Some miscellaneous notes: It would be good bcrypt_newhash and bcrypt_checkpass set errno (to EINVAL and EACCES) upon failure like their crypt_checkpass and crypt_newhash counterparts. bcrypt_gensalt is forward-declared in bcrypt.c and doesn't need to be for any reason, and pwd.h supplies the function prototype regardless. The u_key_perm array in crypt.c is set, but not used and serve no purpose. The crypt(3) code likes to pretend the DES encryption functions can fail, but they have no error cases except bad parameters, and that can easily be seen never happens. Jonas
