Closed Bug 485052 Opened 15 years ago Closed 15 years ago

Embed a list of default OCSP Responder URLs for certain CAs

Categories

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

defect

Tracking

()

RESOLVED FIXED

People

(Reporter: rob, Assigned: rob)

References

()

Details

(Keywords: fixed1.9.1, verified1.9.0.13, verified1.9.0.14)

Attachments

(2 files, 2 obsolete files)

User-Agent:       Mozilla/4.0 (compatible; MSIE 7.0; Windows NT 5.1)
Build Identifier: trunk

Bug #483168 defined a new NSS callback API (targeted for NSS 3.12.3) for providing a default OCSP Responder URL.

This bug is for patching PSM to hook this new NSS API to provide default OCSP Responder URLs for EV certificate chains from (at least) Comodo and Network Solutions.  Previous attempts at writing this patch can be found in bug #477244 and bug #483168.

Reproducible: Always
Assignee: kaie → rob
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: wanted1.9.1?
Flags: blocking1.9.1?
Version: unspecified → Trunk
This patch replaces bug #483168 attachment #368263 [details] [diff] [review].

I've attempted to address the comments from Honza (bug #483168 comment #34) and Nelson (bug #483168 comment #47).

This patch behaves as expected for:
https://secure.comodo.com         (relies on patch for Intermediate CA OCSP URL)
https://www.verisign.com          (patch not used)
https://www.networksolutions.com  (relies on patch for OCSP URLs)
https://www.globalsign.com        (no OCSP URLs, so no EV UI)

But for some reason, browsing to https://bugzilla.mozilla.org consistently gives me:
"security library: bad database.
(Error code: sec_error_bad_database)"
...and browsing to https://www.securetrust.com and/or https://www.trustwave.com often gives me:
"Unrecognized Object Identifier.
(Error code: sec_error_unrecognized_oid)"

These errors only go away when I remove the call to CERT_FindCertIssuer() from my patch.  This leaves me confused, because Honza asked me to use this function.  Is this a problem with my patch, or does my patch highlight a problem with some other code?

Perhaps I should i) avoid trying to lookup the issuer certificate, ii) use cert->issuer instead of issuerCert->subject, and iii) use cert->authKeyID->keyID instead of issuerCert->subjectKeyID ?
Attachment #369152 - Flags: review?
Adding other interested parties to the cc list.
Attached patch Alternative v2 patch (obsolete) — Splinter Review
Further to comment #1:
> Perhaps I should i) avoid trying to lookup the issuer certificate, ii) use
> cert->issuer instead of issuerCert->subject, and iii) use
> cert->authKeyID->keyID instead of issuerCert->subjectKeyID ?

Alternative patch attached.  This works around whatever is causing the sec_error_bad_database and sec_error_unrecognized_oid errors.

Without NSS_ENABLE_PKIX_VERIFY=1, this patch causes the EV UI to appear when I'd expect it to.

With NSS_ENABLE_PKIX_VERIFY=1, I cannot test some sites.  I've just filed bug #485155 about this.
Attachment #369250 - Flags: review?
Johnathan,

Nelson's NSS patch has already been checked in (bug #483168), and I anticipate that some version of my PSM patch will pass review soon...

In reply to bug #477244 comment #63:
> ...If Nelson & Kai find Rob's patch acceptable technically, I will contact the
> other CAs to ask whether they would prefer this approach.

Would now be a good time for you to contact GlobalSign, Trustwave, SECOM Trust and Cybertrust, to see if they would like to provide default OCSP Responders to be included in my PSM patch?
Comment on attachment 369152 [details] [diff] [review]
Patch PSM to provide default OCSP Responders for EV certificate chains from Comodo and Network Solutions v2

CERT_FindCertIssuer will return one issuer cert for the subject cert.  It may not be the issuer cert that is relevant to the EV chain, however. 
So the other patch attached to this bug is more correct in that it does not rely on CERT_FindCertIssuer.
Attachment #369152 - Flags: review? → review-
Attachment #369250 - Flags: review?(kaie)
Attachment #369250 - Flags: review?(honzab.moz)
Attachment #369250 - Flags: review?
Comment on attachment 369250 [details] [diff] [review]
Alternative v2 patch 

