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.

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'.

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

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?).

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....

(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.

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.

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.

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.

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?

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.

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

(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.

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.

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....

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.

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 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

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 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 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 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 on attachment 146268 [details] [diff] [review] Patch for 1.7 branch Checked in on branch. Now back to our regularly scheduled programming. ;)

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

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....

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...

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.

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

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

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 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.

> 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).

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.

> We should try to alert that plugin vendor I already did; see the dep list of this bug.

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.

> Since nsISignatureVerifier just got changed, apparently, You're referring to https://bugzilla.mozilla.org/show_bug.cgi?id=292368#c32?

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

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 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.

>>+ // 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 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 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.

Fixed.

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