240661 - [FIXr]"commonName" wrongly named and bogusly set

Status: RESOLVED FIXED

(Core :: Security, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

Going out on a limb and combining two bugs here, Chris.

First off, what's called "commonName" for certificate principals is not at all
what a "Common Name" is in a certificate.  This can lead to all sorts of
confusion; see bug 240628.

The "commonName" in a certificate principal is simply a way to identify the
principal's certificate to the user.  At the moment, it's set to the
Organization property of the certificate, but that's really an implementation
detail of PSM that principals and their consumers don't need to know about.

So I propose we rename commonName (in nsIPrincipal.idl and in the Certificate
struct member) to principalName or subjectName.  The idea to convey is that this
is a "pretty name" for whoever is identified by this principal.

Issue #2 is that being a pretty name it need not be in English.  And in fact,
the string currently being used starts out as UTF-8, gets converted to UTF-16,
but then get converted to ASCII (see nsNSSComponent::VerifySignature).  We
should just store it as UTF-8 in the Certificate struct.

So to sum up, I propose the following api change to nsIPrincipal.  Instead of:

154     attribute string commonName;

We should have:

        attribute AUTF8String subjectName;

(or whatever we decide to call it; I like "subjectName", but I'm open to better
suggestions).

Then we fix the one caller to not lose information here.

Chris, I'm willing to write a patch for this if we agree on the changes we want
to happen.

Comment 1

[Christopher Aillon (sabbatical, not receiving bugmail)]

I like 'prettyName'.  While you're at it, how about fixing certificateID as well
and removing the XXX comments I have in various places about it not really being
an id?  That should be called 'fingerprint'.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch for 1.7 branch

This part I think we should take on the branch...

Comment 3

[Boris Zbarsky [:bzbarsky]]

So caillon, we're making the following api change to nsIPrincipal:

      readonly attribute AUTF8String fingerprint;
      attribute AUTF8String prettyName;

And making changes to the nsScriptSecurityManager, etc, as needed?  Should the
signature of setCanEnableCapability stay as is or change (and if change, just
the arg name, or the type?).

Comment 4

[Boris Zbarsky [:bzbarsky]]

To be more precise, who exactly should be aware of the fact that we identify
certificates by fingerprint?

Frankly, I'm tempted to leave the certificate id part alone; the fact that it
happens to be the SHA1 fingerprint is an implementation detail of PSM that the
rest of the browser doesn't need to be aware of....

Comment 5

[Christopher Aillon (sabbatical, not receiving bugmail)]

(In reply to comment #4)
> To be more precise, who exactly should be aware of the fact that we identify
> certificates by fingerprint?

Well, I don't think anyone really cares about this anyone except caps
internally, apart from necko but only to clone... There really should be a
better API to do that since its not really cloning, but more like "subclassing".
 That is, the certificate that the JAR channel stores is a generic version of
the caps principal it then hands out which gets an additional URI and whatever
priveleges that URI comes with installed on it.  I don't think anyone else
really needs to know, as in it doesn't even need to be accessed.  But that can
be a different bug.

If you end up making the fingerprint change, sure please change both the name
and type.  I won't mind if you shift that to a different bug though I suppose.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Yeah, I will probably shift the fingerprint issue to a different bug; there's
code all over the tree that calls it an id, and someone who actually owns this
module should be the one to decide where one name ends and another begins, if
anywhere.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

I'm in favor of using names that are meaningful and correct to as many 
developers as possible.  "fingerprint" is understood by MANY people including
most SSH and PGP users, as well as developers.  Even I didn't know what the 
cert ID was until you told me it was a SHA1 hash.  

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Current state of things

I'm deeply unhappy with this patch.  I'm also deeply unhappy with most of the
code I was looking at while writing this patch, but anyway....

Before proceeding any further, there's a rather important question we should
resolve:  What should happen when a certificate principal is requested for
fingerprint 'X' and "pretty name" 'A', and we have one cached for fingerprint X
and "pretty name" 'B'?

Do we just return a cloned certificate principal with the pretty name changed
to 'B'?  Do we assert and error out?  Do we just error out?  Is there a
reasonable way we can have two certs with different pretty names but the same
fingerprint?

As long as I'm asking questions, is there a reason capabilities are restricted
to ASCII?  I left that as-is for now, but it seems odd.

Comment 9

[Boris Zbarsky [:bzbarsky]]

One other thing.  Nelson, I am guessing that if one fails to call
SEC_PKCS7DestroyContentInfo() on the return value of SEC_PKCS7DecoderFinish()
one gets a memory leak.  Is that correct?

For that matter, what happens if the call to SEC_PKCS7DecoderUpdate() returns
failure?  It looks like the caller should clean up the return value of
SEC_PKCS7DecoderStart() at that point, right?

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Borris, in reply to comment 9:
a) Yes, if you don't call SEC_PKCS7DestroyContentInfo, the content info will leak.
b) regardless of any errors that occur during decoder update, you must call
DecoderFinish with the decoder context returned by DecoderStart, or it will leak.

