93103 - Missing intermediate CA certs limit PSM UI capabilities.

Status: VERIFIED FIXED

(Core Graveyard :: Security: UI, defect, P2)

Description

[Stephane Saux]

Solution is in NSS. See bug 93046

Comment 1

[Stephane Saux]

adding 93046 dependency.

Comment 2

[John Unruh]

Mass assigning QA to ckritzer.

Comment 3

[Stephane Saux]

->over to kai.
According to Bob Relyea, it's up to the application to add the certs to the db,
and he believes that all the calls are there to do so.  We have two choices, we
can either add the CA's to the main cert, or we can add them to the temporary
db. If we choose the former, over time the cert db will grow, but not very
fast.  If we choose the latter, the cert db will not grow, the page info will
still be able to use the temp cert db to show the correct information, but if we
cause the user to store the end-entity cert in his cert db (when the user clicks
on "remember this cert permanently" on some of our dialogs) then upon restart,
the cert manager may not be able to validate the cert.

Comment 4

[Stephane Saux]

More notes: the bugs this depends on describes the calls we can use.

Of course we could also use the temp db, but if the remember cert permanently
has been checked, then we can add the chain to the permanent cert db.

Comment 5

[Stephane Saux]

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

Comment 6

[Javier Delgadillo]

The best solution is to keep the certs (in either CERTCertificate* format or der
format, whichever would make implementation easier) with the nsNSSSocketInfo
that corresponds to it.  That way when the nsNSSSocketInfo structure goes away,
so will the cert.  Getting rid of the slow memory leak, and keeping us from
needlessly increasing the size of the user's cert7.db file on disk.  IMHO, it'd
be bad if the cert7.db contained a cert of every single SSL site I've ever visited.

Comment 7

[Stephane Saux]

Just to clarify: I wasn't advocating storing the end-entity cert, just the
intermediate CA chain (whatever's missing to be able to display the correct info
in the page-info dialog). Javi's suggestion to store that in the nsNSSSocketInfo
is fine. Javi also assures me that were the user to add the end-entity cert to
the disk cert db (remember it permanently), then the cert would show as verified
in the cert manager regardless of whether the chain is present in the cert db.

If that's the case (and we should check), then there's never any reason to store
it in the disk cert db.

Unless it's significantly harder to store it in the nsNSSSockedInfo then let's
do that, avoiding the slow mem leak from the temp db.

How many different chains would a user see during a browser session? Probably
very few, even with a huge MTBF, but still.

Comment 8

[Stephane Saux]

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

Comment 9

[(Don't use, old account) - Kai Engert]

Off Topic
=========
While working on this, I saw that PK11PasswordPrompt is not declared as
PR_CALLBACK. Shouldn't it be?
In addition, I added an init value of zero to nsNSSSocketInfo::mPort, which is
missing.


Summary of the problem and what has to be done
==============================================
1) User visits a site with unknown parent CA cert

The missing parent can be an unknown root CA, or the web server might be
misconfigured and does not include the intermediate cert.

The user is shown the "unknown CA" dialog. Display certificate can't display a
complete chain. If the user selects "remember permanently", we only store the
site certificate. This has the effect of adding trust (for this site only)
permanently. Future visits to the site are trusted. Certificate manager shows
this site as trusted, too. This already works, I checked this as Stephane suggested.

==> But: Is the following wrong or misleading? If you click on the lock icon
while you have a connection to that site, the following text is displayed: "The
identity of this web site has been verified by (name of CA as contained in
cert), a certificate authority you trust for this purpose". However, we didn't
verify the certificate using the CA certificate. Should we display a different
message, something like "You decided to trust this site on your own"?


2) User visits a site with a third-or-lower-level cert, but sends us (required
parts of) the chain

Show cert when clicking on the security lock currently doesn't show the chain.
This is the case were we have the choice where to store it. As we can expect the
chain will still be sent to us in future connections, no need to store the chain
in the perm db, the temp db will do fine.

One interesting thing to mention: If you visit the site using a different
hostname (of maybe the ip address), forcing a mismatch of the domain name in the
certificate, you'll see the domain mismatch prompt. When you chose the view the
cert in that dialog, the certificate chain is displayed completely. This proves
that the chain is indeed available during handshake as Bob pointed out in bug 93046.


Implementation
==============
The info in bug 93046 was very helpful. I've chosen to add the callback, but not
replace it completely. In the new callback, I add our logic, but include a call
to the default implementation.

The CAChain is simply stored inside nsNSSSocketInfo, as long as the the instance
lives.

The only thing that is a little bit ugly: When displaying the chain, the
intermediate CA is titled "null" in one place, and completely empty in another.
But this may be caused by an empty common name in the intermediate certificate
site I tested this with (https://meine.db24.de).

Comment 10

[(Don't use, old account) - Kai Engert]

Attachment: Suggested fix

Comment 11

[(Don't use, old account) - Kai Engert]

David, can you please review?

Comment 12

[Stephane Saux]

cc'ing relyea for review as well.

Comment 13

[David P. Drinan]

r=ddrinan.

Comment 14

[Robert Relyea]

Patch looks good.
Only comment: it might be a good idea to save the existing default handler (and
arg) and call that, rather than explicitly calling the default handler by name.
Unfortunately the API doesn't allow that, so the patch should stand as is. I'm a
little worried if the default handler needs to change it's arg value to
something other than the Database handle.

PK11PasswordPrompt should be a PRCALLBACK, though I think we need to fix the
pk11wrap code because the function prototype should also be a PRCALLBACK.

r=relyea

Comment 15

[(Don't use, old account) - Kai Engert]

Bob, your comment motivated me to remove this separate fix from my above patch
and move it to the new bug 98068.

I'm attaching an updated patch, that just has the change to the
PK11PasswordPrompt removed (which has nothing to do with the fix for this bug).

I assume that r= does still apply.

In addition, I filed the new feature enhancement bug 98069 for tracking purposes.

Comment 16

[(Don't use, old account) - Kai Engert]

Attachment: Slightly updated patch, assuming r= does still apply

Comment 17

[Christopher Blizzard (:blizzard)]

Comment on attachment 48073 [details] [diff] [review]
Slightly updated patch, assuming r= does still apply

sr=blizzard

Comment 18

[(Don't use, old account) - Kai Engert]

Patch checked into trunk. I'm not closing this yet, as we want this on the
branch, too.

Comment 19

[Stephane Saux]

marking PDT

Comment 20

[Asa Dotzler [:asa]]

Comment on attachment 48073 [details] [diff] [review]
Slightly updated patch, assuming r= does still apply

a=asa for checkin to 0.9.4 branch

Comment 21

[(Don't use, old account) - Kai Engert]

Patch checked in to branch. Closing bug.

Comment 22

[Stephane Saux]

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

Comment 23

[Stephane Saux]

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

Comment 24

[John Unruh]

Verified fixed.

Comment 25

[John Unruh]

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