190394 - "website certified by an unknown authority'" - nssckbi not found breaks PKI trust

Status: VERIFIED FIXED

(Core Graveyard :: Security: UI, defect)

Description

[Pascal Chevrel:pascalc (PTO until April 26)]

build 2003012315 WinXP

Steps :

1 go to a secure server like https://www.bnpnet.bnp.fr/ or
snews://secnews.netscape.com

expected result : enter site without problem
actual result : you get a "website certified by an unknown authority" message

build 2003012105 does not have this problem

Comment 1

[Matthias Versen [:Matti]]

Please read the component description first security holes belong in
security:general but not crypto

Comment 2

[Jun MUTO]

I can see this also on Win98 2003-01-23-15-trunk installer build 
(mozilla-win32-installer-sea.exe) with new profile.
It has not any authorities' certificates.

Comment 3

[John Unruh]

Confirming with the 1/24 WinXP build. There are no CA's in the Authorities tab.

Comment 4

[John Unruh]

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

Comment 5

[Tracy Walker [:tracy]]

seeing this on win2k also with commercial trunk build 2003-01-24-04-trunk

this dialog is appearing in place of the brief security dialog that is expected
to  pop up when entering a secure site.  depending on the site, the dialog may
pop up several times before actually entering the site.

Comment 6

[Sean Su]

adding chak to the CC: list in case he's seen this in the past.

Comment 7

[Chak Nanga]

Kaie, any ideas....thanks

Comment 8

[Kai Engert (:KaiE:)]

I suspect this bug is caused by the repacking that seems to have been done.

There is one shared library used by PSM/NSS which contains the storage for known
and trusted root CA certificates. The library is "nssckbi".

This library is not linked with the other code (by design of NSS, as far as I
understand it).

Instead of a link time dependency, PSM/NSS try to open that library dynamically
during initialization. The logic tries to find the nssckbi library in the
directory where mozilla-bin is stored (I think).

Did you change the location where nssckbi can be found? That would explain the
failure. I'm not sure how to make it work again by any other approach than
moving it back to the previous location.

Note there is another behaviour (by design of NSS, I'm unsure what the reason
was), that can influence the problem reported in this bug.

NSS remembers the place where the last successfully loaded nssckbi was found.
This location is stored in file secmod.db, contained in the profile directory.
Note, however, that secmod.db stores other data, too.


If you can confirm the place where nssckbi is stored has changed, I suggest to
either undo that change, or to consult with the NSS team (wtc and relyea cc'ed)
what other solution could be used to allow the Mozilla application to find it.


Another snippet of information: This special handling of nssckbi seems to be a
common pitfall. I discovered that the creators of SuSE Linux 8.1 made a similar
mistake, trying to "optimize" the distribution of shared libraries on their
system, with the same bad consequence that on those systems, no CAs are trusted.

Comment 9

[Kai Engert (:KaiE:)]

I confirmed the problem. It is not a code problem, but it is a packaging problem.

On Windows, when installing Mozilla using a installer, file nssckbi.dll is not
getting installed. If I understand correctly, during the last days the packaging
structure of Mozilla has changed. Do you know the bug that tracked this change,
and who could point us to the new files that control what files are getting
installed? We must ensure that we add nssckbi back on all platforms.

As a separate test, I downloaded the same build, but not using an installer, but
in zip file format. This package contains the nssckbi.dll and everything works
correctly.


Not that not everybody might run into this problem, if you install the latest
builds into a separate directory, because your profile will remember the place
of a nssckbi.dll, and if you haven't deleted the older build, everything will
still appear to work.

Comment 10

[Kai Engert (:KaiE:)]

-> me

Comment 11

[Kai Engert (:KaiE:)]

I see the nssckbi library now lives in the GRE directory.
I discussed the issue with nelsonb, to confirm it makes sense to have this
library in the GRE, as opposed to the application. We agreed the GRE is the
right place.

PSM has code that tries to find nssckbi if its location is not yet known.
As of today, it only searches in the application process directory. This was
fine in the day where there was no separation between GRE and application, but
now we must extend this strategy.

I saw it is possible to query the path of the GRE. I will attach a patch that
tries to load the nssckbi from the GRE directory first. Should that fail, it
falls back to the old strategy, looking in the application's directory.

Comment 12

[Kai Engert (:KaiE:)]

Attachment: Patch v1 (real patch with whitespace changes, looks scary)

Comment 13

[Kai Engert (:KaiE:)]

Attachment: Patch v1 (ignoring whitespace for easier reviewing)

Comment 14

[Kai Engert (:KaiE:)]

oops, found a bug in the patch, of course I should divide by sizeof(char*), not
by sizeof(char)...

Comment 15

[Kai Engert (:KaiE:)]

Attachment: Patch v2

Comment 16

[Kai Engert (:KaiE:)]

Attachment: Patch v2 (ignoring whitespace for easier reviewing)

Comment 17

[Kai Engert (:KaiE:)]

Comment on attachment 112608 [details] [diff] [review]
Patch v2 (ignoring whitespace for easier reviewing)

I've test the patch, it fixes the problem on Windows.

Could you please review? Any reviews welcome, in the attempt to get this fixed
soon for 1.3 beta.

Comment 18

[Bedeox]

Reproduced with 2003-01-25-08...

Patch looks nice to me...
Shouldn't break anything.

Comment 19

[Pascal Chevrel:pascalc (PTO until April 26)]

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

Comment 20

[Doug Turner (:dougt)]

Comment on attachment 112608 [details] [diff] [review]
Patch v2 (ignoring whitespace for easier reviewing)

This looks okay.  do you expect mozFile to be valid at any point after the new
continue statement?

I think that you might have some spacing issues at the tail of this patch.  

i would have used an int instead of a size_t and named my variable i rather
than il, but that really isnt too important. :-)