Comment 11

[Boris Zbarsky [:bzbarsky]]

OK, bug 241013 filed on that issue, then...

Comment 12

[Christopher Aillon (sabbatical, not receiving bugmail)]

(In reply to comment #8)
> Before proceeding any further, there's a rather important question we should
> resolve:  What should happen when a certificate principal is requested for
> fingerprint 'X' and "pretty name" 'A', and we have one cached for fingerprint
> X and "pretty name" 'B'?
> 
> Do we just return a cloned certificate principal with the pretty name changed
> to 'B'?

IIRC, we've done this in the past.  I'd like to see some feedback from the
crypto team though before passing final judgement.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Nelson, any chance you could comment on paragraphs 2 and 3 in comment 8?  Those
seem to be in your area of expertise.... especially the last question in
paragraph 3.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Just to make it clear what I propose to do if we can't have the same fingerprint
but different pretty names is that we should continue not storing the pretty
name in prefs, and the first time a request for that cert principal is made,
snag the pretty name from the request and set it on the principal....

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

I'd like some more background here.  Under what circumstances would you 
ask for a cert by both fingerprint and pretty name?  
Depending on the circumstances, accepting one cert in place of another 
could be a security vulnerability, a way of spoofing signatures for others.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Yeah, that's the part I'm worrying about....

Basically, any time anyone with a certificate needs a certificate principal,
they ask the security manager for one.  At the moment, to do so they just pass
in the certificate fingerprint (and then call SetCommonName on the principal
they got back to set the name the user will see).  My patch changes the security
manager interface such that callers must pass in both fingerprint and pretty
name to get back a principal.  This should prevent people from forgetting to set
the pretty name (which makes it impossible for users to make informed decisions).

So in the typical case, the caller passes in a fingerprint and pretty name, the
security manager constructs a principal object, and returns it.   The one
exception to this is if some time in the past a script signed by the certificate
with that fingerprint requested expanded permissions from the user and the user
1) granted the permissions and 2) told Mozilla to remember that decision.  When
that happens, the security manager saves the permission information in a
preference keyed off the fingerprint of the certificate.  The security manager
saves the fingerprint and the permissions but NOT the pretty name, at the moment.

So if this has happened, when GetCertificatePrincipal is called the security
manager sees that it already has a cached principal object for that fingerprint
(created by reading prefs at startup).  But this cached object has no pretty
name.  So here are the possible courses of action:

1)  Just set the pretty name that was passed in on the cached object and return
    the pointer to it.
2)  Clone the cached object, set the pretty name on the clone and return it.
3)  Start storing the pretty name along with the other information when expanded
    permission data is saved.  Then if a principal is requested for the same
    fingerprint and a different pretty name than what we have stored, deny the
    request.

#1 is essentially what we have now, where the callers set the pretty name
   directly on the returned object.
#2 is what I implemented in the patch I attached, just so I had something to
   compile and test.
#3 would mean that either we make a one-time change that breaks all currently
   stored prefs of this sort (since the empty pretty name from prefs won't match
   the requested pretty name) or that we allow a mismatch if the pretty name in
   prefs is empty (and then we still need to do #1 or #2 for those cases).

Note that these prefs are not stored often, so breaking them once may perhaps be
an acceptable solution -- people would be able to just reset them if desired.

Comment 17

[Brendan Eich [:brendan]]

Comment on attachment 146268 [details] [diff] [review]
Patch for 1.7 branch

Doesn't the 1.7 branch need a change to nsPrincipal::SetCommonName?  The branch
and trunk have not diverged, and I see no fix to strdup the passed-in const
char *:

http://lxr.mozilla.org/mozilla/source/caps/src/nsPrincipal.cpp#517

/be

Comment 18

[Boris Zbarsky [:bzbarsky]]

Brendan, mCert->commonName is an nsCString, not a char*.  Note that all I'm
changing in the patch in question is whether we convert to ASCII or UTF8; all
ownershipt, etc, is staying as is (and is correct).

Comment 19

[Brendan Eich [:brendan]]

Comment on attachment 146268 [details] [diff] [review]
Patch for 1.7 branch

Sorry, I should have looked at the declaration.  nsCString member, feh.  I'm
looking forward to your larger cleanup!

/be

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 146268 [details] [diff] [review]
Patch for 1.7 branch

Could this please be approved for 1.7?	This keeps us from losing non-ascii
chars in certificate "pretty names".

Comment 21

[Brendan Eich [:brendan]]

Comment on attachment 146268 [details] [diff] [review]
Patch for 1.7 branch

Sorry, meant to approve too.  Trivial, no need for driver recusal based on sr=
;-).

