Closed Bug 232604 Opened 17 years ago Closed 15 years ago
shlibsign tries to open DBs in $HOME/.netscape, even on windows
Background: The shlibsign program always tries to init NSS DBs, and if that fails, it calls NSS_NoDB_Init(). It takes an optional -d <directory> argument, to tell it where to look for the DBs, but the makefile does not provide a -d argument when it invokes shlibsign to sign the shared libs. In the absense of a -d argument, shlibsign tries to open the DBs in $HOME/.netscape, on all platforms. This is particularly bad for the QA progams, which ought to produce the same results regardless of which user runs them and what that user has in his/her $HOME/.netscape directory. The bug: Shlibsign should not be attempting to open DBs in $HOME/.netscape. I don't know whether there is EVER any case where shlibsign needs any certs or user's private keys, but if not, then shlibsign should not use any DBs. Proposed solutions: a) change the makefile to invoke shlib sign with a -d argument that points to an always non-existent directory, or b) change shlibsign to always call NSS_NoDB_Init, and never attempt to open DBs.
QA Contact: bishakhabanerjee → jason.m.reid
The -d option for shlibsign never worked, as you can see in this patch. SECU_ConfigDirectory was invoked first with the argument value, then again with a NULL value in the following line : rv = NSS_Init(SECU_ConfigDirectory(NULL)); This patch removes the -d option which couldn't have been used successfully.
Assignee: wtchang → julien.pierre.bugs
Status: NEW → ASSIGNED
Attachment #202446 - Flags: review?(nelson)
Comment on attachment 202446 [details] [diff] [review] always call NSS_NoDB_Init I agree this patch is the right fix. The content of the DB files in the directory named by the -d option was never used in any way, AFAICT. But I disagree with the assertion that the -d option never worked, and the implication that SECU_ConfigDirectory was being used improperly. I'll send email about that.
Attachment #202446 - Flags: review?(nelson) → review+
Thanks for the review, Nelson. I didn't fully realize the behavior of SECU_ConfigDirectory with multiple calls. It wasn't my intention to take away an option that worked, even if our scripts didn't use it. So, I have supplied this new patch.
Well, AFAIK, shlibsign doesn't use any certs from the cert DB at all, nor any keys from the key DB. So, whether it finds a cert DB or not really makes no difference. The -d option leads the user to believe that the choice of cert DB makes some difference to the program's behavior, when AFAIK, it does not. So, I think it is best to not offer the -d option and to always use NODB_Init. Bob, if you think there is any reason to retain the -d option for shlibsign, please speak up here and tell us what the cert DB is actually used for (or intended to be used for). Otherwise, I think Julien should proceed with his first patch above (which I already marked r+ ).
Even though no certs are used, shlibsign does a DSA keygen, and attempts to find the best slot for that operation. One might want to do a keygen in a hardware device. The -d option allows for a secmod.db with such a device configured, so it has some value.
I think it's fine to leave out the -d function. If we want allow other tokens to be used, we can add back the -d as well as a -t function to specify the token. bob
Bob, -t would only be needed if one wanted to do the keygen in the non-default token for DSA. secmod.db already has a preference for DSA, which would indicate the token. The -d option and modutil already give you the ability to choose the token for the keygen. I would rather not take away this capability, so I prefer the second patch to the first one.
Comment on attachment 202562 [details] [diff] [review] don't take away working -d option. Only call NSS_NoDB_Init if -d is not supplied This patch does not follow the prevailing coding style of this file, which is the "K&R" style. I'll give this patch r+ provided that the style of braces/indentation is changed to match the rest of the source file.
Attachment #202562 - Flags: review?(nelson) → review+
Thanks, Nelson. I checked in the second patch after changing the braces . Checking in shlibsign.c; /cvsroot/mozilla/security/nss/cmd/shlibsign/shlibsign.c,v <-- shlibsign.c new revision: 1.14; previous revision: 1.13 done Marking fixed.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.