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

classic Classic list List threaded Threaded
16 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Alexandre Oliva-2
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  <[hidden email]>

        * 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  <[hidden email]>

        * 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  <[hidden email]>

        * 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
Reply | Threaded
Open this post in threaded view
|

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

Roland McGrath-4
ENOSYS is the error code for a function that is entirely unimplemented.
For this case, ENOTSUP is a better fit.

-  if(s[0] == __data->current_salt[0] && s[1] == __data->current_salt[1])
-    return;
+  if(s0 != __data->current_salt[0] && s1 == __data->current_salt[1])
+    return 0;

Looks like the first test got inverted.

Is there any standard or precedent for _SC_CRYPTO_FIPS_ENABLED?
We should not take lightly adding this to the public API/ABI.

If the only need for it is an internal one, then the check can be done
using internal functions only.


Thanks,
Roland
Reply | Threaded
Open this post in threaded view
|

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

Alexandre Oliva-2
On May 15, 2012, Roland McGrath <[hidden email]> 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.

> For this case, ENOTSUP is a better fit.

> -  if(s[0] == __data->current_salt[0] && s[1] == __data->current_salt[1])
> -    return;
> +  if(s0 != __data->current_salt[0] && s1 == __data->current_salt[1])
> +    return 0;

> Looks like the first test got inverted.

Eeek!  Thanks, fixed.

> Is there any standard or precedent for _SC_CRYPTO_FIPS_ENABLED?

Nope.  I came up with it myself.  I thought of adding _GNU_ in there
somewhere, and bumping the number way up, as an extension without
conflicts, but I didn't get that far.

> If the only need for it is an internal one

Other userland programs and libraries test FIPS status reading /proc
files directly, but I though they (and any newer programs) could switch
to a more portable interface.

--
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
Reply | Threaded
Open this post in threaded view
|

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

Roland McGrath-4
> On May 15, 2012, Roland McGrath <[hidden email]> 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.  It's only required that for the failures
that are described in standard, you return exactly those error codes.
This is a failure mode not specified by the standard, so returning a
different errno value for it is entirely kosher.

> > Is there any standard or precedent for _SC_CRYPTO_FIPS_ENABLED?
>
> Nope.  I came up with it myself.  I thought of adding _GNU_ in there
> somewhere, and bumping the number way up, as an extension without
> conflicts, but I didn't get that far.

If it's nonstandard then it certainly should have a name that makes that
clear.  But I'm not at all convinced that sysconf is the right place for
such an extension.

> > If the only need for it is an internal one
>
> Other userland programs and libraries test FIPS status reading /proc
> files directly, but I though they (and any newer programs) could switch
> to a more portable interface.

