[RFC] FIPS compliance and other crypt(3) improvements

[Alexandre Oliva <aoliva at redhat dot com>]

We behave in somewhat unexpected ways when presented with semi-standard
arguments for crypt.  Namely, there are various specified extensions to
the salt parameter that we do not implement.  That's fine.  The problem
is that it is not entirely clear that we do not implement them, because
we accept and use salts such as "\0/" or "$", and use them in ways that
not only fail to match documented extensions such as for "$2$", but that
also deviate from the POSIX-specified behavior, in that we perturb the
DES encryption using out-of-spec characters, that don't match
[0-9A-Za-f/.], and that might even be NUL.

This is what the first attached patch fixes: if we're about to fallback
to DES, we check that the salt starts with two characters (and not less
than that) that belong to the specified alphabet, and return NULL if it
doesn't, rather than encrypting the password with DES using out-of-spec
salt.

One of the consequences of our being sloppy with the checking is that we
may access the salt string past its end, which may cause unpredictable
encryption and out-of-spec return values when salt is an empty string.
We may even segfault if the salt happens to be an empty string right
before an unmapped page.  As a matter of good practice, we shouldn't
access the second character of a string before checking that the first
is not NUL.  This is also fixed by the first patch, as the added
testcase shows.


The second building block is support for telling whether the system is
running in a FIPS-compliant mode.  I'll be the first to confess I'm not
familiar with FIPS security specifications, or how that affects the
overall system behavior, but I'm told both the kernel and some userland
behavior is to be affected by the choice of wehther or not to operate in
FIPS mode.  E.g., the kernel can be told at boot time to operate in this
mode, and applications that ought to be affected currently have to check
/proc/sys/crypto/fips_enabled.

It turns out that GNU/Linux-specific sysconf is already perfectly
capable of opening a file in /proc and parsing a number in there, so I
figured I might as well expose the FIPS setting through sysconf.  This
is what the second patch does.


The third patch, that depends on the other 2, introduces FIPS-specific
logic in crypt(3), disabling DES and MD5 algorithms that, per FIPS, must
not be used because they're too weak.  I'm not convinced this is the
best path to follow, for we'd be explicitly deviating from POSIX.

Programs that relied on e.g. the DES algorithm implementation in
crypt(3), for purposes other than checking login passwords (e.g. for
cracking them, or, who knows, reducing other algorithms to DES- or
MD5-based crypt) would break, even though no security issue is at hand.
I'm of the opinion that a POSIX- and FIPS-compliant implementation ought
to audit crypt() callers, assess which of them are actually dealing with
login passwords, and disable those.  However, for completeness, I'm
supplying the third patch, that implements this change IN GLIBC, so
that, if others think this is the way to go, the patch is written and
tested.


I'd appreciate patch reviews for these patches; if there are objections
to any of them, please let me know!  Thanks in advance,


for  ChangeLog
2012-05-15  Alexandre Oliva  <aoliva@redhat.com>

	* crypt/crypt-private.h (_ufc_setup_salt_r): Return int.
	* crypt/crypt-entry.c: Include errno.h.
	(__crypt_r): Return NULL with ENOSYS for bad salt.
	* crypt/crypt_util.c (bad_for_salt): New.
	(_ufc_setup_salt_r): Check that salt is long enough and within
	the specified alphabet.
	* crypt/badsalttest.c: New.
	* Makefile (tests): Add it.
	($(objpfx)badsalttest): New.

