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

Status: RESOLVED FIXED

(Core :: Security: PSM, defect, P1)

Description

[timeless]

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...)

Comment 1

[timeless]

Attachment: Mozilla1872.exe.txt

Comment 2

[timeless]

Attachment: Mozilla1960.exe.txt

Comment 3

[Wan-Teh Chang]

Changed product to PSM.  This is a known problem.

Comment 4

[Kai Engert [:KaiE:]]

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 ***

Comment 5

[timeless]

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.

Comment 6

[John Unruh]

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.

Comment 7

[David Bradley]

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%.

Comment 8

[hacker formerly known as [e-mail]]

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?

Comment 9

[Asa Dotzler [:asa]]

not going to hold 1.3beta for this.

Comment 10

[hacker formerly known as [e-mail]]

Attachment: wip

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.

Comment 11

[Mike Kaply [:mkaply]]

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?

Comment 12

[GA]

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

Comment 13

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

*** Bug 74750 has been marked as a duplicate of this bug. ***

Comment 14

[chris hofmann]

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

Comment 15

[Kai Engert [:KaiE:]]

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.

Comment 16

[Nelson Bolyard (seldom reads bugmail)]

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

[chris hofmann]

any more thoughts on how we could get this fixed?

Comment 18

[Mike Kaply [:mkaply]]

did anyone ever answer the question as to why secomd.db needs a fully qualified
path name to the DLL?

Comment 19

[Robert Relyea]

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

[chris hofmann]

trying to get 1.7b out.  renominate if something appears soon.

Comment 21

[Julien Pierre]

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.

Comment 22

[Alex Vincent [:WeirdAl]]

I've been hitting compile failures in tier_50 with one SeaMonkey open.  Could we
please reconsider the P5, enh setting?

Comment 23

[Robert Relyea]

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).

Comment 24

[Nick Thomas [:nthomas] (UTC+12)]

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.

Comment 25

[Colin S. Miller]

This seems to have block the upgrade from 1.5.0.6 to 1.5.0.7

Comment 26

[timeless]

*** Bug 237982 has been marked as a duplicate of this bug. ***

Comment 27

[Kai Engert [:KaiE:]]

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.

Comment 28

[Kai Engert [:KaiE:]]

related to 346352 ?

Comment 29

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

(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?

Comment 30

[Kai Engert [:KaiE:]]

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

[Wan-Teh Chang]

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.)

Comment 32

[Kai Engert [:KaiE:]]

Attachment: Patch v2 - reviewer version - ignores whitespace changes

Wan-Teh, I like your proposal from comment 31 very much and coded it up. Here is a patch. Can you please review?

Comment 33

[Kai Engert [:KaiE:]]

Attachment: real patch v2 including whitespace changes

Comment 34

[Kai Engert [:KaiE:]]

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?

Comment 35

[Kai Engert [:KaiE:]]

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

Wan-Teh suggested that Bob should review the patch.

Comment 36

[Wan-Teh Chang]

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

[Robert Relyea]

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.

Comment 38

[Kai Engert [:KaiE:]]

(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.

Comment 39

[Kai Engert [:KaiE:]]

Attachment: Patch v3 - reviewer version - ignores whitespace changes

Patch as explained in previous comment.
I'm proposing this patch, unless there is a way to access SECMOD_DeleteModuleEx that I missed.

Comment 40

[Kai Engert [:KaiE:]]

Attachment: Patch v3 - real patch including whitespace changes

Comment 41

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 42

[Kai Engert [:KaiE:]]

(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

[Robert Relyea]

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

[Robert Relyea]

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

Comment 45

[Kai Engert [:KaiE:]]

I filed bug 362967 to export the DeleteModuleEx function.

Comment 46

[Kai Engert [:KaiE:]]

fixed on trunk

Comment 47

[Allan Beaufour]

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

[Robert Relyea]

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

bob

Comment 49

[Allan Beaufour]

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.

Comment 50

[Kai Engert [:KaiE:]]

This bug introduced a reference count leak, which will get fixed with bug 379582.

Comment 51

[Kai Engert [:KaiE:]]

(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.

Comment 52

[Kai Engert [:KaiE:]]

Attachment: Incremental Patch v4

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.

Comment 53

[Robert Relyea]

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

r+=rrelyea

Comment 54

[Kai Engert [:KaiE:]]

Attachment 269839 [details] [diff] checked in to trunk, marking fixed again.