That is an entirely reasonable notion.  But don't conflate it with the
implementation of the crypt change.  Have crypt use an internal function
that is not exported.  Separately and later, we can consider providing
a public interface for FIPS status, but that is an entirely different
conversation (and one that I'm in no hurry to have).


Thanks,
Roland

Reply | Threaded
Open this post in threaded view
|

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

Alexandre Oliva-2
On May 18, 2012, Roland McGrath <[hidden email]> wrote:

>> On May 15, 2012, Roland McGrath <[hidden email]> 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  <[hidden email]>

        * 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  <[hidden email]>

        * 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
Reply | Threaded
Open this post in threaded view
|

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

Roland McGrath-4
> 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.  ENOTSUP is specifically for when a function
is available but the arguments to the function request a particular
detail (flag, mode, etc.) that is optional and is not available.

Since in this case it's a feature that is not specified by POSIX, we
are free to use any error code we like except ENOSYS.  I think ENOTSUP
is a better match for this case: the argument has a specific meaning
that we understand, but we are not supporting that mode.  EINVAL means
it's a malformed argument value, i.e. a bug in the calling program.
Another sensible alternative is EPERM, since it's a system-wide
security policy that is denying access to the functionality.

>  /*
> + * Return zero iff C is in the specified alphabet for crypt salt.
> + */

s/zero/false/

> +/*
>   * 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].
>   */

s/FALSE/false/

> +  int i, n = sizeof (tests) / sizeof (*tests);

Use size_t.  Don't declare I here at all.  Just use C99 style below.

> +  /* 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)

Use MAP_FAILED.

> +      if (munmap (page + pagesize, pagesize))
> + perror ("munmap");

munmap is not required.

> +      if (mmap (page + pagesize, pagesize, 0, MAP_PRIVATE | MAP_ANON,
> + -1, 0) != page + pagesize)

This requires MAP_FIXED.  Without it, the address is just a suggestion.

> + perror ("mmap 2");

It should be a hard failure, shouldn't it?
So return here, or use 'error'.

Use test-skeleton.c for the test program.

> for  ChangeLog
> 2012-05-15  Alexandre Oliva  <[hidden email]>

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

> * sysdeps/unix/sysv/linux/fips-private.h: New.
> * sysdeps/generic/fips-private.h: New, dummy fallback.

"New file", please.

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

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

This file is short enough it doesn't really matter to give it a
copyright header.  But we tend to do so even for files like this.
And either way, at least give it a descriptive comment.

> +#include <stdbool.h>
> +
> +static inline bool
> +fips_enabled_p (void)
> +{
> +  return false;
> +}

As this is the generic file that serves as the model for people
writing new sysdeps files, the comment explaining the contract of
the function should be here.

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

This file needs a copyright header (and don't forget: top line is a
descriptive comment, not the copyright line).

> +{
> +  static int checked;

There's no reason for arcane and unexplained logic with magic int
values here.  Just make it an enum.

> +      if (!checked)
> + checked = -2;

This needs a comment as it is, but would be self-explanatory if it
were an enum.


Thanks,
Roland
Reply | Threaded
Open this post in threaded view
|

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

Alexandre Oliva-2
On May 23, 2012, Roland McGrath <[hidden email]> 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  <[hidden email]>

> 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  <[hidden email]>

        * 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 <[hidden email]>, 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  <[hidden email]>

        * 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 <[hidden email]>, 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 <[hidden email]>, 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
Reply | Threaded
Open this post in threaded view
|

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

Alexandre Oliva-2
On Jun  5, 2012, Alexandre Oliva <[hidden email]> wrote:

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

Ping?

I've now updated this patchset (fixed a Makefile conflict and the
Makefile name in the ChangeLog entry) and pushed to
lxoliva/crypt-fips-bz811753 (minus ChangeLog entries; they're in git
logs only).

Ok for master?

> for  ChangeLog
> 2012-06-05  Alexandre Oliva  <[hidden email]>

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

> for  ChangeLog
> 2012-06-05  Alexandre Oliva  <[hidden email]>

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

--
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
Reply | Threaded
Open this post in threaded view
|

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

Andreas Jaeger-8
On Tuesday, September 04, 2012 20:22:22 Alexandre Oliva wrote:

> On Jun  5, 2012, Alexandre Oliva <[hidden email]> wrote:
> > 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!
>
> Ping?
>
> I've now updated this patchset (fixed a Makefile conflict and the
> Makefile name in the ChangeLog entry) and pushed to
> lxoliva/crypt-fips-bz811753 (minus ChangeLog entries; they're in git
> logs only).
>
> Ok for master?

Please post the complete patch set again - and remove the Contributed by
lines you have in the new files:

+   Contributed by Alexandre Oliva <[hidden email]>, 2012.

We don't add those anymore for new files.

Thanks,
Andreas
-
 Andreas Jaeger aj@{suse.com,opensuse.org} Twitter/Identica: jaegerandi
  SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
   GF: Jeff Hawn,Jennifer Guild,Felix Imendörffer,HRB16746 (AG Nürnberg)
    GPG fingerprint = 93A3 365E CE47 B889 DF7F  FED1 389A 563C C272 A126

Reply | Threaded
Open this post in threaded view
|

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

Alexandre Oliva-2
On Sep  5, 2012, Andreas Jaeger <[hidden email]> wrote:

> Please post the complete patch set again - and remove the Contributed by
> lines you have in the new files:

Done.  lxoliva/crypt-fips-bz811753 updated, too.


Reject out-of-spec salt passed to DES crypt

From: Alexandre Oliva <[hidden email]>

for  ChangeLog
from  Alexandre Oliva  <[hidden email]>

        * 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.
        * crypt/Makefile (tests): Add it.
        ($(objpfx)badsalttest): New.
---

 crypt/Makefile        |    2 +
 crypt/badsalttest.c   |   85 +++++++++++++++++++++++++++++++++++++++++++++++++
 crypt/crypt-entry.c   |    7 +++-
 crypt/crypt-private.h |    3 +-
 crypt/crypt_util.c    |   44 +++++++++++++++++++++++--
 5 files changed, 134 insertions(+), 7 deletions(-)
 create mode 100644 crypt/badsalttest.c


diff --git a/crypt/Makefile b/crypt/Makefile
index 3a61865..3d4f243 100644
--- a/crypt/Makefile
+++ b/crypt/Makefile
@@ -28,7 +28,7 @@ extra-libs-others := $(extra-libs)
 libcrypt-routines := crypt-entry md5-crypt sha256-crypt sha512-crypt crypt \
      crypt_util
 
-tests := cert md5c-test sha256c-test sha512c-test
+tests := cert md5c-test sha256c-test sha512c-test badsalttest
 
 include ../Makeconfig
 
diff --git a/crypt/badsalttest.c b/crypt/badsalttest.c
new file mode 100644
index 0000000..ec9abde
--- /dev/null
+++ b/crypt/badsalttest.c
@@ -0,0 +1,85 @@
+/* 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.
+
+   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"
diff --git a/crypt/crypt-entry.c b/crypt/crypt-entry.c
index 91e2c4e..9fb22bd 100644
--- a/crypt/crypt-entry.c
+++ b/crypt/crypt-entry.c
@@ -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
diff --git a/crypt/crypt-private.h b/crypt/crypt-private.h
index b4bfa8b..54418fc 100644
--- a/crypt/crypt-private.h
+++ b/crypt/crypt-private.h
@@ -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, struct crypt_data * __restrict __data,
 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);
diff --git a/crypt/crypt_util.c b/crypt/crypt_util.c
index a1ff88b..269371e 100644
--- a/crypt/crypt_util.c
+++ b/crypt/crypt_util.c
@@ -57,6 +57,7 @@ STATIC void shuffle_sb (long32 *k, ufc_long saltbits);
 #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

Disable MD5 and DES crypt in FIPS mode

From: Alexandre Oliva <[hidden email]>

for  ChangeLog
from  Alexandre Oliva  <[hidden email]>

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

 crypt/crypt-entry.c                    |   24 +++++++++--
 crypt/md5c-test.c                      |    5 ++
 sysdeps/generic/fips-private.h         |   36 ++++++++++++++++
 sysdeps/unix/sysv/linux/fips-private.h |   73 ++++++++++++++++++++++++++++++++
 4 files changed, 134 insertions(+), 4 deletions(-)
 create mode 100644 sysdeps/generic/fips-private.h
 create mode 100644 sysdeps/unix/sysv/linux/fips-private.h


diff --git a/crypt/crypt-entry.c b/crypt/crypt-entry.c
index 9fb22bd..89c22e6 100644
--- a/crypt/crypt-entry.c
+++ b/crypt/crypt-entry.c
@@ -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.  */
diff --git a/crypt/md5c-test.c b/crypt/md5c-test.c
index f56d0eb..c80e402 100644
--- a/crypt/md5c-test.c
+++ b/crypt/md5c-test.c
@@ -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;
 }
