Improve the crypt() builtin

.editorconfig

@@ -22,6 +22,12 @@ indent_style = tab
 indent_style = space
 indent_size = 2
 
+# Settings for autoconf.
+[*.ac]
+# Indent with two spaces.
+indent_style = space
+indent_size = 2
+
 # Settings for C files.
 [*.{c,h}]
 # Indent with four spaces.

.gitignore

@@ -12,6 +12,7 @@ Makefile
 Makefile.in
 aclocal.m4
 autom4te.cache/
+build-aux/
 compile
 config.h
 config.h.in

README.md

@@ -8,6 +8,9 @@ Source code for the HellCore project, a fork of LambdaMOO.
 * bison
 * gcc
 * gperf
+* libcrypt (any compliant implementation, libxcrypt is a good option if your
+    glibc no longer supplies a libcrypt and you're on a distro/in an environment
+    that for some reason hasn't already added a replacement)
 * libstdc++
 * make
 * sed
@@ -55,6 +58,49 @@ run
 ./autogen.sh && ./configure && make
 ```
 
+### Troubleshooting
+
+If you're having trouble building after updating this repository, parts of the
+build system may be out of date.
+
+Before trying to build again, try running
+```shell
+./autogen.sh && make distclean
+```
+
+### Optional Features
+
+A few features can optionally be enabled/disabled when compiling.
+
+If you use `./build.sh`, you should get a reasonable set of defaults.
+
+#### More Secure Password Hashes
+
+If you have a more modern libcrypt implementation, such as libxcrypt, and it
+provides `crypt_gensalt`, the moo will use this to generate more secure and
+modern password hashes.
+
+This means that if your database uses the `crypt()` builtin to store passwords,
+it will be able to use a format which is far more secure, as the old password
+format can be be cracked fairly quickly on modern computers if someone can
+manage to get a copy of your database.
+
+This should generally be fairly backwards-compatible with older databases, and
+old password hashes will still be read fine after this change, but new passwords
+hashed will be more secure.
+
+The new password hashes won't be properly understood by older versions of the
+`crypt()` builtin, and may not be understood by versions of the moo built
+without this feature, however, so if you want to disable the more secure hashes
+to ensure that your database can be used with older versions of the moo binary,
+you can pass `--disable-crypt-gensalt` to `./configure` when building.
+
+Note that if your distro has built your libcrypt provider without support for
+old, insecure hashes, regardless of whether you have this feature enabled or
+not, you may need to, e.g., install via your package manager a build of
+libxcrypt which has support for insecure hashes enabled in order to authenticate
+against password hashes from older databases.
+
 ### USE AT YOUR OWN RISK. I DENY RESPONSIBILITY FOR:
 * Spontaneous hairy nose
 * Micropenis

autogen.sh

@@ -1,6 +1,20 @@
 #!/bin/sh
 
+# Die on failures.
+set -e
+
 # Regenerate the build system.
 # You probably shouldn't need to modify this.
 
+# Make sure we're in the project directory.
+cd "$(dirname "$0")"
+
+# Make sure we've got an m4 macro directory.
+mkdir -p m4 build-aux
+
+# If we use macros available on the system, copy them into our m4 directory if
+# we don't have them, or if the system has a newer version.
+aclocal --install
+
+# Regenerate our build system.
 autoreconf --install --force

configure.ac

@@ -19,11 +19,20 @@ AC_INIT([HellCore],
         [https://github.com/diatomic-ge/HellCore])
 
 
+# Put extra build tools in the build-aux directory to keep things tidy.
+AC_CONFIG_AUX_DIR([build-aux])
+
+
 # Initialize automake.
 # foreign:      Tell automake not to expect the standard GNU files (NEWS,
 #               AUTHORS, etc.), and not to error when it doesn't find them.
 AM_INIT_AUTOMAKE([foreign])
 
+
+# Keep all our autoconf macros in the m4 directory.
+AC_CONFIG_MACRO_DIRS([m4])
+
+
 # By default, just show what's being compiled while building, rather than the
 # whole command.
 # (This can be overridden by passing --disable-silent-rules to configure, or by
@@ -81,8 +90,68 @@ AC_PROG_SED
 AC_SEARCH_LIBS([atan2], [m])
 
 
+# Figure out what crypt we have.
+
+# Find a crypt.h.
+AC_CHECK_HEADERS([crypt.h])
+
 # Link the crypt library.
-AC_SEARCH_LIBS([crypt], [crypt])
+# If we don't find a valid crypt() implementation, be loud and give up.
+# We can technically use the no-op crypt that's just duplicating the string, but
+# failing to an insecure mode without making the user explicitly request/allow
+# it feels wrong.
+AC_SEARCH_LIBS([crypt],
+               [crypt xcrypt],
+               [],
+               [AC_MSG_ERROR([cannot find a crypt implementation])])
+
+# Give a way to disable crypt_gensalt usage, in case someone wants to stick with
+# the old DES crypt.
+AC_ARG_ENABLE([crypt-gensalt],
+              [AS_HELP_STRING([--disable-crypt-gensalt],
+                              [disable support for newer crypt algorithms])],
+              [],
+              [enable_crypt_gensalt=default])
+
+# Check if we have the more modern helper function to generate a salt.
+# If we do, we can get the benefits of whatever the best algorithm our crypt
+# implementation has, without needing to know what algorithm it is or how we
+# should generate a salt.
+# If we don't find it, we fall back on the less secure DES crypt().
+AC_SEARCH_LIBS([crypt_gensalt],
+               [crypt xcrypt],
+               [crypt_gensalt_available=yes],
+               [crypt_gensalt_available=no])
+
+# Validate our crypt_gensalt setup.
+if test "x$enable_crypt_gensalt" = "xdefault"; then
+  # We should try to use crypt_gensalt, but it's only a warning if we can't.
+  use_crypt_gensalt="$crypt_gensalt_available"
+  if test "x$crypt_gensalt_available" = "xno"; then
+    # Warn that we could be using more secure salts.
+    AC_MSG_WARN([using insecure DES crypt salts as crypt_gensalt is unavailable])
+  fi
+elif test "x$enable_crypt_gensalt" = "xyes"; then
+  # They explicitly requested support, so die if we didn't find it.
+  if test "x$crypt_gensalt_available" = "xno"; then
+    AC_MSG_ERROR([crypt_gensalt is unavailable but newer hash salts were requested])
+  fi
+  use_crypt_gensalt=yes
+elif test "x$enable_crypt_gensalt" = "xno"; then
+  # Stick with the legacy salts.
+  # We don't need to warn about crypt_gensalt being unavailable, since we were
+  # asked to exclude it anyway.
+  use_crypt_gensalt=no
+else
+  AC_MSG_ERROR([unrecognized value '$enable_crypt_gensalt' for --enable-crypt-gensalt])
+fi
+
+# If we should use crypt_gensalt, let our code know.
+if test "x$use_crypt_gensalt" = "xyes"; then
+AC_DEFINE([USE_CRYPT_GENSALT],
+          1,
+          [Define to 1 if crypt_gensalt should be used.])
+fi
 
 
 # Generate the configure script.

flake.nix

@@ -65,6 +65,13 @@
 
             # The gperf perfect hash generator for recognizing keywords.
             gperf
+
+            # A more modern libcrypt.
+            # This is necessary, since libcrypt might be being phased out of
+            # glibc, and we won't get a libcrypt for free from stdenv.
+            # We enable all hashes, so that databases with old DES hashes don't
+            # break.
+            (libxcrypt.override { enableHashes = "all"; })
           ];
 
           # Extra flags to pass to ./configure.

src/list.c

@@ -15,6 +15,8 @@
     [email protected]
  *****************************************************************************/
 
+#include <config.h>
+
 #include "my-ctype.h"
 #include "my-string.h"
 
@@ -38,6 +40,12 @@
 #include "utils.h"
 #include "hash_lookup.h"
 
+#include <errno.h>
+
+#if HAVE_CRYPT_H
+#include <crypt.h>
+#endif
+
 #define TRY_REALLOC_TRICKS 1
 
 Var
@@ -629,33 +637,96 @@ bf_strsub(Var arglist, Byte next, void *vdata, Objid progr)
     }
 }
 
+/*
+ * Generate a DES crypt salt.
+ */
+#if !USE_CRYPT_GENSALT
+static char*
+make_des_crypt_salt()
+{
+    static char salt[3];
+    static char saltstuff[] =
+        "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789./";
+
+    salt[0] = saltstuff[RANDOM() % (int) strlen(saltstuff)];
+    salt[1] = saltstuff[RANDOM() % (int) strlen(saltstuff)];
+    salt[2] = '\0';
+
+    return salt;
+}
+#endif
+
 static package
 bf_crypt(Var arglist, Byte next, void *vdata, Objid progr)
 {				/* (string, [salt]) */
     Var r;
 
-#if HAVE_CRYPT
-    char salt[3];
-    static char saltstuff[] =
-    "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789./";
-    extern const char *crypt(const char *, const char *);
+    const char* salt;
+    const char* hash;
+    int generated_salt = 0;
 
     if (arglist.v.list[0].v.num == 1 || strlen(arglist.v.list[2].v.str) < 2) {
-	salt[0] = saltstuff[RANDOM() % (int) strlen(saltstuff)];
-	salt[1] = saltstuff[RANDOM() % (int) strlen(saltstuff)];
+        /* Generate a salt. */
+        generated_salt = 1;
+#if USE_CRYPT_GENSALT
+        /*
+         * Ask crypt_gensalt for a salt for the best hash with default cost.
+         * We are explicitly allowed to give this pointer to crypt and not worry
+         * about freeing or anything.
+         */
+        salt = crypt_gensalt(NULL, 0, NULL, 0);
+
+        if (salt == NULL) {
+            /* Log whyever we failed to generate a salt and raise an error. */
+            log_perror("Failed to generate crypt salt");
+            free_var(arglist);
+            return make_raise_pack(E_QUOTA, "Failed to generate a crypt() salt", zero);
+        }
+#else
+        /* Use a legacy salt. */
+        salt = make_des_crypt_salt();
+#endif
     } else {
-	salt[0] = arglist.v.list[2].v.str[0];
-	salt[1] = arglist.v.list[2].v.str[1];
+        /* Use our second argument as a salt. */
+        salt = arglist.v.list[2].v.str;
     }
-    salt[2] = '\0';
-    r.type = TYPE_STR;
-    r.v.str = str_dup(crypt(arglist.v.list[1].v.str, salt));
-#else				/* !HAVE_CRYPT */
-    r.type = TYPE_STR;
-    r.v.str = str_ref(arglist.v.list[1].v.str);
-#endif
 
+    /* Hash our input. */
+    hash = crypt(arglist.v.list[1].v.str, salt);
+
+    /* Free our arguments since we don't need them later. */
     free_var(arglist);
+
+    if (hash == NULL) {
+        /* Crypt failed for some reason. */
+        if (errno == EINVAL) {
+            /* We had an invalid salt. */
+            if (generated_salt) {
+                /* Log a bit more specifically. */
+                log_perror("crypt rejected generated salt");
+            } else {
+                /*
+                 * Don't log user-supplied salts being wrong, but do tell the
+                 * user that explicitly since they were the one to supply it.
+                 */
+                return make_raise_pack(E_INVARG, "Invalid crypt() salt", zero);
+            }
+        } else {
+            /* Log whatever other error this is. */
+            log_perror("crypt failed");
+        }
+        /* Report the error. */
+        return make_raise_pack(E_QUOTA, "Call to crypt() failed", zero);
+    } else if (hash[0] == '*') {
+        /* For an unspecified reason, hashing failed. */
+        errlog("crypt returned an invalid/error hash starting with *");
+        return make_raise_pack(E_QUOTA, "Call to crypt() failed", zero);
+    }
+
+    /* Build up our return object. */
+    r.type = TYPE_STR;
+    r.v.str = str_dup(hash);
+
     return make_var_pack(r);
 }
 

src/oldconfig.h

@@ -166,7 +166,6 @@
  * system provides the named functions.
  */
 
-#define HAVE_CRYPT 1
 #define HAVE_MATHERR 1
 #define HAVE_MKFIFO 1
 #define HAVE_REMOVE 1