Follow @Openwall on Twitter for new release announcements and other news
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
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.