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

[Alexandre Oliva <aoliva at redhat dot com>]

On May 23, 2012, Roland McGrath <roland@hack.frob.com> wrote:

>> Aha!  Ok, then.  The description for ENOSYS under crypt still fit, but
>> given your comment, I'm now going with EINVAL for this case.

> No, ENOSYS does not fit.  The wording for that particular function may
> not be clear, but elsewhere in the standard it's quite clear that
> ENOSYS is for a function that is not available at all--every call will
> always fail with ENOSYS.

I see.

> EINVAL means it's a malformed argument value, i.e. a bug in the
> calling program.

So this must be the errno code for an invalid DES salt.

> Another sensible alternative is EPERM,

I've made this the errno code for disabled algorithms for FIPS
compliance.

>> +	perror ("mmap 2");

> It should be a hard failure, shouldn't it?

I don't think so.  Without the patch, we'd crash if the second mmap
succeeded, and access (zero-)uninitialized memory otherwise, so both
would fail, although in different ways.  With the patch, we'll report
the same error (EINVAL) regardless of the result of this second mmap.
So it doesn't seem useful to abort the test for this reason.

Now, I've arranged for us to return EINVAL over EPERM when either one
would be reasonable, even though doing the opposite would make the code
simpler and faster, because I decided to favor consistency, i.e., that
crypt(passwd, "*") will return EINVAL regardless of FIPS status, for
with "*" we can tell it was *not* supposed to use DES, and it should
return EPERM for DES only.

My only doubt is whether to pick EINVAL or ENOTSUPP when we encounter an
invalid DES salt, for an invalid DES salt might very well be some
extended algorithm that we don't support (or even know about).  I'm
going with EINVAL because it is appropriate in more cases than ENOTSUPP,
for only some of the invalid DES salts are algorthm extensions that we
don't support.

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

> Note that we now use date-of-commit in the log lines, so you'll need
> to update this when it goes in.

Sure.  Any chance the mailing list moderator could refrain from blocking
patches that have âfromâ instead of a date?  (assuming this hasn't
already changed since I last tried to post a cvsdiff-generated patch
file).  Then cvsdiff patches would be accepted without manual handling,
and cl2patch would apply them properly, with the current date.

>> -  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;

> Ideally we'd have a way to test the code regardless of the system
> configuration.  But I don't see a straightforward way off hand.

Me neither.  Exposing any alternate entry point would make room for
security-related abuses.

Now, I must confess I'm surprised this FIPS-related restrictions on
crypt are being seriously considered for glibc.  I'd have thought we'd
privilege POSIX-compliant behavior, pushing FIPS password algorithm
rejection to code that uses crypt for actual password checking or
modification, rather than for any code that calls crypt for whatever
reason (e.g., password crackers).

I've implemented your other suggestions and fixes, thanks!

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

	* crypt/crypt-private.h: Include stdbool.h.
	(_ufc_setup_salt_r): Return bool.
	* crypt/crypt-entry.c: Include errno.h.
	(__crypt_r): Return NULL with EINVAL 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 file.
	* Makefile (tests): Add it.
	($(objpfx)badsalttest): New.

Index: crypt/crypt-private.h
===================================================================
--- crypt/crypt-private.h.orig	2012-06-04 23:08:44.012024598 -0300
+++ crypt/crypt-private.h	2012-06-05 01:16:34.472907994 -0300
@@ -26,6 +26,7 @@
 #define CRYPT_PRIVATE_H	1
 
 #include <features.h>
