282945 - Use better strings to identify a CRL in the prefs.js file

Status: RESOLVED INVALID

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

Description

[dave]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.5) Gecko/20050211
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.5) Gecko/20050211

Mozilla will crash when attempting to exit from the suite if auto-update is
enabled in the CRL manager and one or more of the CRLs have an empty value 
under "Organizational Unit" (in my experience, this includes about half of all
CRLs).  Please note that although this can be bug can be reliably reproduced if
the above conditions are met, I have not verified that all of the above
conditions are required for the "crash-on-exit" to occur.  I.e., it may happen
even after disabling auto-update.  It is unlikely though, because the browser
must remain open for some length of time prior to exiting in order for the crash
to happen.

There is a problem with the way in which Mozilla handles the CRL specific
preferences.  Mozilla appends the CA's OU (Organization Unit) name to the
preference key string in its internal hash table, but many of the CRLs have
empty OU fields, which results in the problem of multiple CRLs attempting to
share the same key.  E.g., this is how the key-value pair is supposed to look in
the prefs.js file: user_pref("security.crl.autoupdate.enable.Certification
Services  Division", true); If there is no OU name, the above preference looks
like this: user_pref("security.crl.autoupdate.enable.", true);

I have generated numerous crash dump analyses and they all point to the same
place.  Someone whom is intimately familiar with the Mozilla source tree will
certainly be able to zoom in on the problem.


Reproducible: Always

Steps to Reproduce:
1. Download the followinng CRLs and enable auto-updating:
http://crl.verisign.com/ThawtePersonalFreemailIssuingCA.crl
http://crl.verisign.com/ThawteSSLDomainCA.crl
2. Wait a while (may something to do with the auto-update timer)
3. Exit from the Mozilla suite.




FOLLOWUP_IP: 
xpcom!clearHashEntry+12 [j:\mozilla\xpcom\ds\nshashtable.cpp @ 81]
10006072 83660400         and     dword ptr [esi+0x4],0x0

SYMBOL_STACK_INDEX:  1
SYMBOL_NAME:  xpcom!clearHashEntry+12
MODULE_NAME:  xpcom
IMAGE_NAME:  xpcom.dll
DEBUG_FLR_IMAGE_TIMESTAMP:  420ce883
STACK_COMMAND:  .ecxr ; kb
FAILURE_BUCKET_ID:  ACCESS_VIOLATION_xpcom!clearHashEntry+12
BUCKET_ID:  ACCESS_VIOLATION_xpcom!clearHashEntry+12

Comment 1

[dave]

Attachment: Exception Analyses from Windows Debugger (WinDBG)

Comment 2

[dave]

Note:  This bug report details the origin of the problem reported in bug 200119,
but I thought it warranted filing a separate bug because that thread has become
rather diluted.

Comment 3

[Frank Wein [:mcsmurf]]

(In reply to comment #0)
> 2. Wait a while (may something to do with the auto-update timer)
> 3. Exit from the Mozilla suite.

How long was "a while" for you roughly? I tried reproducing this bug here, but
couldn't crash.

Comment 4

[dave]

(In reply to comment #3)
> (In reply to comment #0)
> > 2. Wait a while (may something to do with the auto-update timer)
> > 3. Exit from the Mozilla suite.
> 
> How long was "a while" for you roughly? I tried reproducing this bug here, but
> couldn't crash.

Unfortunately, I don't know.  I just know from testing that the crash did not
occur if I immediately exited from the browser aftering startup.  However, it
wasn't *days*.  A few hours should be enough time.  I'm sorry that I can't be
more specific.

Installing either one of those CRLs in the original bug report also reveals a
concrete bug in the way that the preference is handled.  Specifically, one
*cannot* enable auto updating of both CRLs for the reason that I described in my
original report.  See the paragraph in the initial bug report that begins:
"[t]here is a problem with the way in which Mozilla handles the CRL specific
preferences. . . ."

The preference string bug is easily exposed by typing "about:config" in the
location bar and then filtering on string "crl".  I will attach a screenshot
from "about:config".

Comment 5

[dave]

Attachment: Screen shot from "about:config" exposing the preference string bug.

Comment 6

[dave]

Attachment: Duplicate screen shot attachment

Comment 7

[dave]

Comment on attachment 180231 [details]
Screen shot from "about:config" exposing the preference string bug.

Please delete the duplicate attachment.  My connection was hung for about 5
minutes after the first submission.  I should have verified that it had not
been transmitted (sorry about that).

Comment 8

[dave]

Is there something else I should be doing to expedite the handling of this bug?
 I'm concerned that this bug is being dismissed either because others have been
unable to reproduce the crash, or not enough people have attempted to reproduce
it.  However, the crash I reported was only a consequence of the real problem,
which is the handling of the preferences by the certificate manager component.

A Bugzilla search revealed that there are quite a number of reports related to
this issue:
https://bugzilla.mozilla.org/buglist.cgi?query_format=specific&order=relevance+desc&bug_status=__open__&product=&content=CRL+certficate+manager
Here is one just for example: https://bugzilla.mozilla.org/show_bug.cgi?id=150589

It should be very easy to fix.
1.) Choose a field other than the Organizational Unit (OU) for the formation of
the preference name (i.e., a field which must be present in every CRL or
certificate).
2.) Validate that the preference string is not an empty value.

CRLs without an OU present are not an exception to the rule.  IIRC, probably at
least half of the CRLs that I've downloaded in the past have not had an OU value
present (unless Mozilla is failing to parse the OU, which I have not checked but
I doubt is the case), and they were from Verisign or Thawte.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Dave, this is a PSM bug.  It's not being dismissed any more than any other 
PSM bugs are.  There simply are no developers actively working on PSM. 
Recruiting PSM developers is about all you can do to expedite it.

Comment 10

[dave]

(In reply to comment #9)
> Dave, this is a PSM bug.  It's not being dismissed any more than any other 
> PSM bugs are.  There simply are no developers actively working on PSM. 
> Recruiting PSM developers is about all you can do to expedite it.

OK.  Noted.  I'm personally unaffected by the bug as I don't even use CRLs
anymore, and I suspect that it's a non-issue for most users as well.  I was just
startled by the number of related bugs that I saw filed in Bugzilla (spanning
several years), and wanted to at least make sure that people were aware of the
relative simplicity of it.  Perhaps a cautionary note somewhere warning users of
the potential danger of attempting to manage CRLs without an associated OU would
be appropriate until the issue is resolved.

Comment 11

[Josh Birnbaum]

I see this crash too.  Only I don't agree that the problem discussed here is 
the cause.  I have one CRL defined, and it does have a OU name (and the prefs 
reflect that name).  It seems more like a double-free.

On shutdown this runs:
nsNSSComponent::StopCRLUpdateTimer()
{
  if(mUpdateTimerInitialized == PR_TRUE){
    if(crlsScheduledForDownload != nsnull){
      crlsScheduledForDownload->Enumerate(crlHashTable_clearEntry);
      crlsScheduledForDownload->Reset();
      delete crlsScheduledForDownload;
      crlsScheduledForDownload = nsnull;
    }
(...)

the call to nsHashtable::Enumerate() |delete|'s each nsStringKey (the hash 
entry's key) within nsNSSComponent::crlHashTable_clearEntry, but doesn't do 
anything to mark it as deleted.  The call to nsHashtable::Reset() then does a 
pldhash::PL_DHashTableEnumerate(&mHashtable, hashEnumerateRemove, NULL), which 
ultimately calls nsHashtable::clearHashEntry() on each entry, again 
|delete|'ing the entry's key (nsHashKey).

That all said, this is the first time I really have looked at Mozilla code, so 
I may have missed something.  Can someone more familiar take a look?  What 
approach would be desired to fix this?  It seems easiest just to remove the 
Enumerate() call, and rely on Reset() to clean up memory.

Comment 12

[Henrik Gemal]

dupe of bug 200119 ?

Comment 13

[Peter Cutts]

Sounds to me like the same issue as Bug 200119

Comment 14

[Kai Engert (:KaiE:)]

Let's make this bug dependent on 200119, so we can retest, once 200119 gets fixed.

Comment 15

[Kai Engert (:KaiE:)]

Josh, I agree with your statement about the double free, see bug 200119.

A potential fix for the crash bug was checked in to the trunk last week.
Is anyone on the CC list of this bug able to try out a nightly trunk build and test if you still crash?

Comment 16

[Kai Engert (:KaiE:)]

I would like to change the intention of this bug to "use better strings to identify a CRL in the prefs.js file".

I confirm we use bad strings.

Comment 17

[Kai Engert (:KaiE:)]

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

Comment 18

[Nelson Bolyard (seldom reads bugmail)]

Use of OU as a unique pref key has another problem:
It has recently been reported that many CAs do not verify the names
that their subjects request for OU names AT ALL.  Consequently, OU 
names need to be treated as unverified, and no security decisions 
by the user or by the software should depend on their values.

Comment 19

[karlsen]

So to get the key for the ff userpref.js unique there are a few options:

SHA-1 Hash of combinations/permutations of the

o the CLRs download URL provided by the user
o CRL issuer DN contained in CRL
o subject key identifier of the CRL signer certificate
o modulus of the key of the CRL signer certificate

The "Manage CRL" gui window should display the full CRL issuer DN and the download URL as well as the other stuff that is already there.

Comment 20

[Kai Engert (:KaiE:)]

Updating subject as proposed in comment 16

Comment 21

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

The "Revocation Lists" feature was removed in bug 867465.