mozilla apps must manage NSS configuration for PKCS#11 shared libs, including nssckbi.dll

RESOLVED FIXED in mozilla1.9alpha6

Status

()

defect
P1
major
RESOLVED FIXED
17 years ago
12 years ago

People

(Reporter: timeless, Assigned: kaie)

Tracking

Trunk
mozilla1.9alpha6
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.3b -
blocking1.7a -
blocking1.7b -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [kerh-coz])

Attachments

(5 attachments, 3 obsolete attachments)

Reporter

Description

17 years ago
this is not a new problem, i notice it whenever i try to upgrade an inactive
mozilla with mozillas open.  steps:
run a few mozillas and xpcshells
with one of them, download a new nightly (zip) and open with winzip. pick a
mozilla directory from which no mozilla.exe instances are running, and extract
mozilla there.

you can get two errors
one for xpcshell (nspr.dll is in use) - that's correct, just quit xpcshell and
remember to check for xpcshell before you decide a mozilla directory is unused
one for nssckbi.dll - that's this bug. 

in this case, i have:
F:\DOCUMENT\ADMINI~1.RAI\Desktop\sea\nssckbi.dll
F:\DOCUMENT\ADMINI~1.RAI\Desktop\bin\nssckbi.dll
F:\DOCUMENT\ADMINI~1.RAI\Desktop\1016\bin\nssckbi.dll

i have two mozilla's running (MOZ_NO_REMOTE=1)
both are running from:
F:\DOCUMENT\ADMINI~1.RAI\Desktop\sea\mozilla.exe
nearly all libraries associated with them come from \sea\. with one exception:
nssckbi.dll

the info is from process explorer (sysinternals.com)

dbradley suggested perhaps somewhere near 
http://lxr.mozilla.org/seamonkey/source/security/nss/lib/nss/nssinit.c#302
but i really have no idea.

What bugs me the most about this isn't that it picked a different directory, but
that it seems to have picked it randomly. (i suppose it could be bound to the
instance that last ran a profile, but...)
Reporter

Comment 1

17 years ago
Posted file Mozilla1872.exe.txt
Reporter

Comment 2

17 years ago
Posted file Mozilla1960.exe.txt

Comment 3

17 years ago
Changed product to PSM.  This is a known problem.
Assignee: wtc → ssaux
Component: Libraries → Client Library
Product: NSS → PSM
QA Contact: bishakhabanerjee → junruh
Assignee

Comment 4

17 years ago
This should have been fixed by bug 147280.
Older builds will use the DLL that is referenced in secmod.db.
Builds with 147280 will use the DLL in secmod.db only if it is new enough,
otherwise the DLL from the currenty running build's directory is used.

*** This bug has been marked as a duplicate of 147280 ***
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → DUPLICATE
Reporter

Comment 5

17 years ago
that's not good enough.

1. all the builds mentioned with the exception of 1016 which is the dud your
stupid library used are much newer than your fix.
2. you should never do this. if you need to upgrade a library prompt the user
and ask for permission. if i don't get a prompt explaining you're going to do
something stupid, then i will consider behavior like this to be a bug.
3. you should prefer the local version over the database version and update the
stupid path pointer.

the current system (which is clearly in place post your fix) resulted in three
profiles with different opinions of newest stupid dll. and poor mozilla was
forced to use that preference over a local version.

what should have happened is:
* when i ran sea, the date+version info which was newer than the info for my
profiles should have forced an update of the path record (if the file was
writable, which it was) and in any case been used instead of the older (by date
if not version) module specified by the file.

what should happen in the event that I run an old version of mozilla against a
profile which knows about a newer version of this library:
* i should get a prompt which says that the profile is associated with an
updated crypo database and I have the following choices:
A. Update mozilla to use the new database
B. Use the new database from <inform me about where it lives>
--optional--
C. Use the internal database, ignoring the new database (some warning: ...)

Finally. If at all possible, load the library and unlock it so that it can be
deleted.

