488396 - DBM needs to be FIPS certifiable.

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Robert Relyea]

The shared db/dbm split was made to try to remove code from the FIPS boundary. One way of doing that is to not FIPS validate the DBM code.

This would be fine normally, as all the objects passed accross the sdb_ boundary (the interface to both sqlite and dbm) are all encrypted. The problem is DBM needs to decrypt that data and reencrypt it to preserve it's date interface. That means for dbm to be FIPS, the legacy dbm code needs to be included in the FIPS boundary.

To date we believe the only thing that needs to happen to make this work is to include a .chk file and verify the dbm .so file when in FIPS mode.

This bug is for making that change and any other change needed by the lab.

bob

Comment 1

[Robert Relyea]

Attachment: Add .chk file and check it in fips mode for dbm .so

I have not yet tested this patch. I intend to run 2 tests: 1) run all.sh as normal, and 2) run all.sh after deleting the .chk file for libnssdbm3.

Comment 2

[Robert Relyea]

Notes on the patch:

There are 3 parts:
part 1) build the .chk file (one line make file change.
part 2) call freebl to check the library. This should only happen in fips mode. One corner case (actually a common case) is the library gets loaded and used to read secmod.db to see if we are in FIPS mode or not. secmod.db doe no crypto ops, so we are safe using the library at this point. We only deal with CSPs once we've opened a key or certdb. Those all go through open, so at that point we know whether or not we are in FIPS mode.
part 3) We need to communicate the fact we are in fips mode to the library. This involves passing that indication down to sftk_DBInit.

Comment 3

[Robert Relyea]

I'll ask for a review once I verify that it works (I've verified that it builds already).

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 372758 [details] [diff] [review]
Add .chk file and check it in fips mode for dbm .so

Even though Bob hasn't asked for review of this yet, I'm asking myself to review this, so that it will be on my review radar.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 372758 [details] [diff] [review]
Add .chk file and check it in fips mode for dbm .so

r=nelson

Comment 6

[Robert Relyea]

Attachment: Need to pass address of a symbol in the target shared library

OK, my tests turned up a false positive. (deleting the .chk file did not prevent all.sh from passing).

the problem was I was passing the address of a symbol in softoken, not nssdbm, so the check code was checking softoken, not nssdbm.

This patch properly fails all.sh if I delete the .chk file (and passes when I don't).

bob

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 373033 [details] [diff] [review]
Need to pass address of a symbol in the target shared library

Bob, in this patch, you moved one of the two calls to BLAPI_SHVerify
so that both calls are now in the same function.  The moved call 
sets legacy_glue_libCheckSucceeded if it succeeds, but, unlike 
the previous patch, it does not set legacy_glue_libCheckFailed 
when it fails.  It that intentional?  Or a mistake?

Comment 8

[Wan-Teh Chang]

Comment on attachment 373033 [details] [diff] [review]
Need to pass address of a symbol in the target shared library

r=wtc.  Two nits below:

> static PRLibrary *
>-sftkdb_LoadLibrary(const char *libname)
>+sftkdb_LoadLibrary(const char *libname, PRBool isFIPS)

This function ignores the 'isFIPS' parameter you added.  Is
this a mistake?

>+    /* verify the loaded directory if we are in FIPS mode */
>+    if (isFIPS) {
>+	if (!BLAPI_SHVerify(LEGACY_LIB_NAME, legacy_glue_open)) {
>+	    PR_UnloadLibrary(lib);
>+	    return SECFailure;
>+	}
>+    	legacy_glue_libCheckSucceeded = PR_TRUE;
>+    } 

What does "loaded directory" mean?

Comment 9

[Robert Relyea]

re comment 7:

It's intentional. We aren't going to load the library so we don't need to mark the check 'failed' at this point.

re comment 8: 

isFIPS. good catch it was used in the previous patch, but not in this one and is no longer needed.

yes "loaded directory" should be "loaded library"

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Bob, if you don't record that the test failed, doesn't this result is many
repetitions of [ load, test, fail, unload ] ??

Comment 11

[Robert Relyea]

Attachment: Address wtc's review comments.

This patch removes the no longer necessary isFIPS parameter from the load function. It also moves the globals to near where they are used. and fixes the comment.

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 373157 [details] [diff] [review]
Address wtc's review comments.

There are two paths through one function that are both capable of 
detecting a failure to pass FIPS tests.  One of those paths sets 
a flag to remember this result, apparently to avoid repeating the
failed test over and over.  The other path does not.  I don't 
understand why they're different with regards to remembering the 
failure.  But I guess it's only a matter of efficiency and not of
correctness, and I'm not worried about the efficiency of the case
of failing to pass FIPS test while trying to be in FIPS mode.
So, r=nelson

Comment 13

[Robert Relyea]

Checking in cmd/shlibsign/Makefile;
/cvsroot/mozilla/security/nss/cmd/shlibsign/Makefile,v  <--  Makefile
new revision: 1.19; previous revision: 1.18
done
Checking in lib/softoken/lgglue.c;
/cvsroot/mozilla/security/nss/lib/softoken/lgglue.c,v  <--  lgglue.c
new revision: 1.13; previous revision: 1.12
done
Checking in lib/softoken/lgglue.h;
/cvsroot/mozilla/security/nss/lib/softoken/lgglue.h,v  <--  lgglue.h
new revision: 1.4; previous revision: 1.3
done
Checking in lib/softoken/pkcs11.c;
/cvsroot/mozilla/security/nss/lib/softoken/pkcs11.c,v  <--  pkcs11.c
new revision: 1.166; previous revision: 1.165
done
Checking in lib/softoken/sftkdb.c;
/cvsroot/mozilla/security/nss/lib/softoken/sftkdb.c,v  <--  sftkdb.c
new revision: 1.22; previous revision: 1.21
done
Checking in lib/softoken/sftkdb.h;
/cvsroot/mozilla/security/nss/lib/softoken/sftkdb.h,v  <--  sftkdb.h
new revision: 1.10; previous revision: 1.9
done

patch checked in...

I'll keep this open in case additional DBM FIPS issues arrise

Comment 14

[Wan-Teh Chang]

Comment on attachment 373157 [details] [diff] [review]
Address wtc's review comments.

r=wtc.

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

I think we can declare victory now.
We can reopen if the Lab has issues.