diff --git a/sysdeps/generic/fips-private.h b/sysdeps/generic/fips-private.h
new file mode 100644
index 0000000..0dff087
--- /dev/null
+++ b/sysdeps/generic/fips-private.h
@@ -0,0 +1,36 @@
+/* Dummy implementation of FIPS compliance status test.
+   Copyright (C) 2012 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   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 */
diff --git a/sysdeps/unix/sysv/linux/fips-private.h b/sysdeps/unix/sysv/linux/fips-private.h
new file mode 100644
index 0000000..0f24925
--- /dev/null
+++ b/sysdeps/unix/sysv/linux/fips-private.h
@@ -0,0 +1,73 @@
+/* FIPS compliance status test for GNU/Linux systems.
+   Copyright (C) 2012 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   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
Reply | Threaded
Open this post in threaded view
|

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

Andreas Jaeger-8
On Wednesday, September 05, 2012 16:32:09 Alexandre Oliva wrote:
> On Sep  5, 2012, Andreas Jaeger <[hidden email]> wrote:
> > Please post the complete patch set again - and remove the
> > Contributed by
> > lines you have in the new files:
> Done.  lxoliva/crypt-fips-bz811753 updated, too.

sysdeps/unix/sysv/linux/fips-private.h has:
> +  return checked > FIPS_UNTESTED;

I suggest to change this to:
return checked == FIPS_ENABLED

Please make that change and submit everything - and add an entry to
NEWS.

Thanks,
Andreas
--
 Andreas Jaeger aj@{suse.com,opensuse.org} Twitter/Identica: jaegerandi
  SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
   GF: Jeff Hawn,Jennifer Guild,Felix Imendörffer,HRB16746 (AG Nürnberg)
    GPG fingerprint = 93A3 365E CE47 B889 DF7F  FED1 389A 563C C272 A126

Reply | Threaded
Open this post in threaded view
|

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

Alexandre Oliva-2
On Sep  6, 2012, Andreas Jaeger <[hidden email]> wrote:

> sysdeps/unix/sysv/linux/fips-private.h has:
>> +  return checked > FIPS_UNTESTED;

> I suggest to change this to:
> return checked == FIPS_ENABLED

> Please make that change and submit everything - and add an entry to
> NEWS.

Here's the updated patchset, with the requested changes.

Is this ok to install?


Reject out-of-spec salt passed to DES crypt

From: Alexandre Oliva <[hidden email]>

for  ChangeLog

        * 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.
        * crypt/Makefile (tests): Add it.
        ($(objpfx)badsalttest): New.
---

 crypt/Makefile        |    2 +
 crypt/badsalttest.c   |   85 +++++++++++++++++++++++++++++++++++++++++++++++++
 crypt/crypt-entry.c   |    7 +++-
 crypt/crypt-private.h |    3 +-
 crypt/crypt_util.c    |   44 +++++++++++++++++++++++--
 5 files changed, 134 insertions(+), 7 deletions(-)
 create mode 100644 crypt/badsalttest.c


diff --git a/crypt/Makefile b/crypt/Makefile
index 3a61865..3d4f243 100644
--- a/crypt/Makefile
+++ b/crypt/Makefile
@@ -28,7 +28,7 @@ extra-libs-others := $(extra-libs)
 libcrypt-routines := crypt-entry md5-crypt sha256-crypt sha512-crypt crypt \
      crypt_util
 
-tests := cert md5c-test sha256c-test sha512c-test
+tests := cert md5c-test sha256c-test sha512c-test badsalttest
 
 include ../Makeconfig
 
diff --git a/crypt/badsalttest.c b/crypt/badsalttest.c
new file mode 100644
index 0000000..ec9abde
--- /dev/null
+++ b/crypt/badsalttest.c
@@ -0,0 +1,85 @@
+/* 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.
+
+   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"
diff --git a/crypt/crypt-entry.c b/crypt/crypt-entry.c
index 91e2c4e..9fb22bd 100644
--- a/crypt/crypt-entry.c
+++ b/crypt/crypt-entry.c
@@ -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
diff --git a/crypt/crypt-private.h b/crypt/crypt-private.h
index b4bfa8b..54418fc 100644
--- a/crypt/crypt-private.h
+++ b/crypt/crypt-private.h
@@ -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, struct crypt_data * __restrict __data,
 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);
diff --git a/crypt/crypt_util.c b/crypt/crypt_util.c
index a1ff88b..269371e 100644
--- a/crypt/crypt_util.c
+++ b/crypt/crypt_util.c
@@ -57,6 +57,7 @@ STATIC void shuffle_sb (long32 *k, ufc_long saltbits);
 #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

Disable MD5 and DES crypt in FIPS mode

From: Alexandre Oliva <[hidden email]>

for  ChangeLog

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

 crypt/crypt-entry.c                    |   24 +++++++++--
 crypt/md5c-test.c                      |    5 ++
 sysdeps/generic/fips-private.h         |   36 ++++++++++++++++
 sysdeps/unix/sysv/linux/fips-private.h |   73 ++++++++++++++++++++++++++++++++
 4 files changed, 134 insertions(+), 4 deletions(-)
 create mode 100644 sysdeps/generic/fips-private.h
 create mode 100644 sysdeps/unix/sysv/linux/fips-private.h


diff --git a/crypt/crypt-entry.c b/crypt/crypt-entry.c
index 9fb22bd..89c22e6 100644
--- a/crypt/crypt-entry.c
+++ b/crypt/crypt-entry.c
@@ -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.  */
diff --git a/crypt/md5c-test.c b/crypt/md5c-test.c
index f56d0eb..c80e402 100644
--- a/crypt/md5c-test.c
+++ b/crypt/md5c-test.c
@@ -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;
 }