/be

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 146268 [details] [diff] [review]
Patch for 1.7 branch

Checked in on branch.  Now back to our regularly scheduled programming.  ;)

Comment 23

[Boris Zbarsky [:bzbarsky]]

Nelson?  See comment 16....  What are your thoughts?

Comment 24

[Boris Zbarsky [:bzbarsky]]

Nelson, it looks like AOL is up to its mail-blocking tricks again -- I've been
having mails from my MIT account bouncing from AOL addresses all day.  I don't
know whether you received my last message telling you that I have not in fact
gotten any mail from you about this bug...

Please put all information related to this bug in the bug itself; it seems this
is the only semi-reliable channel of communication we have.

All that said, I'd very much like to hear something, even just an idea of what
the issues are or how long you think it will take to address the questions in
comment 13 and later.  I'd very much like to get this done, one way or another,
during 1.8a (which means by two weeks from now in my case); the only question is
what the scope of the changes should be....

Comment 25

[Boris Zbarsky [:bzbarsky]]

OK, Nelson and I had a nice long conversation about this back in January.  I
kept meaning to actually act on the results, but clearly that's not happening,
so I should just summarize in the bug.

The basic problem is that the current code uses the Organization field from the
certificate for the "pretty name".  This field is optional.  Therefore, using it
will sometimes show nothing useful to the user.

Nelson said the following at some point:

------------------------------------------------------------
The cert's SubjectName and IssuerName are both instances of
DistinguishedName (DN).  A DN is a hierarchichally structured collection
of "Attribute Value Assertions" (AVAs, a.k.a. "Attribute Types and Values",
ATAVs), e.g.  CommonName=<something>, Organization=<something>,
OrganizationalUnit=<something>, Country=<somewhere>, STate=<somewhere>,
Locality=<somewhere>, ...

There is a large list of known Attribute Types at
http://lxr.mozilla.org/security/source/security/nss/lib/certdb/alg1485.c#55
There are more attribute types than we recognize, so not all attributes in
a DN will have known types.

I'm proposing that we show the user all the AVAs with known attribute types.
------------------------------------------------------------

He also said:

------------------------------------------------------------
Looking at
http://lxr.mozilla.org/security/source/security/manager/ssl/public/nsIX509Cert.idl#85
it appears to me that your choices are:

subjectName - which presumably contains all the AVAs in the subjectName DN,
including known and unknown types :-/

commonName,
organization,
organizationalUnit,  - which are 3 of the many known attribute types.

I guess I'd rather err on the side of too many attributes, than on not enough.
So, I'd suggest using subjectName 
------------------------------------------------------------

After some more discussion, Nelso suggested using UI similar to the certificate
manager's for presenting the information.  Neil thought this was a good idea
too, when I asked him about it.

The cert manager UI needs an actual nsIX509Cert object, so I think we should
just expose one on our principals instead of the current "commonName".

Further, Nelson suggested that the prefs be keyed off not only the cert
fingerprint (as now) but also off the full subjectName string.  Certificates
that have a different subjectName shouldn't get each others prefs in the
(unlikely) event of a hash collision in fingerprint.  This is option 3 from
comment 16, and I guess we could avoid breaking existing prefs by doing option
#1 when the prefs subjectName is empty, and then saving out the new subjectName
to guard against future issues...

Comment 26

[Boris Zbarsky [:bzbarsky]]

Attachment: Compiled, not so much tested

Requesting reviews for the API changes only, for now.

I tried not changing nsIPrincipal, but private module methods
(getPreferences(), in particular) are exposed on this interface....  Since I
had to change it anyway, I figured I might as well make things as simple as
possible.  I also had to change nsIScriptSecurityManager, because I don't think
consumers should ever be able to change the subject or cert of a principal.  So
those need to be passed to getCertificatePrincipal.