+#include <stdbool.h>
 
 /* crypt.c */
 extern void _ufc_doit_r (ufc_long itr, struct crypt_data * __restrict __data,
@@ -36,7 +37,7 @@ 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,
+extern bool _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);
Index: crypt/crypt-entry.c
===================================================================
--- crypt/crypt-entry.c.orig	2012-06-04 23:08:43.871035745 -0300
+++ crypt/crypt-entry.c	2012-06-05 02:29:22.972672212 -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))
+    {
+      __set_errno (EINVAL);
+      return NULL;
+    }
 
   /*
    * Setup key schedule
Index: crypt/crypt_util.c
===================================================================
--- crypt/crypt_util.c.orig	2012-06-04 23:08:44.195010129 -0300
+++ crypt/crypt_util.c	2012-06-05 01:50:57.000000000 -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 bool bad_for_salt (char c);
 
 
 /*
@@ -596,23 +597,56 @@ shuffle_sb(k, saltbits)
 #endif
 
 /*
+ * Return false iff C is in the specified alphabet for crypt salt.
+ */
+
+static bool
+bad_for_salt (c)
+     char c;
+{
+  switch (c)
+    {
+    case '0' ... '9':
+    case 'A' ... 'Z':
+    case 'a' ... 'z':
+    case '.': case '/':
+      return false;
+
+    default:
+      return true;
+    }
+}
+
+/*
  * Setup the unit for a new salt
  * Hopefully we'll not see a new salt in each crypt call.
+ * Return false if an unexpected character was found in s[0] or s[1].
  */
 
-void
+bool
 _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 false;
+
+  s1 = s[1];
+  if(bad_for_salt (s1))
+    return false;
+
+  if(s0 == __data->current_salt[0] && s1 == __data->current_salt[1])
+    return true;
+
+  __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 true;
 }
 
 void
Index: crypt/Makefile
===================================================================
--- crypt/Makefile.orig	2012-06-04 23:08:43.630054800 -0300
+++ crypt/Makefile	2012-06-05 01:16:34.772884198 -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-06-05 02:10:54.188799012 -0300
@@ -0,0 +1,86 @@
+/* Test program for bad DES salt detection in crypt.
+   Copyright (C) 2012 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+   Contributed by Alexandre Oliva <aoliva@redhat.com>, 2012.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+#include <stdio.h>
+#include <unistd.h>
+#include <sys/mman.h>
+#include <crypt.h>
+
+static 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 }
+};
+
+static int
+do_test (void)
+{
+  int result = 0;
+  struct crypt_data cd;
+  size_t n = sizeof (tests) / sizeof (*tests);
+  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 == MAP_FAILED)
+    {
+      perror ("mmap");
+      n--;
+    }
+  else
+    {
+      if (mmap (page + pagesize, pagesize, 0,
+		MAP_PRIVATE | MAP_ANON | MAP_FIXED,
+		-1, 0) != page + pagesize)
+	perror ("mmap 2");
+      page[pagesize - 1] = '*';
+      tests[n - 1][1] = &page[pagesize - 1];
+    }
+
+  for (size_t 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;
+}
+
+#define TIMEOUT 5
+#define TEST_FUNCTION do_test ()
+#include "../test-skeleton.c"

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

	* crypt/crypt-entry.c: Include fips-private.h.
	(__crypt_r, __crypt): Disable MD5 and DES if FIPS is enabled.
	* crypt/md5c-test.c (main): Tolerate disabled MD5.
	* sysdeps/unix/sysv/linux/fips-private.h: New file.
	* sysdeps/generic/fips-private.h: New file, dummy fallback.

Index: crypt/crypt-entry.c
===================================================================
--- crypt/crypt-entry.c.orig	2012-06-05 02:29:22.972672212 -0300
+++ crypt/crypt-entry.c	2012-06-05 02:33:29.649072455 -0300
@@ -28,6 +28,7 @@
 #endif
 #include <string.h>
 #include <errno.h>
+#include <fips-private.h>
 
 #ifndef STATIC
 #define STATIC static
@@ -92,8 +93,16 @@ __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)
-    return __md5_crypt_r (key, salt, (char *) data,
-			  sizeof (struct crypt_data));
+    {
+      /* FIPS rules out MD5 password encryption.  */
+      if (fips_enabled_p ())
+	{
+	  __set_errno (EPERM);
+	  return NULL;
+	}
+      return __md5_crypt_r (key, salt, (char *) data,
+			    sizeof (struct crypt_data));
+    }
 
   /* Try to find out whether we have to use SHA256 encryption replacement.  */
   if (strncmp (sha256_salt_prefix, salt, sizeof (sha256_salt_prefix) - 1) == 0)
