Bug 811753 - crypt() is broken in fips mode
crypt() is broken in fips mode
Product: Fedora
Classification: Fedora
Component: glibc (Show other bugs)
Unspecified Unspecified
unspecified Severity unspecified
: ---
: ---
Assigned To: Alexandre Oliva
Fedora Extras Quality Assurance
Depends On: 829088
Blocks: 717789 867144
  Show dependency treegraph
Reported: 2012-04-11 17:58 EDT by Paul Wouters
Modified: 2016-11-24 10:49 EST (History)
9 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
: 829088 867144 (view as bug list)
Last Closed: 2012-10-16 15:57:24 EDT
Type: Bug
Regression: ---
Mount Type: ---
Documentation: ---
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---

Attachments (Terms of Use)
crypttest.c test program (491 bytes, text/x-csrc)
2012-04-11 17:58 EDT, Paul Wouters
no flags Details
Patch to return NULL for bad salt, e.g. unsupported algorithm (5.59 KB, patch)
2012-05-15 01:54 EDT, Alexandre Oliva
no flags Details | Diff
Introduce sysconf(_SC_CRYPTO_FIPS_ENABLED) (1.33 KB, patch)
2012-05-15 03:35 EDT, Alexandre Oliva
no flags Details | Diff
Disable MD5 and DES when FIPS is enabled (2.72 KB, patch)
2012-05-15 03:51 EDT, Alexandre Oliva
no flags Details | Diff
test source file (4.67 KB, text/x-csrc)
2012-06-05 12:45 EDT, Paul Wouters
no flags Details

  None (edit)
Description Paul Wouters 2012-04-11 17:58:23 EDT
Created attachment 576902 [details]
crypttest.c test program

Description of problem:
crypt() returns NULL in fipsmode 

Version-Release number of selected component (if applicable):

How reproducible:
Always, see attached test program

Actual results:
crypt returned NULL for salt $6$FOOBAR
crypt returned NULL for salt $5$FOOBAR
crypt returned NULL for salt $1$FOOBAR

Expected results:

"Expected results" were obtained by not running in fips mode on the identical system that produced the "Actual results" while in fips mode.

Additionlly, in fips mode it should never return non-NULL for unknown $X$ methods but as can be seen it returns DES encryption for unknown algo ids.

Additional info:
Comment 1 Steve Grubb 2012-04-16 17:34:06 EDT
To help clarify the bug report, the approved hashes in FIPS mode are: SHA224, 256, 384, and 512. SHA1 is tolerated for backwards compatibility and will be sunset soon. Blowfish and MD5 are not permitted.
Comment 2 Paul Wouters 2012-04-19 15:35:05 EDT
It turns out this was due to prelinking causing fips failures.

So we are now getting the "expected" results back. However that means we still have some problems in fips mode:

1) The $1$ salt mode (MD5) should not be allowed in fipsmode

This might not be considered a glibc bug, but instead a bug in the various routines calling crypt(), though it is easy to fix here by returning NULL,
though it would be some linux specific code or patch.
(Steve: do we want such a patch for glibc on RHEL?)

2) All unsupported salt modes (2,2a,3 and 4) use a fallback mechanism to DES.

I think the proper return value for this error condition should be NULL.
Comment 3 Paul Wouters 2012-04-30 18:16:50 EDT
The issue is what should crypt() return when the salt defines the hashing algorithm method, using th $x$ syntax.

Currently, it does not support 

