762198 - nsslowhash sha224 powerup self-test fails on fips mode

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Elio Maldonado]

Originally reported in Fedora 17:

Alexandre Oliva 2012-06-05 18:34:08 EDT

+++ This bug was initially created as a clone of Bug #811753 +++

Description of problem:
crypt() returns NULL in fipsmode 

crypt returned NULL for salt $6$FOOBAR
crypt returned NULL for salt $5$FOOBAR

Expected results:
$6$FOOBAR$yPOQuPE1ruwOrYiApWDYw9BxeLk/oIjajCgzubzO0PYEos/vnqpTnNI12VIdiUF8hiHT9bMslahk/0Oikb.nA.
$5$FOOBAR$cDCnc4lB6JRog7XV9JxxoKiWTHbH09P9xkfnf237ke7

--- Additional comment from [e-mail] on 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.

[reply] [-]
Private
Comment 1 Elio Maldonado Batiz 2012-06-06 15:26:03 EDT

Though we can't currently enable system wide fips mode by doing a buildwith tis change
-    if (!post && nsslow_GetFIPSEnabled()) {
+    if (!post && (1 || nsslow_GetFIPSEnabled())) { /* force the test to be run always */
 	crv = freebl_fipsPowerUpSelfTest();
 	if (crv != CKR_OK) {
 	    post_failed = 1;
that forces execution of the powerup self-tests regradless and I can reproduce the problem. I booted in single user mode okay and switched to run level 3 where authentication of root failed, likewise on run level 5 regular user password was rejected. I checked the expected answer and it matches a value from
http://csrc.nist.gov/groups/ST/toolkit/documents/Examples/SHA224.pdf
so the computed value is indeed wrong.

Comment 1

[Elio Maldonado]

> user password was rejected. I checked the expected answer and it matches a
> value from
> http://csrc.nist.gov/groups/ST/toolkit/documents/Examples/SHA224.pdf
> so the computed value is indeed wrong.

No, it doesn't match. In fact the messages there are are quite diffrent from what we use which is a common message for all algorithms. This self-test is intended to check that we produce the same answer to the same message it is not a known answer test in the fips sense. It is not the actual implemnttion of sha224 (based on sha256) but the value used as expected in the test which is wrong. 

For some odd reason I have in mozilla/security/nss/lib/freebl/nsslowhash.c,v
/* known SHA224 hmac (28 bytes) */
    static const PRUint8 known_SHA224_hmac[] = {
        0x1c, 0xc3, 0x06, 0x8e, 0xce, 0x37, 0x68, 0xfb, 
        0x1a, 0x82, 0x4a, 0xbe, 0x2b, 0x00, 0x51, 0xf8,
        0x9d, 0xb6, 0xe0, 0x90, 0x0d, 0x00, 0xc9, 0x64,
        0x9a, 0xb8, 0x98, 0x4e };

wheres in mozilla/security/nss/lib/softoken/fipstest.c
/* SHA-224 Known Digest Message (224-bits). */
    static const PRUint8 sha224_known_digest[] = {
        0x89,0x5e,0x7f,0xfd,0x0e,0xd8,0x35,0x6f,
        0x64,0x6d,0xf2,0xde,0x5e,0xed,0xa6,0x7f, 
        0x29,0xd1,0x12,0x73,0x42,0x84,0x95,0x4f, 
        0x8e,0x08,0xe5,0xcb
    };

Sure enough, when I replace the softoken value with the one from nsslowhash the higher level fips tests (set and unset fips mode) fail for sha224. The value from softoken/fipstest is the correct one and the fix is to use that on the nsslowhash.c. I also verified that this fixes the problem in fedora.

Comment 2

[Elio Maldonado]

Attachment: Use the same expected value as in softoken selftests

Comment 3

[Elio Maldonado]

Attachment: Lowhash API test suite

Theis test suite goes beyond the scope of this bug and is proabably worth a separate bug.

Comment 4

[Elio Maldonado]

Attachment: V2: Lowhash API test suite

Comment 5

[Elio Maldonado]

A minimalist reproducer with instructions on how to, in a safe manner, reproduce the bug and confirm its fix is at https://bugzilla.redhat.com/attachment.cgi?id=591327.

Comment 6

[Robert Relyea]

Comment on attachment 632328 [details] [diff] [review]
Use the same expected value as in softoken selftests

r+ rrelyea

Comment 7

[Robert Relyea]

Comment on attachment 632740 [details] [diff] [review]
V2: Lowhash API test suite

r+ with some comments:

1) don't check in our fipstest.c change. There is a more complete version in my DSA-2 checkin awaiting review. (DSA-2 picks up lots of new hwshes. as part of this I've aded support for 224 in a more generic way). Your patch would needlessly conflict with mine.

OK, there is only one comment, I've convinced myself that the long test (1 million) is sufficient to cover the rollover case we were worried about (which happens at 64 K).

bob

Comment 8

[Elio Maldonado]

Fix and new tests checked in to trunk:
/cvsroot/mozilla/security/nss/cmd/manifest.mn,v  <--  manifest.mn
new revision: 1.33; previous revision: 1.32
done
RCS file: /cvsroot/mozilla/security/nss/cmd/lowhashtest/Makefile,v
done
Checking in mozilla/security/nss/cmd/lowhashtest/Makefile;
/cvsroot/mozilla/security/nss/cmd/lowhashtest/Makefile,v  <--  Makefile
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/cmd/lowhashtest/lowhashtest.c,v
done
Checking in mozilla/security/nss/cmd/lowhashtest/lowhashtest.c;
/cvsroot/mozilla/security/nss/cmd/lowhashtest/lowhashtest.c,v  <--  lowhashtest.c
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/cmd/lowhashtest/manifest.mn,v
done
Checking in mozilla/security/nss/cmd/lowhashtest/manifest.mn;
/cvsroot/mozilla/security/nss/cmd/lowhashtest/manifest.mn,v  <--  manifest.mn
initial revision: 1.1
done
Checking in mozilla/security/nss/tests/all.sh;
/cvsroot/mozilla/security/nss/tests/all.sh,v  <--  all.sh
new revision: 1.59; previous revision: 1.58
done
RCS file: /cvsroot/mozilla/security/nss/tests/lowhash/lowhash.sh,v
done
Checking in mozilla/security/nss/tests/lowhash/lowhash.sh;
/cvsroot/mozilla/security/nss/tests/lowhash/lowhash.sh,v  <--  lowhash.sh
initial revision: 1.1
done

Comment 9

[Elio Maldonado]

Attachment: V3: The test patch as it was actually checked in, no fipstest changes.

Comment 10

[Elio Maldonado]

Changes checked in to fix Tinderbox
new revision: 1.2; previous revision: 1.1
done

Comment 11

[Elio Maldonado]

A syntax error was also breaking Tinderfox
/cvsroot/mozilla/security/nss/cmd/lowhashtest/lowhashtest.c,v  <--  lowhashtest.c
new revision: 1.2; previous revision: 1.1
done

Comment 12

[Elio Maldonado]

Attachment: Remove faulty line from makefile - already applied

Comment 13

[Elio Maldonado]

Attachment: printf fix on usage function - applied

Comment 14

[Elio Maldonado]

Attachment: Build lowhashtest only for Linux.

Comment 15

[Elio Maldonado]

Comment on attachment 632953 [details] [diff] [review]
Build lowhashtest only for Linux.

Would 

+ifeq ($(FREEBL_LOWHASH),1)
+LOWHASHTEST_SRCDIR = lowhashtest  # Add the lowhashtest directory to DIRS.
+endif

be a better way of testing? It resembles what is done on freebl.

Comment 16

[Robert Relyea]

Comment on attachment 632953 [details] [diff] [review]
Build lowhashtest only for Linux.

r- you need to build only if the proper FREEBL define is set.

Comment 17

[Robert Relyea]

r+ for the patch in comment 15  (please test it first, of course).

Comment 18

[Elio Maldonado]

Attachment: Run lowhashtest only if built and build it if FREEBL_LOWHASH=1

Comment 19

[Kai Engert [:KaiE:]]

Only Linux machines succeeded building.

Mac, Windows, SunOS tinderbox build machines are all busted.

I'll have a quick look if I can figure out what to do, if I can't, I'll backout.

Comment 20

[Kai Engert [:KaiE:]]

Are the NSSLOWHASH_Begin functions supposed to be Linux specific, 
or supposed to be cross platform code?

Comment 21

[Kai Engert [:KaiE:]]

Comment on attachment 633078 [details] [diff] [review]
Run lowhashtest only if built and build it if FREEBL_LOWHASH=1

This appears to make sense, although this should get some cleanup.

I'd use a default init of variable LOWHASHTEST_SRCDIR to empty (prior to the check for FREEBL_LOWHASH, but that's my personal taste, your code will work.

Instead of checking explicitly for ".exe", it would be good to use a check with $BINSUFFIX (variable might have a slightly similar name).

Given tinderbox redness, I'll attempt to checkin this patch, and you could address review requests in a followup patch.

Comment 22

[Kai Engert [:KaiE:]]

Elio was quicker than I...

Comment 23

[Elio Maldonado]

This checkin happened before I had read your comments, and done for the same reason.

Checking in mozilla/security/nss/cmd/Makefile;
/cvsroot/mozilla/security/nss/cmd/Makefile,v  <--  Makefile
new revision: 1.10; previous revision: 1.9
done
Checking in mozilla/security/nss/cmd/manifest.mn;
/cvsroot/mozilla/security/nss/cmd/manifest.mn,v  <--  manifest.mn
new revision: 1.34; previous revision: 1.33
done
Checking in mozilla/security/nss/tests/lowhash/lowhash.sh;
/cvsroot/mozilla/security/nss/tests/lowhash/lowhash.sh,v  <--  lowhash.sh
new revision: 1.2; previous revision: 1.1
done

I will address your comments soon, let us see green first.  Thanks you.

Comment 24

[Robert Relyea]

Comment on attachment 633078 [details] [diff] [review]
Run lowhashtest only if built and build it if FREEBL_LOWHASH=1

r+ rrelyea

Comment 25

[Elio Maldonado]

Attachment: Clanup suggested by Kai on Comment 21

Comment 26

[Kai Engert [:KaiE:]]

Comment on attachment 655714 [details] [diff] [review]
Clanup suggested by Kai on Comment 21

Elio, PROG_SUFFIX already includes the dot

Please use
  lowhashtest${PROG_SUFFIX}

r- on this patch.

r+ on a new patch with the dot removed.

Comment 27

[Elio Maldonado]

Checked in to trunk:
rity/nss/cmd/Makefile ./mozilla/security/nss/tests/lowhash/lowhash.sh
Checking in ./mozilla/security/nss/cmd/Makefile;
/cvsroot/mozilla/security/nss/cmd/Makefile,v  <--  Makefile
new revision: 1.11; previous revision: 1.10
done
Checking in ./mozilla/security/nss/tests/lowhash/lowhash.sh;
/cvsroot/mozilla/security/nss/tests/lowhash/lowhash.sh,v  <--  lowhash.sh
new revision: 1.3; previous revision: 1.2
done

Comment 28

[Elio Maldonado]

Attachment: The patch as it was applied.

Dropped the unwanted period.

Comment 29

[Elio Maldonado]

This last checkin caused Tinderbox build failures on Mac OS X and Windows.

I tested on Mac OS X and the suspect is the first line in:
+LOWHASHTEST_SRCDIR = ""
 ifeq ($(FREEBL_LOWHASH),1)
 LOWHASHTEST_SRCDIR = lowhashtest  # Add the lowhashtest directory to DIRS.
 endif

When I change it to 
LOWHASHTEST_SRCDIR=
The build proceeds. I'm currently waiting for the test suite to complete.

Comment 30

[Elio Maldonado]

Attachment: Fix for MacOSX/Windows build failures

Comment 31

[Kai Engert [:KaiE:]]

Comment on attachment 656485 [details] [diff] [review]
Fix for MacOSX/Windows build failures

checked in as an attempt to fix bustage

Checking in Makefile;
/cvsroot/mozilla/security/nss/cmd/Makefile,v  <--  Makefile
new revision: 1.12; previous revision: 1.11
done

Comment 32

[Robert Relyea]

Comment on attachment 656485 [details] [diff] [review]
Fix for MacOSX/Windows build failures

r+ rrelyea

Comment 33

[Elio Maldonado]

Reopening, this bug is not fixed because attachment 632328 [details] [diff] [review] was never applied.

Comment 34

[Elio Maldonado]

Attachment: V2: Use same expected values as in softoken selftest

Essentially the same as the one that got r+' - updated to account for the code having moved by one line.

Comment 35

[Elio Maldonado]

The tests didn't detect this because they were actually testing the code softoken and not the one in nsslowhash.c. That source file has CK_RV freebl_fipsPowerUpSelfTest( void ) which the tests should call to ensure they are testing the correct library. I propose that we declare it in nsslowhash.h and have the test call it. If it is beyond the original scope of this bug I would enter a a separate bug. Bob, what do you think? Are there any security reasons to keep freebl_fipsPowerUpSelfTest() test private?

Comment 36

[Elio Maldonado]

If adding it to nsslowhash.h is not wise perhaps we could create a private header for the benefit of the tests.

Comment 37

[Robert Relyea]

Comment on attachment 684123 [details] [diff] [review]
V2: Use same expected values as in softoken selftest

r+ rrelyea

Comment 38

[Robert Relyea]

Do not close this bug until you've added a freebl test which tests the NSSLOWHASH interface for freebl in FIPS mode. This will save you many headaches in the future....

bob

Comment 39

[Elio Maldonado]

Committed to the TRUNK for 3.14.1
curity/nss/lib/freebl/nsslowhash.c
Checking in ./mozilla/security/nss/lib/freebl/nsslowhash.c;
/cvsroot/mozilla/security/nss/lib/freebl/nsslowhash.c,v  <--  nsslowhash.c
new revision: 1.10; previous revision: 1.9
done

Keeping it open as there is more to come testing wise.

Comment 40

[Elio Maldonado]

(In reply to Robert Relyea from comment #38)
> Do not close this bug until you've added a freebl test which tests the
> NSSLOWHASH interface for freebl in FIPS mode. This will save you many
> headaches in the future....

For a headache-free future I must ensure that one of our Tinderbox reporting test systems has fips_enabled turned on. Only specific relaeses of RHEL/Fedora are guaranteed to work with FIPS turned on. I'll work with Kai to find a suitable one.

Comment 41

[Wan-Teh Chang]

Elio: all of the Tinderboxes run some tests with FIPS mode enabled. So if you
make NSSLOWHASH tests part of the Softoken FIPS power-up self-test, you will
have test coverage, with no Tinderbox changes needed.

A note on the meaning of two version-related fields:

Version: the NSS version in which the bug was introduced. If unknown, just use
the default value "trunk".

Target Milestone: the NSS version in which the bug is fixed.

The sha224_known_digest array was added in NSS 3.13 (bug 356713), so the Version
field of this bug should be 3.13.

Comment 42

[Elio Maldonado]

(In reply to Wan-Teh Chang from comment #41)

> Elio: all of the Tinderboxes run some tests with FIPS mode enabled. So if you
> make NSSLOWHASH tests part of the Softoken FIPS power-up self-test, you will
> have test coverage, with no Tinderbox changes needed.

The softoken FIPS powerup self tests test softoken but the Low Hash test I need to to tests in the one on freebl and that's the one that glibc calls. In the main function for my test
http://mxr.mozilla.org/security/source/security/nss/cmd/lowhashtest/lowhashtest.c#409 I have the call initCtx = NSSLOW_Init(); Look at its implementation at
http://mxr.mozilla.org/security/source/security/nss/lib/freebl/nsslowhash.c#287

...
304     if (!post && nsslow_GetFIPSEnabled()) {
305 	crv = freebl_fipsPowerUpSelfTest();
306 	if (crv != CKR_OK) {
307 	    post_failed = 1;
308 	    return NULL;
309 	}
....
Unless nsslow_GetFIPSEnabled() return tre Line 305 dosn't get executed. 

See http://mxr.mozilla.org/security/source/security/nss/lib/freebl/nsslowhash.c#218
Unless it's a Linux installation that has "/proc/sys/crypto/fips_enabled" that whe you query it gives you 1 (because the system was configured to be on FIPS mode) the freebl powerup self-test won't get executed.  That's what hid from me that I had actually not applied the my patch. 

We do need a system that supports FIPS mode - the system-wide FIPS mode.  RHEL-6.2 is guaranteed to work as it was actually certified by Red Hat. Fedora-12 I verified supported FIPS but it is too old. Recent versions of fedora don't cut it though I expect the upcoming F-18 will but it hasn't been fixed yet.

Comment 43

[Elio Maldonado]

Instructions to set a Red Hat Enterprise Linux system, or Fedora, on FIPS mode are at
https://access.redhat.com/knowledge/docs/en-US/Red_Hat_Enterprise_Linux/6/html/Security_Guide/sect-Security_Guide-Federal_Standards_And_Regulations-Federal_Information_Processing_Standard.html
See section 7.2.1. Enabling FIPS Mode
and as root you can execute cat /proc/sys/crypto/fips_enabled to see fips is enabled.

Comment 44

[Kai Engert [:KaiE:]]

I'll install a RHEL 6 VM with FIPS enabled.

Comment 45

[Kai Engert [:KaiE:]]

Comment on attachment 633078 [details] [diff] [review]
Run lowhashtest only if built and build it if FREEBL_LOWHASH=1

>+    for fips_mode in 0 1; do
>+      echo "lowhashtest with fips mode=${mode}"
>+      export NSS_FIPS=${fips_mode}
>+      for TEST in ${TESTS}
>+      do
>         echo "lowhashtest ${TEST}"
>         ${BINDIR}/lowhashtest ${TEST} 2>&1
>         RESULT=$?
>         html_msg ${RESULT} 0 "lowhashtest with fips mode=${mode} for ${TEST}"
>+      done
>     done


Elio, the lowhash.sh script still refers to ${mode} in two places, that should be changed to ${fips_mode} everywhere, to make sure the created output helpful.

(Currently our output never says "mode=0" nor "mode=1", it always says "mode=" (empty)).

Comment 46

[Kai Engert [:KaiE:]]

Attachment: patch to fix the mode output when running the lowhash text [checked in]

Comment 47

[Kai Engert [:KaiE:]]

It would be good if the output of the test in some way indicated that the system has FIPS enabled, and that the additional power up tests indeed have been executed.

Comment 48

[Kai Engert [:KaiE:]]

Attachment: patch to report the system's FIPS setting in the test output [checked in]

Comment 49

[Kai Engert [:KaiE:]]

A new build slave has been added to http://tinderbox.mozilla.org/showbuilds.cgi?tree=NSS

Its name is "nssfips", it's a RHEL 6.3 system with FIPS enabled systemwide.

For testing purposes, the system uses the two small patches that I attached.

Comment 50

[Kai Engert [:KaiE:]]

Comment on attachment 691394 [details] [diff] [review]
patch to fix the mode output when running the lowhash text [checked in]

Checking in lowhash.sh;
/cvsroot/mozilla/security/nss/tests/lowhash/lowhash.sh,v  <--  lowhash.sh
new revision: 1.4; previous revision: 1.3
done

Comment 51

[Kai Engert [:KaiE:]]

Comment on attachment 691420 [details] [diff] [review]
patch to report the system's FIPS setting in the test output [checked in]

Checking in lowhash.sh;
/cvsroot/mozilla/security/nss/tests/lowhash/lowhash.sh,v  <--  lowhash.sh
new revision: 1.5; previous revision: 1.4
done