Index: crypt/crypt-private.h
===================================================================
--- crypt/crypt-private.h.orig	2012-05-15 04:32:37.535596006 -0300
+++ crypt/crypt-private.h	2012-05-15 05:47:07.701786291 -0300
@@ -36,8 +36,8 @@ extern void _ufc_doit_r (ufc_long itr, s
 extern void __init_des_r (struct crypt_data * __restrict __data);
 extern void __init_des (void);
 
-extern void _ufc_setup_salt_r (const char *s,
-			       struct crypt_data * __restrict __data);
+extern int _ufc_setup_salt_r (const char *s,
+			      struct crypt_data * __restrict __data);
 extern void _ufc_mk_keytab_r (const char *key,
 			      struct crypt_data * __restrict __data);
 extern void _ufc_dofinalperm_r (ufc_long *res,
Index: crypt/crypt-entry.c
===================================================================
--- crypt/crypt-entry.c.orig	2012-05-15 04:32:37.664594140 -0300
+++ crypt/crypt-entry.c	2012-05-15 06:11:53.689651729 -0300
@@ -27,6 +27,7 @@
 #include <stdio.h>
 #endif
 #include <string.h>
+#include <errno.h>
 
 #ifndef STATIC
 #define STATIC static
@@ -108,7 +109,11 @@ __crypt_r (key, salt, data)
   /*
    * Hack DES tables according to salt
    */
-  _ufc_setup_salt_r (salt, data);
+  if (_ufc_setup_salt_r (salt, data) != 0)
+    {
+      __set_errno (ENOSYS);
+      return NULL;
+    }
 
   /*
    * Setup key schedule
Index: crypt/crypt_util.c
===================================================================
--- crypt/crypt_util.c.orig	2012-05-15 04:32:37.782592432 -0300
+++ crypt/crypt_util.c	2012-05-15 05:47:07.870783842 -0300
@@ -57,6 +57,7 @@ STATIC void shuffle_sb (long32 *k, ufc_l
 #else
 STATIC void shuffle_sb (long64 *k, ufc_long saltbits);
 #endif
+static int bad_for_salt (char c);
 
 
 /*
@@ -596,23 +597,56 @@ shuffle_sb(k, saltbits)
 #endif
 
 /*
+ * Return zero iff C is in the specified alphabet for crypt salt.
+ */
+
+static int
+bad_for_salt (c)
+     char c;
+{
+  switch (c)
+    {
+    case '0' ... '9':
+    case 'A' ... 'Z':
+    case 'a' ... 'z':
+    case '.': case '/':
+      return 0;
+
+    default:
+      return -1;
+    }
+}
+
+/*
  * Setup the unit for a new salt
  * Hopefully we'll not see a new salt in each crypt call.
+ * Return nonzero if an unexpected character was found in s[0] or s[1].
  */
 
-void
+int
 _ufc_setup_salt_r(s, __data)
      const char *s;
      struct crypt_data * __restrict __data;
 {
   ufc_long i, j, saltbits;
+  char s0, s1;
 
   if(__data->initialized == 0)
     __init_des_r(__data);
 
-  if(s[0] == __data->current_salt[0] && s[1] == __data->current_salt[1])
-    return;
-  __data->current_salt[0] = s[0]; __data->current_salt[1] = s[1];
+  s0 = s[0];
+  if(bad_for_salt (s0))
+    return -1;
+
+  s1 = s[1];
+  if(bad_for_salt (s1))
+    return -1;
+
+  if(s0 != __data->current_salt[0] && s1 == __data->current_salt[1])
+    return 0;
+
+  __data->current_salt[0] = s0;
+  __data->current_salt[1] = s1;
 
   /*
    * This is the only crypt change to DES:
@@ -646,6 +680,8 @@ _ufc_setup_salt_r(s, __data)
   shuffle_sb((LONGG)__data->sb3, __data->current_saltbits ^ saltbits);
 
   __data->current_saltbits = saltbits;
+
+  return 0;
 }
 
 void
Index: crypt/Makefile
===================================================================
--- crypt/Makefile.orig	2012-05-15 04:32:37.928590320 -0300
+++ crypt/Makefile	2012-05-15 05:47:07.902783379 -0300
@@ -44,11 +44,12 @@ LDLIBS-crypt.so = -lfreebl3
 else
 libcrypt-routines += md5 sha256 sha512
 
-tests += md5test sha256test sha512test
+tests += md5test sha256test sha512test badsalttest
 
 $(objpfx)md5test: $(objpfx)md5.o
 $(objpfx)sha256test: $(objpfx)sha256.o
 $(objpfx)sha512test: $(objpfx)sha512.o
+$(objpfx)badsalttest: $(objpfx)badsalttest.o
 endif
 
 include ../Rules
Index: crypt/badsalttest.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ crypt/badsalttest.c	2012-05-15 05:47:07.917783162 -0300
@@ -0,0 +1,64 @@
+#include <stdio.h>
+#include <unistd.h>
+#include <sys/mman.h>
+#include <crypt.h>
+
+const char *tests[][2] = {
+  { "no salt", "" },
+  { "single char", "/" },
+  { "first char bad", "!x" },
+  { "second char bad", "Z%" },
+  { "both chars bad", ":@" },
+  { "un$upported algorithm", "$2$" },
+  { "unsupported_algorithm", "_1" },
+  { "end of page", NULL }
+};
+
+int
+main (int argc, char *argv[])
+{
+  int result = 0;
+  int i, n = sizeof (tests) / sizeof (*tests);
+  struct crypt_data cd;
+  size_t pagesize = (size_t) sysconf (_SC_PAGESIZE);
+  char *page;
+
+  /* Check that crypt won't look at the second character if the first
+     one is invalid.  */
+  page = mmap (NULL, pagesize * 2, PROT_READ | PROT_WRITE,
+	       MAP_PRIVATE | MAP_ANON, -1, 0);
+  if (page == (void*)-1)
+    {
+      perror ("mmap");
+      n--;
+    }
+  else
+    {
+      if (munmap (page + pagesize, pagesize))
+	perror ("munmap");
+      if (mmap (page + pagesize, pagesize, 0, MAP_PRIVATE | MAP_ANON,
+		-1, 0) != page + pagesize)
+	perror ("mmap 2");
+      page[pagesize - 1] = '*';
+      tests[n - 1][1] = &page[pagesize - 1];
+    }
+
+  for (i = 0; i < n; i++)
+    {
+      if (crypt (tests[i][0], tests[i][1]))
+	{
+	  result++;
+	  printf ("%s: crypt returned non-NULL with salt \"%s\"\n",
+		  tests[i][0], tests[i][1]);
+	}
+
+      if (crypt_r (tests[i][0], tests[i][1], &cd))
+	{
+	  result++;
+	  printf ("%s: crypt_r returned non-NULL with salt \"%s\"\n",
+		  tests[i][0], tests[i][1]);
+	}
+    }
+
+  return result;
+}

for  ChangeLog
2012-05-15  Alexandre Oliva  <aoliva@redhat.com>

	* bits/confname.h (_SC_CRYPTO_FIPS_ENABLED): New.
	* sysdeps/unix/sysv/linux/sysconf.c (__sysconf): Support it.

Index: bits/confname.h
===================================================================
--- bits/confname.h.orig	2012-05-15 04:31:06.434914368 -0300
+++ bits/confname.h	2012-05-15 04:32:22.858808416 -0300
@@ -526,8 +526,10 @@ enum
 
     _SC_THREAD_ROBUST_PRIO_INHERIT,
 #define _SC_THREAD_ROBUST_PRIO_INHERIT	_SC_THREAD_ROBUST_PRIO_INHERIT
-    _SC_THREAD_ROBUST_PRIO_PROTECT
+    _SC_THREAD_ROBUST_PRIO_PROTECT,
 #define _SC_THREAD_ROBUST_PRIO_PROTECT	_SC_THREAD_ROBUST_PRIO_PROTECT
+    _SC_CRYPTO_FIPS_ENABLED
+#define _SC_CRYPTO_FIPS_ENABLED		_SC_CRYPTO_FIPS_ENABLED
   };
 
 /* Values for the NAME argument to `confstr'.  */
Index: sysdeps/unix/sysv/linux/sysconf.c
===================================================================
--- sysdeps/unix/sysv/linux/sysconf.c.orig	2012-05-15 04:31:06.309916177 -0300
+++ sysdeps/unix/sysv/linux/sysconf.c	2012-05-15 04:32:22.908807693 -0300
@@ -114,6 +114,12 @@ __sysconf (int name)
       procfname = "/proc/sys/kernel/rtsig-max";
       break;
 
+#ifdef _SC_CRYPTO_FIPS_ENABLED
+    case _SC_CRYPTO_FIPS_ENABLED:
+      procfname = "/proc/sys/crypto/fips_enabled";
+      break;
+#endif
+
     default:
       break;
     }