assumping an okay answer for the "is mozFile expected to be valid" question.

Comment 21

[Kai Engert (:KaiE:)]

> This looks okay.  do you expect mozFile to be valid at any point after the new
> continue statement?

mozFile only has to be valid within the loop. Oh, good point. I have moved the
declaration of nsCOMPtr mozFile into the loop, to make sure the value from
earlier iterations won't get reused. I'll attach a slightly updated patch.


> I think that you might have some spacing issues at the tail of this patch.  

What you reviewed was created with "diff -w", the spacing should be fine in the
real patch.


> i would have used an int instead of a size_t and named my variable i rather
> than il, but that really isnt too important. :-)

When I used int, I got a "signed/unsigned warning", because the result of
sizeof/sizeof is unsigned.
I don't want to use i, because in the same function scope there is already
another loop using i.

Comment 22

[Kai Engert (:KaiE:)]

Attachment: Patch v3

Updated patch, moved declaration of nsCOMPtr into loop.

Comment 23

[Kai Engert (:KaiE:)]

Attachment: Patch v3 ignoring whitespace

Comment 24

[Kai Engert (:KaiE:)]

Comment on attachment 112748 [details] [diff] [review]
Patch v3 ignoring whitespace

carrying forward r=dougt
Darin, could you sr=?

Comment 25

[Kai Engert (:KaiE:)]

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

Comment 26

[Sean Su]

should this be a topembed bug? Should this be a 1.3b blocker?

Comment 27

[Darin Fisher]

Comment on attachment 112748 [details] [diff] [review]
Patch v3 ignoring whitespace

sr=darin

Comment 28

[Kai Engert (:KaiE:)]

ssu: it is already a 1.3b blocker.

Comment 29

[Doug Turner (:dougt)]

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

Comment 30

[Asa Dotzler [:asa]]

Comment on attachment 112748 [details] [diff] [review]
Patch v3 ignoring whitespace

a=asa (on behalf of drivers) for checkin to 1.3beta.

Comment 31

[Zook Valem]

Let us know if it is already checked into the trunk..  I have no way of knowing
it myself.

Comment 32

[Kai Engert (:KaiE:)]

Checked in, marking fixed.

Comment 33

[John Unruh]

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

Comment 34

[Tracy Walker [:tracy]]

verified fixed with windows commercial trunk build 2003-01-28-04-trunk

Comment 35

[Zook Valem]

So far, so good for Win2000 using same build as Tracy reported.  This is for
some who reported problem with Win2000 even though the OS is reported as WinXP.

Comment 36

[Matthias Versen [:Matti]]

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