Closed Bug 406999 Opened 15 years ago Closed 15 years ago

Enhance EV performance; Never combine EV with Cert Overrides


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






(Reporter: KaiE, Assigned: KaiE)



(Keywords: perf)


(4 files, 1 obsolete file)

As of today, PSM will call into NSS for EV verification (function CERT_PKIXVerifyCert) quite often. It will make such a call whenever the Mozilla UI requests information related to EV.

Right now that means:
- twice for every page load
- again twice each time you click a link, 
  even though you remain on the same site

This should be optimized.
Keywords: perf
Blocks: evtracker
Attached patch caching v1Splinter Review
This is a patch to implement caching of EV status.
The cache is limited to the lifetime of a wrapper object around a raw NSS cert object.

This means we'll call into CERT_PKIXVerifyCert only once for each page load (where the server cert has any EV Oid).
Attached patch performance v1Splinter Review
As of today, each time we navigate from one web page to the next, we throw away the cert from the current page, receive new callbacks from NSS, will create a new PSM wrapper object around the cert, and associate it with the new page.

With such a simple implementation it's impossible to make use of caching in the PSM object.

This patch implements a mechanism to extend the lifetime of PSM cert objects.

You should know that each SSL socket will produce a socket object having a reference to the associated cert. It is possible to query information about the "container" where this socket lives in, where "container" is the document window.

The idea is, each time we produce a new socket object, we obtain the certificate object currently associated to our parent document window. The socket object will remember a pointer to that current cert.

Later, when the handshake is completed, NSS will notify us with the cert used by this socket. At that point we can compare the certs. If equal, we may continue to use the old PSM wrapper object, and keep all the data it may have cached (including EV status).
Attached patch Patch v1 (obsolete) — Splinter Review
This patch combines "caching v1" and "performance v1" patches.

I'm using this one for approvals and reviews.
You might want to look at the individual patches for easier reviewing.
Attachment #291698 - Flags: review?(rrelyea)
Merging conflicts strike again!!!
I intended to fix the following as a separate bug, but the changes conflict, so I'm adding it to the scope of this bug... :-/

Request : Never show EV UI when a cert override is effective

Now you might wonder:
EV blessing depends on information added at compile time and thus must refer to one of the certs that are built-in. So, how could you ever have an exception for a server cert issued by one of the built-in roots?

Well, an end user might decide to edit CA trust for whatever reason. He might decide to revoke the trust for Verisign. But for some reason, the same user decides to make an exception for PayPal and adds an override.

As of today, you'll get the EV UI treatment.

This is probably a strange edge case. But if a "manual trust override" if effective, we should probably not show the EV UI.
Summary: Enhance EV performance → Enhance EV performance; Never combine EV with Cert Overrides
This is not a real patch, it contains only the snippets that are used especially to achieve:
  "never combine EV with cert overrides"

Only used for assisting reviewing.
Attached patch Patch v2Splinter Review
This is the full set of all changes proposed, it's the combination of the 3 individual patches.
Attachment #291698 - Attachment is obsolete: true
Attachment #291708 - Flags: review?(rrelyea)
Attachment #291698 - Flags: review?(rrelyea)
Requesting blocking1.9, because Dan had requested that for bug 406034.
Flags: blocking1.9?
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2
Comment on attachment 291708 [details] [diff] [review]
Patch v2


but I would like more explanation of what is going on in the 'performance code'.

particularly how prevCert and mServerCert are used.

Attachment #291708 - Flags: review?(rrelyea) → review+
(In reply to comment #9)
> but I would like more explanation of what is going on in the 'performance
> code'.
> particularly how prevCert and mServerCert are used.

The only strategy to enhance performance is the use of caching.

nsNSSCertificate is a PSM wrapper object around a CERTCertificate (with additional state). The PSM wrapper objects have a shorter lifetime than the NSS cert objects.

With this patch we attempt to extend the lifetime of the PSM wrapper object. When navigating between browser pages, we remember the currently used server cert (variable prevCert) and when we learn the new server cert, we do a comparison. If both previous and new page use the same SSL server cert, we keep using the PSM wrapper object (and avoid destroying it, creating a new one).

With this strategy, the EV verification state stored in the PSM wrapper object is preserved. Thus, future calls to obtain the EV state can make use of the cached state, avoiding to calculate the state again.

mServerCert is simply the member variable of a SSL status object, which belongs to a browser window.
checked in, marking fixed.
Closed: 15 years ago
Resolution: --- → FIXED
Blocks: 412455
You need to log in before you can comment on or make changes to this bug.