for  ChangeLog
2012-05-15  Alexandre Oliva  <aoliva@redhat.com>

	* crypt/crypt-entry.c: Include unistd.h.
	(fips_enabled): New.
	(__crypt_r, __crypt): Disable MD5 and DES if FIPS is enabled.
	* crypt/md5c-test.c (main): Tolerate disabled MD5.

Index: crypt/crypt-entry.c
===================================================================
--- crypt/crypt-entry.c.orig	2012-05-15 06:09:37.390412983 -0300
+++ crypt/crypt-entry.c	2012-05-15 06:09:40.429373764 -0300
@@ -28,6 +28,7 @@
 #endif
 #include <string.h>
 #include <errno.h>
+#include <unistd.h>
 
 #ifndef STATIC
 #define STATIC static
@@ -75,6 +76,27 @@ static const char sha512_salt_prefix[] =
 /* For use by the old, non-reentrant routines (crypt/encrypt/setkey)  */
 extern struct crypt_data _ufc_foobar;
 
+/* Return nonzero if FIPS mode is enabled, i.e., whether MD5 and DES
+   algorithms should be rejected.  */
+static inline int
+fips_enabled (void)
+{
+  static int checked;
+
+  if (!checked)
+    {
+      int res = -1;
+
+#ifdef _SC_CRYPTO_FIPS_ENABLED
+      res = __sysconf (_SC_CRYPTO_FIPS_ENABLED);
+#endif
+
+      checked = (res > 0) ? 1 : -1;
+    }
+
+  return checked > 0;
+}
+
 /*
  * UNIX crypt function
  */
