Last Comment Bug 347037 - Make shlibsign depend on the softoken only
: Make shlibsign depend on the softoken only
Status: RESOLVED FIXED
FIPS
:
Product: NSS
Classification: Components
Component: Tools (show other bugs)
: 3.11
: All All
: P4 enhancement (vote)
: 3.12.3
Assigned To: glen beasley
:
:
Mentors:
Depends on: 372162 1154106
Blocks: 360426 FIPS2008
  Show dependency treegraph
 
Reported: 2006-08-02 12:07 PDT by Wan-Teh Chang
Modified: 2015-04-13 17:28 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
modified shlibsign tool to call the PKCS #11 functions (45.64 KB, patch)
2008-10-27 21:38 PDT, glen beasley
no flags Details | Diff | Splinter Review
shlibsign to call PKCS #11 functions (46.32 KB, patch)
2008-10-28 16:23 PDT, glen beasley
rrelyea: review-
Details | Diff | Splinter Review
make shlibsign depend on NSPR and pkcs#11 softokn/freebl (44.37 KB, patch)
2008-11-07 09:54 PST, glen beasley
rrelyea: review+
Details | Diff | Splinter Review

Description Wan-Teh Chang 2006-08-02 12:07:12 PDT
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.
Comment 1 Wan-Teh Chang 2006-08-02 16:25:08 PDT
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.
Comment 2 Nelson Bolyard (seldom reads bugmail) 2006-08-02 17:36:11 PDT
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 ?
Comment 3 Wan-Teh Chang 2006-08-02 19:03:19 PDT
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
Comment 4 glen beasley 2008-10-27 21:38:16 PDT
Created attachment 345047 [details] [diff] [review]
modified shlibsign tool to call the PKCS #11 functions

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 5 glen beasley 2008-10-28 07:39:13 PDT
Comment on attachment 345047 [details] [diff] [review]
modified shlibsign tool to call the PKCS #11 functions

I have some cleanup to do.
Comment 6 Robert Relyea 2008-10-28 14:17:38 PDT
Is this against the top of the tree? I thought Nelson had a patch which dispenses with generating the keypair on the fly.

bob
Comment 7 Nelson Bolyard (seldom reads bugmail) 2008-10-28 15:29:29 PDT
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
Comment 8 glen beasley 2008-10-28 16:23:55 PDT
Created attachment 345190 [details] [diff] [review]
shlibsign to call PKCS #11 functions

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.
Comment 9 Robert Relyea 2008-10-28 16:29:27 PDT
Ah, right, it was PQG, not key gen (the latter is more deterministic).
Comment 10 Robert Relyea 2008-10-28 18:28:09 PDT
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
Comment 11 Robert Relyea 2008-10-28 18:35:22 PDT
---- 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 12 glen beasley 2008-11-07 09:54:13 PST
Created attachment 346917 [details] [diff] [review]
make shlibsign depend on NSPR and pkcs#11 softokn/freebl
Comment 13 Robert Relyea 2008-11-17 13:58:26 PST
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
Comment 14 glen beasley 2008-11-20 07:47:09 PST
/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

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