diff --git a/sysdeps/generic/fips-private.h b/sysdeps/generic/fips-private.h
new file mode 100644
index 0000000..0dff087
--- /dev/null
+++ b/sysdeps/generic/fips-private.h
@@ -0,0 +1,36 @@
+/* Dummy implementation of FIPS compliance status test.
+   Copyright (C) 2012 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   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 */
diff --git a/sysdeps/unix/sysv/linux/fips-private.h b/sysdeps/unix/sysv/linux/fips-private.h
new file mode 100644
index 0000000..510cc71
--- /dev/null
+++ b/sysdeps/unix/sysv/linux/fips-private.h
@@ -0,0 +1,73 @@
+/* FIPS compliance status test for GNU/Linux systems.
+   Copyright (C) 2012 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   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_ENABLED;
+}
+
+#endif /* _FIPS_PRIVATE_H */

Add NEWS entry about fips mode

From: Alexandre Oliva <[hidden email]>

for  ChangeLog

        * NEWS: Add note about FIPS mode.
---

 NEWS |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)


diff --git a/NEWS b/NEWS
index fe7c78d..b288ea5 100644
--- a/NEWS
+++ b/NEWS
@@ -47,6 +47,10 @@ Version 2.17
   the tty file descriptor in /dev/pts or /dev if /proc is not available.  This
   allows creation of chroots without the procfs mounted on /proc.
 