@@ -91,7 +113,8 @@ __crypt_r (key, salt, data)
 
 #ifdef _LIBC
   /* Try to find out whether we have to use MD5 encryption replacement.  */
-  if (strncmp (md5_salt_prefix, salt, sizeof (md5_salt_prefix) - 1) == 0)
+  if (strncmp (md5_salt_prefix, salt, sizeof (md5_salt_prefix) - 1) == 0
+      && !fips_enabled ())
     return __md5_crypt_r (key, salt, (char *) data,
 			  sizeof (struct crypt_data));
 
@@ -109,7 +132,7 @@ __crypt_r (key, salt, data)
   /*
    * Hack DES tables according to salt
    */
-  if (_ufc_setup_salt_r (salt, data) != 0)
+  if (fips_enabled () || _ufc_setup_salt_r (salt, data) != 0)
     {
       __set_errno (ENOSYS);
       return NULL;
@@ -148,7 +171,8 @@ crypt (key, salt)
 {
 #ifdef _LIBC
   /* Try to find out whether we have to use MD5 encryption replacement.  */
-  if (strncmp (md5_salt_prefix, salt, sizeof (md5_salt_prefix) - 1) == 0)
+  if (strncmp (md5_salt_prefix, salt, sizeof (md5_salt_prefix) - 1) == 0
+      && !fips_enabled ())
     return __md5_crypt (key, salt);
 
   /* Try to find out whether we have to use SHA256 encryption replacement.  */
Index: crypt/md5c-test.c
===================================================================
--- crypt/md5c-test.c.orig	2012-05-15 05:52:24.296198551 -0300
+++ crypt/md5c-test.c	2012-05-15 05:52:46.588875435 -0300
@@ -9,7 +9,10 @@ main (int argc, char *argv[])
   int result = 0;
 
   cp = crypt ("Hello world!", salt);
-  result |= strcmp ("$1$saltstri$YMyguxXMBpd2TEZ.vS/3q1", cp);
+
+  /* MD5 is disabled in FIPS mode.  */
+  if (cp)
+    result |= strcmp ("$1$saltstri$YMyguxXMBpd2TEZ.vS/3q1", cp);
 
   return result;
 }

-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist      Red Hat Brazil Compiler Engineer