The system asis means that if there is a newer library on my system, it isn't
guarnateed to taint across profiles, and some old library which i might want to
delete by running an installer will be locked. given that i use mozilla for very
long periods of time and rarely can afford to quit it, the lock on some random
(and it is essentially random, since it's whichever mozilla was around when a
profile was created), it means that if i were a network administrator and was
trying to upgrade n7 while some user was running mozilla, it might fail because
of this stupid lock.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Summary: mozilla seems to load a random nssckbi.dll → mozilla should not lock a random nssckbi.dll

Comment 6

17 years ago
Based on comment #5, setting this to enhancement. Based upon the rest of the
list of PSM bugs, this problem does not seem to be a common occurrence.
Severity: major → enhancement
OS: Windows 2000 → All
Priority: -- → P5
Hardware: PC → All
Target Milestone: --- → Future
Version: unspecified → 2.4

Comment 7

17 years ago
I see possible problems when an embedded version would run along side a version
of Mozilla or Netscape in another directory. This could cause problems during
updates and such.

Personally I run into this a fair bit when I'm building, as Netscape has chosen
to use a dll from my build rather than its own. What I don't know is if this was
the result of me mixing use of profiles. I generally try and keep Netscape on
one profile and Mozilla on another, but I can't say I've done that 100%.
Flags: blocking1.3b?
I see this often as well.  It's annoying to have to close down Netscape 7
everytime I want to build Mozilla on win32.  This is not an enhancement request.
 This is a bug that needs to be fixed.  Why is the path to the dll being stored
in the profile to begin with?

Severity: enhancement → major

Comment 9

17 years ago
not going to hold 1.3beta for this.
Flags: blocking1.3b? → blocking1.3b-

Updated

17 years ago
Blocks: 134113
Posted patch wip (obsolete) — Splinter Review
Here's a hack to avoid storing the root certs altogether.  Unfortunately, it
doesn't remove already stored root certs so you'd have to remove the secmod.db
file (probably not generally recommended) before it will be effective.
This has become a bigger issue for OS/2 now that we have two builds (GCC and
VACPP) - a profile cannot be shared between the two builds.

Can we get this fixed please?

Updated

16 years ago
No longer blocks: 134113

Comment 12

16 years ago
hello -- I think I'm observing this after renaming a Mozilla profile in v1.3 and
then attempting to install/use v1.4x.    

Moz build tested from (including) v1.4a to present all tell me "unable to load
PSM" when I access SSL-enabled web/imap.  

I checked contents of secmod.db and it has reference to OLD profile location.  -GA
*** Bug 74750 has been marked as a duplicate of this bug. ***
Assignee: ssaux → kaie
Status: REOPENED → NEW
Flags: blocking1.7a?
QA Contact: junruh → bmartin

Comment 14

16 years ago
kai, do you have cycles to work on this?  any ideas on who could help?

Updated

16 years ago
Flags: blocking1.7b?
Flags: blocking1.7a?
Flags: blocking1.7a-
Assignee

Comment 15

16 years ago
If your problem is with the contents of secmod.db pointing to an invalid
library, I think we should classify this bug as a NSS bug.

The NSS library is loading the referenced library automatically.
I believe the pathname of the DLLs recorded in secmod.db is under the control
of PSM.  NSS opens the pathnames that it has been told to open.

Comment 17

16 years ago
any more thoughts on how we could get this fixed?
did anyone ever answer the question as to why secomd.db needs a fully qualified
path name to the DLL?

Comment 19

16 years ago
So the problem with relative paths is making sure the 'right' version is loaded.

PSM could do the following:
1) when initializing NSS, change directory to the executible directory.
2) initalize nss.
3) check that nssckbi was properly loaded, if not then load ./nssckbi.dll (.so,
.dylib, etc).
4) restore current directory.

The problem with his is Older versions of mozilla would not be able to load
nssckbi.dll, and add their own full path back into secmod.db

This assumes other threads aren't accessing files when NSS is initializing. If
this restriction could be guarrenteed, then only the local version of nssckbi
will get loaded every time.

Comment 20

16 years ago
trying to get 1.7b out.  renominate if something appears soon.
Flags: blocking1.7b? → blocking1.7b-

Comment 21

15 years ago
Mike,

With the NSS secmod.db, you have a choice of using either relative or relative
pathnames.