+* Recognize and enable FIPS mode from Linux's /proc/sys/crypto/fips_enabled,
+  disabling MD5 and DES crypt algorithms when the mode is enabled.  Reject
+  out-of-spec salt bytes in DES implementation.
+
 
 Version 2.16
 



--
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
Reply | Threaded
Open this post in threaded view
|

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

Andreas Jaeger-8
On 10/02/2012 08:07 AM, Alexandre Oliva wrote:
> Here's the updated patchset, with the requested changes.
>
> Is this ok to install?

Thanks, this looks fine now,

Andreas
--
  Andreas Jaeger aj@{suse.com,opensuse.org} Twitter/Identica: jaegerandi
   SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
    GF: Jeff Hawn,Jennifer Guild,Felix Imendörffer,HRB16746 (AG Nürnberg)
     GPG fingerprint = 93A3 365E CE47 B889 DF7F  FED1 389A 563C C272 A126
Reply | Threaded
Open this post in threaded view
|

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

Roland McGrath-4
In reply to this post by Alexandre Oliva-2
> +static const char *tests[][2] = {

Brace on new line, indent appropriately.

>  /*
> + * Return false iff C is in the specified alphabet for crypt salt.
> + */
> +
> +static bool
> +bad_for_salt (c)
> +     char c;

Use a prototype defn.

> +fips_enabled_p (void)
> +{
> +  static enum {

Brace on new line, indent appropriately.

> +* Recognize and enable FIPS mode from Linux's /proc/sys/crypto/fips_enabled,
> +  disabling MD5 and DES crypt algorithms when the mode is enabled.  Reject
> +  out-of-spec salt bytes in DES implementation.

This looks like a log entry more than a NEWS entry.  
A NEWS entry should describe things meaningful to a user:

* The `crypt' function now fails if passed salt bytes that violate the
  specification for those values.  On Linux, the `crypt' function will
  consult /proc/sys/crypto/fips_enabled to determine if "FIPS mode" is
  enabled, and fail on encrypted strings using the MD5 or DES algorithm
  when the mode is enabled.


Thanks,
Roland
Reply | Threaded
Open this post in threaded view
|

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

Alexandre Oliva-2
On Oct  2, 2012, Roland McGrath <[hidden email]> wrote:

>> +static bool
>> +bad_for_salt (c)
>> +     char c;

> Use a prototype defn.

Even in a file that's K&R?  Ok, I removed the now-redundant prototype
declaration, too.

> This looks like a log entry more than a NEWS entry.  
> A NEWS entry should describe things meaningful to a user:

Heh.  Indeed.  Thanks for the review and the NEWS entry.

Here's the revised, retested patchset I'm going to check in some 24+h
from now, if I don't get objections.  It's also available in branch
lxoliva/crypt-fips-bz811753.


Reject out-of-spec salt passed to DES crypt

From: Alexandre Oliva <[hidden email]>

for  ChangeLog

        * 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.
        * crypt/Makefile (tests): Add it.
        ($(objpfx)badsalttest): New.
---

 crypt/Makefile        |    2 +
 crypt/badsalttest.c   |   86 +++++++++++++++++++++++++++++++++++++++++++++++++
 crypt/crypt-entry.c   |    7 +++-
 crypt/crypt-private.h |    3 +-
 crypt/crypt_util.c    |   42 ++++++++++++++++++++++--
 5 files changed, 133 insertions(+), 7 deletions(-)
 create mode 100644 crypt/badsalttest.c


diff --git a/crypt/Makefile b/crypt/Makefile
index 3a61865..3d4f243 100644
--- a/crypt/Makefile
+++ b/crypt/Makefile
@@ -28,7 +28,7 @@ extra-libs-others := $(extra-libs)
 libcrypt-routines := crypt-entry md5-crypt sha256-crypt sha512-crypt crypt \
      crypt_util
 
-tests := cert md5c-test sha256c-test sha512c-test
+tests := cert md5c-test sha256c-test sha512c-test badsalttest
 
 include ../Makeconfig
 
diff --git a/crypt/badsalttest.c b/crypt/badsalttest.c
new file mode 100644
index 0000000..e0e207b
--- /dev/null
+++ b/crypt/badsalttest.c
@@ -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.
+
+   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"
diff --git a/crypt/crypt-entry.c b/crypt/crypt-entry.c
index 91e2c4e..9fb22bd 100644
--- a/crypt/crypt-entry.c
+++ b/crypt/crypt-entry.c
@@ -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
diff --git a/crypt/crypt-private.h b/crypt/crypt-private.h
index b4bfa8b..54418fc 100644
--- a/crypt/crypt-private.h
+++ b/crypt/crypt-private.h
@@ -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, struct crypt_data * __restrict __data,
 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);
diff --git a/crypt/crypt_util.c b/crypt/crypt_util.c
index a1ff88b..e08dd8f 100644
--- a/crypt/crypt_util.c
+++ b/crypt/crypt_util.c
@@ -596,23 +596,55 @@ shuffle_sb(k, saltbits)
 #endif
 
 /*
+ * Return false iff C is in the specified alphabet for crypt salt.
+ */
+
+static bool
+bad_for_salt (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 +678,8 @@ _ufc_setup_salt_r(s, __data)
   shuffle_sb((LONGG)__data->sb3, __data->current_saltbits ^ saltbits);
 
   __data->current_saltbits = saltbits;
+
+  return true;
 }
 
 void

Disable MD5 and DES crypt in FIPS mode

From: Alexandre Oliva <[hidden email]>

for  ChangeLog

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

 crypt/crypt-entry.c                    |   24 +++++++++-
 crypt/md5c-test.c                      |    5 ++
 sysdeps/generic/fips-private.h         |   36 ++++++++++++++++
 sysdeps/unix/sysv/linux/fips-private.h |   74 ++++++++++++++++++++++++++++++++
 4 files changed, 135 insertions(+), 4 deletions(-)
 create mode 100644 sysdeps/generic/fips-private.h
 create mode 100644 sysdeps/unix/sysv/linux/fips-private.h


diff --git a/crypt/crypt-entry.c b/crypt/crypt-entry.c
index 9fb22bd..89c22e6 100644
--- a/crypt/crypt-entry.c
+++ b/crypt/crypt-entry.c
@@ -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.  */
diff --git a/crypt/md5c-test.c b/crypt/md5c-test.c
index f56d0eb..c80e402 100644
--- a/crypt/md5c-test.c
+++ b/crypt/md5c-test.c
@@ -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;
 }
diff --git a/sysdeps/generic/fips-private.h b/sysdeps/generic/fips-private.h
new file mode 100644
index 0000000..0dff087
--- /dev/null
+++ b/sysdeps/generic/fips-private.h
@@ -0,0 +1,36 @@
+/* Dummy implementation of FIPS compliance status test.
+   Copyright (C) 2012 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   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 */
diff --git a/sysdeps/unix/sysv/linux/fips-private.h b/sysdeps/unix/sysv/linux/fips-private.h
new file mode 100644
index 0000000..81d1b61
--- /dev/null
+++ b/sysdeps/unix/sysv/linux/fips-private.h
@@ -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.
+
+   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_ENABLED;
+}
+
+#endif /* _FIPS_PRIVATE_H */

