Closed Bug 249004 Opened 20 years ago Closed 20 years ago

Importing false CA certificate leading to error -8182 (perm DoS), especially exploitable by email

Categories

(Core Graveyard :: Security: UI, defect, P2)

Other Branch
defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: marboesc, Assigned: KaiE)

References

Details

(4 keywords, Whiteboard: [sg:fix]blocking1.4.3+)

Attachments

(2 files, 10 obsolete files)

1.01 KB, text/plain
Details
11.70 KB, patch
Details | Diff | Splinter Review
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
Group: security
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.
Assignee: kaie → brendan
OS: other → All
Hardware: Other → All
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.
Flags: blocking1.8a2?
Flags: blocking1.7.2?
Flags: blocking-aviary1.0RC1?
Whiteboard: [sg:fix]
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-.
Attachment #153139 - Flags: review-
Whiteboard: [sg:fix] → [sg:fix]blocking1.4.3+
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
Flags: blocking1.8a2?
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
Flags: blocking-aviary1.0RC1? → blocking-aviary1.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?
Attached patch Patch v2 to ImportEmailCerts (obsolete) — Splinter Review
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.)
Attachment #153518 - Attachment is obsolete: true
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.

Attached patch Patch v3 (obsolete) — Splinter Review
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.
Attachment #153584 - Attachment is obsolete: true
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.
Attachment #153590 - Flags: review?(nelson)
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);
>     }
Summary: Overriding built-in certificate leading to error -8182 (DoS), especially exploitable by email → Overriding built-in certificate leading to error -8182 (perm DoS), especially exploitable by email
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.
Summary: Overriding built-in certificate leading to error -8182 (perm DoS), especially exploitable by email → Importing false CA certificate leading to error -8182 (perm DoS), especially exploitable by email
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.
Attachment #153590 - Attachment is obsolete: true
Attachment #153590 - Flags: review?(nelson)
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.
Attachment #153596 - Attachment is obsolete: true
Attachment #153139 - Attachment is obsolete: true
Attached patch Patch v4 (obsolete) — Splinter Review
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.
Attached patch Patch v5 (obsolete) — Splinter Review
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.
Attachment #153698 - Attachment is obsolete: true
(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.
Attached patch Patch v6 (obsolete) — Splinter Review
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... ;-)
Attachment #153767 - Attachment is obsolete: true
Comment on attachment 153783 [details] [diff] [review]
Patch v6

Bob, could you please review the calls to NSS?
Attachment #153783 - Flags: review?(rrelyea0264)
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
Attachment #153783 - Flags: review?(rrelyea0264) → review-
Assignee: brendan → kaie
> 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.
Attached patch Patch v7 (obsolete) — Splinter Review
Bob's requested changes and whitespace/style cleanup.
Attachment #153783 - Attachment is obsolete: true
Comment on attachment 153803 [details] [diff] [review]
Patch v7

marking r=relyea

Brendan, can you please sr?
Attachment #153803 - Flags: superreview?
Attachment #153803 - Flags: review+
Attachment #153803 - Flags: superreview? → superreview?(bzbarsky)
I'm not really doing srs in code I don't know at this time... Please ask someone
else.
Attachment #153803 - Flags: superreview?(bzbarsky) → superreview?(jst)
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.
Attachment #153803 - Flags: superreview?(jst) → superreview+
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.
Attachment #153803 - Flags: approval1.7.2?
Attachment #153803 - Flags: approval1.4.3?
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
Attachment #153803 - Flags: approval1.7.2? → approval1.7.2+
Attached patch Patch v7 with nits addressed (obsolete) — Splinter Review
>>+  /* 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.
Attachment #153803 - Attachment is obsolete: true
Attachment #153803 - Flags: approval1.4.3?
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
Attachment #154493 - Flags: superreview?(brendan)
Attachment #154493 - Flags: review+
Attachment #154493 - Flags: approval1.7.2+
Attachment #154493 - Flags: approval1.4.3?
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
Attachment #154493 - Flags: superreview?(brendan) → superreview+
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.
Blocks: 252271
No longer blocks: 252271
Depends on: 252271
Checked in to MOZILLA_1_7_BRANCH
adding fixed1.7.2 keyword
Keywords: fixed1.7.2
Comment on attachment 154493 [details] [diff] [review]
Patch v7 with nits addressed

a=blizzard for 1.4.3.
Attachment #154493 - Flags: approval1.4.3? → approval1.4.3+
Checked in to MOZILLA_1_4_BRANCH
adding fixed1.4.3 keyword
Keywords: fixed1.4.3
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.
Attachment #154493 - Attachment is obsolete: true
Patch checked in to cvs HEAD, marking fixed.
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Whiteboard: [sg:fix]blocking1.4.3+ → [sg:fix]blocking1.4.3+ needed-aviary1.0
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?
Attachment #154493 - Flags: approval-aviary+
Flags: blocking1.8a3?
Flags: blocking1.7.2?
Checked in to AVIARY_1_0_20040515_BRANCH
Adding fixed-aviary1.0 keyword
Keywords: fixed-aviary1.0
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.
Whiteboard: [sg:fix]blocking1.4.3+ needed-aviary1.0 → [sg:fix]blocking1.4.3+
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
Keywords: fixed1.7.2
Blocks: sbb?
Blocks: sbb+
No longer blocks: sbb?
Product: PSM → Core
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. ***
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.