This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [RFC] FIPS compliance and other crypt(3) improvements
On May 18, 2012, Roland McGrath <roland@hack.frob.com> wrote:
>> On May 15, 2012, Roland McGrath <roland@hack.frob.com> wrote:
>>
>> > ENOSYS is the error code for a function that is entirely unimplemented.
>>
>> It's the only POSIX-documented error code for crypt. That's why I went
>> with it.
> It's a general part of POSIX that functions can return different errno
> codes than the ones listed.
Aha! Ok, then. The description for ENOSYS under crypt still fit, but
given your comment, I'm now going with EINVAL for this case.
> I'm not at all convinced that sysconf is the right place for such an
> extension.
Okiedokie, I've dropped the sysconf change, and implemented
fips_enabled_p using identical logic in a new fips-private.h header in
sysdeps.
I also used bool rather than int for newly-introduced return types.
Here are the two revised patches, down from three, because I'm not
longer changing sysconf. How are these?
for ChangeLog
2012-05-15 Alexandre Oliva <aoliva@redhat.com>
* crypt/crypt-private.h: Include stdbool.h.
(_ufc_setup_salt_r): Return bool.
* crypt/crypt-entry.c: Include errno.h.
(__crypt_r): Return NULL with EINVAL for bad salt.
* crypt/crypt_util.c (bad_for_salt): New.
(_ufc_setup_salt_r): Check that salt is long enough and within
the specified alphabet.
* crypt/badsalttest.c: New.
* Makefile (tests): Add it.
($(objpfx)badsalttest): New.
Index: crypt/crypt-private.h
===================================================================
--- crypt/crypt-private.h.orig 2012-05-23 06:56:12.618445828 -0300
+++ crypt/crypt-private.h 2012-05-23 07:03:39.675270309 -0300
@@ -26,6 +26,7 @@
#define CRYPT_PRIVATE_H 1
#include <features.h>
+#include <stdbool.h>
/* crypt.c */
extern void _ufc_doit_r (ufc_long itr, struct crypt_data * __restrict __data,
@@ -36,7 +37,7 @@ extern void _ufc_doit_r (ufc_long itr, s
extern void __init_des_r (struct crypt_data * __restrict __data);
extern void __init_des (void);
-extern void _ufc_setup_salt_r (const char *s,
+extern bool _ufc_setup_salt_r (const char *s,
struct crypt_data * __restrict __data);
extern void _ufc_mk_keytab_r (const char *key,
struct crypt_data * __restrict __data);
Index: crypt/crypt-entry.c
===================================================================
--- crypt/crypt-entry.c.orig 2012-05-23 06:56:12.618445828 -0300
+++ crypt/crypt-entry.c 2012-05-23 07:04:17.614250905 -0300
@@ -27,6 +27,7 @@
#include <stdio.h>
#endif
#include <string.h>
+#include <errno.h>
#ifndef STATIC
#define STATIC static
@@ -108,7 +109,11 @@ __crypt_r (key, salt, data)
/*
* Hack DES tables according to salt
*/
- _ufc_setup_salt_r (salt, data);
+ if (!_ufc_setup_salt_r (salt, data))
+ {
+ __set_errno (EINVAL);
+ return NULL;
+ }
/*
* Setup key schedule
Index: crypt/crypt_util.c
===================================================================
--- crypt/crypt_util.c.orig 2012-05-23 06:56:12.619445821 -0300
+++ crypt/crypt_util.c 2012-05-23 06:56:15.004430817 -0300
@@ -57,6 +57,7 @@ STATIC void shuffle_sb (long32 *k, ufc_l
#else
STATIC void shuffle_sb (long64 *k, ufc_long saltbits);
#endif
+static bool bad_for_salt (char c);
/*
@@ -596,23 +597,56 @@ shuffle_sb(k, saltbits)
#endif
/*
+ * Return zero iff C is in the specified alphabet for crypt salt.
+ */
+
+static bool
+bad_for_salt (c)
+ char c;
+{
+ switch (c)
+ {
+ case '0' ... '9':
+ case 'A' ... 'Z':
+ case 'a' ... 'z':
+ case '.': case '/':
+ return false;
+
+ default:
+ return true;
+ }
+}
+
+/*
* Setup the unit for a new salt
* Hopefully we'll not see a new salt in each crypt call.
+ * Return FALSE if an unexpected character was found in s[0] or s[1].
*/
-void
+bool
_ufc_setup_salt_r(s, __data)
const char *s;
struct crypt_data * __restrict __data;
{
ufc_long i, j, saltbits;
+ char s0, s1;
if(__data->initialized == 0)
__init_des_r(__data);
- if(s[0] == __data->current_salt[0] && s[1] == __data->current_salt[1])
- return;
- __data->current_salt[0] = s[0]; __data->current_salt[1] = s[1];
+ s0 = s[0];
+ if(bad_for_salt (s0))
+ return false;
+
+ s1 = s[1];
+ if(bad_for_salt (s1))
+ return false;
+
+ if(s0 == __data->current_salt[0] && s1 == __data->current_salt[1])
+ return true;
+
+ __data->current_salt[0] = s0;
+ __data->current_salt[1] = s1;
/*
* This is the only crypt change to DES:
@@ -646,6 +680,8 @@ _ufc_setup_salt_r(s, __data)
shuffle_sb((LONGG)__data->sb3, __data->current_saltbits ^ saltbits);
__data->current_saltbits = saltbits;
+
+ return true;
}
void
Index: crypt/Makefile
===================================================================
--- crypt/Makefile.orig 2012-05-23 06:56:12.619445821 -0300
+++ crypt/Makefile 2012-05-23 06:56:15.017430735 -0300
@@ -44,11 +44,12 @@ LDLIBS-crypt.so = -lfreebl3
else
libcrypt-routines += md5 sha256 sha512
-tests += md5test sha256test sha512test
+tests += md5test sha256test sha512test badsalttest
$(objpfx)md5test: $(objpfx)md5.o
$(objpfx)sha256test: $(objpfx)sha256.o
$(objpfx)sha512test: $(objpfx)sha512.o
+$(objpfx)badsalttest: $(objpfx)badsalttest.o
endif
include ../Rules
Index: crypt/badsalttest.c
===================================================================
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ crypt/badsalttest.c 2012-05-23 06:56:15.028430665 -0300
@@ -0,0 +1,64 @@
+#include <stdio.h>
+#include <unistd.h>
+#include <sys/mman.h>
+#include <crypt.h>
+
+const char *tests[][2] = {
+ { "no salt", "" },
+ { "single char", "/" },
+ { "first char bad", "!x" },
+ { "second char bad", "Z%" },
+ { "both chars bad", ":@" },
+ { "un$upported algorithm", "$2$" },
+ { "unsupported_algorithm", "_1" },
+ { "end of page", NULL }
+};
+
+int
+main (int argc, char *argv[])
+{
+ int result = 0;
+ int i, n = sizeof (tests) / sizeof (*tests);
+ struct crypt_data cd;
+ size_t pagesize = (size_t) sysconf (_SC_PAGESIZE);
+ char *page;
+
+ /* Check that crypt won't look at the second character if the first
+ one is invalid. */
+ page = mmap (NULL, pagesize * 2, PROT_READ | PROT_WRITE,
+ MAP_PRIVATE | MAP_ANON, -1, 0);
+ if (page == (void*)-1)
+ {
+ perror ("mmap");
+ n--;
+ }
+ else
+ {
+ if (munmap (page + pagesize, pagesize))
+ perror ("munmap");
+ if (mmap (page + pagesize, pagesize, 0, MAP_PRIVATE | MAP_ANON,
+ -1, 0) != page + pagesize)
+ perror ("mmap 2");
+ page[pagesize - 1] = '*';
+ tests[n - 1][1] = &page[pagesize - 1];
+ }
+
+ for (i = 0; i < n; i++)
+ {
+ if (crypt (tests[i][0], tests[i][1]))
+ {
+ result++;
+ printf ("%s: crypt returned non-NULL with salt \"%s\"\n",
+ tests[i][0], tests[i][1]);
+ }
+
+ if (crypt_r (tests[i][0], tests[i][1], &cd))
+ {
+ result++;
+ printf ("%s: crypt_r returned non-NULL with salt \"%s\"\n",
+ tests[i][0], tests[i][1]);
+ }
+ }
+
+ return result;
+}
for ChangeLog
2012-05-15 Alexandre Oliva <aoliva@redhat.com>
* crypt/crypt-entry.c: Include fips-private.h.
(__crypt_r, __crypt): Disable MD5 and DES if FIPS is enabled.
* crypt/md5c-test.c (main): Tolerate disabled MD5.
* sysdeps/unix/sysv/linux/fips-private.h: New.
* sysdeps/generic/fips-private.h: New, dummy fallback.
Index: crypt/crypt-entry.c
===================================================================
--- crypt/crypt-entry.c.orig 2012-05-23 07:04:17.614250905 -0300
+++ crypt/crypt-entry.c 2012-05-23 07:52:21.954465503 -0300
@@ -28,6 +28,7 @@
#endif
#include <string.h>
#include <errno.h>
+#include <fips-private.h>
#ifndef STATIC
#define STATIC static
@@ -91,7 +92,8 @@ __crypt_r (key, salt, data)
#ifdef _LIBC
/* Try to find out whether we have to use MD5 encryption replacement. */
- if (strncmp (md5_salt_prefix, salt, sizeof (md5_salt_prefix) - 1) == 0)
+ if (strncmp (md5_salt_prefix, salt, sizeof (md5_salt_prefix) - 1) == 0
+ && !fips_enabled_p ())
return __md5_crypt_r (key, salt, (char *) data,
sizeof (struct crypt_data));
@@ -109,7 +111,7 @@ __crypt_r (key, salt, data)
/*
* Hack DES tables according to salt
*/
- if (!_ufc_setup_salt_r (salt, data))
+ if (fips_enabled_p () || !_ufc_setup_salt_r (salt, data))
{
__set_errno (EINVAL);
return NULL;
@@ -148,7 +150,8 @@ crypt (key, salt)
{
#ifdef _LIBC
/* Try to find out whether we have to use MD5 encryption replacement. */
- if (strncmp (md5_salt_prefix, salt, sizeof (md5_salt_prefix) - 1) == 0)
+ if (strncmp (md5_salt_prefix, salt, sizeof (md5_salt_prefix) - 1) == 0
+ && !fips_enabled_p ())
return __md5_crypt (key, salt);
/* Try to find out whether we have to use SHA256 encryption replacement. */
Index: crypt/md5c-test.c
===================================================================
--- crypt/md5c-test.c.orig 2012-05-23 07:04:17.775250807 -0300
+++ crypt/md5c-test.c 2012-05-23 07:04:23.869246982 -0300
@@ -9,7 +9,10 @@ main (int argc, char *argv[])
int result = 0;
cp = crypt ("Hello world!", salt);
- result |= strcmp ("$1$saltstri$YMyguxXMBpd2TEZ.vS/3q1", cp);
+
+ /* MD5 is disabled in FIPS mode. */
+ if (cp)
+ result |= strcmp ("$1$saltstri$YMyguxXMBpd2TEZ.vS/3q1", cp);
return result;
}
Index: sysdeps/generic/fips-private.h
===================================================================
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ sysdeps/generic/fips-private.h 2012-05-23 07:48:08.214924392 -0300
@@ -0,0 +1,12 @@
+#ifndef _FIPS_PRIVATE_H
+#define _FIPS_PRIVATE_H
+
+#include <stdbool.h>
+
+static inline bool
+fips_enabled_p (void)
+{
+ return false;
+}
+
+#endif /* _FIPS_PRIVATE_H */
Index: sysdeps/unix/sysv/linux/fips-private.h
===================================================================
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ sysdeps/unix/sysv/linux/fips-private.h 2012-05-23 07:51:25.170585521 -0300
@@ -0,0 +1,51 @@
+#ifndef _FIPS_PRIVATE_H
+#define _FIPS_PRIVATE_H
+
+#include <errno.h>
+#include <fcntl.h>
+#include <stdlib.h>
+#include <unistd.h>
+#include <not-cancel.h>
+#include <stdbool.h>
+
+/* Return true if FIPS mode is enabled.
+
+ This is only relevant within crypt, to tell whether MD5 and DES
+ algorithms should be rejected. */
+static inline bool
+fips_enabled_p (void)
+{
+ static int checked;
+
+ if (!checked)
+ {
+ int fd = open_not_cancel_2 ("/proc/sys/crypto/fips_enabled", O_RDONLY);
+
+ if (fd != -1)
+ {
+ /* This is more than enough, the file contains a single integer. */
+ char buf[32];
+ ssize_t n;
+ n = TEMP_FAILURE_RETRY (read_not_cancel (fd, buf, sizeof (buf) - 1));
+ close_not_cancel_no_status (fd);
+
+ if (n > 0)
+ {
+ /* Terminate the string. */
+ buf[n] = '\0';
+
+ char *endp;
+ long int res = strtol (buf, &endp, 10);
+ if (endp != buf && (*endp == '\0' || *endp == '\n'))
+ checked = (res > 0) ? 1 : -1;
+ }
+ }
+
+ if (!checked)
+ checked = -2;
+ }
+
+ return checked > 0;
+}
+
+#endif /* _FIPS_PRIVATE_H */
--
Alexandre Oliva, freedom fighter http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/ FSF Latin America board member
Free Software Evangelist Red Hat Brazil Compiler Engineer