Kai or Honza should review this patch and then shepherd it through the rest of the process.
I won't probably get to this patch sooner then in several days.
Rob, you should probably not use NSS trunk, but current version merged to mozilla cvs head and just apply patches necessary for this bug to work.
The NSS team is planning on making a release from the trunk on Monday, so
developing things based on the NSS trunk is probably reasonable right now.
Flags: blocking1.9.1? → blocking1.9.1+
I find your code
  if "opposite of what I want" ; // where ";" means "do nothing"
  else if "opposite of what I want" ;
  else do-it

a bit uncommon and tricky to read.

I usually prefer code that says

  if "what I want
      && what I want
      && what I want"
    do-it
Comment on attachment 369250 [details] [diff] [review]
Alternative v2 patch 

>+struct OCSPDefaultResponders {
>+    char *issuerName_string;
>+    CERTName *issuerName;
>+    char *issuerKeyID_base64;
>+    SECItem *issuerKeyID;
>+    char *ocspUrl;
>+};

let's change the the char* to const char*
This will require one const_cast.
I think function CERT_AsciiToName will not modify the string...


>+CERT_StringFromCertFcn oldOCSPAIAInfoCallback = nsnull;

let's make this static, too


>+SECStatus RegisterMyOCSPAIAInfoCallback() {
...
>+    // Create a CERTName structure from the issuer name string.
>+    myDefaultOCSPResponders[i].issuerName = CERT_AsciiToName(
>+                                  myDefaultOCSPResponders[i].issuerName_string);

const_cast<char*>(myDefaultOCSPResponders[i].issuerName_string));



I will attach a new patch where I address my own minor review comments.
Attachment #369250 - Flags: review?(kaie)
Attachment #369250 - Flags: review?(honzab.moz)
Attached patch Patch v3Splinter Review
This is Rob's patch with my minor tweaks.

r=kaie
Attachment #369250 - Attachment is obsolete: true
Attachment #370049 - Flags: review+
Comment on attachment 369152 [details] [diff] [review]
Patch PSM to provide default OCSP Responders for EV certificate chains from Comodo and Network Solutions v2