$2$  Obsolete, should return NULL, not DES
$2a$   (blowfish - only partially supported, straight crypt() call fails)
$2x$, $2y$ (blowfish, fixed, there is migration from 2a to 2x possible, but prob
      all of this is in openbsd libc, not glibc. Prob just return NULL for all
      these variations
$3$   Used on freebsd to signify NT-hashman, which is weak and we don't support
      Should return NULL
$4$   I can find no trace of this one ever having been defined anywhere. 
      Return NULL ?

It does support:

no $x$ marker   (DES)
$1$   (MD5)
$5$   (SHA256)
$6$   (SHA512)

If glibc could be made fips-aware, it should return NULL for DES and MD5 along with the others listed above that should always return NULL. But if that's not possible, we will just have to check the applications calling crypt() and fix it there.

I think it should treat all $...$ salts it does not know as not implemented and therefor return NULL, eg for salts with $9$ or $9a$ or $15abc$, etc. to avoid similar issues in the future. Currently, all unrecognised $..$ sales values are interpreted as just regular salts for DES.
Comment 4 Alexandre Oliva 2012-05-08 13:08:59 EDT
My reading of POSIX tells me that taking a valid DES salt (i.e., a string that matches [A-Za-z0-9./]{2,} and returning say NULL (ENOSYS) would be a deviation of the standard, or at least very unexpected (to me) behavior, although it could be defended by arguing that DES encryption is not supported when FIPS-compliance is activated.  Which leads to IMHO the most relevant issue: how to activate FIPS compliance within glibc.  I don't see that glibc has any awareness of FIPS-compliance requirements stated by the application, the operating system, or whatever it is that gets the “FIPS compliant” box checked (nevermind different versions of FIPS compliance requirements for the sake of this argument).  Do we have any precedent for controlling glibc behavior from such a “check box” (or write-version-in box)?  Should glibc have gain an entry point to control FIPS-relevant runtime behavior?  Regardless, this would be a deviation from the behavior currently specified in the glibc manual, but I guess that given suitable environmental conditions to change the behavior, that could be acceptable.

The other, much simpler but still uncertain issue, is whether glibc should validate the salt as matching the DES requirements, instead of using DES as a fallback.  This is not required by POSIX, but I can see value in returning NULL (ENOSYS) for out-of-spec salt values, particularly for ones that have been specified in extensions.  Testing two characters shouldn't be too expensive, after all, but I'm concerned it might break existing programs that have come to rely on (abuse?) the out-of-spec behavior.  The requirements are stated both in POSIX and in the glibc manual, but do we want to risk exposing this unwarranted reliance on out-of-spec behavior by causing applications to break?  I'm somewhat hesitant here.  The safest path is probably to implement range checks upstream only.

If we implemented both of these changes, the glibc crypt and __crypt_r code in crypt/crypt-entry.c ought to change to test for out-of-spec characters in the first two bytes of the salt if none of the supported $method$s are selected, and return NULL (ENOSYS) if the test fails.

As for disabling DES and MD5, I suppose the best change points would be in the implementation of each of the algorithms (__md5_crypt, __md5_crypt_r, and before the DES-related calls in __crypt_r), but the best interface to query requirements derived from specific FIPS versions, levels and enablement is quite unclear to me.  Do we already have any design for that and, if so, can you please point me at it?

Thanks in advance,
Comment 5 Frank Ch. Eigler 2012-05-08 13:25:10 EDT
lxo, other userspace crypto libraries check for non-'0' content in /proc/sys/crypto/fips_enabled to determine whether FIPS-140 mode was enabled at kernel boot time.
Comment 6 Paul Wouters 2012-05-14 12:34:32 EDT
The check for /proc/sys/crypto/fips_enabled is the easiest, as it does not require linking against fipscheck libraries.

I'm not familiar with the practise of upstream glibc vs redhat glbc. But I think the danger of fallback for non-recognised salts is more important then to facilitate people's support for DES. At least in the RHEL product line, if not also for upstream.

As for the FIPS check, I would prefer us to properly return NULL in fips mode for DES and MD5, even if it is not upstream of POSIX. Perhaps this is best done in Fedora first though before we do this in RHEL. But I think it is safe to say people who enable fips would prefer their apps to break when using unsafe crypto over a fallback to insecure crypto.

Note that checking for the existence of /proc/sys/crypto/fips_enabled is unfortunately not enough, as it indeed contains "0" if not running in fips mode.
Comment 7 Steve Grubb 2012-05-14 14:25:27 EDT
The fipscheck library is not used to determine if you are in fips mode. Its used for the mandatory integrity test imposed by fips. Checking /proc is how to do it. In fips mode you have to assume DES and MD5 are unavailable, so falling back to them will also produce an error.
Comment 8 Alexandre Oliva 2012-05-15 01:54:47 EDT
Created attachment 584543 [details]
Patch to return NULL for bad salt, e.g. unsupported algorithm

This patch implements the first part of the fix, making sure that we don't fallback to DES when the salt doesn't fit the alphabet specified and documented for the DES salt: we'll return NULL instead.  This will cover unsupported algorithms too, for $ is outside the specified alphabet, so if we don't recognize the algorithm at first, we'll now return NULL rather than using DES.  I'm proposing this upstream too.
Comment 9 Alexandre Oliva 2012-05-15 03:35:26 EDT
Created attachment 584565 [details]
Introduce sysconf(_SC_CRYPTO_FIPS_ENABLED)

This patch introduces support for sysconf to return the contents of /proc/sys/crypto/fips_enabled, when given the appropriate _SC_ key.  I'm also submitting it upstream.
Comment 10 Alexandre Oliva 2012-05-15 03:51:53 EDT
Created attachment 584570 [details]
Disable MD5 and DES when FIPS is enabled

I don't think this patch is correct in terms of standard compliance, and I concerned it may break password cracking programs and any other programs that may legitimately use crypt(3) for purposes other than verifying login passwords or encoding them for storage in passwd files or equivalent.

I don't see that FIPS requires this change; IMHO, for both POSIX and FIPS standard compliance, we ought to adjust crypt(3) callers that have to do with checking or changing passwords so as to reject the forbidden algorithms, for nothing in the crypt(3) specification says it can only be used in these scenarios.

That said, since I had my hands on it, I figured I might as well implement it so anyone could give it a spin and see what breaks (besides the glibc testsuite, now suitably adjusted).  I don't expect this to be welcome upstream, but I'll give it a shot anyway.
Comment 11 Paul Wouters 2012-06-05 12:44:19 EDT
So I've applied your patches, but I'm still getting the same results.

crypt() calls the __sha512() which calls NNSLOW_Init() which is defined in nsslowhash.c, though it does not seem to be compiled with compiling nss at all, and instead stubs.[ch] is used, which performs some kind of dlopen magic.

somehow, we call FREEBL_InitStubs() which calls nsslow_GetFIPSEnabled(), which succeeds and then it calls freebl_fipsPowerUpSelfTest() (again in nsslowhash.c which does not seem compiled at all, confirmed by debugging printfs)

This function fails, ending up causing the crypt() call to return null for every hash type.

If i try to step into freebl_fipsPowerUpSelfTest() it looks like it passes freebl_fips_MD2_PowerUpSelfTest() and freebl_fips_MD5_PowerUpSelfTest() and fails at freebl_fips_SHA_PowerUpSelfTest(), however I don't trust my gdb session here at all, as these functions are all in nsslowhash.c, gdb cannot find breakpoints for it, and when I add an "#error" line in nsslowhash.c or even empty the file, the nss compile still works, and gdb still looks and claims it enters these functions.

The odd thing this, nss itself seems to work fine in fips mode using the attached nss-example.c
Comment 12 Paul Wouters 2012-06-05 12:45:15 EDT
Created attachment 589580 [details]
test source file
Comment 13 Alexandre Oliva 2012-06-05 15:55:55 EDT
FWIW there are newer versions of the patches in http://sourceware.org/ml/libc-alpha/2012-06/msg00169.html

Now, I'm surprised you got a regression since comment 2, in which you indicate you were getting the expected results for $5 and $6 algorithms.  My patches, that addressed the remaining problems, shouldn't have had an effect on that.  What changed?  Are you by any chance running into the prelink problems again?  Or did it succeed on a glibc environment prior to the introduction of --enable-nss-crypt?
Comment 14 Alexandre Oliva 2012-06-05 18:14:33 EDT
FWIW, I've managed to duplicate the problem on a F16-ish box, with glibc master (with or without my patches) and nss-softokn-{freebl,debuginfo}-3.13.4-1.fc16.x86_64

I didn't have FIPS enabled, so I used gdb to pretend it was enabled, setting a breakpoint on nsslowhash.c:305 and setting d to '1' before proceeding (that's where it tests what it read from /proc/sys/crypto/fips_enabled).

Then I stepped into freebl_fips_SHA_PowerUpSelfTest, only run when FIPS is enabled, and noticed the problem was that the SHA224 self-test was failing: SHA224_HashBuf set sha_computed_digest to a bitstream completely different from sha224_known_digest.

I'm not sufficiently familiar with these crypto algorithms to tell whether it is the test or the implementation that is broken, but I'm pretty sure that once either of them is fixed, SHA256 and SHA512 password crypto is going to work with glibc's crypt() build with --enable-nss-crypt.

So, I'm going to clone this bug and assign the clone to nss-softokn, so that this one remains about the glibc crypt changes.
Comment 15 Paul Wouters 2012-06-13 01:43:38 EDT
The nss-softkn patch from the cloned bug does fix this problem properly. crypt() is now working properly for $5$ and $6$ salts in fips mode. I have not yet ported/tested your latest patches to see if the DES fallback is not performed in fips mode.
Comment 18 Fedora Update System 2012-12-11 14:07:19 EST
glibc-2.16-28.fc18 has been submitted as an update for Fedora 18.
Comment 19 Fedora Update System 2013-01-11 18:45:48 EST
glibc-2.16-28.fc18 has been pushed to the Fedora 18 stable repository.  If problems still persist, please make note of it in this bug report.

Note You need to log in before you can comment on or make changes to this bug.