Closed Bug 347037 Opened 15 years ago Closed 13 years ago
Make shlibsign depend on the softoken only
The shlibsign tool generates the .chk files for the shared libraries (the softoken and freebl) of the NSS cryptographic module. So it is best to consider shlibsign part of the "softoken package" and make it depend on the softoken only. Right now shlibsign uses the NSS functions defined in the "nss3" shared library.
To fix this bug, shlibsign can call either the PKCS #11 functions in the softokn3 shared library or the freebl functions in the freebl3 shared library. The latter may be easier.
Am I right to infer that the present design of shlibsign effectively drags in a bunch of non-softoken code into the crypto boundary/perimeter for FIPS analysis purposes, and the objective is to keep that code out of the set of code requiring evaluation ?
Nelson, shlibsign is outside the FIPS cryptographic boundary, but it is the tool used to generate the .chk files inside the cryptographic boundary. So it is best that shlibsign only depends on the code inside the cryptographic boundary, so that we can create a "package" (e.g., Solaris package or Linux RPM) for the softoken that contains: - the softokn3 shared library - the freebl*3 shared libraries - the .chk files - the shlibsign tool
I modified shlibsign tool to call the PKCS #11 functions in the softokn3 shared library. shlibsign is now dependent on NSPR and softokn/freebl shared libraries.
Comment on attachment 345047 [details] [diff] [review] modified shlibsign tool to call the PKCS #11 functions I have some cleanup to do.
Is this against the top of the tree? I thought Nelson had a patch which dispenses with generating the keypair on the fly. bob
The trunk no longer generates PQG params in shlibsign. See http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/cmd/shlibsign/shlibsign.c&rev=1.17&mark=133#133
I tried to make this patch a little more cvs friendly. I also built/tested on multiple platforms HP, AIX, RHEL4, SOLARIS, MACOSX, and VISTA. this shlibsign has same functionality as the previous version and roughly the same execution time as it make use of the generated PQG params that nelson provided in bug 305693.
Attachment #345047 - Attachment is obsolete: true
Ah, right, it was PQG, not key gen (the latter is more deterministic).
Comment on attachment 345190 [details] [diff] [review] shlibsign to call PKCS #11 functions r- I have a long list which I'll attach to the bug. Don't let the length discourage you. I've tried to identify everything. The basic flow is OK, there are just a couple of areas that need tightening up. bob
Attachment #345190 - Flags: review?(rrelyea) → review-
---- Must fix: The old lperror() prints out the NSS/NSPR error code as well as the given information. The new code losses this error information. It appears the conversion to using PKCS #11 calls attempted to preserve the use of this very specific error function, and thus lost it's actual functionality. This needs to be repaired. I suggest restoring the old lperror() and adding a new error handler which deals with PKCS #11 error codes specifically. (You're adding additional info in lperror is OK, If you want to keep this, it's probably easiest to just use 2 parameters. Trying to make this work with var args is probably to much work for too little benefit). I would suggest the new pkcs11 error handler simply take a string and an error code and have it produce the 0x%0x : (%s) output automatically. Otherwise simply use fprintf (which your current lperror really is). You should probably open the databases ReadOnly (flags=ReadOnly in softokn_Init). This is different than the current code (which also should have opened the database ReadOnly). Kill the confidir check. It only works with old databases. Softoken will tell you if you can't open the databases. Kill the configdir supplied requires pwd check. Not all databases have passwords (nor all tokens). In fact in FIPS mode even non-databases still require password. Please double check that PR_GetLibraryName(NULL, "softokn3") works correctly on all platforms. The pk11mode application is a test application meant to deal with a softoken library in the current directory. shlibsign is often run from some random location, and may need to do a search for the appropriate libsoftokn3.so. (will shlibsign still work finding softokn3 in the default library patch, for instance). I see 2 situations where the password semantics of the tool has changes. 1. If you supply a password without having a database (or a database without a password). The old (current) shlibsign will succeed while the new one will fail. 2. If you do not supply a password and you open a database that has now the old shlibsign will prompt you for the password, the new one will just fail. Neither situation is at all common (the most common use is running shlibsign with no database from some automatic script). I would like to see at list #1 fixed. You can use C_GetTokenInfo to determine if you need to login or not. (token_info.flags & CKF_LOGIN_REQUIRED). Semantic 2 may take some extra work. NSS will automatically keep prompting on bad passwords until the user gives up. I can be convinced that it isn't necessary... UPDATE: on further review, you can never get a password prompt. You are always using the non-FIPS crypto slot, which never needs a password. There is currently no way to convince the code to use the database slot or the FIPs slot (which is the only way to get it to prompt for a password). The question is, is this a problem. If it is, we can probably put it off to the next patch. I would still like to see semantic 1 handled correctly, especially if we had someone using shlibsign tied to a FIPS token (which is possible, but unlikely... you can't sign your own softoken and freebl that way [chicken & egg problem]). --- Must fix nits: Formatting. You have a typedef (CK_C_INITIALIZE_ARGS_NSS) in a middle of a function. It's easier to read if you just move it out. Also, I believe their is already a public declaration of this structure with these parameters in our public headers. There are lots of white space changes in this patch. Please make sure those changes are toward the NSS standard (indents: 4 spaces, tab, tab 4 spaces, etc.). It appears some tabs have been converted into spaces (ignore this if the opposite has happened). --- Would like, but not required: It's perfectly acceptable to use GetAttributeValue with only one attribute, so were you pull out the 'whole key', you can simply just pull out the CKA_VALUE attribute (which is the only one you need, since you already have PQ&G and you don't need any of the other attributes). C_Logout isn't needed to free resources. Closing all the session will automatically log the token out, and C_Finalize will automatically close all the sessions and delete all the temp objects. -- Would like, but probably more work than necessary (lack of this will not draw any comments on the next review... There is lots of code clearly copied from somewhere else (probably pk11mode). It would probably make sense to put that code in a library and share it between the two apps (not required for this checkin). bob
Comment on attachment 346917 [details] [diff] [review] make shlibsign depend on NSPR and pkcs#11 softokn/freebl r+, but there are some things I would like to see changed before you check int. 1) slotID variable should be called 'slotIndex' it is not a real slot ID (that's pSlotLists[slotIndex]). 2) Go ahead and always use slotIndex '0' for everything. 3) I wouldn't be upset if you added an lperror() call before the FPrintf after the PR_Open of the pwfile. None of these changes are required, but they are strongly encourages (particularly 1 & 2). bob
Attachment #346917 - Flags: review?(rrelyea) → review+
/cvsroot/mozilla/security/nss/cmd/shlibsign/Makefile,v <-- Makefile new revision: 1.18; previous revision: 1.17 done Checking in shlibsign.c; /cvsroot/mozilla/security/nss/cmd/shlibsign/shlibsign.c,v <-- shlibsign.c new revision: 1.18; previous revision: 1.17 done Checking in mangle/Makefile; /cvsroot/mozilla/security/nss/cmd/shlibsign/mangle/Makefile,v <-- Makefile new revision: 1.3; previous revision: 1.2 done
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.12.3
Version: 3.11 → 3.12.3
You need to log in before you can comment on or make changes to this bug.