100215 - Certificate issuer displayed inconsistently

Status: VERIFIED FIXED

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

Description

[Stephane Saux]

In bug 93103, we store the intermediate CA certs temporarily so that the page
info can display who issued the cert.
Bug 98833 was marked as a duplicate of 93103.
Apparently following the instructions in the 93103 does produce a problem in the
cert viewer.  The page info does correctly identify the Issuer, but the view
cert has an inconsistent display, the Issuer is identified in the General tabs
as "Verisign Trust Network" but the view first line says "Could not verify this
certificate because the issuer is unknown.

to reproduce:
http://www.egg.com
scroll to to "Your account" link (in the footer line just above the legal
disclaimer)
Click on the link.
When the lock is locked, mouse over -> "Signed by Verisign Trust Network"
Click on the lock.
Page info states that the cert has been verified by "Verisign Trust Network".
Click on View.
See attached screenshot.

Comment 1

[Stephane Saux]

Attachment: screenshot of view certificate window.

Comment 2

[Stephane Saux]

Note that on linux it works.  The general tab is correct, and when you look at
the details, you see the hierarchy of certs. The only annoying thing is that the
intermdiate cert shows as "null" in the details window.

WinNT shows the same problem as win200.

Comment 3

[Stephane Saux]

Attachment: corresponding detail linux view cert

Comment 4

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

I understand now what happens. The implementation for bug 93103 used an approach
that doesn't work all the time.

It was suggested to store the cert chain together with the nsNSSSocketInfo.
However, depending on page structure, sometimes the first object of this kind
goes away very soon, and with it the remembered chain.

In addition, the handshake callback, where we remember the server cert, is
called once for every opened window. But if pages from the same server are
loaded into multiple browser windows, the AuthCallback is called only once -
another possibility to lose the remembered chain if the wrong window is closed
first.


I conclude that we need to store the chain in a global manner.
I have two suggestions:


1.) Let's indeed store the CA chain parts in the temp

During auth callback, we get the chain. We iterate over the chain. We ignore the
server cert. We ignore certs found in the perm db. We only add intermediate
certs to the temp db.

If you agree to this approach, I'd like to know:
- what NSS function should be used to store a cert in the temp db?
- as the GetChain functions use DupCertificate, how should I compare a cert from
the chain with the original server cert?
- how can I look up whether a cert is in the perm db?
- how can I look up whether a cert is already in the temp db?
  (as I don't want to store intermediate CA certs multiple times)

With this approach, the temp db will grow within one session, but not too much.


2.) Create a list of mappings from hostname => stored chain

I don't know if this is a better approach than 1.)
Everytime in AuthCallback, check we already have a chain for that host, if not,
add it to the global list.

In this case we prefer this one I need to find out:
- which container type should I use (1:1 string => pointer mapping, with quick
lookup ability)
- how can I find out the raw host name that was used to connect during the
callback (which might be different from what is listed in the cert)


Tips appreciated.


(I still assume that the chain is available during AuthCertificateCallback only,
but not during HandshakeCallback - is that correct? If it were available, it
would it a bit easier, as the handshake callback seems to be called for every
window once, and I might be able to store the chain in the SSLStatus object,
where we already store the server cert. Let me know if I should try that out.)

Comment 5

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

(Please ignore the very last braced paragraph from the previous comment. I tried
it, is not a viable strategy.)

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Kai,

When SSL receives a cert chain from the peer system, it enters all the certs 
into the temp cert DB.  The certs in the temp DB are all reference counted.  
When the reference count on each one goes to zero, the cert is deleted from 
the temp DB. 