Add NEWS entry about fips mode

From: Alexandre Oliva <[hidden email]>

for  ChangeLog

        * NEWS: Add note about FIPS mode.  Wording suggested by Roland
        McGrath.
---

 NEWS |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)


diff --git a/NEWS b/NEWS
index ead7dea..bcfe0aa 100644
--- a/NEWS
+++ b/NEWS
@@ -47,6 +47,12 @@ Version 2.17
   the tty file descriptor in /dev/pts or /dev if /proc is not available.  This
   allows creation of chroots without the procfs mounted on /proc.
 
+* The `crypt' function now fails if passed salt bytes that violate the
+  specification for those values.  On Linux, the `crypt' function will
+  consult /proc/sys/crypto/fips_enabled to determine if "FIPS mode" is
+  enabled, and fail on encrypted strings using the MD5 or DES algorithm
+  when the mode is enabled.
+
 
 Version 2.16
 



--
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
Reply | Threaded
Open this post in threaded view
|

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

Alexandre Oliva-2
On Oct  3, 2012, Alexandre Oliva <[hidden email]> wrote:

> Here's the revised, retested patchset I'm going to check in some 24+h
> from now, if I don't get objections.  It's also available in branch
> lxoliva/crypt-fips-bz811753.

Patches installed, temp branch removed.

Thanks again for all the reviews.

> Reject out-of-spec salt passed to DES crypt

> From: Alexandre Oliva <[hidden email]>

> for  ChangeLog

> * 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.
> * crypt/Makefile (tests): Add it.
> ($(objpfx)badsalttest): New.

> Disable MD5 and DES crypt in FIPS mode

> From: Alexandre Oliva <[hidden email]>

> for  ChangeLog

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

> Add NEWS entry about fips mode

> From: Alexandre Oliva <[hidden email]>

> for  ChangeLog

> * NEWS: Add note about FIPS mode.  Wording suggested by Roland
> McGrath.

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