Re: crypt_r()?

[Taylor R Campbell <campbell+netbsd-tech-kern%mumble.net@localhost>]

Discussion about a reasonable API is ongoing, but I'll give some
review comments anyway -- including some of my thoughts on why the
crypt_r API is not ideal -- in case they're helpful.


   --- include/unistd.h	15 Oct 2021 22:32:28 -0000	1.162
   +++ include/unistd.h	12 Feb 2022 12:47:04 -0000
   @@ -325,8 +325,15 @@
     * Implementation-defined extensions
     */
    #if defined(_NETBSD_SOURCE)
   +struct crypt_data {
   +    int initialized; /* glibc compat */
   +    char __buf[512]; /* buffer returned by crypt_r */
   +};

I'd avoid putting this in unistd.h -- maybe a public <crypt.h> header
file instead if it's to match the API documented in
<https://www.man7.org/linux/man-pages/man3/crypt.3.html>.

Using _NETBSD_SOURCE _or_ _GNU_SOURCE, for APIs compatible with glibc,
has precedent -- e.g., for feenableexcept/fedisableexcept/fegetexcept.

Where does 512 come from?  This should be documented in a comment.

If the result is to be statically allocated -- and I'm not opining on
that, just taking it as a premise -- it needs to be large enough for
all of the password hashes in tree.  512 bytes is almost certainly
enough, but this magic number should be documented somewhere, and
verified with __CTASSERT for all of the password hashes involved.

