User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.6) Gecko/20040206 Firefox/0.8 Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7) Gecko/20040608 Importing a self-made certificate (call it x) with the same DN (but different serial nr) as a built-in CA root cert (called b) overrides the built-in one: trying to open a SSL page protected by a cert signed by b throws an error -8182 ('certificate presented by xyz.com is invalid or corrupt') -> Denial of Service. This could be automated when importing x via mime type application/x-x509-email-cert, causing Mozilla to import the cert silently (bug Nr. 2). This is also possible via email messages, calling the cert x link inside an <iframe> tag, leading to a silent import of x when opening or previewing the message (bug Nr. 3). Conclusion: fully automatical DoS of the entire cert store via email is possible, no user interaction needed. Reproducible: Always Steps to Reproduce: 1. craft a self-signed cert (openssl) with the same DN as a built-in CA root cert. 2. import it into the cert store, either manually or by providing it as pem encoded using the mime content type application/x-x509-email-cert for _silent import_. 3. Your certificate store is "corrupted" from this time on: open a web site protected by an SSL certificate signed by the root CA cert you've been forging (e.g. https://www.thawte.com/) and you'll get an error -8182. 4. The same could be reached via email when including an <ifram> pointing to the certs' location, leading to fully automatical silent import of the cert. Send me an email to get a link to a page showing it (passwort needed, for legal reasons): marboesc@student.ethz.ch Actual Results: Mozilla imports the "forged" root cert into the "authorities" tab of the cert manager as an untrusted root. You can identify it by the column "security device": its stored in the "software security device" instead of the "Builtin Object Token". However your certificate store is "corrupted" from this time on: open a web site protected by an SSL certificate signed by the root CA cert you've been forging (e.g. https://www.thawte.com/) and you'll get an error -8182. Expected Results: Mozilla silently (without any warning/message!) imports the root cert into the "authorities" tab of the cert manager as an untrusted root when serving it as type application/x-x509-email-cert. According to the principles Visibility and Clarity for 'safe and secure CA-related UI-Dialogs' proposed in chapter 4.2. of my diploma thesis, instead of no user-feedback, an adequate treatment of this situation would be to show the import dialogue. During my diploma thesis on Rogue CA's possibilities, one part of the work was to evaluate today's browsing software. Contact me: This bug was found as part of my diploma thesis which is still going on. If you have any suggestions or ideas, contact me at marboesc@student.ethz.ch (PGP Key: 0x0AA132A7141D27C8)

Vulnerability not only in Mozilla for Windows but also affecting Linux, tested positively for Mozilla 1.6 and 1.7 on debian 3.0 stable.

since there is little echo here, marcel also posted that to cert.org and redhat: https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=127186 is there a phone number or skype nickname to discuss the issue with anybody? marcel is marboesc and I am "ralfhauser" on skype

The bug is also tracked at http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=257559

Confirming per comments. Removing security sensitive flag since this was also posted to n.p.m.crypto

It seems that no-one works on PSM any more. Here is an NSS-centric view of this. Error -8182 is "Peer's certificate has an invalid signature." PSM displays the wrong error code for it. There is a difference between "importing" a cert and merely encountering it in an email or on a web site. NSS does not store CA certs in any "permanent" cert DB (e.g. on disk) merely because they were encountered in a cert chain. NSS stores certs when the application (e.g. mozilla) tells it do so. That is "importing" a cert. In the past, the mozilla applications did not "import" a CA cert automatically when such a cert was merely encountered. It's not clear to me from the above description whether the reporter is asserting that mozilla now DOES import a CA cert upon encountering it, or if he is merely saying that once the bad CA cert is imported (e.g. by manual user action) that it has a detrimental affect (which is expected). It is also possible that a certificate reference leak has crept into PSM somewhere, resulting in certs continuing to hang around in memory long after they should have disappeared. That would make it appear to be imported, until the client was stopped and restarted.

nelson, from what I saw, the wrong root certificate gets into the cert-store without user interaction and it stays there even after client restart. So, if you craft about 64 fake root certs (=~ the number of built-in root certs as currently shipped with mozilla)and put 64 iframe tags into a html mail - a user just previewing this mail (not even opening it) will essentially no longer be able to use https with mozilla or netscape. The sample mail marcel has crafted for proof of concept right now retrieves the rogue cert from a server. I am sure, however, an ingenuous spammer will find a way to carry the certificates within the mail like one can do with images (<img src="cid:image0"> tag and corresponding image mime part) and thus not leave traces that can be easily prosecuted.

This is a PSM bug. Enhancements to NSS *may* be needed to fix it. PSM recognizes 3 (or more) different MIME content types for importing certs. They (or most of them) are documented at http://wp.netscape.com/eng/security/comm4-cert-download.html#communicator (This is the old Communicator 4 spec, and mozilla still honors that spec.) The idea is that the MIME type tells the client what type of cert is to be imported, and the client selects the UI accordingly. What is missing, apparently, is any enforcement that the type of cert being imported is in fact the type indicated by the MIME content type. There are at least 2 possible avenues of approach to fix this: a) enforce what the MIME type says, or b) forget the MIME type, and choose UI based on whatever type of cert you find is actually being presented. I think IE does this. NSS doesn't know about these MIME types, but PSM does. So, PSM needs to take the lead in fixing this. If PSM cannot determine what it needs to know using the existing NSS APIs, then enhancements to NSS's APIs can be made. However, the immediate problem is this: NO ONE works on PSM any more, apparently, witness how long this bug sat before being looked at. Until we get some staffing on PSM, I don't see how this will be fixed.

Chad form CERT Coordination Center <cert@cert.org> VU#160360

> There are at least 2 possible avenues of approach to fix this: > a) enforce what the MIME type says, or > b) forget the MIME type, and choose UI based on whatever type of cert > you find is actually being presented. I think IE does this. I'd propose c) show a warning when encountering application/x-x509-email-cert mime type, just as the other mime types do. Doing this at least stops the possibility to *silently* change something in the local cert store, which isn't advisable in any case in my opinion.

c is useless. it doesn't solve the problem at all, people would still have an even chance at choosing the wrong button and becoming victims.

c at least solves the possibility of a denial-of-service through scripted importing hundred or thousands of certs.

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

*** When to warn the users? *** Since there is basically no fix of this bug found at the usual places, neither technical nor at least a resource explaining the cause of bug -8182, I'm scared of the bad guys start exploiting it. According to my disposition to aid the users I'd like to divulge this bug and its remedy to a greater audience (like ZDNET and heise.de). /marcel

It's obvious, Mozilla doesn't ask anything if the Content-Type is application/x-x509-email-cert, the cert is imported silently. If over HTTP, it's done automatically. If it's in an email attachment, one has to open the attachment (but the iframe version as outlined by Marcel is possible too). That alone is IMHO a bug (bug 184659). I think Nelson is right in b. Currently the place where it's imported (self signed certs go into "Authorities" even if type is email-cert) is based on the content and so the UI should base on this too. Nelson, re comment #5 > Error -8182 is "Peer's certificate has an invalid signature." > PSM displays the wrong error code for it." True, a break is missing in nsNSSIOLayer.cpp at line 680. But I think the message could be more informative. Currently I can't see a way for a user to find out what exactly is wrong. > that it has a detrimental affect (which is expected). Is it possible that the preference for additional certs over the builtin with same DN is by design (don't know if PSM or NSS does this)? I mean, for the case one wants to insert a new cert from a Root CA if the old (build in) experies or so.

