Closed Bug 483168 Opened 15 years ago Closed 15 years ago

NSS Callback API for looking up a default OCSP Responder URL

Categories

(NSS :: Libraries, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED FIXED
3.12.3

People

(Reporter: rob, Assigned: nelson)

References

Details

Attachments

(2 files, 4 obsolete files)

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-GB; rv:1.9.0.7) Gecko/2009031211 Gentoo Firefox/3.0.7
Build Identifier: trunk

The combination of stricter EV certificate revocation checking plus incomplete CDP support threatens to leave some CAs' valid EV certificates without the EV UI in the upcoming Firefox 3.5 release, even though Firefox 3.0.x does show the EV UI successfully.

Bug #477244 discusses various options for fixing this before it becomes a real-world problem:
  1. Finish CDP support before launching FF 3.5.
  2. Embed a list of URLs for CRLs and have Firefox fetch and update them periodically (relying on FF's periodic CRL fetching, which does already work).
  3. Require affected CAs to reissue CDP-only EV certificates in order to add AIA->OCSP.
  4. Embed a list of default OCSP Responder URLs for certain CAs, which would be referred to only when AIA->OCSP is not present.

This bug is for further discussion of option 4.  To summarize so far...
In Bug #477244:
- I proposed a patch (comments #26, #27 and #53).
- Nelson gave my patch r- (comment #50), saying that a pure NSS patch is the wrong approach (but that a modified patch for PSM/NSS would potentially be acceptable).  Wan-Teh questioned (comment #65) and disagreed (comment #82) with Nelson, saying that he supports a pure NSS patch.
- Kai questioned (comment #41) my patch's mapping of Issuer Name string -> OCSP Responder URL, recommending that a stricter "failsafe identification" method would be better.  Robin suggested (comment #30) that my patch's approach is good enough, and I disagreed (comment #66) with some of Kai's thoughts (although there was some confusion over terminology and we didn't conclude the discussion).
- Johnathan (comment #63) said that he could contact the other CAs that are known to issue CDP-only EV certificates, to ask them if they would prefer option 4 to option 2.

Reproducible: Always
In reply to bug #477244 comment #90:
> It's reasonable to delay landing of NSS 3.12.3 until both CRL-require and CRL
> fetching are available.

If this does happen, it would (I think) render my patch unnecessary for fixing the FF 3.5 EV UI problem with CDP-only EV certificates.

However, I would actually like to see my patch accepted now and kept in place even after CDP support has been completed.  This is because my patch would avoid a big increase in potentially expensive CRL traffic for Comodo's and NetSol's CDP-only certificates once a FF version with CDP support is shipped.  Comodo's and NetSol's OCSP Responders would be used instead.  OCSP traffic is considerably cheaper than CRL traffic.

(I'm not suggesting that helping CAs to cut costs should be particularly high on Mozilla's agenda, but I think it's relevant enough to this bug for me to mention it).
For now, I've marked this bug as blocking bug #474606 and bug #479231, just as bug #477244 does.
Blocks: 474606, 479231
Severity: enhancement → normal
Version: unspecified → trunk
If CRLDP is implemented why should your hack be preferable? There might be more CAs which would need that feature in case a fail-over is implemented (OCSP -> CRL) for example. Then the NSS team would start to work as CRL maintainers upon every change at a CA...

Also, I understand that this is a time-limited problem and with the relative short life-time of EV certs will be solved within ~ 2 years, right?
In reply to comment #3:
> If CRLDP is implemented why should your hack be preferable?

I have *never* suggested that my "hack" should be preferred to full CRLDP support in FF/NSS.

The "If" in your question is the important word!  AFAIK, Mozilla have not promised that CRLDP will definitely be implemented in time for FF 3.5.  If they had promised this, I would not have bothered to write my patch.

And if CRLDP support (option 1) is not implemented in FF 3.5, my patch (option 4) is Comodo's and NetSol's preferred approach (over options 2 and 3) for ensuring that our customers' EV certificates get the EV UI in FF 3.5.

And even once CRLDP support is implemented, IMHO my patch is still a nice-to-have (see comment #1).

> Also, I understand that this is a time-limited problem and with the relative
> short life-time of EV certs will be solved within ~ 2 years, right?

No.  Longer.  See bug #477244 comments #58 and #59.
Status: UNCONFIRMED → NEW
Ever confirmed: true
CRLDP fetches the CRL only when a site is encountered and no cached valid copy exists, I don't understand where the increase you anticipate comes from. I agree that something similar to your patch might have to be necessary in case CRLDP isn't being implemented in time. However it looks like that it will, so lets hope for the better solution to succeed.
> CRLDP fetches the CRL only when a site is encountered and no 
> cached valid copy exists, I don't understand where the increase 
> you anticipate comes from. 
We still have a body of certificates out there with CRLDPs but no AIA OCSP URIs.  
Today FF does not fetch CRLs automatically from CRLDPs, so we get zero CRL fetches from FF users as a result of them encountering certificates with CRLDPs alone.
When bug 321755 is fixed and released as part of FF we will get lots of CRL fetches from FF users as a result of them encountering certificates with CRLDPs alone.
"lots" is an increase over "none".

> I agree that something similar to your patch might have to be 
> necessary in case CRLDP isn't being implemented in time.
In time for what?  Revocation checking is still broken in FF *today*. 

> However it looks like that it will [be implemented in time]
Bug 321755 comment 51 does look promising for a fix soon, but I can't predict when that will result in a version of FF that follows CRLDPs.

> so lets hope for the better solution to succeed.
We hope for the best, but have to be prepared for the worst.
Why do you consider CRLs better than OCSP?
(In reply to comment #6)
> When bug 321755 is fixed and released as part of FF we will get lots of CRL
> fetches from FF users as a result of them encountering certificates with 
> CRLDPs alone.

LOL, that's not the problem of Mozilla, you should be prepared for that :-)

> We hope for the best, but have to be prepared for the worst.
> Why do you consider CRLs better than OCSP?

I absolutely don't! CRLDP comes into play when no OCSP exists, no? Preferable we'll have one day a fail-over functionality for cases when OCSP fails for some reason.
(In reply to comment #7)
> > ...we will get lots of CRL fetches from FF users...
>
> LOL, that's not the problem of Mozilla,

We didn't suggest that it was.

> you should be prepared for that :-)

We are prepared for that, although we would prefer...see comment #1.

> > Why do you consider CRLs better than OCSP?
>
> I absolutely don't! CRLDP comes into play when no OCSP exists, no? Preferable
> we'll have one day a fail-over functionality for cases when OCSP fails for
> some reason.

+1

IINM, IE7 on Vista can do this already.
Having this patch reviewed and tested is a good thing, and a scoped thing, while CRLDP work progresses, I absolutely agree with that much.

As for the question of whether to leave it in once CRLDP support is in place - that's a harder sell, but I also don't think we need to cross that bridge yet.  The interim fix is a good thing to have as an interim fix, so let's move on that and we can revisit the other question once we reach that happier world of CRLDP implementation.

Nelson - you originally objected to Rob's NSS-based patch, but then WTC disagreed. Do his arguments sway you?

Rob - in the meantime, care to re-attach the patch here while we work it through?  No need to request review yet.
(In reply to comment #9)

> Nelson - you originally objected to Rob's NSS-based patch, but then WTC
> disagreed. Do his arguments sway you?

No.  NSS is used by many applications other than Mozilla browsers, and for
those applications we have very strict backwards binary compatibility 
requirements.  If we change NSS to unilaterally inject OCSP addresses into
certs for certain issuers, we will change the behavior of those existing 
applications, and we will have to support that behavior essentially for ever
thereafter.  

I am quite willing to create a new NSS callback API by which the application
(PSM, in this case) can register a function to do this very thing.  I think
it's much more appropriate for that to take place in PSM than in NSS.  That
gives Mozilla more freedom to change this table without needing to force new
NSS updates, and it decouples Mozilla's desire for this feature from other
customers' desire to avoid unplanned changes to their applications' network
activities.
Thanks for the prompt reply.  WTC may well have rebuttals, but in the interim I agree that a patch like the one you suggest sounds like a good thing to have.
It's unfortunate that the political issues around this bug
have clouded the underlying technical issue.

The technical issue is simple: official representatives of
Comodo said it is fine to treat a batch of old certificates
issued by Comodo that only have CRLDP as if they had both
CRLDP and AIA.

Does this break backward compatibility?  The certs issued by
Comodo today have both CRLDP and AIA, so this hack will not
introduce new behavior to applications that need to support
certs issued by Comodo today.

Does NSS need to maintain this hack forever?  When this
batch of old certificates all expire, we can remove this
hack.  We can ask Comodo to annotate each entry in the
myDefaultOCSPResponders array with an expiration date,
as a condition for checkin.

I don't support adding new NSS callback API for this hack,
because I think this hack is useful to all NSS-based apps.

I'm removing myself from the decision of this bug.  Hopefully
that'll lead to quick resolution of this issue.
In reply to comment #9:
> Rob - in the meantime, care to re-attach the patch here while we work it
> through?  No need to request review yet.

Re-attached.
In reply to comment #10:
> ...If we change NSS to unilaterally inject OCSP addresses into certs for
> certain issuers, we will change the behavior of those existing applications,

I don't understand why that would be a problem.  Simply replacing a CDP-only certificate with one that contains AIA->OCSP would trigger the same behaviour change anyway.

> and we will have to support that behavior essentially for ever thereafter.

Surely NSS already supports "that behaviour", because it already supports OCSP?

> I am quite willing to create a new NSS callback API by which the application
> (PSM, in this case) can register a function to do this very thing.  I think
> it's much more appropriate for that to take place in PSM than in NSS.  That
> gives Mozilla more freedom to change this table without needing to force new
> NSS updates,

I concede that having the freedom to change myDefaultOCSPResponders[] without forcing a new NSS update would be desirable.  But, realistically, how often would the table change once CDP support has been completed?

> and it decouples Mozilla's desire for this feature from other customers'
> desire to avoid unplanned changes to their applications' network activities.

These customers cannot choose whether their certificates contain AIA->OCSP or not.  The CA decides.  So, if a customer replaces one CDP-only certificate with a certificate that contains AIA->OCSP, they would experience the same "unplanned change to their applications' network activities".
Network Solutions have confirmed that they are no longer including AIA->OCSP in their EV certificates (see Bug #477244 comment #98), due to bug #479029 preventing the EV UI from appearing when they did include AIA->OCSP.

In Bug #477244, Nelson (comment #70), Kai (comment #87) and Johnathan (comment #96) all called for NetSol to refain from doing this, but I have to say that I fully support NetSol's action.  Not getting the EV UI in any widely used version of Firefox seriously hampers an EV CA's ability to compete with other EV CAs.

I thought it would be useful to gather some statistics to try to predict how long it might take for the vast majority of Firefox users to be running a version that includes the fix for bug #477244.  The statistics below cover all the hits from Firefox clients to Comodo's OCSP Responders for the 24hr period commencing March 12th 03:00 GMT.

FF Release  FF Version          % of all FF
Notes Date                       OCSP hits
            1.5.0.x or lower    0.01%
            2.x or lower        0.11%
17-JUN-08   3.0.0 or lower      1.22%
16-JUL-08   3.0.1 or lower      3.49%
23-SEP-08   3.0.2 or lower      3.60%
26-SEP-08   3.0.3 or lower      4.90%
12-NOV-08   3.0.4 or lower      6.50%
16-DEC-08   3.0.5 or lower     10.04%
03-FEB-09   3.0.6 or lower     17.77%
04-MAR-09   3.0.7 or lower     99.50%
            3.1beta or lower  100.00%

This data suggests that ~82% of Firefox users have upgraded to the latest version (3.0.7) within 1 week of its release. That's pretty good so far, but...

This data also suggests, for example, that ~5% of Firefox users are still using versions for which at least 1 update has been available for over 4 months.

Assuming users' browser updating trends stay the same in the future, I predict that it will take at least 1 year for the fix for bug #479029 to have reached a sufficient number of Firefox users for NetSol to feel happy enough to start including OCSP Responder URLs in EV certificates again.

(These statistics probably vastly underestimate the FF 1.x and 2.x userbase, since these browsers have OCSP checking disabled by default).
Were the same consideration made by your CA or that of NetSol in relation to the EV UI of Microsoft's Internet Explorer - version 6 versus 7? What would you tell your customers running IE6 still? What about those which run IE7 on XP (and I think there had been some issues which required certain updates too)? Some surveys suggest that there are still about 25% of IE6 users out there...
Further to comments #1 and #15...
Bug #479029 leaves NetSol serving much more CRL traffic (instead of OCSP traffic) than they would otherwise have done.  Which will cost them more than it would otherwise have done.

My patch (or Nelson's proposed PSM/NSS variant of my patch) seems to be the only way that FF would be able to use NetSol's OCSP Responder in the short/medium term.  Therefore, it's the only way to help NetSol cut the costs they're going to have to spend on working around Mozilla's bug #479029.

The completion of CDP support for FF will further increase NetSol's costs (again, more than it would otherwise have done, due to bug #479029), and my patch would still be useful in helping them to cut costs.
But bug 479029 is fixed, which means that the vast majority will experience correct behavior. Can we view this situation similar to that of other browsers which don't support EV? An upgrade/update would solve the problem and the update take-up records are rather positive with Firefox (specially when compared with other browsers)?
In reply to comment #16:
Eddy, I don't understand what point you're trying to make.  You seem to be
completely off-topic.  But I'll do my best to answer.

> Were the same consideration made by your CA or that of NetSol in relation to
> the EV UI of Microsoft's Internet Explorer - version 6 versus 7?

IE6 has no EV UI.  Therefore, all EV CAs are equally disadvantaged in being
unable to show an EV UI in IE6.

In contrast, one EV CA (NetSol) are unfairly disadvantaged in being unable to
show an EV UI in FF 3.0.x, and several EV CAs could be unfairly disadvantaged
if FF 3.5 requires OCSP for EV.

> What would you tell your customers running IE6 still?

We would certainly advise our customers to upgrade.  But the people who visit
https sites that use Comodo certificates are our customers' customers, with
whom we don't have any kind of direct relationship to be able to advise them to
upgrade.

> What about those which run IE7 on XP (and I think there had been some issues
> which required certain updates too)?

IE7 on XP is the latest version for that OS.  We would advise customers to
install all critical security patches from Windows Update, plus the latest
Microsoft Root Certificate Update patch (which is an optional update).

For our customers' customers who don't install the latest Root Certificate
Update patch, it is a bit of a challenge to trigger the EV UI in IE7/XP for our
customers' EV sites.  Comodo have a patented solution for this problem which is
used by many of our customers.
In reply to comment #18:
> But bug 479029 is fixed, which means that the vast majority will experience
> correct behavior.

That depends on your definition of "vast majority" and the timescale within which they "will experience correct behaviour".

> Can we view this situation similar to that of other browsers which don't
> support EV?

I don't think so.

> An upgrade/update would solve the problem and the update take-up records are
> rather positive with Firefox (specially when compared with other browsers)?

Not positive enough IMHO, as I tried to show in comment #15.
(In reply to comment #19)
> In contrast, one EV CA (NetSol) are unfairly disadvantaged in being unable to
> show an EV UI in FF 3.0.x, and several EV CAs could be unfairly disadvantaged
> if FF 3.5 requires OCSP for EV.

Correct, point taken.

> We would certainly advise our customers to upgrade.

Right, I think the same effort has to be made here as well.

(In reply to comment #20)
> That depends on your definition of "vast majority" and the timescale within
> which they "will experience correct behaviour".

I think Johnathan could provide some answers here.

> Not positive enough IMHO, as I tried to show in comment #15.

Well, that was the point I tried to make. If I read your details correctly, than there are still more browsers in use which don't provide a distinctive UI for EV than the amount of users which wouldn't upgrade and fall into the same category.

What I'd like to see - and that's the reason why I'm posting here at all - that the affected CAs keep on the path to INCLUDE AIA OCSP URI in their certificates and move on with the transition to OCSP. IMO removing it is a step backward and undesirable for the industry and for the advancing of this revocation checking method.
In reply to comment #21:
> Well, that was the point I tried to make. If I read your details correctly,
> than there are still more browsers in use which don't provide a distinctive UI
> for EV than the amount of users which wouldn't upgrade and fall into the same
> category.

That may be true, but I think you're comparing apples with oranges.  Nobody expects to see any EV UI in those browsers that have no EV UI.  In contrast, NetSol's customers do expect FF 3.0.0->3.0.latest to show the Firefox EV UI for NetSol EV certificates.

> What I'd like to see - and that's the reason why I'm posting here at all -
> that the affected CAs keep on the path to INCLUDE AIA OCSP URI in their
> certificates

NetSol have not suggested that they will never include AIA->OCSP ever again.

> and move on with the transition to OCSP.

When client-side support for OCSP works properly (think bug #479029 and see comment #15), then I imagine NetSol will conclude that a "transition to OCSP" is both possible and sensible.

> IMO removing it is a step backward

Yes, in a way, but...

> and undesirable for the industry

IMO, retaining AIA->OCSP when it causes the EV UI to disappear would be very undesirable for NetSol and would also be undesirable for the industry.  The industry wants EV to work.

> and for the advancing of this revocation checking method.

See bug #477244 comments #58 and #59 (again) for what the industry has to say about "the advancing of this revocation checking method".
Your comment #59 at bug 477244 is simply wrong!

Section 26. EV Certificate Status Checking says:

It is *strongly* RECOMMENDED that all CAs support OCSP when a majority of
deployed Web servers support the TLS 1.0 extension in accordance to RFC 3546,
to return “stapled” OCSP responses to EV-enabled applications. CAs *MUST*
support an OCSP capability for Subscriber Certificates that are issued after Dec
31, 2010.

THIS is what the industry apparently has to say...
In reply to comment #23:
> Your comment #59 at bug 477244 is simply wrong!

I stand by comment #59.

> It is *strongly* RECOMMENDED...
> ...THIS is what the industry apparently has to say...

I've already addressed that quote from the EV Guidelines in #477244 comment #94.
I think Mozilla should take that to the CAB Forum to make it clear what OCSP "capability" means. Thanks for pointing out the nuances in the guidelines.
Gentlemen:
Let's take the discussion about pros and cons of AIA/OCSP to Mozilla's 
security policy newsgroup/list, or the crypto newsgroup/list, and leave
this bug for issues directly pertaining to coding a fix for this problem.
Assignee: nobody → nelson
This alternative to Rob's patch creates a new callback API, with which PSM
(or any application) can register its own function that finds default OCSP
URLs for certs that don't have them.  

I'm asking Julien to review.  Julien, let me explain the background of this
before you review it.  Thanks.

My next attachment will be a sample code snippet illustrating how this new
NSS API could be used to register a callback function (that is basically 
Rob's code).
Attachment #368193 - Flags: review?(julien.pierre.boogz)
Perhaps Rob could work this snipped into a new PSM patch.
In reply to comment #27:
> This alternative to Rob's patch creates a new callback API, with which PSM
> (or any application) can register its own function that finds default OCSP
> URLs for certs that don't have them.

Thanks Nelson.

Your patch adds two calls to PR_ExitMonitor() in ocsp_GetResponderLocation().  Shouldn't the first of these be a call to PR_EnterMonitor() instead?

In reply to comment #28:
> Perhaps Rob could work this snipped into a new PSM patch.

I'll have a go.
Attached patch alternative patch v2 (obsolete) — Splinter Review
Good catch.  Copy-paste error.
Attachment #368193 - Attachment is obsolete: true
Attachment #368236 - Flags: review?(julien.pierre.boogz)
Attachment #368193 - Flags: review?(julien.pierre.boogz)
Further to comment #29:
> In reply to comment #28:
> > Perhaps Rob could work this snipped into a new PSM patch.
>
> I'll have a go.

PSM patch attached.
Attachment #368263 - Flags: review?
Attachment #368263 - Flags: review? → review?(honzab.moz)
Attachment #368236 - Flags: review?(honzab.moz)
Rob, did the combination of your patch and mine work satisfactorily for you?
Priority: -- → P2
Target Milestone: --- → 3.12.3
In reply to comment #32:
> Rob, did the combination of your patch and mine work satisfactorily for you?

Yes.  With Firefox HEAD + NSS HEAD + your NSS patch + my PSM patch, I see the same behaviour as I did with Firefox HEAD + NSS HEAD + my NSS patch...

https://www.verisign.com (OCSP URLs in all the certificates) shows the EV UI.
https://secure.comodo.com (OCSP URL in the end-entity certificate only; patch provides OCSP URLs for the CA certificates) shows the EV UI.
https://www.networksolutions.com (no OCSP URLs in the certificates; patch provides OCSP URLs for all of the certificates) shows the EV UI.
https://www.globalsign.com (no OCSP URLs in the certificates or the patch) does not show the EV UI.
Attachment #368263 - Flags: review?(honzab.moz) → review-
Comment on attachment 368263 [details] [diff] [review]
Patch PSM to hook Nelson's NSS callback API to use default OCSP Responders for EV certificate chains from Comodo and Network Solutions

Please:
1. Save the old callback when setting CERT_RegisterAlternateOCSPAIAInfoCallBack and use it as chained in your callback implementation. Extensions or other parts of PSM may register another callbacks, at least it's possible so let's don't break it.
2. Prevent subsequent calls to CERT_RegisterAlternateOCSPAIAInfoCallBack while your callback has already been registered, probably using condition like (!oldCallback) ?
3. Don't forget to revert to the old callback while shutting down.
4. Please instead of comparing by common name use comparison by a fingerprint, same as EV trust is checked. Use following code in your callback to get the issuer's fingerprint, successfully tested:

    CERTCertificate *issuerCert = CERT_FindCertIssuer(cert, PR_Now(), certUsageAnyCA);
    CERTCertificateCleaner issuerCertCleaner(issuerCert);

    nsNSSCertificate c(issuerCert);

    nsAutoString fingerprint;
    if (NS_FAILED(c.GetSha1Fingerprint(fingerprint)))
      return NULL;


You can fill the array with nsDepenedentString("XX:YY:...") and then do if (fingerprint == myDefaultOCSPResponders[i].fingerprint) ...

This has advantages as we won't use not very safe strcmp and also this "hack" will stop working for newly issued certificates automatically as their fingerprint will change.

If you have strong arguments against don't hesitate to publish them.

I was testing with following pages while applied also the patch for bug 479029:
https://www.softballoutlet.com/ - ok
https://secure.comodo.com/ - ok
https://www.gandi.net/ - ok
https://ev.tbs-x509.com/ - ok, until there is made an unsecure request
https://www.globalsign.com/ - doesn't show EV UI
https://www.interfimo.fr/ not ok, even I imported all CAs from bug 479029 comment 10, but this bug's patches has no affect here, your callback is not invoked for this site
Comment on attachment 368236 [details] [diff] [review]
alternative patch v2

Just some formatting details here: https://bugzilla.mozilla.org/attachment.cgi?id=368236&action=diff#mozilla/security/nss/lib/certhigh/ocsp.c_sec2

And, are you sure it's a good idea to call the callback function inside the global lock?
Attachment #368236 - Flags: review?(honzab.moz) → review+
In reply to comment #34:

Thanks for your comments Honza.

> 1. Save the old callback...
> 2. Prevent subsequent calls...
> 3. Don't forget to revert to the old callback while shutting down.

Good points.  I'll try to address these.

> 4. Please instead of comparing by common name use comparison by a fingerprint,
> same as EV trust is checked. Use following code in your callback to get the
> issuer's fingerprint, successfully tested:

Thanks, your code snippet answers one of my questions from bug #477244 comment #51.  But...

> This has advantages as we won't use not very safe strcmp

To avoid that strcmp, I suggest comparing against cert->derIssuer instead of cert->issuerName.

> and also this "hack" will stop working for newly issued certificates
> automatically as their fingerprint will change.

IMHO, that's a disadvantage, hence why I still think that Issuer Name (using cert->derIssuer) and Issuer Key are better comparison fields than Issuer Fingerprint.

If we use Issuer Fingerprint:
- I'll need to double the number of entries in myDefaultOCSPResponders[] in order to explicitly cope with all of Comodo's and NetSol's existing cross-certificates.
- Any future cross-certificates issued to any of these Issuers will fail to trigger the "hack".  The problem is...I would want them to trigger the "hack".

If we use Issuer Name and Issuer Key, neither of these issues arises.

> I was testing with following pages...
> https://www.interfimo.fr/ not ok, even I imported all CAs from bug 479029
> comment 10, but this bug's patches has no affect here, your callback is not
> invoked for this site

See bug #479029 comment #12.  Then look at bug #479508 comment #2.
(In reply to comment #35)
> And, are you sure it's a good idea to call the callback function inside the
> global lock?

I'm not sure that the lock is needed at all here.  I expect that the contents 
of this global pointer will not change after the process has initialized NSS.
But use of this Monitor is something about which Julien is passionate, which
is why I've also asked him to review it.   

If we're worried that the global pointer could change while this code is 
executing, then we might also worry that the shared library in which the 
function to which it points lives might be dynamically unloaded while it
is in use.  Holding the lock around the entire call ensures that will not 
happen.  That's why it's coded like that, but personally, I think all 
this locking is unnecessary.  

I will attach another patch that does not hold the lock around call, and
let Julien decide which to use.
It would be good to carry the callback address out to a local variable under the lock and then call it outside the lock, something similar you do for "old" variable in the registering function. When the callback invocation would lead to deregistration and say that from inside the callback it would also try to deregister the callback from NSS, we would probably get deadlocked or prepare land to be suddenly deadlocked somewhere else, that's how PRLock behaves and contains assertion not to enter the same lock more then ones on the same thread. Also my experience tells me that calling "foreign" methods or functions under a lock, more a global one, is a certain path to a deadlock.

And I'd say it's good to have the lock for this as NSS is multithreaded and the callback will probably be set and then invoked on different threads.
This patch does not hold the monitor around the call, which means it offers
no protection against the called function disappearing while it is in use.
I'm not sure that the lock offers any protection against anything as used in
this patch, and would be willing to make a third patch that doesn't use it 
here at all.  

Julien, Please decide, today, which of these alternative patches to use.
Thanks.
Attachment #368573 - Flags: review?(julien.pierre.boogz)
(In reply to comment #38)
> When the callback invocation would lead to deregistration and say that 
> from inside the callback it would also try to deregister the callback 
> from NSS, we would probably get deadlocked or prepare land to be suddenly 
> deadlocked somewhere else, that's how PRLock behaves and contains 
> assertion not to enter the same lock more then once on the same thread.

True, but we're not using a PRLock.  We're using a PRMonitor, and PRMonitor
does allow reentrant (nested) enters and exits on the thread holding the 
PRMonitor.
(In reply to comment #36)
> In reply to comment #34:
> > This has advantages as we won't use not very safe strcmp
> 
> To avoid that strcmp, I suggest comparing against cert->derIssuer instead of
> cert->issuerName.

I'm not 100% sure, but please read bug 400085, you have to go through it from top to a bottom.

> 
> > and also this "hack" will stop working for newly issued certificates
> > automatically as their fingerprint will change.
> 
> IMHO, that's a disadvantage, hence why I still think that Issuer Name (using
> cert->derIssuer) and Issuer Key are better comparison fields than Issuer
> Fingerprint.
> 
> If we use Issuer Fingerprint:
> - I'll need to double the number of entries in myDefaultOCSPResponders[] in
> order to explicitly cope with all of Comodo's and NetSol's existing
> cross-certificates.

That is not an argument for me.

> - Any future cross-certificates issued to any of these Issuers will fail to
> trigger the "hack".  The problem is...I would want them to trigger the "hack".
> 
> If we use Issuer Name and Issuer Key, neither of these issues arises.

As I understand, you have issued in the past a set of CAs that doesn't contain AIA->OCSP. We want to, instead of reissue of all those certificates, that are not that many, to inject the missing AIA->OCSP information into FF. 

Any newly issued certificate should already contain that information, or not? Maybe I just don't understand correctly the politics here, and need (ones more) to simply explain.
(In reply to comment #40)
> (In reply to comment #38)
> True, but we're not using a PRLock.  We're using a PRMonitor, and PRMonitor
> does allow reentrant (nested) enters and exits on the thread holding the 
> PRMonitor.

Ah, I was convinced it's purely using PR_Lock. Then, I have absolutely no objections to your patch.
Further to comment #36:
> > I was testing with following pages...
> > https://www.interfimo.fr/ not ok, even I imported all CAs from bug 479029
> > comment 10, but this bug's patches has no affect here, your callback is not
> > invoked for this site
> 
> See bug #479029 comment #12.  Then look at bug #479508 comment #2.

NSS_ENABLE_PKIX_VERIFY=1 with the current Firefox HEAD + NSS HEAD gives
"security library: invalid arguments.
(Error code: sec_error_invalid_args)"
with...
https://secure.comodo.com/
https://www.verisign.com/
https://www.globalsign.com/
...but not with...
https://www.entrust.net/
https://www.startssl.com/

This happens without applying any of the patches in this bug.
(In reply to comment #43)

> NSS_ENABLE_PKIX_VERIFY=1 with the current Firefox HEAD + NSS HEAD gives
> "security library: invalid arguments.
> (Error code: sec_error_invalid_args)"

Rob, please file a separate NSS bug and give complete steps to reproduce.
Thanks.
In reply to comment #41:
> > To avoid that strcmp, I suggest comparing against cert->derIssuer instead of
> > cert->issuerName.
>
> I'm not 100% sure, but please read bug 400085, you have to go through it from
> top to a bottom.

I can vouch that a raw DER comparison will always work for certificates issued
by Comodo.  If NetSol (and any other CAs whose OCSP details we might add to the
patch) can say the same, then it would be overkill to implement full RFC5280
name comparison rules for this "hack".  

> > If we use Issuer Name and Issuer Key, neither of these issues arises.
>
> As I understand, you have issued in the past a set of CAs that doesn't contain
> AIA->OCSP.

It's not just CA certificates.  There are many end-entity certificates without
AIA->OCSP as well.  My patch addresses both.

> We want to, instead of reissue of all those certificates, that are
> not that many, to inject the missing AIA->OCSP information into FF.

Yes, although I wouldn't call it "not that many".

> Any newly issued certificate should already contain that information, or not?

Yes for Comodo.  No for NetSol at the moment (see comment #15).

> Maybe I just don't understand correctly the politics here, and need (ones 
> more) to simply explain.

Example:
Intermediate CA Certificate CA1 has Subject Name SN1 and Public Key PK1.  CA1
has a default OCSP Responder URL in our "hack".  CA1 has issued end-entity EV
certificates without AIA->OCSP.
The owner of CA1 decides to replace CA1 with new Intermediate CA Certificate
CA2.  CA2 also has Subject Name SN1 and Public Key PK1, making it
interchangeable with CA1.  (CA2 may differ from CA1 in terms of validity
period, issuer, presence/absence of AIA extension or AIA->OCSP, etc).
The owner of CA1 and CA2 decides to instruct existing EV customers to switch
from using CA1 to using CA2.  The end-entity EV certificates are not changed.

Problem: CA2 does not have a default OCSP Responder URL in our "hack", but the
end-entity certificates still have no AIA->OCSP.  Therefore, switching from CA1
to CA2 would make the EV UI disappear in FF3.5.

Solution: Instead of directly referencing CA1 in our "hack", let's just
reference SN1 and PK1.  Then the "hack" will work seamlessly for both CA1 and
CA2.

This is a real-world scenario.
Comodo have issued multiple Intermediates with matching Subject Names/Keys in
the past.
Comodo's current EV Intermediate CA Certificates don't contain AIA->OCSP.  It's
very possible that we might choose to replace these with new Intermediates that
do contain AIA->OCSP and which have the same Names/Keys as the current
Intermediates.  If or when we do this, we would expect existing end-entity EV
certificates that don't contain AIA->OCSP to still get the EV UI.
In reply to comment #44:
> > NSS_ENABLE_PKIX_VERIFY=1 with the current Firefox HEAD + NSS HEAD gives
> > "security library: invalid arguments.
> > (Error code: sec_error_invalid_args)"
>
> Rob, please file a separate NSS bug and give complete steps to reproduce.
> Thanks.

I've just filed bug #484466.
In reply to Rob's comment 45,
> If NetSol (and any other CAs whose OCSP details we might add to the
> patch) can say the same, then it would be overkill to implement full 
> RFC 5280 name comparison rules for this "hack".  

NSS already has a function for comparing two parsed DNs.  See 
http://mxr.mozilla.org/security/source/security/nss/lib/certdb/secname.c#608

But it seems to me that issuer name is insufficient, by itself.  
There's also the issuer's subject key ID.  IINM, that must also be taken 
into account when considering which source of revocation information is 
the right one.
I meant to write:
NSS already has a function for comparing two parsed DNs that is thought
to conform to the full requirements for RFC 3280.  Dunno about 5280.
Comment on attachment 368236 [details] [diff] [review]
alternative patch v2

I prefer not to lock around the call to the callback function. This serialization could be an unnecessary performance problem. I say let the callback function itself perform any serialization it requires, hopefully less than a global lock.
Attachment #368236 - Flags: review?(julien.pierre.boogz) → review-
Comment on attachment 368573 [details] [diff] [review]
alternative patch v3 - don't hold lock around call (Checked in)

This patch is OK.

I would suggest the following improvements :

1. In CERT_RegisterAlternateOCSPAIAInfoCallback, check the input argument newCallBack and return SECFailure with SEC_ERROR_INVALID_ARGS if NULL.

2. This is more of a design issue, and I haven't been involved all along with the design for this, but I don't like global settings such as this one. I would much prefer if CERT_RegisterAlternateOCSPAIAInfoCallBack would set this callback in some kind of verification context, as opposed to something global, in a way that could be parameterized for various PKIX callers within an NSS application, which might not all agree on the choice of AIA callback.
Attachment #368573 - Flags: review?(julien.pierre.boogz) → review+
Comment on attachment 368573 [details] [diff] [review]
alternative patch v3 - don't hold lock around call (Checked in)

Thanks for the reviews.  Committed on trunk

certdb/certt.h;         new revision: 1.49; previous revision: 1.48
certhigh/ocsp.c;        new revision: 1.58; previous revision: 1.57
certhigh/ocsp.h;        new revision: 1.14; previous revision: 1.13
nss/nss.def;            new revision: 1.198; previous revision: 1.197
Attachment #368573 - Attachment description: alternative patch v3 - don't hold lock around call → alternative patch v3 - don't hold lock around call (Checked in)
Attachment #368236 - Attachment is obsolete: true
Two questions for the participants in this bug:

1) Who (if anyone) is going to finish the PSM patch?
2) Now that the NSS part of this bug is done, should there be a separate 
PSM bug for the remaining PSM work?
In reply to comment #52:
> 1) Who (if anyone) is going to finish the PSM patch?

I've been working on it.  I'm sure others who are more familiar with PSM would do it much quicker, but I'm almost ready to post an updated patch.

> 2) Now that the NSS part of this bug is done, should there be a separate PSM bug
> for the remaining PSM work?

That would make sense.
Further to comment #53:
> > 2) Now that the NSS part of this bug is done, should there be a separate PSM
> > bug for the remaining PSM work?
>
> That would make sense.

I've just filed bug #485052.
Summary: Embed a list of default OCSP Responder URLs for certain CAs → NSS Callback API for looking up a default OCSP Responder URL
Comment on attachment 367752 [details] [diff] [review]
Patch NSS to use default OCSP Responders for EV certificate chains from Comodo and Network Solutions

Moved to bug #485052.
Attachment #367752 - Attachment is obsolete: true
Comment on attachment 368263 [details] [diff] [review]
Patch PSM to hook Nelson's NSS callback API to use default OCSP Responders for EV certificate chains from Comodo and Network Solutions

Moved to bug #485052.
Attachment #368263 - Attachment is obsolete: true
Blocks: 477244
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Blocks: 485052
You need to log in before you can comment on or make changes to this bug.