[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20200715185539.GA12903@openwall.com>
Date: Wed, 15 Jul 2020 20:55:39 +0200
From: Solar Designer <solar@...nwall.com>
To: owl-dev@...ts.openwall.com
Subject: Re: [PATCH 0/5] pam_tcb update
On Wed, Jul 15, 2020 at 09:25:31PM +0300, Dmitry V. Levin wrote:
> On Wed, Jul 15, 2020 at 06:57:18PM +0200, Solar Designer wrote:
> > 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.
OK, this is convincing.
> > 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?
I think a NULL return (which we have above) is good enough, hoping that
a caller wouldn't both fail to check for a possible error return and
access the generated salt directly from the output buffer instead of via
the returned pointer. So no need for a NULL dereference crash inside
the function.
> > 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.
OK, so you're already responsible for this.
> > 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
> }
I'm not sure what exactly to make of this, but I'll trust your judgment.
I've just test-built tcb 1.1.9.1 on Owl for i686, got some warnings:
support.c: In function 'unix_run_helper_binary':
support.c:330:3: warning: pointer targets in passing argument 1 of 'execve' differ in signedness [-Wpointer-sign]
/usr/include/unistd.h:480:12: note: expected 'const char *' but argument is of type 'const unsigned char *'
support.c: In function 'check_crypt':
support.c:406:6: warning: pointer targets in passing argument 1 of 'crypt_gensalt_ra' differ in signedness [-Wpointer-sign]
/usr/include/ow-crypt.h:39:14: note: expected 'const char *' but argument is of type 'const unsigned char *'
support.c: In function 'do_crypt':
support.c:700:6: warning: pointer targets in passing argument 1 of 'crypt_gensalt_ra' differ in signedness [-Wpointer-sign]
/usr/include/ow-crypt.h:39:14: note: expected 'const char *' but argument is of type 'const unsigned char *'
support.c: In function '_set_ctrl':
support.c:842:30: warning: pointer targets in assignment differ in signedness [-Wpointer-sign]
support.c:845:31: warning: pointer targets in assignment differ in signedness [-Wpointer-sign]
support.c:849:24: warning: pointer targets in assignment differ in signedness [-Wpointer-sign]
Will you look into these, please?
Alexander
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.