In response to the questions in comment 14: Re the issue about informative error messages: NSS has many unique error codes that are quite specific about error problems. Unfortunately, PSM tends to lump many of them together into a single error message "is invalid or corrupt". The more specific error cause is programmatically available, but PSM discards it and substitutes a generic error message. There is a LONG STANDING bug about this (more than one, actually), and a contribution in the area of error message handling would be most welcome for PSM. Re the cert selection algorithm: NSS's cert selection algorithm, that leads to the newest cert among certs with the same names, key IDs, etc., is as designed. This bug report shows no flaw in that algorithm. Only known and trusted certs should be imported into the DBs.

this seems to fix the problem in 1.7 on linux. don't know what it breaks. with it i am still able to browse secure sites and import certificates from "privacy and security". have not tested SMIME.

usage: in a terminal: nc -l -p 1234 <testcert then from mozilla open: http://localhost:1234/ then examine your certicates. if "internet widgets......" certificate is present, there is bug.

Your patch is a workaround for the moment, though no solution. It just disables the MIME types. Disabling ca-cert is necessary though the user gets asked whether a cert should get importet. Unfortunately when submitting multiple certificates in a PKCS#7 structure (DER format), the user only gets asked for the first cert and the following get importet silently when allowing the first one. Disabling email-cert is necessary of course. But server-cert, user-cert and the crl types? I wasn't able to import an illegal cert using these types.

>I wasn't able to import an illegal cert using these types. "illegal cert" ?? this seems the expected behavior.

at least "application/x-pkcs7-crl" and "application/x-x509-crl" seem to try to silently install themselves but give an error probably because i am not using the correct cert. "application/x-x509-ca-cert" ask the user for permission.

> "illegal cert" ?? this seems the expected behavior. Sorry, let's say "unwanted". And I meant import without disabling server-cert, user-cert or any crl. > at least "application/x-pkcs7-crl" and "application/x-x509-crl" seem to try > to silently install themselves but give an error probably because i am not > using the correct cert. I guess a crl is expected, no cert. > "application/x-x509-ca-cert" ask the user for permission. Yes, but only for the first cert. As I wrote one can send a dozend certs at once.

Problem seems to stem from: http://lxr.mozilla.org/seamonkey/source/security/manager/ssl/src/nsNSSComponent.cpp#2037 http://lxr.mozilla.org/seamonkey/source/security/manager/ssl/src/nsNSSCertificateDB.cpp#427 => http://lxr.mozilla.org/seamonkey/source/security/manager/ssl/src/nsNSSCertificateDB.cpp#254 327 nsCOMPtr<nsICertificateDialogs> dialogs; 328 nsresult rv = ::getNSSDialogs(getter_AddRefs(dialogs), 329 NS_GET_IID(nsICertificateDialogs), 330 NS_CERTIFICATEDIALOGS_CONTRACTID); (presumably this is the case that works correctly) http://lxr.mozilla.org/seamonkey/source/security/manager/ssl/src/nsNSSCertificateDB.cpp#604 649 /* user wants to import the cert */ http://lxr.mozilla.org/seamonkey/source/security/manager/ssl/src/nsNSSCertificateDB.cpp#482 520 srv = CERT_ImportCerts(CERT_GetDefaultCertDB(), certUsageEmailSigner, 521 numcerts, rawCerts, NULL, PR_TRUE, PR_FALSE, 522 NULL); http://lxr.mozilla.org/seamonkey/source/security/manager/ssl/src/nsCRLManager.cpp#83 136 crl = SEC_NewCrl(CERT_GetDefaultCertDB(), NS_CONST_CAST(char*, url.get()), &derCrl, 137 aType);

> (presumably this is the case that works correctly) That works right if one cert is presented, yes. > nsNSSCertificateDB.cpp#604 As I wrote, I can't see a problem for user-cert. The cert doesn't get imported if the public key isn't associated to a private key in the local db. > 520 srv = CERT_ImportCerts(CERT_GetDefaultCertDB(), certUsageEmailSigner, That's true. The whole list is thrown into this function without any question.

i am not sure more than web server certificates need to be managed from the web. not all users understand how certificates work. and users always can import the cert from "privacy and security".