Comment 27

[Boris Zbarsky [:bzbarsky]]

Darin, Doug, could you also take a look at the proposed API and let me know
whether it works for you?

Comment 28

[Boris Zbarsky [:bzbarsky]]

Another option is to just de-facto freeze these interfaces, create nsIPrincipal2
and nsIScriptSecurityManager2, and start using those... or something like that.

Comment 29

[Doug Turner (:dougt)]

Comment on attachment 184657 [details] [diff] [review]
Compiled, not so much tested

what is the |fingerprint| going to be used for?

Do you want to drop |hasCertificate| now that |certificate| is exposed?

I also wonder, why not just expose the x509 directly and remove the prettyName
and subjectName -- Maybe.

Comment 30

[Doug Turner (:dougt)]

Comment on attachment 184657 [details] [diff] [review]
Compiled, not so much tested

what is the |fingerprint| going to be used for?

Do you want to drop |hasCertificate| now that |certificate| is exposed?

I also wonder, why not just expose the x509 directly and remove the prettyName
and subjectName -- Maybe.

Comment 31

[Boris Zbarsky [:bzbarsky]]

> what is the |fingerprint| going to be used for?

It's currently used as a unique identifier for the certificate.  I can add
documentation about that to the api.

> Do you want to drop |hasCertificate| now that |certificate| is exposed?

hasCertificate can be true while the nsISupports is false...  (For example, in
the enablePrivilege code I'm patching, or in principals read from prefs because
I didn't go into the whole "serialize the certificate into prefs" thing).  Doing
this as things stand would break the nsPrincipal::Equals method, which I don't
think we want.

I did just realize that I'm effectively making
nsJVMManager::IsAllPermissionGranted always return false because
GetCertificatePrincipal requires a cert.  We should probably just remove this
method, like the comment says.

> I also wonder, why not just expose the x509 directly and remove the prettyName
> and subjectName -- Maybe.

You mean why not use |attribute nsIX509Cert whatever;| in nsIPrincipal?  Because
nsIX509Cert.idl is in a module that caps doesn't depend on (and shouldn't; we
could have other types of certs).

Comment 32

[Darin Fisher]

I think we should try to clean up these caps APIs (instead of hacking around
defactor ugly APIs).  We should try to alert that plugin vendor (JEP) as soon as
possible about the impending change for Firefox 1.1.

Comment 33

[Boris Zbarsky [:bzbarsky]]

> We should try to alert that plugin vendor

I already did; see the dep list of this bug.

Comment 34

[Boris Zbarsky [:bzbarsky]]

So.. I felt strongly about this patch because I wanted to change as few security
APIs as possible for 1.8 and we needed to change nsIScriptSecurityManager anyway
to get this right eventually.  Since nsISignatureVerifier just got changed,
apparently, I have no strong feelings about what happens here for 1.8 I guess. 
Though I do still think that if we know we'll need an api change it's better to
do it sooner than later.

Comment 35

[Steven Michaud [:smichaud] (Retired)]

> Since nsISignatureVerifier just got changed, apparently,

You're referring to
https://bugzilla.mozilla.org/show_bug.cgi?id=292368#c32?

Comment 36

[Boris Zbarsky [:bzbarsky]]