You could use relative pathnames in secmod.db if all the PKCS#11 DLLs you are
loading are stored in a location relative to that secmod.db file.

However, you cannot guarantee that because :

1) Third-party PKCS#11 libraries are installed to a location chosen by the
vendor. They don't reside in a directory relative to the secmod.db . On Windows
and OS/2, secmod.db and the PKCS#11 library may be on different drives. So an
absolute pathname is required for them.

2) On Windows and OS/2, the profile and therefore the secmod.db can be located
on a different drive than the one nssckbi.dll is installed to. So again, an
absolute pathname is required to load nssckbi.dll

Therefore, on Windows and OS/2, absolute pathnames are required in order to
guarantee that Mozilla can load any and all PKCS#11 DLLs, both built-in and
third-party. On Unix you might be able to work your way up to "/" using "..",
and always build relative pathnames (I'm not sure, due to symlinks).

The good news is that you don't have to store a reference to an absolute
pathname to nssckbi.dll in secmod.db in the profile in order to load nssckbi.dll
.  PSM could load nssckbi.dll dynamically using the SECMOD_LoadModule API, which
will *not* add it to secmod.db, unlike its counterpart SECMOD_AddModule . This
technique has been used successfully in at least one commercial application before.

For SECMOD_LoadModule to work, you would have to determine the absolute location
of the version of nssckbi.dll you want to load. But you don't have to determine
that location from the profile data. You could do it programmatically. Since PSM
loads NSS on-demand, it already knows the location of all the correct NSS DLLs,
including nssckbi.dll .

So, the fix to this problem is to do the following in the PSM security
initialization, while no other browser threads can call any NSS/PSM security APIs :

1) load the NSS3 DLL dynamically, and call its NSS_Init()
2) Call SECMOD_HasRootCerts() to find if the root cert module was loaded
3) If it was not, call SECMOD_LoadModule to load nssckbi.dll from the known-good
location. End of story.
4) If it was already loaded (by NSS_Init), enumerate all the PKCS#11 modules
loaded, using SECMOD_GetModuleSpecList
5) check if any of them is a root cert module, by calling PK11_HasRootCerts() on
each module
6) if it is, delete and unload it, using SECMOD_DeleteModule() . Repeat. You
don't want multiple root cert modules loaded !
7) call SECMOD_LoadModule() to load nssckbi.dll from the same location as the
NSS DLLs were dynamically loaded

For all purposes and intent, the above fix corrects the problem reported in this
bug, which is that the wrong DLL gets used. It still leaves the possibility that
the wrong nssckbi.dll may be initially loaded from the existing reference in
secmod.db. However, that wrong version will immediately get unloaded, and the
right one will be loaded, so that the wrong one never gets used for anything
meaningful except for its temporary loading.

The above fix does nothing for Mike's OS/2 issue in comment #11, which should be
addressed in a separate bug.
Assignee

Updated

15 years ago
Assignee: kaie → nobody
Reporter

Updated

15 years ago
Product: PSM → Core
I've been hitting compile failures in tier_50 with one SeaMonkey open.  Could we
please reconsider the P5, enh setting?

Comment 23

14 years ago
changing the subject to make it clear what is really happening (mozilla does not
lock any nssckbi.dll, operating systems lock the .dll because mozilla has loaded
it).
Summary: mozilla should not lock a random nssckbi.dll → mozilla should not load a random nssckbi.dll
Assignee

Updated

14 years ago
Whiteboard: [kerh-coz]
Blocks: 332972
Component: Security: UI → Security: PSM
Summary: mozilla should not load a random nssckbi.dll → mozilla apps must manage NSS configuration for PKCS#11 shared libs, including nssckbi.dll
Target Milestone: Future → ---
Some users of both Firefox and Thunderbird are having problems using the updater - basically Firefox cannot update itself because Thunderbird is using the nssckbi.dll in the Firefox directory. See bug 340125 for details. 

In this situation client apps should only use the files in their own directory, otherwise the updater will fail when the file is locked or different from what it expects to find.
Blocks: 340125

Comment 25

13 years ago
This seems to have block the upgrade from 1.5.0.6 to 1.5.0.7
Reporter