marking obsolete per comment 5
Attachment #369152 - Attachment is obsolete: true
adding URLs for manual testing.
(In reply to comment #4)
> Johnathan,
> 
> Nelson's NSS patch has already been checked in (bug #483168), and I anticipate
> that some version of my PSM patch will pass review soon...
> 
> In reply to bug #477244 comment #63:
> > ...If Nelson & Kai find Rob's patch acceptable technically, I will contact the
> > other CAs to ask whether they would prefer this approach.
> 
> Would now be a good time for you to contact GlobalSign, Trustwave, SECOM Trust
> and Cybertrust, to see if they would like to provide default OCSP Responders to
> be included in my PSM patch?

Hi Rob,

Thanks for the updated patches (thanks Kai and Nelson and Honza too!) I'll
contact the other CAs but I don't think those conversations should slow down
this bug - the sooner it is reviewed and landed, the sooner we get testing. 
Their responders can be added as separate bugs if need be.
Keywords: checkin-needed
In reply to comments #11 an #12:
Kai, thanks for your comments and thanks for tweaking my patch!

In reply to comment #10:
> I find your code
>   if "opposite of what I want" ; // where ";" means "do nothing"
>   else if "opposite of what I want" ;
>   else do-it
> a bit uncommon and tricky to read.

Sorry about that.  It's a habit I picked up years ago after using some ancient C compiler that always evaluated every clause in an if() statement - i.e. "if (a && a->b)" would cause the program to crash when a == NULL.

I don't object to you changing it to your preference if you want to.
I believe this can't be checkin-needed until a new NSS tag is landed and pulled for mozilla-central, is that right, Kai?  Is there a bug tracking the landing of the upcoming tag?
(In reply to comment #17)
> I believe this can't be checkin-needed until a new NSS tag is landed and pulled
> for mozilla-central, is that right, Kai?

Unfortunately you're right.
While this patch is only in PSM (that's what led me to adding checkin-needed) it depends on a new base NSS feature which hasn't arrived in the codebase yet.

We need the upcoming NSS 3.12.3 release.
Keywords: checkin-needed
Depends on: 486182
Depends on: 483168
Depends on: 479029
Tinderbox cycled OK on Linux+OSX, pending Windows, but I'm optimistic and declare this bug as resolved fixed.

We shall file follow up bugs to get OCSP responder URLs for other CAs added?
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
This patch caused an Lk regression on all platforms of about 15kB for Firefox, a similar regression is seen on the Thunderbird tinderboxes.

From the logs the additional leak info:

15668 malloc
  14952 malloc (/builds/moz2_slave/mozilla-central-linux-debug/build/tools/trace-malloc/lib/nsTraceMalloc.c:1150)
    15032 PR_Malloc (/builds/moz2_slave/mozilla-central-linux-debug/build/nsprpub/pr/src/malloc/prmem.c:467)
      15032 PL_ArenaAllocate (/builds/moz2_slave/mozilla-central-linux-debug/build/nsprpub/lib/ds/plarena.c:229)
        15032 PORT_ArenaAlloc_Util (/builds/moz2_slave/mozilla-central-linux-debug/build/security/nss/lib/util/secport.c:246)
          14497 CERT_CreateName (/builds/moz2_slave/mozilla-central-linux-debug/build/security/nss/lib/certdb/secname.c:383)
            14497 ParseRFC1485Name (/builds/moz2_slave/mozilla-central-linux-debug/build/security/nss/lib/certdb/alg1485.c:482)
              14497 CERT_AsciiToName (/builds/moz2_slave/mozilla-central-linux-debug/build/security/nss/lib/certdb/alg1485.c:547)
                14497 RegisterMyOCSPAIAInfoCallback() (/builds/moz2_slave/mozilla-central-linux-debug/build/security/manager/ssl/src/nsNSSCallbacks.cpp:1154)
                  14497 nsNSSComponent::InitializeNSS(int) (/builds/moz2_slave/mozilla-central-linux-debug/build/security/manager/ssl/src/nsNSSComponent.cpp:1688)
                    14497 nsNSSComponent::Init() (/builds/moz2_slave/mozilla-central-linux-debug/build/security/manager/ssl/src/nsNSSComponent.cpp:1804)
                      14497 nsNSSComponentConstructor (/builds/moz2_slave/mozilla-central-linux-debug/build/security/manager/ssl/src/nsNSSModule.cpp:184)
                        14497 nsGenericFactory::CreateInstance(nsISupports*, nsID const&, void**) (/builds/moz2_slave/mozilla-central-linux-debug/build/obj-firefox/xpcom/build/nsGenericFactory.cpp:80)
                          14497 nsComponentManagerImpl::CreateInstanceByContractID(char const*, nsISupports*, nsID const&, void**) (/builds/moz2_slave/mozilla-central-linux-debug/build/xpcom/components/nsComponentManager.cpp:1684)
                            14497 nsComponentManagerImpl::GetServiceByContractID(char const*, nsID const&, void**) (/builds/moz2_slave/mozilla-central-linux-debug/build/xpcom/components/nsComponentManager.cpp:2251)
                              14497 CallGetService(char const*, nsID const&, void**) (/builds/moz2_slave/mozilla-central-linux-debug/build/obj-firefox/xpcom/build/nsComponentManagerUtils.cpp:94)
          535 secmod_NewModule (/builds/moz2_slave/mozilla-central-linux-debug/build/security/nss/lib/pk11wrap/pk11pars.c:68)
            535 SECMOD_CreateModule (/builds/moz2_slave/mozilla-central-linux-debug/build/security/nss/lib/pk11wrap/pk11pars.c:117)
              535 SECMOD_LoadModule (/builds/moz2_slave/mozilla-central-linux-debug/build/security/nss/lib/pk11wrap/pk11pars.c:310)
                535 SECMOD_LoadUserModule (/builds/moz2_slave/mozilla-central-linux-debug/build/security/nss/lib/pk11wrap/pk11pars.c:391)
                  535 nsNSSComponent::InstallLoadableRoots() (/builds/moz2_slave/mozilla-central-linux-debug/build/security/manager/ssl/src/nsNSSComponent.cpp:871)
                    535 nsNSSComponent::InitializeNSS(int) (/builds/moz2_slave/mozilla-central-linux-debug/build/security/manager/ssl/src/nsNSSComponent.cpp:1693)
                      535 nsNSSComponent::Init() (/builds/moz2_slave/mozilla-central-linux-debug/build/security/manager/ssl/src/nsNSSComponent.cpp:1804)
                        535 nsNSSComponentConstructor (/builds/moz2_slave/mozilla-central-linux-debug/build/security/manager/ssl/src/nsNSSModule.cpp:184)
                          535 nsGenericFactory::CreateInstance(nsISupports*, nsID const&, void**) (/builds/moz2_slave/mozilla-central-linux-debug/build/obj-firefox/xpcom/build/nsGenericFactory.cpp:80)
                            535 nsComponentManagerImpl::CreateInstanceByContractID(char const*, nsISupports*, nsID const&, void**) (/builds/moz2_slave/mozilla-central-linux-debug/build/xpcom/components/nsComponentManager.cpp:1684)
                              535 nsComponentManagerImpl::GetServiceByContractID(char const*, nsID const&, void**) (/builds/moz2_slave/mozilla-central-linux-debug/build/xpcom/components/nsComponentManager.cpp:2251)

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1239070965.1239072312.11106.gz&fulltext=1
Uh....  So is it me, or is the NSS code in PORT_FreeArena just buggy?  It never frees the memory in the area (in recent NSPR without some special env vars set); it just tosses it on the arena freelist, and then never frees the freelist.
It's you, Boris.  :)
There's a function to empty the arena free list.  
I gather that it's not being called in the right place(s).
Nelson, there's PL_ArenaFinish.  It's not called anywhere in NSS code, nor anywhere else I see.  I see nothing else that empties the arena freelist.

Note that this is NOT the freelist within the PLArenaPool I'm talking about; this is the global arena freelist.  See the difference between PL_FreeArenaPool (which is what NSS calls) and PL_FinishArenaPool.
Priority: -- → P1
Boris, I'm intimately familiar with the PLArenaPool code.

The whole idea of the global PLArena Free list is to last process lifetime 
(or until PL_ArenaFinish is called) to recycle PLArenas from one allocation 
to the next.  It's not broken.  It's serving its intended purpose.  

If you're measuring "leaks" in the middle of a running process lifetime,
then you will see freed arenas on the PLArena free list.  If you're 
concerned that PLArenas are not being returned to the heap immediate after 
being freed, and you're saying you don't WANT the PL arena free list to serve 
its design purpose, then turn off NSS's use of the arena free list by 
defining the environment variable NSS_DISABLE_AREA_FREE_LIST.
Priority: P1 → --
Priority: -- → P1
Nelson, we are seeing leaks on shutdown as signified by a increase in Lk on the tinderboxes measured by trace-malloc.
Nelson, I think we're both familiar with the PLArenaPool code and both understand what it's doing.

The facts are that:

1)  No one ever calls PL_ArenaFinish in Gecko (and certainly not in NSS, in
    particular!).  I said this above; I'm not sure why you chose to ignore that.
2)  The current NSS code effectively leaks memory for the lifetime on the
    process.  That is, whatever high water mark of PLArena use is hit, that
    memory will always stay allocated and will never be released.  As you point
    out, we can work around this in Gecko, where this behavior is
    unacceptable.  It might be fine for other NSS consumers, of course.

We should probably file a followup bug to set this environment variable as part of app startup before NSS init and stop worrying about this "allocate a bunch of memory and never free it" so-called "design purpose".
Boris,

NSS does not call PR_ArenaFinish because NSS does not presume that it is 
the sole user of NSPR.  It is the application's job to call it, not NSS's. 

NSS uses many PLArenas.  It has done so for 10 years or more.  This is not
a recent change in behavior.  Any PLArenas that NSS has ever allocated 
have always gone onto the PLArenaFreeList where they live for (most of) 
the rest of the process lifetime.  If Gecko has not called PR_ArenaFinish 
for 10 years, then PLArenas have been living for the process lifetime for 
10 years.  

Why this 10+ year old behavior has suddenly become a problem in early April 
2009 is a mystery.  Why it is considered an NSS bug is an even bigger mystery.
> It is the application's job to call it, not NSS's

Arguably, yes.  I'm rather glad it doesn't, in this case, because that would have papered over the problem.

> Why this 10+ year old behavior has suddenly become a problem

Because before this no one on this side of the NSS API realized NSS has this behavior.  It was recognized as a problem because some CERT_CreateName calls were added and the resulting jump in leak statistics was clearly showing the leak to be of arenas allocated by NSS.

Now that we know NSS has this wacky and poorly documented behavior (do YOU see anything in the CERT_DestroyName documentation that makes it clear that it doesn't bother actually deallocating the memory for the CERTName's arena until some other function is called?), we should take steps on our end to deal with it.

I do see that https://developer.mozilla.org/en/NSS_Memory_allocation documents the behavior in general, which I appreciate.  I wonder how many people have seen this page....

In any case, we need a bug on fixing that leak.  Rob, please file one.
I think Nelson says, allocated, but no longer used arena memory blocks will be remembered in a list, and Nelson says, the memory blocks will get reused.

If that's correct, this sounds fine to me in general.
Boris do you have reason to believe this mechanism is failing and causing real leaks?
Or do you think it's not acceptable for Gecko to have those memory blocks waiting, because they add to bloat?
That setup means that whatever high water mark of memory usage we hit for those arenas, we'll never be using less memory than that.

If these are all transient objects, then it might be ok.  If these are per-page objects (such that opening a lot of pages and then closing them will lead to all the memory not being deallocated), or worse yet if the the server can trigger large allocations in certain cases, then it's a pretty serious problem.

Note that all places where we use arenas in gecko we either use them for very transient objects (for which the total arena usage is bounded in various ways) or we Finish (not Free) the arena within a well-defined lifetime (e.g. on page unload).
This discussion needs to move to a new PSM bug.
Yes; I asked the author of this patch (who introduced the leak regression discussed above) to file just such a bug, no?
In reply to comment #29:
> In any case, we need a bug on fixing that leak.  Rob, please file one.
...and comment #34:
> Yes; I asked the author of this patch (who introduced the leak regression
> discussed above) to file just such a bug, no?

I've just filed bug #487394.  I hope I've understood the issue correctly.
Perhaps regardless of whatever conclusion bug #487394 will come to...

Is there anything that my/Kai's already committed patch in this bug should do differently regarding memory allocation / de-allocation?

The patch allocates and precomputes values for a number of CERTNames and SECItems when PSM initializes NSS, and it only attempts to de-allocate these when PSM de-initializes NSS.  I designed it this way to make the MyAlternateOCSPAIAInfoCallback() callback function run as quickly as possible.

We could reduce bloat by:
- moving all of the patch's allocation and computation into the MyAlternateOCSPAIAInfoCallback() callback function.
- de-allocating all of that memory (to the heap!) just before that callback function returns.

Which is best in this case: speed (for a callback function that will only be called when an SSL Certificate without AIA->OCSP is encountered) or lack-of-bloat?
> I've just filed bug #487394.  I hope I've understood the issue correctly.

Yes.  Thank you for filing!

> Is there anything that my/Kai's already committed patch in this bug should do
> differently regarding memory allocation / de-allocation?

That's up to kai, as module owner, but it sounds like you're allocating a bounded and not too big amount of memory.  If so, I wouldn't worry about it.  The problem that worries me is potential unbounded growth in the amount of memory allocated, combined with not deallocating the memory until shutdown.  You have the latter, but not the former, right?
Of course, the allocated memory gets reused MANY TIMES before shutdown.
In reply to comment #37:
> You have the latter, but not the former, right?

Right.  My intention was to do i) all memory allocation at PSM/NSS startup, and ii) all memory de-allocation at PSM/NSS shutdown.