As each cert is put into the DB, it gets one reference which is held by the 
SSL code.  When all the certs are in the temp DB, SSL calls the certificate 
authentication callback function.  When the cert authentication callback 
function returns, SSL immediately releases its references to all the certs 
in the chain except the "leaf" cert (the server's cert itself).  SSL 
releases the reference to the leaf cert when the connection is closed. 
When SSL releases its references, if there are no other references already
established, the certs are deleted from the temp DB.  

So, if the application wants to keep the certs in the chain around past the
end of the cert authentication callback, the application must itself 
aquire new references to the certs before returning from the cert 
authentication callback.  The certs will stay in the temp DB as long as the
application holds those references, so the application must remember to 
release the references when it is done with them, or they will leak.

To get a new reference for a cert when you already have the address of the
CERTCertificate structure, you call CERT_DupCertificate and pass it that
address.  It returns the address of a new reference to the cert.  To 
release that new reference, you call CERT_DestroyCertificate and pass it 
the reference to be released.  

Comment 7

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

Attachment: Patch for discussion

Comment 8

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

Thanks for your comments.

This is a re-implementation of what I did in bug 93103.

The idea is to iterate over the certs in the chain, look at the signers of the
server cert only. For every signer, decide whether we need to remember it or not.

My idea is: Certs that are already stored in the perm DB must not be remembered.
The root CAs will already be known in most cases. Only in such cases where an
intermediate CA is seen, or where an unknown root CA is seen, we need to
remember the cert.

In the attached patch, I try to make the decision based on the "isperm"
attribute of the CERTCertificate. However, this does not work. When I go to the
site that Stephane reported, the root CA is already known. But when I iterate
over the certs in the chain, for both intermediate and root CA, the isperm flag
is not set.

How could I find out, whether I cert is already stored in the perm db?

CouldI use CERT_GetDefaultCertDB in conjunction with either CERT_FindCertByKey
or CERT_FindCertByKeyID to find this out?

Comment 9

[Stephane Saux]

For built-in root ca the default cert db may not cut it.  That's a different token.

Comment 10

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

Ok, I think I have a good working patch. McGreer gave me good hints. The trick
is, that the cert structure already contains the information whether the cert is
available on any token, and that's just the info we need.

In most cases, the attached patch does not cause memory to be allocated, and
quickly iterates over the cert chain.

If you go to a site that uses the intermediate CA, this cert will be remembered
for the rest of the browser session.

If you to multiple sites that all use the same intermediate CA (e.g.,
new.egg.com, meine.db24.de, www.1822direkt.de), this will not result in
additional certs remembered in the temp db.
I'll attach the patch.

Comment 11

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

Attachment: Suggested fix

Comment 12

[Robert Relyea]

r=relyea

Some truth-in-advertising comments: PL_NewHashTable() does allocate new memory
(PL_HashTableAdd probably does as well). This is balanced by the fact the
CERTCertificateList * is freed. The Memory for PL_HashTableAdd and the
NewHashTable is freed correctly, but as the Kai's description points out, this
is at the end of the session. Note that both the certs in memory, and the
PL_HashTable will continue to grow for any Netscape 6 session until PSM is
shutdown. This shouldn't be an issue since the number of unique inermediate
Certs a user is likely to encounter over the lifetime of a pretty intense
Netscape 6 session is most likely under 10. The only potential problem is a
malicious site may include a sets of long cert chains, but then the chain is
only stored if the chain verified, which means duplicity on the part of some
trusted domain, or the explicit permission of the user. (and SSL validation
limits the chain length).

Kai, there isn't a problem caching the Perm certs in the database, though it's
probably good not to as it keeps down the potential issue above, and the code
for detecting those certs are pretty simple. It is important not to cache the
Slot certs. In some obscure environments (like FORTEZZA) the tokens these certs
are attached to may disappear.

All that being said, the patch looks good, particularly on the use of NSS.

Comment 13

[Christopher Blizzard (:blizzard)]

Comment on attachment 51278 [details] [diff] [review]
Suggested fix

sr=blizzard

Comment 14

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

Patch checked in to trunk, closing bug.

Do we want this on the 094 branch?

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

Changed summary to reflect real problem.

Comment 16

[Stephane Saux]

this can't be defined as a stop-ship.  The patch is extensive.  Therefore it's
not wise to include in the branch.

Comment 17

[John Unruh]

Verified on Win98 trunk 10/12 build.