Comment 26

13 years ago
*** Bug 237982 has been marked as a duplicate of this bug. ***
Assignee

Comment 27

13 years ago
Could the Firefox/Thunderbird application unload the pipnss module prior to updating itself? Actually, there are some profile-change announcement messages that PSM catches to shutdown NSS. That would close the NSS database and hopefully unload the DLL.
Assignee

Comment 28

13 years ago
related to 346352 ?
(In reply to comment #27)
> Could the Firefox/Thunderbird application unload the pipnss module prior to
> updating itself? Actually, there are some profile-change announcement messages
> that PSM catches to shutdown NSS. That would close the NSS database and
> hopefully unload the DLL.

The actual updating happens at the beginning of the new run of Firefox, so I don't see how this is relevant.  It shouldn't have loaded anything, and firefox shouldn't actually be running at all.

Why can't PSM/NSS just use the DLL that comes with the app rather than finding some other, inappropriate, one?
Assignee

Comment 30

13 years ago
Ok, I misread comment 24, sorry.

NSS uses secmod.db to cache the "best" nssckbi.dll it found.
I'm not sure yet how this cross-application entry could be produced.

Maybe the user copied *.db from one profile to another?

Maybe we should do a verification step at the time the NSS module starts up.
If the nssckbi.dll currently registered in secmod.db points to foreign directory, test whether there is a nssbkci.dll in self's application dir, and if that module is acceptable, unload that other dll, load self's dll and change the regisration.

Comment 31

13 years ago
I agree that the current design of PSM using the latest
libnssckbi.so/nssckbi.dll is problematic.

Can PSM use SECMOD_LoadUserModule instead of SECMOD_AddNewModule
to load libnssckbi.so/nssckbi.dll?  Then the pathname of
libnssckbi.so/nssckbi.dll won't be recorded in secmod.db.

(PSM would also need to use SECMOD_UnloadUserModule to unload
libnssckbi.so/nssckbi.dll on shutdown.)
Assignee

Comment 32

13 years ago
Wan-Teh, I like your proposal from comment 31 very much and coded it up. Here is a patch. Can you please review?
Attachment #115614 - Attachment is obsolete: true
Assignee

Updated

13 years ago
Assignee: nobody → kengert
Assignee

Comment 34

13 years ago
Comment on attachment 240517 [details] [diff] [review]
Patch v2 - reviewer version - ignores whitespace changes

I forgot to set the review flag. Wan-Teh, can you please review?
Attachment #240517 - Flags: review?(wtchang)
Assignee

Comment 35

13 years ago
Comment on attachment 240517 [details] [diff] [review]
Patch v2 - reviewer version - ignores whitespace changes

Wan-Teh suggested that Bob should review the patch.
Attachment #240517 - Flags: review?(wtchang) → review?(rrelyea)

Comment 36

13 years ago
Comment on attachment 240517 [details] [diff] [review]
Patch v2 - reviewer version - ignores whitespace changes

I reviewed your patch quickly.  I found three problems.

1.

>@@ -642,108 +649,135 @@ nsNSSComponent::InstallLoadableRoots()

>   if (RootsModule) {
>     CK_INFO info;
>
>+    if (SECSuccess == PK11_GetModInfo(RootsModule, &info)) {
>         PRInt32 modType;
>         SECMOD_DeleteModule(RootsModule->commonName, &modType);
>+    }
> 
>+    SECMOD_DestroyModule(RootsModule);
>         RootsModule = nsnull;
>       }

The 'info' local variable and the PK11_GetModInfo call
are not necessary.

2. 

>       /* If a module exists with the same name, delete it. */
>       NS_ConvertUTF16toUTF8 modNameUTF8(modName);
>       int modType;
>       SECMOD_DeleteModule(NS_CONST_CAST(char*, modNameUTF8.get()), &modType);

This SECMOD_DeleteModule call seems redundant because
we already called SECMOD_DeleteModule above.

3.

>+    if (RootsModule) {
>+      CK_INFO info;
>+  
>+      if (SECSuccess == PK11_GetModInfo(RootsModule, &info)) {
>+        // NSS_BUILTINS_LIBRARY_VERSION_MAJOR and NSS_BUILTINS_LIBRARY_VERSION_MINOR
>+        // define the version we expect to have.
>+        // Later version are fine.
>+        // Older versions are not ok, and we will replace with our own version.

This comment is out of date, but more important, I think
this comment and the code it describes should be removed.
You should not need to check the root certs module's
version.

Comment 37

13 years ago
Comment on attachment 240517 [details] [diff] [review]
Patch v2 - reviewer version - ignores whitespace changes

r- There are a number of issues, only one forcing the minus.

1. the - problem is with the handling of the RootsModule after an SECMOD_UnloadUserModule(). The latter does not free your existing reference, so you need to call SECMOD_DestroyModule() after the unload.

2. The second issue is one about semantics. The current code will delete any builtin modules found in the secmod.db file. This means if you have older versions Firefox which share your profile, they will re-add the module when they are started. This means you will ping from adding to removing as you swap back and forth between various FF versions. The function SECMOD_DeleteModuleEx() takes a parameter which will allow you to remove the module from your instance without removing it from secmod.db.

3) I'm not sure the builtins version checks are necessary anymore. They were a way of upgrading the system's view of ckbi in the secmod.db file in the case where you have multiple versions of FF installed in different directories, and all the versions will use the most up-to-date nssckbi.

4) a minor nit- the indentation seems wrong in the diffs. This means there are inconsistant use of tabs and spaces in the file. Figure out what the file's standard tab/space configuration is and stick to it.
Attachment #240517 - Flags: review?(rrelyea) → review-
Assignee

Comment 38

13 years ago
(In reply to comment #37)
> (From update of attachment 240517 [details] [diff] [review] [edit])
> r- There are a number of issues, only one forcing the minus.
> 
> 1. the - problem is with the handling of the RootsModule after an
> SECMOD_UnloadUserModule(). The latter does not free your existing reference, so
> you need to call SECMOD_DestroyModule() after the unload.

Ok, call added whenever the code does an Unload.


> 2. The second issue is one about semantics. The current code will delete any
> builtin modules found in the secmod.db file. This means if you have older
> versions Firefox which share your profile, they will re-add the module when
> they are started. This means you will ping from adding to removing as you swap
> back and forth between various FF versions. The function
> SECMOD_DeleteModuleEx() takes a parameter which will allow you to remove the
> module from your instance without removing it from secmod.db.

Unfortunately, SECMOD_DeleteModuleEx() is a private API!
IINM, it is not exported.

You said, this is not a real problem. I guess we can live with this adding and removing, in the case where users are switching between old and new software versions with a single profile?


> 3) I'm not sure the builtins version checks are necessary anymore. They were a
> way of upgrading the system's view of ckbi in the secmod.db file in the case
> where you have multiple versions of FF installed in different directories, and
> all the versions will use the most up-to-date nssckbi.

We are currently trying three possible locations in order to find ckbi. First the product installation dir, then the former GRE dir, then the global search path.

IIUC you are proposing: Let's use the first ckbi that we find.

I removed all version checks from the code.


> 4) a minor nit- the indentation seems wrong in the diffs. This means there are
> inconsistant use of tabs and spaces in the file. Figure out what the file's
> standard tab/space configuration is and stick to it.