My callback function, which is called whenever OCSP is enabled and revocation information is required for a certificate that does not contain AIA->OCSP, calls...
  - PORT_Strdup() (just as NSS's ocsp_GetResponderLocation() function does, which is what calls my callback function).
  - SECITEM_CompareItem() (which doesn't appear to allocate or de-allocate any memory).
  - CERT_CompareName() (which calls various functions that allocate and de-allocate memory, but I don't know the underlying NSS code well enough to be able to tell just how de-allocated this memory is once my callback function returns).
Flags: wanted1.9.0.x+
Flags: blocking1.9.0.13?
Daniel, I just saw that you've added the wanted1.9.0.x+/blocking1.9.0.13? flags to this bug.

This bug depends on Bug 483168, which was implemented in NSS 3.12.3.  However, I believe that 1.9.0.x is still stuck on NSS 3.12.2 (NSS_3_12_2_WITH_CKBI_1_75_RTM).

Are there any plans to upgrade to a newer version of NSS for 1.9.0.13?
Blocks: BH-2009
Flags: wanted1.9.1?
Flags: blocking1.9.0.13?
Flags: blocking1.9.0.13+
Flags: blocking1.9.0.13+
Comment on attachment 391268 [details] [diff] [review]
Patch for Mozilla 1.9.0 branch (includes fixes for bug 497184 and bug 497172)

r=dveditz

Approved for 1.9.0.13/14, a=dveditz for release-drivers

In the interest of expediency I'll check this in.
Attachment #391268 - Flags: review?(kaie)
Attachment #391268 - Flags: review+
Attachment #391268 - Flags: approval1.9.0.14?
Attachment #391268 - Flags: approval1.9.0.14+
Checking in ssl/src/nsNSSCallbacks.cpp;
/cvsroot/mozilla/security/manager/ssl/src/nsNSSCallbacks.cpp,v  <--  nsNSSCallbacks.cpp
new revision: 1.74; previous revision: 1.73
done
Checking in ssl/src/nsNSSCallbacks.h;
/cvsroot/mozilla/security/manager/ssl/src/nsNSSCallbacks.h,v  <--  nsNSSCallbacks.h
new revision: 1.18; previous revision: 1.17
done
Checking in ssl/src/nsNSSComponent.cpp;
/cvsroot/mozilla/security/manager/ssl/src/nsNSSComponent.cpp,v  <--  nsNSSComponent.cpp
new revision: 1.169; previous revision: 1.168
done
Keywords: fixed1.9.0.14
mozilla/security/manager/ssl/src/nsNSSCallbacks.cpp 	1.73.4.1
mozilla/security/manager/ssl/src/nsNSSCallbacks.h 	1.17.26.1
mozilla/security/manager/ssl/src/nsNSSComponent.cpp 	1.168.4.1
Keywords: fixed1.9.0.13
Rob, could you help us verify this bug on Fx3.0.13?
Wan-Teh, thanks for back-porting the patches.

Juan, it all looks good to me.  I see the EV UI in Fx3.0.13 for all of the following sites:

COMODO EV (no AIA->OCSP URL in the end-entity or Intermediate certs):
https://comodocertificationauthority-ev.comodoca.com/

COMODO EV (no AIA->OCSP URL in the Intermediate cert):
https://secure.comodo.com/products/frontpage

Network Solutions EV (no AIA->OCSP URL in the end-entity or Intermediate certs):
https://www.networksolutions.com/

I see that Trustwave have already confirmed that they're happy (bug #497184 comment #26 and #27).

I don't know a URL for any GlobalSign EV certificates that don't contain AIA->OCSP URLs.  The AMO site is for some reason currently serving an old DV certificate for *.mozilla.org instead of the GlobalSign EV certificate for addons.mozilla.org which it had been serving for many months.
Thanks for your help, Rob.

Marking as verified1.9.0.13 per comment #46. For 1.9.0.14 we'll use the links in it for re-verification.
Verified for 1.9.0.14 with the links.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: