Closed Bug 488396 Opened 11 years ago Closed 11 years ago

DBM needs to be FIPS certifiable.

Categories

(NSS :: Libraries, defect, P1, critical)

3.12.4
defect

Tracking

(Not tracked)

RESOLVED FIXED
3.12.4

People

(Reporter: rrelyea, Assigned: rrelyea)

Details

(Whiteboard: FIPS SUN_MUST_HAVE)

Attachments

(1 file, 2 obsolete files)

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
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.
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.
I'll ask for a review once I verify that it works (I've verified that it builds already).
Priority: -- → P1
Whiteboard: FIPS SUN_MUST_HAVE
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.
Attachment #372758 - Flags: review? → review?(nelson)
Comment on attachment 372758 [details] [diff] [review]
Add .chk file and check it in fips mode for dbm .so

r=nelson
Attachment #372758 - Flags: review?(nelson) → review+
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
Attachment #372758 - Attachment is obsolete: true
Attachment #373033 - Flags: review?(nelson)
Attachment #373033 - Flags: superreview?(wtc)
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?
Attachment #373033 - Attachment description: Need to pass address of a symbol in the target .sh → Need to pass address of a symbol in the target shared library
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?
Attachment #373033 - Flags: superreview?(wtc) → superreview+
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"
Bob, if you don't record that the test failed, doesn't this result is many
repetitions of [ load, test, fail, unload ] ??
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.
Attachment #373033 - Attachment is obsolete: true
Attachment #373157 - Flags: superreview?(wtc)
Attachment #373157 - Flags: review?(nelson)
Attachment #373033 - Flags: review?(nelson)
Attachment #373157 - Flags: review?(nelson) → review+
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
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
Status: NEW → ASSIGNED
Attachment #373157 - Flags: superreview?(wtc) → superreview+
Comment on attachment 373157 [details] [diff] [review]
Address wtc's review comments.

r=wtc.
I think we can declare victory now.
We can reopen if the Lab has issues.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.