Quick back-of-the-envelope estimate: with a 32-byte salt and a 32-byte
hash (which is the largest that there is any cryptographic motivation
for, even in the face of quantum computers), base64-encoded into up to
43 bytes each, that leaves 426 bytes for parameters and other overhead
like the `$argon2' tag, which seems like a lot more than the caller
has to reserve stack space for.

So maybe, say, 188 bytes instead of 512 bytes would be enough (still
leaves over 100 bytes for overhead); then the whole struct fits in 192
bytes or three cache lines on many CPUs.  I'm not saying 188 is the
right choice, just giving an example with reasoning -- maybe 512 is
better but it needs a rationale.  Obviously we want to overestimate
this to future-proof the ABI, but this also shouldn't be unreasonably
burdensome on the caller's stack space which is often much more
limited than heap space.

And, of course, this is a reason to prefer heap allocation, even if
that means slightly more API complexity (caller must free the result):
we just don't need to think about burden on the caller's stack usage.

On the other hand, there is some value to just being able to drop this
API in and have existing code work.  Certainly the glibc approach of
putting >128 KB in struct crypt_data is, uhhh, not great!  No modern
justification for doing that -- either it's too little for modern
password hashes, or it's a waste of stack space because they'll
allocate storage separately on the heap anyway.

   --- lib/libcrypt/bcrypt.c       16 Oct 2021 10:53:33 -0000      1.22
   +++ lib/libcrypt/bcrypt.c       12 Feb 2022 12:47:04 -0000
   @@ -74,9 +74,9 @@
    static void encode_base64(u_int8_t *, u_int8_t *, u_int16_t);
    static void decode_base64(u_int8_t *, u_int16_t, const u_int8_t *);

   -crypt_private char *__bcrypt(const char *, const char *);      /* XXX */
   +/* crypt_private char *__bcrypt(const char *, const char *);*/ /* XXX */

   -static char    encrypted[_PASSWORD_LEN];
   +/* static char    encrypted[_PASSWORD_LEN]; */
 
Just delete unused things like this, instead of commenting them out,
and __CTASSERT sizeof((struct crypt_data *)0) >= _PASSWORD_LEN.

(or maybe struct crypt_data::__buf should just be _PASSWORD_LEN bytes
long)
 
   --- lib/libcrypt/crypt-argon2.c 22 Nov 2021 14:30:24 -0000      1.15
   +++ lib/libcrypt/crypt-argon2.c 12 Feb 2022 12:47:04 -0000
   @@ -379,10 +379,10 @@
           /* argon2 pwd buffer */
           char pwdbuf[128];
           /* returned static buffer */
   -       static char rbuf[512];
   +       /* static char rbuf[512]; */
 
delete, __CTASSERT

This should also be a __CTASSERT to confirm that the size in
crypt_data is enough -- with a name or reference for where 512 came
from, like the Argon2id paper or API documentation:

__CTASSERT(sizeof(data->__buf) >= ARGON2_MAX_HASH);

   --- lib/libcrypt/crypt-sha1.c   29 Oct 2021 13:22:08 -0000      1.10
   +++ lib/libcrypt/crypt-sha1.c   12 Feb 2022 12:47:04 -0000
   @@ -107,12 +107,12 @@
     * hmac key.
     */
    crypt_private char *
   -__crypt_sha1 (const char *pw, const char *salt)
   +__crypt_sha1 (const char *pw, const char *salt, struct crypt_data *data)
    {
        static const char *magic = SHA1_MAGIC;
        static unsigned char hmac_buf[SHA1_SIZE];
   -    static char passwd[(2 * sizeof(SHA1_MAGIC)) +
   -                      CRYPT_SHA1_SALT_LENGTH + SHA1_SIZE];
   +    /* static char passwd[(2 * sizeof(SHA1_MAGIC)) +
   +                      CRYPT_SHA1_SALT_LENGTH + SHA1_SIZE]; */

Same deal with __CTASSERT here.

   --- lib/libcrypt/crypt.c        22 Feb 2020 10:29:17 -0000      1.38
   +++ lib/libcrypt/crypt.c        12 Feb 2022 12:47:04 -0000
   @@ -466,7 +466,8 @@


    static C_block constdatablock;                 /* encryption constant */
   -static char    cryptresult[1+4+4+11+1];        /* encrypted result */
   +/* static char cryptresult[1+4+4+11+1];*/      /* encrypted result */

delete

   +static struct crypt_data cryptresult;          /* encrypted result */
 
   @@ -594,6 +595,8 @@
           }
           /* End non-DES handling */

   +       explicit_memset(data->__buf, 0, sizeof(data->__buf));

Maybe this should be done first, in crypt_r, before anything uses
data, so we don't have to copy & paste it in all the different
password hash functions?
 
--- lib/libcrypt/crypt.h        16 Oct 2021 10:53:33 -0000      1.8
+++ lib/libcrypt/crypt.h        12 Feb 2022 12:47:04 -0000
@@ -2,17 +2,19 @@
  * $NetBSD: crypt.h,v 1.8 2021/10/16 10:53:33 nia Exp $
  */
 
Not new with your change, but this file should have an include guard:

#define	LIBCRYPT_CRYPT_H
#define	LIBCRYPT_CRYPT_H
...
#endif	/* LIBCRYPT_CRYPT_H */

   -crypt_private char *__md5crypt(const char *, const char *);    /* XXX */
   -crypt_private char *__bcrypt(const char *, const char *);      /* XXX */
   -crypt_private char *__crypt_sha1(const char *, const char *);
   +crypt_private char *__md5crypt(const char *, const char *, struct crypt_data *data);   /* XXX */
   +crypt_private char *__bcrypt(const char *, const char *, struct crypt_data *data);     /* XXX */
   +crypt_private char *__crypt_sha1(const char *, const char *, struct crypt_data *data);

Break lines to fit in 80 columns, with 4-space continuation lines, and
omit the parameter name (see /usr/share/misc/style).  For example:

crypt_private char *__md5crypt(const char *, const char *,
    struct crypt_data *);

Not sure why there's an XXX comment on this line!  Maybe we should
look through the history to find when and why it appeared, and either
delete it, elaborate on it, or fix it.

   --- lib/libcrypt/md5crypt.c     16 Oct 2021 10:53:33 -0000      1.15
   +++ lib/libcrypt/md5crypt.c     12 Feb 2022 12:47:04 -0000
   @@ -37,9 +37,9 @@
     * MD5 password encryption.
     */
    crypt_private char *
   -__md5crypt(const char *pw, const char *salt)
   +__md5crypt(const char *pw, const char *salt, struct crypt_data *data)
    {
   -       static char passwd[120], *p;
   +       char *p;

__CTASSERT