@@ -115,6 +124,13 @@ __crypt_r (key, salt, data)
       return NULL;
     }
 
+  /* FIPS rules out DES password encryption.  */
+  if (fips_enabled_p ())
+    {
+      __set_errno (EPERM);
+      return NULL;
+    }
+
   /*
    * Setup key schedule
    */
@@ -148,7 +164,9 @@ 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
+      /* Let __crypt_r deal with the error code if FIPS is enabled.  */
+      && !fips_enabled_p ())
     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-06-05 02:29:23.224652191 -0300
+++ crypt/md5c-test.c	2012-06-05 02:29:29.911120891 -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;
 }
Index: sysdeps/generic/fips-private.h
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ sysdeps/generic/fips-private.h	2012-06-05 02:29:29.932119222 -0300
@@ -0,0 +1,37 @@
+/* Dummy implementation of FIPS compliance status test.
+   Copyright (C) 2012 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+   Contributed by Alexandre Oliva <aoliva@redhat.com>, 2012.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+#ifndef _FIPS_PRIVATE_H
+#define _FIPS_PRIVATE_H
+
+#include <stdbool.h>
+
+/* Return true if compliance with the FIPS security standards is
+   enabled.
+
+   This is only relevant within crypt, to tell whether MD5 and DES
+   algorithms should be rejected.  */
+
+static inline bool
+fips_enabled_p (void)
+{
+  return false;
+}
+
+#endif /* _FIPS_PRIVATE_H */
Index: sysdeps/unix/sysv/linux/fips-private.h
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ sysdeps/unix/sysv/linux/fips-private.h	2012-06-05 02:29:29.955117395 -0300
@@ -0,0 +1,74 @@
+/* FIPS compliance status test for GNU/Linux systems.
+   Copyright (C) 2012 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+   Contributed by Alexandre Oliva <aoliva@redhat.com>, 2012.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+#ifndef _FIPS_PRIVATE_H
+#define _FIPS_PRIVATE_H
+
+#include <errno.h>
+#include <fcntl.h>
+#include <stdlib.h>
+#include <unistd.h>
+#include <not-cancel.h>
+#include <stdbool.h>
+
+/* Return true if FIPS mode is enabled.  See
+   sysdeps/generic/fips-private.h for more information.  */
+
+static inline bool
+fips_enabled_p (void)
+{
+  static enum {
+    FIPS_UNTESTED = 0,
+    FIPS_ENABLED = 1,
+    FIPS_DISABLED = -1,
+    FIPS_TEST_FAILED = -2
+  } checked;
+
+  if (checked == FIPS_UNTESTED)
+    {
+      int fd = open_not_cancel_2 ("/proc/sys/crypto/fips_enabled", O_RDONLY);
+
+      if (fd != -1)
+	{
+	  /* This is more than enough, the file contains a single integer.  */
+	  char buf[32];
+	  ssize_t n;
+	  n = TEMP_FAILURE_RETRY (read_not_cancel (fd, buf, sizeof (buf) - 1));
+	  close_not_cancel_no_status (fd);
+
+	  if (n > 0)
+	    {
+	      /* Terminate the string.  */
+	      buf[n] = '\0';
+
+	      char *endp;
+	      long int res = strtol (buf, &endp, 10);
+	      if (endp != buf && (*endp == '\0' || *endp == '\n'))
+		checked = (res > 0) ? FIPS_ENABLED : FIPS_DISABLED;
+	    }
+	}
+
+      if (checked == FIPS_UNTESTED)
+	checked = FIPS_TEST_FAILED;
+    }
+
+  return checked > FIPS_UNTESTED;
+}
+
+#endif /* _FIPS_PRIVATE_H */


-- 
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