This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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

>> On May 15, 2012, Roland McGrath <roland@hack.frob.com> wrote:
>> 
>> > ENOSYS is the error code for a function that is entirely unimplemented.
>> 
>> It's the only POSIX-documented error code for crypt.  That's why I went
>> with it.

> It's a general part of POSIX that functions can return different errno
> codes than the ones listed.

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

> I'm not at all convinced that sysconf is the right place for such an
> extension.

Okiedokie, I've dropped the sysconf change, and implemented
fips_enabled_p using identical logic in a new fips-private.h header in
sysdeps.

I also used bool rather than int for newly-introduced return types.

Here are the two revised patches, down from three, because I'm not
longer changing sysconf.  How are these?

for  ChangeLog
2012-05-15  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.
	* Makefile (tests): Add it.
	($(objpfx)badsalttest): New.

Index: crypt/crypt-private.h
===================================================================
--- crypt/crypt-private.h.orig	2012-05-23 06:56:12.618445828 -0300
+++ crypt/crypt-private.h	2012-05-23 07:03:39.675270309 -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-05-23 06:56:12.618445828 -0300
+++ crypt/crypt-entry.c	2012-05-23 07:04:17.614250905 -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-05-23 06:56:12.619445821 -0300
+++ crypt/crypt_util.c	2012-05-23 06:56:15.004430817 -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 zero 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-05-23 06:56:12.619445821 -0300
+++ crypt/Makefile	2012-05-23 06:56:15.017430735 -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-23 06:56:15.028430665 -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>

	* 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.
	* sysdeps/generic/fips-private.h: New, dummy fallback.

Index: crypt/crypt-entry.c
===================================================================
--- crypt/crypt-entry.c.orig	2012-05-23 07:04:17.614250905 -0300
+++ crypt/crypt-entry.c	2012-05-23 07:52:21.954465503 -0300
@@ -28,6 +28,7 @@
 #endif
 #include <string.h>
 #include <errno.h>
+#include <fips-private.h>
 
 #ifndef STATIC
 #define STATIC static
@@ -91,7 +92,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_p ())
     return __md5_crypt_r (key, salt, (char *) data,
 			  sizeof (struct crypt_data));
 
@@ -109,7 +111,7 @@ __crypt_r (key, salt, data)
   /*
    * Hack DES tables according to salt
    */
-  if (!_ufc_setup_salt_r (salt, data))
+  if (fips_enabled_p () || !_ufc_setup_salt_r (salt, data))
     {
       __set_errno (EINVAL);
       return NULL;
@@ -148,7 +150,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_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-05-23 07:04:17.775250807 -0300
+++ crypt/md5c-test.c	2012-05-23 07:04:23.869246982 -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-05-23 07:48:08.214924392 -0300
@@ -0,0 +1,12 @@
+#ifndef _FIPS_PRIVATE_H
+#define _FIPS_PRIVATE_H
+
+#include <stdbool.h>
+
+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-05-23 07:51:25.170585521 -0300
@@ -0,0 +1,51 @@
+#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.
+
+   This is only relevant within crypt, to tell whether MD5 and DES
+   algorithms should be rejected.  */
+static inline bool
+fips_enabled_p (void)
+{
+  static int checked;
+
+  if (!checked)
+    {
+      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) ? 1 : -1;
+	    }
+	}
+
+      if (!checked)
+	checked = -2;
+    }
+
+  return checked > 0;
+}
+
+#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

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]