Yeah; thanks for tracking it down!  (The checkin comment didn't have the bug#.)

Comment 37

[Boris Zbarsky [:bzbarsky]]

Attachment: Now with testing and all

I did a fair bit of testing with
jar:http://www.mozilla.org/projects/security/components/signed-script-demo.jar!/signed-script-demo.html
on this one.  It's basically the same as the previous patch with an off-by-one
arithmetic error fixed and a slightly better behavior for the case when two
certs with different subject names have the same fingerprint.

Comment 38

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 188232 [details] [diff] [review]
Now with testing and all

>Index: caps/idl/nsIPrincipal.idl
...
>     /**
>      * The domain security policy of the principal.
>      */
>     // XXXcaa should this be here?  The script security manager is the only
>     // thing that should care about this.  Wouldn't storing this data in one
>     // of the hashtables in nsScriptSecurityManager be better?
>+    // XXXbz why is this writable?  Who should have write access to
>+    // this?  What happens if this principal is in our hashtable and
>+    // we pass it out of the security manager and someone writes to
>+    // this field?  Especially if they write garbage?  If we need to
>+    // give someone other than the security manager a way to set this
>+    // (which I question, since it can increase the permissions of a
>+    // page; what the ActiveX code does in resetting this to NULL is
>+    // really suspicious), it should be a |void
>+    // clearSecurityPolicy()| method.
>     attribute voidPtr securityPolicy;

That's more or less what I was musing about when I wrote the XXXcaa portion.  I
think we should get rid of this sooner rather than later.  File a bug on me?



>Index: caps/include/nsScriptSecurityManager.h
...
>+    // This is just like the API method, but it doesn't check that the subject
>+    // name is nonempty or aCertificate is non-null, and it doesn't change the

non-empty

>+    // certificate in the table (if any) in any way if aModifyTable is false.
>+    nsresult
>+    DoGetCertificatePrincipal(const nsACString& aCertFingerprint,
>+                              const nsACString& aSubjectName,
>+                              const nsACString& aPrettyName,
>+                              nsISupports* aCertificate,
>+                              nsIURI* aURI,
>+                              PRBool aModifyTable,
>+                              nsIPrincipal **result);
>+


>Index: caps/src/nsPrincipal.cpp
...
>@@ -230,23 +234,33 @@ nsPrincipal::Equals(nsIPrincipal *aOther
>   }
> 
>   if (this != aOther) {
>     if (mCert) {
>       PRBool otherHasCert;
>       aOther->GetHasCertificate(&otherHasCert);
>       if (!otherHasCert) {
>         return NS_OK;
>       }
> 
>-      nsXPIDLCString otherCertID;
>-      aOther->GetCertificateID(getter_Copies(otherCertID));
>-      *aResult = otherCertID.Equals(mCert->certificateID);
>+      nsCAutoString str;
>+      aOther->GetFingerprint(str);
>+      *aResult = str.Equals(mCert->fingerprint);
>+
>+      // If either subject name is empty, just let the result stand (so that
>+      // nsScriptSecurityManager::SetCanEnableCapability works), but if they're
>+      // both nonempty, only claim equality if they're equal.

non-empty




>Index: caps/src/nsScriptSecurityManager.cpp
>===================================================================
>RCS file: /home/bzbarsky/mozilla/cvs-mirror/mozilla/caps/src/nsScriptSecurityManager.cpp,v
>retrieving revision 1.263
>diff -u -p -d -1 -0 -r1.263 nsScriptSecurityManager.cpp
>--- caps/src/nsScriptSecurityManager.cpp	29 Jun 2005 16:29:49 -0000	1.263
>+++ caps/src/nsScriptSecurityManager.cpp	4 Jul 2005 20:50:27 -0000
>@@ -1678,86 +1678,135 @@ nsScriptSecurityManager::SubjectPrincipa
>         // No subject principal means no JS is running;
>         // this is the equivalent of system principal code
>         *aIsSystem = PR_TRUE;
>         return NS_OK;
>     }
> 
>     return mSystemPrincipal->Equals(subject, aIsSystem);
> }
> 
> NS_IMETHODIMP
>-nsScriptSecurityManager::GetCertificatePrincipal(const char* aCertID,
>+nsScriptSecurityManager::GetCertificatePrincipal(const nsACString& aCertFingerprint,
>+                                                 const nsACString& aSubjectName,
>+                                                 const nsACString& aPrettyName,
>+                                                 nsISupports* aCertificate,
>                                                  nsIURI* aURI,
>                                                  nsIPrincipal **result)
> {
>+    *result = nsnull;
>+    
>+    NS_ENSURE_ARG(!aCertFingerprint.IsEmpty() &&
>+                  !aSubjectName.IsEmpty() &&
>+                  aCertificate);
>+
>+    return DoGetCertificatePrincipal(aCertFingerprint, aSubjectName,
>+                                     aPrettyName, aCertificate, aURI, PR_TRUE,
>+                                     result);
>+}
>+    
>+nsresult
>+nsScriptSecurityManager::DoGetCertificatePrincipal(const nsACString& aCertFingerprint,
>+                                                   const nsACString& aSubjectName,
>+                                                   const nsACString& aPrettyName,
>+                                                   nsISupports* aCertificate,
>+                                                   nsIURI* aURI,
>+                                                   PRBool aModifyTable,
>+                                                   nsIPrincipal **result)
>+{
>+    NS_ENSURE_ARG(!aCertFingerprint.IsEmpty());
>+    
>     // Create a certificate principal out of the certificate ID
>     // and URI given to us.  We will use this principal to test
>     // equality when doing our hashtable lookups below.
>     nsRefPtr<nsPrincipal> certificate = new nsPrincipal();
>     if (!certificate)
>         return NS_ERROR_OUT_OF_MEMORY;
> 
>-    nsresult rv = certificate->Init(aCertID, aURI);
>+    nsresult rv = certificate->Init(aCertFingerprint, aSubjectName,
>+                                    aPrettyName, aCertificate, aURI);
>     NS_ENSURE_SUCCESS(rv, rv);
> 
>     // Check to see if we already have this principal.
>     nsCOMPtr<nsIPrincipal> fromTable;
>     mPrincipals.Get(certificate, getter_AddRefs(fromTable));
>     if (fromTable) {
>         // Bingo.  We found the certificate in the table, which means
>-        // that it has escalated priveleges.
>+        // that it has escalated privileges.
>+
>+        if (aModifyTable) {
>+            // Make sure this principal has names, so if we ever go to save it
>+            // we'll save them.  If we get a name mismatch here we'll throw,
>+            // but that's desirable.
>+            rv = NS_STATIC_CAST(nsPrincipal*,
>+                                NS_STATIC_CAST(nsIPrincipal*, fromTable))
>+                ->EnsureCertData(aSubjectName, aPrettyName, aCertificate);
>+            if (NS_FAILED(rv)) {
>+                // We have a subject name mismatch for the same cert id.
>+                // Hand back the |certificate| object we created and don't give

certificate principal?





>Index: modules/oji/src/nsJVMManager.cpp
>===================================================================
>RCS file: /home/bzbarsky/mozilla/cvs-mirror/mozilla/modules/oji/src/nsJVMManager.cpp,v
>retrieving revision 1.73
>diff -u -p -d -1 -0 -r1.73 nsJVMManager.cpp
>--- modules/oji/src/nsJVMManager.cpp	25 Feb 2005 20:46:24 -0000	1.73
>+++ modules/oji/src/nsJVMManager.cpp	27 May 2005 06:17:59 -0000
>@@ -896,50 +896,60 @@ nsJVMManager::IsLiveConnectEnabled(void)
> 	return PR_TRUE;
> }
> 
> /*
>  * Find out if a given signer has been granted all permissions. This
>  * is a precondition to loading a signed applet in trusted mode.
>  * The certificate from which the fingerprint and commonname have
>  * be derived, should have been verified before this method is 
>  * called.
>  */
>-
>+// XXXbz this function has been deprecated for 4.5 years.  We have no
>+// callers in the tree.  Is it time to remove it?  It's returning
>+// PRBools for a NS_METHOD, and NS_METHOD for an interface method, for
>+// goodness' sake!
> NS_METHOD
> nsJVMManager::IsAllPermissionGranted(
>     const char * lastFP,
>     const char * lastCN, 
>     const char * rootFP,
>     const char * rootCN, 
>     PRBool * isGranted)
> {
>+    if (!lastFP || !lastCN) {
>+        return PR_FALSE;
>+    }
>+    

Seems like the prevailing style here is |if (!foo) return| on a single line, no
braces.

But, I agree that we should probably just consider yanking this out of the
tree.  4.5 years is enough time in limbo.  Sooner rather than later is good.


Looks pretty good to me, so r=caillon and the fact that you've tested is a
bonus.

Comment 39

[Boris Zbarsky [:bzbarsky]]

>>+                // We have a subject name mismatch for the same cert id.
>>+                // Hand back the |certificate| object we created and don't give
>
> certificate principal?

Yes, but specifically the one named "certificate" which we created about 10
lines up in this method...  How would you prefer I phrase this?

Comment 40

[Daniel Veditz [:dveditz]]

Comment on attachment 188232 [details] [diff] [review]
Now with testing and all

>+    // page; what the ActiveX code does in resetting this to NULL is
>+    // really suspicious), 

The ActiveX code is calling CControlSite::SetSecurityPolicy(), something else
entirely.

sr=dveditz

Comment 41

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 188232 [details] [diff] [review]
Now with testing and all

Requesting 1.8b4 approval for this fix.  This makes us do better validation of
certificates when auto-enabling capabilities (ensuring the subject name is the
same, not just the fingerprint), and exposes more information about the
certificate on the principal, which should allow the UI to present the user
with more informative dialogs.	Also, being able to get at the cert off the
principal is needed for software update, apparently.

Comment 42

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to tip

Comment 43

[Boris Zbarsky [:bzbarsky]]

Fixed.

Comment 44

[Benjamin Smedberg]

This caused https://bugzilla.mozilla.org/show_bug.cgi?id=301875#c3