Oh, sorry, both attached patches were the version that ignores the whitespace changes. I think I got the indenting correct.


(In reply to comment #36)
> The 'info' local variable and the PK11_GetModInfo call
> are not necessary.

Removed, as Bob also proposed to remove the version checks.


> This SECMOD_DeleteModule call seems redundant because
> we already called SECMOD_DeleteModule above.

The first call will delete whatever module is currently stored in secmod.db - I don't know the full history, but I assume the existing roots module might have any name.

The second call is a kind of safety measure, it will ensure that no module exists with the name we are going to use for adding.

This code has always been in. I'd prefer to leave it in, unless you are sure it's not necessary.


> This comment is out of date, but more important, I think
> this comment and the code it describes should be removed.
> You should not need to check the root certs module's
> version.

I'll remove this block completely.
Assignee

Comment 39

13 years ago
Patch as explained in previous comment.
I'm proposing this patch, unless there is a way to access SECMOD_DeleteModuleEx that I missed.
Attachment #240517 - Attachment is obsolete: true
Attachment #240520 - Attachment is obsolete: true
Attachment #246849 - Flags: review?(rrelyea)
Is there any reason not to simply expose SECMOD_DeleteModuleEx in NSS ?
I'd be OK with doing that in 3.11.5, if no one else objects.
Assignee

Comment 42

13 years ago
(In reply to comment #41)
> Is there any reason not to simply expose SECMOD_DeleteModuleEx in NSS ?
> I'd be OK with doing that in 3.11.5, if no one else objects.

I believe we are looking for a fix that could be applied to stable branches without requiring an NSS update.

Exporting the function in nss.def would not be sufficient, we'd also require to change headers, because currently SECMOD_DeleteModuleEx is declared in the internal header file secmodi.h only.

Comment 43

13 years ago
SECMOD_DeleteModuleEx should be safe to export. I'll entertain a patch to export it in the trunk. Be sure to move the declaration from secmodi.h to secmod.h in the patch as well.

bob

Comment 44

13 years ago
Comment on attachment 246849 [details] [diff] [review]
Patch v3 - reviewer version - ignores whitespace changes

r+=relyea

I looks like my concerns have been addressed.

This patch is fine until you have SECMOD_DeleteModuleEx.

bob
Attachment #246849 - Flags: review?(rrelyea) → review+
Assignee

Comment 45

13 years ago
I filed bug 362967 to export the DeleteModuleEx function.

Assignee

Comment 46

13 years ago
fixed on trunk
Status: NEW → RESOLVED
Closed: 17 years ago13 years ago
Resolution: --- → FIXED
Depends on: 362980
Assignee

Updated

13 years ago
Blocks: 362980
No longer depends on: 362980

Comment 47

13 years ago
Something is a bit wonky here.

For me, the search for nssckbi ends up with fullLibraryPath pointing to a directory without nssckbi (the first in possible_ckbi_locations), but SECMOD_LoadUserModule() actually returns something non-null.

Could it be that the "migration" from secmod.db storing leaves the module in a module list somewhere, so lookup by name works, but the library ends up pointing in the wrong direction?

Comment 48

13 years ago
what does the spec string you passed to SECMOD_LoadUserModule look like?

bob

Comment 49

13 years ago
Yeah, good question. I need to re-create the scenario again then. Which means I have to revert changes, re-create dbs, upgrade, etc. I'll try to get to that.
Assignee

Comment 50

12 years ago
This bug introduced a reference count leak, which will get fixed with bug 379582.
Assignee

Comment 51

12 years ago
(In reply to comment #47)
> Something is a bit wonky here.
> 
> For me, the search for nssckbi ends up with fullLibraryPath pointing to a
> directory without nssckbi (the first in possible_ckbi_locations), but
> SECMOD_LoadUserModule() actually returns something non-null.

Indeed. Sorry, the patch had a bug. It seems that SECMOD_LoadUserModule always returns an object, and we must check returnvalue->loaded to find out whether the load succeed. Only if returnvalue->loaded is true we can exit our search loop.

I was able to reproduce this when I investigated bug 375040.

Reopening. Patch coming up.

Blocks: 375040
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee

Comment 52

12 years ago
Bob, can you please review?

I assume, if SECMOD_LoadUserModule returned data->loaded==PR_FALSE, there is NO need to call SECMOD_UnloadUserModule, but destroying the reference is sufficient.
Attachment #269839 - Flags: review?(rrelyea)
Assignee

Updated

12 years ago
Status: REOPENED → ASSIGNED
Priority: P5 → P1
Target Milestone: --- → mozilla1.9alpha6
Version: psm2.4 → Trunk

Comment 53

12 years ago
Comment on attachment 269839 [details] [diff] [review]
Incremental Patch v4

r+=rrelyea
Attachment #269839 - Flags: review?(rrelyea) → review+
Assignee

Comment 54

12 years ago
Attachment 269839 [details] [diff] checked in to trunk, marking fixed again.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.