(In reply to comment #24) > i am not sure more than web server certificates need to be managed from the web. > not all users understand how certificates work. but consider S/MIME certificates from other people, that you want to be able to write encrypted mails to

(In reply to comment #25) > but consider S/MIME certificates from other people, that you want to be able to > write encrypted mails to Do you refer to disabling application/x-x509-email-cert would disable adding a senders S/MIME certificate to "Other People's"? In this case, don't mind, it still works.

Comment on attachment 153139 [details] [diff] [review] ditches some content types and seems to fix the problem. against 1.7 This patch disables importing of all PKIX types. It makes it impossible for a user to get certificate revocation information from CAs that don't support OCSP (most CAs). That would be considered introducing yet another vulnerability. On that basis, I would give r- to any patch that disables CRL import. It makes it impossible for a user to get his own new cert, and the associated chain of CA certs. It makes it impossible for users to add new CAs to their lists. mozilla could arguably live with that for a short time. Disabling application/x-x509-email-cert makes it impossible to download another user's email from a CA's web site, for the purpose of sending secure email to that user. But that is not the primary way that users get the email certs for other users, so mozilla could live without it for a SHORT time. The ONE mime content type that seems to be quite unnecessary is application/x-x509-server-cert, because users get those certs by visiting the https server itself, not by visiting some other server. All the others are necessary. There are several possible solutions to the issue of importing multiple CA certs at once. When certs are imported, They may be imported as "temporary" or "permanent" certs. Temporary certs live only as long as a reference is held in the process. Among the possible solutions are: a) import the certs as temp certs, then invoke UI for each new root CA cert that is not already in the perm DB, then convert the approved temp root CA certs to perm certs, then convert any intermediate CA certs that can be verified by the trusted root CAs, and that are not already in the perm DB, into perm certs, then drop the references to all those temp certs. b) as a, except import the certs as perm certs, and then delete any unapproved certs that were just imported. Without more investigation, I'm not sure if any of those ideas require any enhancement to NSS or not.

can someone give a verifiable testcase for which the patch is broken? as far as i tested, importing certs from "privacy and security" works excelent. https dialogs also works for web sites - both temporary and forever. and i am against the web installing certs to users who have no idea what a cert is.

Georgi, you've tested that the importing of pkcs12 files works. But users get certs by "enrolling" for them from CAs, over the web. You've disabled that. You've also disabled all cert revocation info from CAs in the form of CRLs. Go get yourself a cert from cacert.org, and get their CRL too.

Comment on attachment 153139 [details] [diff] [review] ditches some content types and seems to fix the problem. against 1.7 Thanks for investigating this bug and submitting a patch. Unfortunately, this patch disables key PKI features in the Mozilla client. Therefore I am marking it review-.

Timeless: thanks for the drive-by, but I am not PSM owner. That PSM's owners have disowned it is a shame, but it doesn't qualify me to fix bugs in it. Are you up for the job? It would be a big help. Or maybe Christian? I'm looking for a few good hackers. /be

Just for information. I'm working on a patch that adds appropriate checks and queries the user. But I don't want and can't make any promises when it will be done resp. if I get it managed. Brendan, jumping in for PSM would be an honor. But since I've done nearly nothing on PSM till now, I don't know if I'm qualified.

Christian, anyone: start patching righteously, you'll own something soon enough. That's the best way open source works. Worse would be me trying to fill in, or making a star-chamber appointment. /be

let's try for aviary 1.0rc1

First, thanks all for the work on this. As this bug has been fished by the some sources now and there are more and more people on the CC, I've planned to give out a short communique next week. http://banquo.inf.ethz.ch:8080/ is now the link a page set up during the diploma thesis explaining the bug. The page is not secured by SSL (self-signed) any more to not disturb visitors with "certificate errors" any more.

Disable write access on cert8.db and/or key3.db is a possible workaround until the fix is present. Works OK on Mozilla 1.6 and Firefox 0.9.1. Only a warning is issued when you use ssl related features for the first time.

I'm trying to come up with a patch. If I understand correctly, the primary problem is, we receive certs, assume they are not CA certs, and therefore skip the application CA handling logic. I'm currently trying to enhance the code paths, where we assume we are not importing a CA cert, to ensure we are indeed not importing a CA cert. I saw there is NSS function CERT_IsCACert, sounds helpful for this purpose. If you have suggestions or would like to discuss, or have already started on a patch, please feel free to find me online on IRC.m.o.

--- Website with Error Informations? --- I'v searched once more about the "error -8182" in mozilla browsing help file, mozilla web site as well as google.com, and what I found is pretty interesting: Nothing mentioning this problem and how to get rid of. The only resource is a technical error list (http://www.mozilla.org/projects/security/pki/nss/ref/ssl/sslerr.html) stating that error -8182 means "Peer's certificate has an invalid signature". I think it would be useful to have a resource where users can get infos for this error codes (cf. MS knowledge base). I don't know if something exists in this way, as part of my thesis I've also set up a page stating how to get rid of the error in case you've tried the testing cases on my page: http://banquo.inf.ethz.ch:8080/Mozilla_Error_-8182.html If you want you can link to it or copy parts of it, altough the page won't live for a long time after I'll have finished my thesis.

Thanks to Christian Eyrich for discussing the issues on IRC and providing very helpful input. BTW, thanks to Marcel for coming up with the report, and for giving me access to the testcases. To explain my thoughts from the prevoius comment in more detail: I think, only if we import a cert having the CA flag, the Denial-of-Service scenario is possible. I propose, as the immediate fix to the problem, we stop importing CA certs without user interaction. We could examine all code paths that do certificate import. If we are told to import, and we have been told it's a CA, we bring up the CA import dialog, fine. If we are told to import, but we have been told the cert is not a CA, and we therefore do not bring the import CA dialog, then ensure the certificate we are trying to install is indeed not a CA certificate. However, Christian suspects, this approach will stop us from importing anything, if the peer gives us a list of certificates, and one of them is an intermediate certificate. Maybe we can find a way to distinguish the different scenarios. (I'm fine to have a separate discussion about doing more user interaction on certificate downloads in the future, but I fear this will involve a discussion that will take some time, and I guess we should come up with some sort of fix very soon.) I am attaching a first patch for you to look at. It seems to fix the problem in the "silent download" scenario, when downloading the cert from a web page. However, I need to discuss this patch with Nelson, and we should check if a similar change should be applied to other import routines.

I think we already deal correctly with content type application/x-x509-ca-cert The patch is for conten type application/x-x509-email-cert I looked at the code that handles content type application/x-x509-server-cert While we have code that tells the core of Mozilla, that PSM wants to be responsible for it, we actually do not import the received certificate, we do nothing. I looked at the code that handles content type application/x-x509-user-cert At a very early stage in content handling, we try to find out if the user has owns the matching private key for the incoming cert. I don't know an external way to automatically install a private key in the certificate store, so I guess it's not possible for an attacker to make use of this content type.

... with content type application/x-x509-user-cert, if there is no matching private key, we do not import the arriving cert, that's why I assume we are safe here. But in addition to the reported handling of mime type email-cert, I see one more code path that automatically imports incoming certificates: When processing signed S/Mime messages, we automatically import arriving certs in order to be able to reply encrypted. The code is in nsCMS.cpp, calling NSS function NSS_CMSSignedData_ImportCerts. I suspect if somebody sends a signed email message, carefully prepared, so that is contains certificates of email certificate, that are actually fake CA certificates, one could cause the same failure. I will investigate more and if I'm right, I will produce an additional fix.

(In reply to comment #41) > I think we already deal correctly with content type > application/x-x509-ca-cert We ask the user, yes. But only for the first cert. If one gets the user to agree importing it, other certs included in an pkcs#7 packet are automatically imported too. So there's a barrier, but it's not insuperable. > I looked at the code that handles content type application/x-x509-user-cert > At a very early stage in content handling, we try to find out if the user has > owns the matching private key for the incoming cert. I don't know an external > way to automatically install a private key in the certificate store, so I > guess it's not possible for an attacker to make use of this content type. You don't need the private key. You only need a users certificate. Create a pkcs#7 der file, first cert your certificate and further certs are the faked root certs. Sending this with application/x-x509-user-cert also imports the fakes, the user cert is either replaced or whatever, but nothing bad. Though it's potentially possible to use this way, it's unlikely an attacker will use it since he needs your cert. So this method isn't mass compatible.

Thanks for making it clear. If we are downloading mime type ca-cert, we should probably show a separate prompt for each incoming CA cert? If we are downloading mime type user-cert, the code indeed assumes that certs after the first are CA certs. We should probably bring up the CA cert prompt for each received certs, too?

(In reply to comment #44) > If we are downloading mime type ca-cert, we should probably show a separate > prompt for each incoming CA cert? Either that or only the root certs in the list, see comment #27, option a. As I understand it, intermediate certs without according root CA will be discarded without alert. > If we are downloading mime type user-cert, the code indeed assumes that certs > after the first are CA certs. We should probably bring up the CA cert prompt > for each received certs, too? That's the simpler to implement version but can become annoying for the user. The other version is the more complex chain building thing. Don't ask if the chain resolves to a trusted root CA. Otherwise ask once for the whole chain.

From the POV of an interested outsider, this entire process is /broken/. ALL certificates that are presented need to be validated (traced to a valid, trusted signing certificate which is already in the store and authorized to sign certificates). ALL certificates that are entered into the store need to be echoed and given the chance to be accepted or rejected, EVEN IF THEY ARE SIGNED BY A TRUSTED AUTHORITY. (The mechanics of trusting trust are such that if the trusted authority becomes compromised [however unlikely it may be], one must be able to choose not to accept certificates issued by that authority. Once a certificate is in the store, it is accepted as 'trusted' unless it's explicitly in the 'untrusted' store, if I understand correctly.) Furthermore, the original bug can be avoided by the simple expedient of comparing the entire certificate against first untrusted certificates (if a complete match is found in untrusted certificates, it is untrusted), then against the trusted certificates. Not just the DN, but the serial number and key fingerprint. In the case of certificate chains, this is appropriate, since you have the full certificate chain that signed the presented certificate. In the case of S/MIME that isn't associated with a cert chain, you can just as easily attempt to verify the certificate against the untrusted certificate first (if it matches, the issuer had the private key associated with the untrusted certficate's public key, therefore it is untrusted); if it doesn't match, go through any other DNs that are the same in the untrusted or trusted store, and verify against those. Whichever one verifies as a valid signature is the certificate that issued the signature. The problem is that you are trusting DNs to be unique, when there's obviously an import process that doesn't enforce that uniqueness. This is the root of the problem, and this is what needs to be fixed. (But 'automatically importing certificates into the store without user intervention' is another critical problem, as well.)

I think I agree with comment #46 on this as to the root cause. 1. In no circumstances should a certificate that cannot be tracd to an already trusted root CA certificate in the store be silently added to the certificate store not matter what kind of certificate it is. E-mail. webserver, CA, signing, whatever. 2. Certificates that fail the above should be able to be added to the permanent certificate store by user accepting them. If such certificates were not allowed then a company would not be able to self sign server certificates and have people add the appropriate root cert to the database. This behavior should be overridable by a security preference. A boolean preference for reject silently or prompt me. 3. Certificates that do pass the test in #1 above above should not silently be added to the permanent certificate store without user interaction. This behavior should be overridable by a different security preference than that used for #2. A boolean preference for accept silently or prompt me. 4. In all cases where multiple ceriticates are included in a single file the user prompting action must be carried out for each certificate in the file.

If something needs to be done to certificate handling, would it be possible to fix Bug 245609 at this opportunity?

I think this is a better patch for the email-cert problem. While investigating the potential additional problem I mentioned in comment 42, I found the implementation implementation of NSS function NSS_CMSSignedData_ImportCerts to look solid, because it verifies certificates before they get imported. I have produced this new patch that uses the same logic I found in that NSS function. It turns out this new patch fixes the reported problem. Since the cert import on incoming emails uses therefore the same logic, I think we can conclude, when receiving incoming mails, we are not vulnerable (as I had suspected in comment 42). (In addition, I'm changing the certusage to email-recipient. I don't see why we should use email-signer, this is probably still from the days where dual-key-certs were not common.)

I confirm the user-cert hack explained in comment 43 works, I have a testcase that causes the same trouble as the initially reported problem in this bug.

I hope to have addressed all urgent problems in this patch. As said before, we verify all incoming certs with mime type email-cert, and import only those that can be verified. When processing mime type user-cert, we will process the first cert as we did before. If additional certs are delivered to us, we assume they are CA certs, and bring up UI that allows the user to confirm or decline the import. In addition to mime type user-cert, there is also a javascript function that can be used to install a user certificate. I fixed that function the same way (but I don't have a test case). When processing mime type ca-cert, we will bring up the import dialog for each delivered cert.

Comment on attachment 153590 [details] [diff] [review] Patch v3 Nelson, what do you think? Anybody, please review and give feedback. I can upload a Linux test build in a couple of hours if it helps.

Kai, does your patch also fix bug 236461?

Comment on attachment 153590 [details] [diff] [review] Patch v3 >Index: ssl/src/nsCrypto.cpp >@@ -1983,35 +1984,45 @@ > if (numCAs > 0) { > CERTCertListNode *node; >+ nsCOMPtr<nsIMutableArray> array; >+ nsresult rv = NS_NewArray(getter_AddRefs(array)); >+ if (NS_FAILED(rv)) { > goto loser; > } this scares me. i don't like the idea of mixing gotos and comptrs. >+ // let's create some certs to work with >+ nsCOMPtr<nsIX509Cert> x509Cert; >Index: ssl/src/nsNSSCertificateDB.cpp >@@ -245,179 +245,95 @@ >+ NS_ASSERTION(0,"Couldn't create cert from DER blob\n"); NS_ERROR >+ if (!allows) allowed? >@@ -477,63 +393,121 @@ >+ for (i=0; i < numcerts; i++) { >+ CERTCertificate *cert = certArray[i]; >+ if (cert) { >+ cert = CERT_DupCertificate(cert); >+ if (cert) >+ CERT_AddCertToListTail(certList, cert); } > } >+ /* filter out the certs we don't want */ >+ srv = CERT_FilterCertListByUsage(certList, certusage, PR_FALSE); all these different *rvs are really confusing :( >+ (void )CERT_ImportCerts(certdb, certusage, certChain->len, strange whitespace pattern >@@ -643,27 +615,45 @@ >+ SECItem *currItem; could this be a SECItem &currItem instead? >+ for (int i=1; i<collectArgs->numcerts; i++) { >+ currItem = &collectArgs->rawCerts[i]; >+ nssCert = nsNSSCertificate::ConstructFromDER((char*)currItem->data, currItem->len); >+ if (!nssCert) >+ return NS_ERROR_FAILURE; >+ x509Cert = do_QueryInterface((nsIX509Cert*)nssCert); >+ array->AppendElement(x509Cert, PR_FALSE); > }

Comment 46 bears some misconceptions about mozilla's cert stores with respect to trust, and with respect to cert path validation. The local cert stores are mostly nothing more than a cache of certs that may be received through various channels. The act of storing a cert does not impute trust in it. (However, a user may choose to add explicit trust to a stored cert.) Storing a cert locally serves two primary purposes: a) it facilitates initiating a secured communication. For example, sending an encrypted email to a party without having to fetch that party's encryption cert again. b) It provides a way for the local user to attach trust to the cert, so that the cert will be seen as valid when encountered in situations where it might otherwise be deemed invalid and/or untrusted. This is intended for root CAs and for self-signed email or server certs that the user wishes to use. It may also serve a secondary purpose: filling in missing certs into incomplete chains received from misconfigured SSL or email peers. In the PKI model, a cert that verifiably chains to an explicitly trusted (root) CA cert is "valid", and usable for its allowed purposes, without requiring any additional user approval of the certs in the chain. For example, when you visit an SSL server whose server cert verifiably chains to a locally explicitly trusted root CA through one or more intermediate CAs, you are not asked if you trust the intermediate CAs, nor if you trust the server's cert, even though you (and your client program) may never have seen any of those certs (other than the trusted root CA) before. Storing certs is no different. When you are asked to store a cert that verifiably chains to a locally explicitly trusted root CA, (or that is an exact copy of an already stored cert) there is no need to ask the user to make any decision about that cert. The mere act of storing it locally will not change the outcome of any decisions based on it in the future. User intervention is required only for certs that do NOT verifiably chain to a known and explicitly locally trusted (root CA) cert. The certs that cause the problem reported in this bug do not chain to any known and trusted CA. When evaluating whether a new cert does or doesn't chain to a trusted root, it is necessary to have all the certs in that chain present in SOME local store. For that reason, NSS provides a temporary store, into which one can dump all the certs from a set being evaluated. Then one validates a chain, and may choose to move the cert from a temporary store to a permanent one if it validated. One more related point about the UI. Most of the time, the user's browser will encounter one of these MIME content types wen the user has clicked a link or button for the explicit purpose of downloading a cert or cert chain. If user intervention is required, we should ask the user to make just one trust decision, not many. Otherwise, the user is likely to repeat his first decision each subsequent time he is asked to decide. E.g. if the user is trying to download an email cert that does not chain to a known root, we should ask him whether or not to proceed to import (and trust) that email cert, and then drop the subsequent certs, not ask the user about them. That is why the user cert must be first in the downloaded chain. If we ask the user about them, the user will likely (a) decide to keep the email cert of his intended recipient, and then when he encounters other certs whose names and/or purposes he may not understand, he is likely to think that he MUST accept them too, in order for the email cert to work, and so not excersize proper judgement on those certs. This is exactly what we do for SSL server certs, BTW. When a user visits an SSL server whose cert doesn't validate against a known trusted root, we offer to let the user trust the server cert itself, and NOTHING else. This prevents a user from accidentally trusting a rogue CA, thinking that it is necessary to visit the chosen site.

My comment #46 does have some misconceptions, and I appreciate Nelson clarifying the role of the store. However, comment #56 ignores a certain level of trust that is implicitly being granted to the bogus CA root certificates in the store (in the 'untrusted certificates' store) -- the trust that the DN in the certificate is unique, and thus can be relied upon without double-checking for other identifying characteristics. The correct uniqueness should be determined by DN + keyhash + serial number, not merely DN. (and if a certificate purports to be self-signed, to get around this restriction, but the signature doesn't match... then the certificate should not be added to the store/cache at all -- only valid certs should be added.) If a bogus certificate can be added to the store, which is 'untrusted', why should that untrusted certificate be trusted enough to affect operations involving already-existing trust of other, pre-installed or explicitly-trusted certificates? Re comment #47 point 4, in support of comment #56: As for the mechanics of 'trusting trust' -- an intermediate CA (or a root CA which has been explicitly certified by a trusted root) is trusted as far as its certificate is explictly trusted by the user, or by the user's implicit trust in the trusted root. If the intermediate CA is compromised, and the trusted root CA finds out about it, the trusted root CA revokes the certificate and, thus, notifies the world that it has revoked its trust in the authenticity of any certificates which that CA may have issued. (If a user explicitly trusts the CA certificate, then there's not much that can be done automatically.) I don't see any problem with this behavior. Re Comment #56's concept of 'secondary purpose' (and thus, related to purpose a. for SSL): Personally, I think the idea of a certificate 'cache' like this only serves to create confusion for users and helpdesks, as well as website operators who may not understand how to install certificate chains properly. "It works for person A, using FireFox, but not for person B, using FireFox of the same version -- what gives?" If person A has the proper certificates in her cache, then it'll work, but if person B doesn't, it won't. Unless the website operator is a Mozilla expert (and let's face it, they've got enough trouble configuring Apache or whatever they're using as a webserver to be able to come up to speed on MSIE and Mozilla and Opera and all the other browser platforms and their idiosyncracies), they're not going to know to look in the certificate cache, nor even how to. Consistent failure is better than intermittent success in applications of this nature. (If necessary, it might be useful to word the error message in a 'could not verify up to trusted certificate' circumstance thus: "Mozilla was unable to verify that the site you are connected to is truly the site you are attempting to reach. Mozilla verifies site identity through "site certificates" that are issued by trusted agencies; however, this site's certificate could not be validated as being issued by any of these agencies. This could be a serious issue for this site's security, and should be reported to the site administrator at once; in your report, please ask the site administrator to ensure that the webserver is properly configured to send the entire "certificate chain" to your browser." This would allow users to suggest to site admins the most common cause of this error, and provide a hook into their webserver's documentation for proper configuration.) I also see a problem with caching email certificates in the store in an untrusted state -- the act of encrypting to a certificate implies that the certificate is trusted to at least belong to the email address that it's used to send to. (This is akin to the "Thawte Freemail Member" default name on certificates issued by Thawte under its Freemail program -- the certificate will certainly encrypt to the proper address, thus it meets all the 'technical' requirements of trust, but nobody in their right mind would believe that the name of the person at that mailbox would be "Mr. Thawte F. Member". Thus, it doesn't meet the 'political' or 'social' requirements of trust.) What does 'explicitly trusting an email certificate' mean? What does 'not trusting an email certificate' mean? For a distinction to be useful, there must be something to distinguish between... and this STILL doesn't solve the problem of certificates being silently added to the store with no UI to even notify that something's happening. (cf bug #251690 )

Nelson, you wrote > E.g. if the user is trying to download an email cert that does not chain to a > known root, we should ask him whether or not to proceed to import (and trust) > that email cert, and then drop the subsequent certs, not ask the user about > them. So this means, just set this single S/MIME cert to trust but don't set trust flags for any intermediate or root CA (if already in perm or just in temp)? But then what is the reason/benefit of a package containing a whole chain? The S/MIME cert will chain up to the root included, but since it's not known nothing else, except the S/MIME cert itself, will ever get imported. Is this right and wanted?

re comment #58: I'd like to have a dialog that says something like: "This email certificate has been issued by an authority that Mozilla does not know about (##CN of CA, link to full certificate information##). Only you can decide if you believe that the certificate belongs to the person that it claims to belong to. In addition, if you believe that this authority will only issue email certificates after verifying the identities of those requesting certificates, you can choose to accept and trust the issuing authority. This will mean that you will automatically accept email certificates issued by this authority, as though you had chosen to accept each and every one of them. What would you like to do? ##dropdown## [OK] [CANCEL] [HELP]" dropdown includes: "Do not accept this email certificate [default]" "Accept this email certificate (##CN of email##)" "Accept the issuer of this email certificate (##CN of CA##)" "Accept the issuer of the authority (##CN of CA2##)" ... "Accept the master (root) authority (##CN of ROOTCA##)" A lot of the trouble for users is that they don't understand the key concepts involved. I don't know if this particular wording would be appropriate, or if end-users would be able to understand the concept in this way -- but doing /anything/ to make it easier to understand should get them to take a more active interest. (and hopefully spawn a few more "oh, is that what that means? Duh, I get it now!" epiphanies.) of course, I'm also of the mindset that software should teach the user how to use it automatically, without the aid of a book or manual. Feel free to disregard this comment if it's too far out of scope.

Comment on attachment 153590 [details] [diff] [review] Patch v3 I had a chat with Nelson, and I agree with his statements in comment 56. This means I will come up with another patch. Please note that I have very limited time to work on this, because my day job is not related to Mozilla. Therefore I'm trying to come up with a patch that does the minimal effort to fix the problem.

Re comment 57: Kyle, I suggest you go read and study RFS 3280 before making any additional suggestions. Learn about how an issuer cert is identified based on the information in an issued cert. This bug report is not the place to be conducting education about how X509v3 based PKI works. Re comment 58: Yes, in that case, trust the leaf, drop the chain. The act of importing a leaf cert is NOT the occasion to be adding trust to a root. When the user decidees to accept and trust a cert that has already FAILED the usual PKI validity checks, we need to MINIMIZE the scope of the new trust. The fact that the user wants to accept the email cert from an unknown and possibly rogue CA ought not to imply that the user will thereafter trust all the certs from that CA. If the user wants to add a root CA cert and trust it, the user should visit the CA, the source of the cert. Re: comment 59: trusting a CA is the single most crucial decision the user can make. No way should it be made as a side effect of some other action, or be made without presenting the user with dialogs outlining the seriousness of the action and enough information that the user has at least a chance of looking at the right information with which to make the decision. The proposed drop-down dialog would like have the wrong effects, increasing the likelihood of the user accepting and trusting a rogue CA cert. Kai pointed out that a "hot fix" to a bug like this must avoid designing new UI. Aside: I'm on vacation, and will soon be travelling. I expect to be out of contact with the Internet for most of the rest of the week.

@Heikki: Having read Nelson's arguments, I agree that one download should deliver one cert (chain) only. So I will not fix bug 236461, at least not with a patch to this bug.

Here is the new patch based on Nelson's proposal. On importing ca-cert, we continue to show only one cert to the user. However, if there are more certs, from now on they will only get imported, if they can be verified as CA certs. On importing email-cert, we continue to import any email cert delivered. However, only certs that can be verified will be imported. On importing user-cert, we will continue to import the first cert as before, assuming it is a user cert. However, if there are more certs, from now on they will only get imported, if they can be verified as CA certs. Christian, you reported a crash with the previous patch. Hopefully this new patch no longer crashes, I'd appreciate if you could do some testing again.

(In reply to comment #63) > Christian, you reported a crash with the previous patch. Hopefully this new > patch no longer crashes, I'd appreciate if you could do some testing again. It still crashes. But I don't think it's your code. It crashes when first calling CERT_ImportCerts() from ImportEmailCertificate() when importing a package containing > 1 cert. Interestingly it doesn't if I copy that CERT_NewTempCertificate-loop directly into ImportEmailCertificate(). Strange thing I've to investigate tomorrow. Besides this I wonder if this patch works for you. I can't import any email-cert and no ca-cert except the first (same with user-cert I guess, but untested). Firstly it has to be !CERT_LIST_END(node,certList) in the for loops. And secondly CERT_VerifyCertificate() doesn't use the SECCertUsage enums but the values ones from certt.h#485 ff. > However, if there are more certs, from now on they will only get imported, if > they can be verified as CA certs. Besides I don't think that's what Nelson meant, this way I still can make Mozilla import my faked Thawte Root cert if only the first cert in the package (the one for which the dialog is shown) gets in. A CA cert should never, never, never get imported without user interaction. And as Nelson wrote e.g. in bug 236461 "A PKCS7 file of certs is supposed to contain a single cert chain, not a collection of potentially unrelated certs." So I don't see a need for the "for (node = CERT_LIST_HEAD(certList) ..." loop at all, for any cert type. And last thing for tonight I can't see the reason for this calls to CERT_FilterCertListByUsage() since CERT_VerifyCertificate() CERT_CertChainFromCert() IMO already filter for usage, no?

Re comment 64: The key word in Kai's sentence is "verified". Any subordinate CA cert that can be verified as issued by a trusted root CA can be imported without user interaction. If it was found in a cert chain, it would be honored because the chain validates, whether or not it was cached locally. cacheing it doesn't change that. There is no point in putting tighter acceptance criteria on the cert for import than the criteria that would be used when validating a cert chain.

(In reply to comment #65) > Re comment 64: The key word in Kai's sentence is "verified". > Any subordinate CA cert that can be verified as issued by a trusted root CA > can be imported without user interaction. Yes, sorry. In > A CA cert should never, never, never get imported without user interaction. I meant unrelated certs. The certs I used for testing where completeley different and both root certs. So there was no chain the second cert can be validated to. > Any subordinate CA cert that can be verified as issued by a trusted root CA > can be imported without user interaction. The last patch (as well as the current behaviour) doesn't honor this. The user will allways receive a dialog for the first cert, if it chains up to a trusted root or not.

Ok, I found several problems with the patch. While for now I would like to stay with the described logical approach, there are several problems at the code level, due to lack of testing. You're right with !CERT_LIST_END for some reason I missed the ! while copying the code. Regarding the crash in CERT_ImportCerts, I see I can not not pass over directly the array returned from certCollection, it's required to copy it over to a separate array of SECItems and use that for importing. With that changed, I no longer crash. But third, there is a problem with function CERT_VerifyCertificate ! It does not use the passed in certUsage correctly! In my testing, when not passing in a pointer to return the verified usages, it always uses certUsageSSLServer... So I have to say I don't like this function. Yes, you're right, the NSS implementation comment says, old function CERT_VerifyCert (without the trailing ...ificate) should no longer be used for new code, however, it works better for me, so I'll use that. Regarding filtering the list, I think it makes sense to avoid verifying certificates we are not interested in anyway. I'll attach a new patch in the next hour.

Next attempt. This time I have done several tests and it looks good so far. However, please repeat your tests. We should make it work first, before asking the NSS team to review the patch. The code calls NSS function CERT_VerifyCert with parameter certUsageVerifyCA as recommended by Bob. However, this makes it crash and it was necessary to patch NSS to make it work.

During testing I found bug 252268. It has a similar result, producing error -8182, however, limited to the current session only.

Bug 252271 tracks the necessary NSS change CERT_VerifyCert.

> The last patch (as well as the current behaviour) doesn't honor this. The user > will allways receive a dialog for the first cert, if it chains up to a trusted > root or not. If the package delivered with x-x509-ca-cert contains more than 1 cert, we will show only the first (or the second, if the first turns out be to signed by the second). Yes, in addition to the displayed cert, the new logic might import valid verifiable CA certs (but ONLY verifiable certs) that are unrelated to the first cert. I'm doing it because it will not cause any harm if the unrelated cert can be verified. Yes, we could start a discussion whether we should enhance the code further and limit the import to related certs only, but if you agree this is not a security problem, given my time constraints, I would ask that we do that additional work in a separate step/bug.

Comment on attachment 153767 [details] [diff] [review] Patch v5 >Index: manager/ssl/src/nsNSSCertificateDB.cpp >@@ -380,46 +381,98 @@ >+ for (node = CERT_LIST_HEAD(certList) ; >+ !CERT_LIST_END(node,certList); >+ node= CERT_LIST_NEXT(node)) { that can't be house style for whitespace (space before semi and equals, no space before semi, no space before equals). > if (!certCollection) { >+ if ( !rawArray ) { what is house whitespace style? >- if ( !rawCerts ) { oh nevermind consistency. ... > loser: >- if (cert) >- CERT_DestroyCertificate(cert); >+ if (certArray) { >+ CERT_DestroyCertArray(certArray, numcerts); >+ } >+ if (certList) { >+ CERT_DestroyCertList(certList); >+ } 2-4 space indenting is bad, don't do it ;-) > if (arena) > PORT_FreeArena(arena, PR_TRUE); > return nsrv; > }

Timeless, I would like to propose we delay any whitespace cleanup discussion until we know the patch works.

(In reply to comment #67) > I see I can not not pass over directly the array returned from certCollection, > it's required to copy it over Yep, that's what I also just discovered. > But third, there is a problem with function CERT_VerifyCertificate ! > It does not use the passed in certUsage correctly! > In my testing, when not passing in a pointer to return the verified usages, it > always uses certUsageSSLServer... You used certificateUsageEmailRecipient resp. certificateUsageVerifyCA for requiredUsages, not certUsageEmailRecipient resp. certUsageVerifyCA, yes?

(In reply to comment #68) > This time I have done several tests and it looks good so far. > However, please repeat your tests. > We should make it work first, before asking the NSS team to review the patch. Up to now the patch seems to close the hole. I wasn't able to inject unwanted certs. Though this fix shouldn't contain UI, I recommend at least a standard error box if we don't import a email-cert because it doesn't chains up. If we just silently drop it, users will start filing bugs that Mozilla doesn't import email-certs.

> I recommend at least a standard error box Two problems with this approach: - we would have to add a string for the error message, and embeddors do not want to start over with their localization process on a stable branch like 1.7 (I remember mkaply@ibm saying so). There are currently no suitable error strings contained in module security/manager/ssl. - the better approach would be a callback, implemented either by Mozilla's security/manager/pki UI module, or the embeddors UI. But that requires changing the callback interfaces, which is also not appropriate for a stable branch. Unless mozilla staff grants explicit permission to include new UI strings on the branch, I'd like to avoid it. We can do anything we want on the trunk and for future main Mozilla versions, but for now I'd like to start with a patch that is suitable for 1.4 branch, 1.7 branch and the trunk.

(In reply to comment #76) > - we would have to add a string for the error message Oh yes, I forgot that. So then we can only hope people will only import certs that base on already imported CA certs. Adding a "known Issue" can also be done - though I suspect anyone reads readmes.

> You used certificateUsageEmailRecipient resp. certificateUsageVerifyCA for > requiredUsages, not certUsageEmailRecipient resp. certUsageVerifyCA, yes? Well, you're right, I did not. I used the certUsage constants. However, have a look at CERT_VerifyCertificate in certvfy.c, the switch action is based on value of certUsage, which appears to always remain at 0, which I think it confusing and not what we desire. Since Bob also recommended to use CERT_VerifyCert, I don't want to investigate further.

This patch is nearly identical to the previous one. However, there were two identical code sections that I combined into a function. And it has some whitespace cleanup... ;-)

Comment on attachment 153783 [details] [diff] [review] Patch v6 Bob, could you please review the calls to NSS?

Comment on attachment 153783 [details] [diff] [review] Patch v6 What I didn't review: 1) that all the places we import certs have been found. 2) CERTCertificateListCleaner and CERTCertListCleaner do what I think they do (destroy the appropriate type with the correct NSS destroy function when the go out of scope). 3) That the style is consistant with PSM or Mozilla. Those things aside, the code looks mostly correct. A minor nit, I agree with timeless that we should be consistant in our cleanup strategy (at least within a single function). That in itself did not warrent the r-. The more major issue is the use of NULL for the wincx parameter for the CERT_VerifyCert calls. If we are not authenticate AND if PSM's password callback requires a window context (or some other password arg) to fetch the password, AND we are running in FIPs mode (where authentication is required to verify signatures, for instance), then it is possible for CERT_VerifyCert may fail because it couldn't fetch the password. This issue would show up in a rare, and inconsistant, failure to import an otherwise legitimate certificate (and probably wouldn't be caught by system testing). the r- can be changed to an r+ automatically under the following conditions: 1) the appropriate wincx argument is found and passed to CERT_VerifyCert(), or 2) the PSM password function is reviewed and it is determined that it will function correctly (still prompt for a password) if a NULL wincx is supplied. The rest of the logic, including cleanup, appears functionally correct. bob

> I didn't review that all the places we import certs have been found. I searched sources with "grep -i importcert" and "grep -i toperm" I think all these places are safe now. We don't touch the logic that permanently installs a server on visiting an untrusted server, but the user is prompted, and AFAICT we don't import additional certs. We also don't touch the implementation of nsIX509CertDB2::addCertFromBase64, which provides a mechanism to import a given cert into the perm store with the given trust. I can't find any active code that makes use of this function, I supposed some embeddors might do, but that's outside of our responsibility. > I didn't review that CERTCertificateListCleaner and CERTCertListCleaner do what I think they do (destroy the appropriate type with the correct NSS destroy function when the go out of scope). Your assumption is correct. > I didn't review that the style is consistant with PSM or Mozilla. > I agree with timeless that we should be consistant in our cleanup strategy (at > least within a single function). Now that the code is ready I will address the style and cleanup suggestion. I hope the style in the forthcoming patch will be sufficient. > The more major issue is the use of NULL for the wincx parameter for the > CERT_VerifyCert calls. If we are not authenticate AND if PSM's password > callback requires a window context (or some other password arg) to fetch the > password, AND we are running in FIPs mode (where authentication is required to > verify signatures, for instance), then it is possible for CERT_VerifyCert may > fail because it couldn't fetch the password. This issue would show up in a > rare, and inconsistant, failure to import an otherwise legitimate certificate > (and probably wouldn't be caught by system testing). I agree. Luckily the context arguments where already available in all modified scopes, so in all new calls to CERT_VerifyCert I set param 6 to variable ctx. > the r- can be changed to an r+ automatically under the following conditions: > 1) the appropriate wincx argument is found and passed to CERT_VerifyCert(), or > The rest of the logic, including cleanup, appears functionally correct. Thanks a lot. Because I fulfil condition 1), I'll mark the next patch with r=relyea.

Bob's requested changes and whitespace/style cleanup.

Comment on attachment 153803 [details] [diff] [review] Patch v7 marking r=relyea Brendan, can you please sr?

I'm not really doing srs in code I don't know at this time... Please ask someone else.

In order to speed up the process, and to allow more testing of the patch, I have produced a test build (Linux only). It's a snapshot from yesterday's MOZILLA_1_7_BRANCH with patch v7 applied. http://kuix.de/mozilla/20040727/mozilla-171+-20040727-249004.tar.gz No installer, just extract and run. Below is a GPG signed md5sum (my gpg key is in the key servers). -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 1601e49fe57d707f50cdf3ad9ec121ab mozilla-171+-20040727-249004.tar.gz -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.2.4 (GNU/Linux) iD8DBQFBBmnPdUk7WZN5lQ4RAsnCAJ4phL3C5rh22UpIPktpZHGdhcA5ZgCfdC7M DqWyxtiadFNm+fq6axDDXEw= =M+DC -----END PGP SIGNATURE----- PLEASE DO NOT DISTRIBUTE this build, it's unofficial. Please give feedback in the bug. Thanks.

Comment on attachment 153803 [details] [diff] [review] Patch v7 Thanks for the superreview. I will check in the patch to the trunk now. Requesting permission to land the patch on both MOZILLA_1_7_BRANCH and MOZILLA_1_4_BRANCH. This patch works on the 1.4 branch, too. There is a small context conflict which I will resolve manually when checking in. I'm not sure which branch this should land on for Firefox/Thunderbird, please advice.

Comment on attachment 153803 [details] [diff] [review] Patch v7 Sorry, I was away last week but wanted to get a few nits picked: >- // Now it's time to add the rest of the certs we just downloaded. >- // Since we didn't prompt the user about any of these certs, we >- // won't set any trust bits for them. >- nsNSSCertTrust defaultTrust; >- defaultTrust.SetValidCA(); >- defaultTrust.AddCATrust(0,0,0); >+ // Import additional delivered certificates that can be verified. >+ >+ /* build a CertList for filtering */ Use a C++ one-line comment here too? > CERTCertificate *tmpCert2 = > CERT_NewTempCertificate(certdb, &der, nsnull, PR_FALSE, PR_TRUE); > > if (!tmpCert2) { > NS_ASSERTION(0, "Couldn't create temp cert from DER blob\n"); Use NS_ERROR(...) here, not NS_ASSERTION(0, ...). >+ /* >+ * CertChain returns an array of SECItems, import expects an array of >+ * SECItem pointers. Create the SECItem Pointers from the array of >+ * SECItems. >+ */ >+ rawArray = (SECItem **)PORT_Alloc(certChain->len*sizeof (SECItem *)); Odd spacing (lack of spaces around binary *, space after sizeof), contrast with other (sizeof(SECItem *) * numcerts) expressions visible in this patch. >+ /* go down the remaining list of certs and verify that they have >+ * valid chains, if yes, then import. >+ */ >+ PRTime now = PR_Now(); >+ CERTCertListNode *node = NULL; >+ for (node = CERT_LIST_HEAD(certList); No need to NULL-init node here before the for loop that reinits it. >+ !CERT_LIST_END(node,certList); >+ node = CERT_LIST_NEXT(node)) { >+ CERTCertificateList *certChain = NULL; Could move this down, to where it's first set, I think/hope. >+ >+ if (CERT_VerifyCert(CERT_GetDefaultCertDB(), node->cert, >+ PR_TRUE, certUsageVerifyCA, now, ctx, NULL) != SECSuccess) { >+ continue; >+ } >+ >+ certChain = CERT_CertChainFromCert(node->cert, certUsageAnyCA, PR_FALSE); >+ if (!certChain) { >+ continue; >+ } >+ >+ CERTCertificateListCleaner chainCleaner(certChain); >+ >+ /* >+ * CertChain returns an array of SECItems, import expects an array of >+ * SECItem pointers. Create the SECItem Pointers from the array of >+ * SECItems. >+ */ >+ rawArray = (SECItem **)PORT_Alloc(certChain->len*sizeof (SECItem *)); Ditto spacing comment on sizeof-bearing argument expression. >+ if (!rawArray) { >+ continue; >+ } >+ for (int i=0; i < certChain->len; i++) { >+ rawArray[i] = &certChain->certs[i]; >+ } >+ CERT_ImportCerts(CERT_GetDefaultCertDB(), certUsageAnyCA, certChain->len, >+ rawArray, NULL, PR_TRUE, PR_TRUE, NULL); >+ >+ PORT_Free(rawArray); >+ } >+ >+ return NS_OK; General comment: is Out Of Memory hidden in favor of best-effort, even if not all certs are imported? On most modern VM-using OSes, OOM can't happen (you swap to death and the kernel kills processes), but it seems worth asking, in case this code runs in a Minimo small-device setting. /be

Comment on attachment 153803 [details] [diff] [review] Patch v7 a=chofmann for 1.7.2

>>+ /* build a CertList for filtering */ > Use a C++ one-line comment here too? changed > Use NS_ERROR(...) here, not NS_ASSERTION(0, ...). In this place you're not reviewing my changes, but diff context...! Anyway, Changed. Looks like other Mozilla code doesn't use a trailing \n, so I removed it, too. > Odd spacing (lack of spaces around binary *, space after sizeof), contrast with > other (sizeof(SECItem *) * numcerts) expressions visible in this patch. ... changed >>+ CERTCertListNode *node = NULL; > No need to NULL-init node here before the for loop that reinits it. changed >>+ CERTCertificateList *certChain = NULL; > Could move this down, to where it's first set, I think/hope. moved, both places >>+ rawArray = (SECItem **)PORT_Alloc(certChain->len*sizeof (SECItem *)); > Ditto spacing comment on sizeof-bearing argument expression. changed >>+ if (!rawArray) { >>+ continue; >>+ } > > is Out Of Memory hidden in favor of best-effort, even if not > all certs are imported? I didn't have a specific Out-Of-Memory strategy in mind when putting together the patch. I'm just making sure not to use NULL pointers, and not introduce too many exit pathes.

Comment on attachment 154493 [details] [diff] [review] Patch v7 with nits addressed carrying forward r= and approval1.7.2+ re-asking brendan for sr requesting 1.4.3 approval

Comment on attachment 154493 [details] [diff] [review] Patch v7 with nits addressed a=asa for aviary checkin

Comment on attachment 154493 [details] [diff] [review] Patch v7 with nits addressed talked to brendan, marking sr=jst/brendan

I am marking this patch to be blocked by 252271. This has relevance for trunk development only, and has to do with NSS release tags used by Mozilla. We are able to land this patch on any Mozilla branch, including the change to mozilla/security/nss. However, before we can land this patch on cvs HEAD, we must wait for the NSS team to move NSS_CLIENT_TAG to a snapshot that includes the fix. I've asked them in bug 252271.

Checked in to MOZILLA_1_7_BRANCH adding fixed1.7.2 keyword

Comment on attachment 154493 [details] [diff] [review] Patch v7 with nits addressed a=blizzard for 1.4.3.

Checked in to MOZILLA_1_4_BRANCH adding fixed1.4.3 keyword

This new revision of the patch has a very small change, just one loop variable was renamed from i to i2, in order to allow it to compile on OS/2. I have already checked in this bustage fix to MOZILLA_1_4_BRANCH and MOZILLA_1_7_BRANCH.

Patch checked in to cvs HEAD, marking fixed.

The approval-aviary flag (oops, not on the PSM product yet) corresponds to AVIARY_1_0_20040515_BRANCH, which is for Firefox 1.0 and Thunderbird 1.0.

Comment on attachment 154493 [details] [diff] [review] Patch v7 with nits addressed Who can check this in to the aviary branch?

Checked in to AVIARY_1_0_20040515_BRANCH Adding fixed-aviary1.0 keyword

Marcel, thanks again for your report and for producing the testcases. If you get a chance to download a nightly build that includes the fix, it would be great to hear from you, whether you agree the issue is fixed.

Marcel says he is very busy at the moment, we're looking for volunteers to verify the bug is fixed with latest builds. Thanks in advance.

Should this go into the 1.7.2 mini-branch too? (MOZILLA_1_7_2_BRANCH)

(In reply to comment #105) > Should this go into the 1.7.2 mini-branch too? (MOZILLA_1_7_2_BRANCH) I'll get it merged in.

Kai, thanks for the test build. I've filed the thesis in now, the busy time is over. According to my tests with http://kuix.de/mozilla/20040727/mozilla-171+-20040727-249004.tar.gz the silent import with mime type x-x509-emailcert is fixed (however i'd like some info message when clicking the .ecert link instead of "nothing happens".) However, when using mime type x-x509-cacert the import is still possible, with the ca cert message dialogue. When clicking through the cert is imported as untrusted. So far so good. Now surfing to https://thawte.com still throws a -8182, but only before restarting the browser. Btw, the thawte site seems to have changed their certificate... So in my opinion the bug is fixed, thank you all. /marcel

Comment 107 seems to imply a cert reference leak. The certs that are not imported should not remain in the temp cert DB (their references should be deleted before finishing the processing of the MIME content type that imported them), and so they should not affect subsequent https URLs. If they actually do, some reference is apparently being leaked. This will negatively affect a profile switch. I'd say there is more work to do here.

@Nelson and @Marcel During my testing I had experienced the same behaviour that you describe in comment 107. Please read comment 69 which points to bug 252268. I agree there is a cert leak, however, the leak is not in the new code we have added. The leak is apparently within NSS, please read the NSS bug and the duplicate it resolves to.

what would be a good test case to verify this bug? thanks!

that is, what would be specific steps to take for an end-user to test this?

per comment 106 (and bonsai), adding fixed1.7.2 keyword

FYI: http://cve.mitre.org/cgi-bin/cvename.cgi?name=CAN-2004-0758

the underlying transportation mechanisms as per comment 6 for the now fixed exploits might still not be addressed --> see Bug 276991

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