|
Message-ID: <20200715182531.GA23225@altlinux.org> Date: Wed, 15 Jul 2020 21:25:31 +0300 From: "Dmitry V. Levin" <ldv@...linux.org> To: owl-dev@...ts.openwall.com Subject: Re: [PATCH 0/5] pam_tcb update On Wed, Jul 15, 2020 at 06:57:18PM +0200, Solar Designer wrote: > > > > On Thu, Jul 05, 2018 at 02:29:19AM +0300, Dmitry V. Levin wrote: > > > > > I've got a few patches for pam_tcb. Tested in Sisyphus. > > On Wed, Jul 15, 2020 at 05:08:54PM +0300, Dmitry V. Levin wrote: > > I've finally managed to commit these changes. > > Thank you! > > On Wed, Jul 15, 2020 at 04:58:26PM +0300, Owl CVS (ldv) wrote: > > +#ifdef CRYPT_GENSALT_IMPLEMENTS_AUTO_ENTROPY > > + salt = crypt_gensalt_ra(pam_unix_param.crypt_prefix, > > + pam_unix_param.count, NULL, 0); > > How exactly will this fail in case CRYPT_GENSALT_IMPLEMENTS_AUTO_ENTROPY > was defined at compile time, but the library in use at run time lacks > the support? When a user of crypt_gensalt_ra is linked with -lcrypt from libxcrypt, it gets a versioned dependency, e.g. $ readelf -s /lib64/security/pam_tcb.so | grep crypt_gensalt_ra 68: 0000000000000000 0 FUNC GLOBAL DEFAULT UND crypt_gensalt_ra@...YPT_2.0 (5) ELF objects linked this way are rejected by the dynamic linker at some stage if libcrypt does not provide XCRYPT_2.0 interface. If that's not enough, crypt_blowfish/wrapper.c has the following check: char *__crypt_gensalt_rn(const char *prefix, unsigned long count, const char *input, int size, char *output, int output_size) { ... /* This may be supported on some platforms in the future */ if (!input) { __set_errno(EINVAL); return NULL; } Besides that, prefix-specific crypt_gensalt_rn methods check the size argument and return EINVAL when it's too small for the corresponding prefix. > I am concerned that this will silently return an empty > salt for some hash types, instead of crashing. Can we ensure there will > be a NULL pointer dereference crash, maybe by specifying NULL along with > non-zero size (and large enough that an implementation wouldn't likely > round it down to zero)? Would a NULL pointer dereference crash provide a better diagnostics than EINVAL? > Maybe we're better off not using CRYPT_GENSALT_IMPLEMENTS_AUTO_ENTROPY > and its corresponding feature at all, since we have our own code anyway? > Supporting both ways how tcb may be built increases the likelihood of > issues that we'd be responsible for. With my ALT hat on, libxcrypt is already in use and its get_random_bytes implementation is good enough, so I don't really need two different implementations for the same feature. > I have similar concerns about CRYPT_GENSALT_IMPLEMENTS_DEFAULT_PREFIX. > What will happen if this is defined at compile time, but then the run > time library has no default? Besides the guarantee given by the dynamic linker, __crypt_gensalt_rn from crypt_blowfish/wrapper.c calls strncmp(prefix, "$2a$", 4) right after the input check. One can manage to build libxcrypt with the default algorithm disabled, in this case crypt_gensalt_ra fails with EINVAL. The implementation of crypt_gensalt_rn in libxcrypt has the following comment: /* If the prefix is 0, that means to use the current best default. Note that this is different from the behavior when the prefix is "", which selects DES. HASH_ALGORITHM_DEFAULT is not defined when the current default algorithm was disabled at configure time. */ if (!prefix) { #if defined HASH_ALGORITHM_DEFAULT prefix = HASH_ALGORITHM_DEFAULT; #else errno = EINVAL; return 0; #endif } -- ldv
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.