[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
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.
