401292 - application and addon updates fail when Danish Government browser extension is installed

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

[Jesper Kristensen]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; da; rv:1.8.1.8) Gecko/20071008 Firefox/2.0.0.8
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; da; rv:1.8.1.8) Gecko/20071008 Firefox/2.0.0.8

TDC Digital Signatur is a solution for logging in to Danish government websites and for communicating with Danish public institutions by e-mail.

There is currently two ways to install the TDC Digital Signatur in Firefox and Thunderbird:

The first method is to export  the user's private certificate to PKCS#12 format and then import it into Firefox's or Thunderbird's certificate store. This method has some serious usability bugs, one of them is bug 322617.

Therefore TDC created another installation method, where the internal security module is replaced with another module from TDC, which can read the user's private certificate from MSCAPI. (I am not exactly sure how that works)

http://mxr.mozilla.org/mozilla1.8/source/toolkit/mozapps/shared/src/badCertHandler.js#59

Firefox and Thunderbird expect the certificate for the update site to be issued by "Builtin Object Token". This is not the case when the user has installed the security module from TDC, because then the certs are read from the TDC module instead of the built in one.

I am filing this under Software Update, but this could just as well be fixed by fixing the usability bugs in the first install method above, so that users won't have to install the security module from TDC.

Bug report and workaround in Danish:
http://forum.mozilladanmark.dk/viewtopic.php?p=17434



Reproducible: Didn't try

Steps to Reproduce:
1. Click Help -> Check for updates
Actual Results:  
Get error message "AUS: Update XML File Not Found (404)"

Expected Results:  
Update checking succeeded

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

After reading comment 0, I conclude that

- Because of a usability issue with passwords controlling access to the 
private key for a user's own email cert, a new PKCS#11 module was written 
for Mozilla clients.

- This module not only changes the way that users access their own email 
private keys, but also changes the way that mozilla products find the root
CA cert for the issuer of the users' certs.  It causes the root CA cert to
be found in the new PKCS#11 module, rather than (or perhaps in addition to) 
Mozilla's own root CA cert module.  Perhaps it causes ALL root CA certs to
be appear to be found in the new PKCS#11 module, rather than (or in 
addition to) Mozilla's own root CA cert module.

- Mozilla's new addon update security validation logic (see bug 378216) 
requires that server certs for https servers from which manifests for 
addons or updates are downloaded must chain up to a root CA in mozilla's built-in list of known root CA certs.  (Do I have that right, Dave?)
This requirement is imposed by Mozilla's addon security code, not by NSS.

- With this new PKCS#11 module in place, the root CA cert(s) in the chain(s)
for one or more https addon servers now appear to come from this other 
PKCS#11 module, rather than from Mozilla's own root CA module. Consequently,
those https addon servers' certs no longer meet the addon update code's 
criteria for validity.

The description in comment 0 suggests that the new PKCS#11 module actually 
replaces mozilla's normal module, rather than merely supplementing it with 
an additional module.  If it replaces Mozilla's own PKCS#11 modules, so 
that no root CA certs ever appear to come from Mozilla's own modules, 
then it's simply violating a Mozilla (not NSS) design requirement, and 
must be changed not to do that.  

On the other hand, if it is adding an ADDITIONAL PKCS#11 module to the 
set of PKCS#11 modules visible to Mozilla clients, so that the root CA
certs now accessible to Mozilla clients from multiple PKCS#11 modules, 
including Mozilla's own, then the problem may simply be that Mozilla 
needs to do an additional check, to see if the root CA cert is ALSO 
found in Mozilla's own cert list, in addition to anywhere else that it
may be found. 

We just don't know enough about this new module to know which of those
is the case.

A few more thoughts:

- If the new PKCS#11 module was created to solve a perceived usability 
issue with password prompts for private keys (bug 322617), then it was 
unnecessary for the solution to have any impact on where root CA certs 
are found.  The new PKCS#11 module does more than it needs to do to solve
the perceived usability issue.  The password issue can be addressed by 
simply making private keys accessible to Mozilla in an additional PKCS#11 
module.  Problems with Users' private keys necessitate no changes to 
Mozilla's access to root CA certs.

- I gather that this new PKCS#11 module must be doing its own UI, prompting
the user for a private key password on every attempt to use the private key.
But the functions in which that UI must have been inserted are blocking 
functions, having no way to return until the job is done, and has succeeded
or failed.  There is no EWOULDBLOCK return from PKCS#11.  So the thread 
that calls that function would be hung while the new password UI persists.
This makes me wonder: does mozilla's own UI become non-responnsive to user 
input while this module's prompt is displayed?

Comment 2

[Jesper Kristensen]

(In reply to comment #1)
What I do know:
* This breaks both app updates and addon updates when the program ("TDC Digital Signatur CSP") is installed
* A security module called "Nyt PCKS#11 Modul" ("New PCKS#11 Module" in English) is added to Firefox and Thunderbird
* The update (for both app and addon) is broken because the exception in /toolkit/mozapps/shared/src/badCertHandler.js#59 is thrown

What I am not sure about:
* How the installation of this program causes the exception to be thrown.

Comment 3

[hansen]

(In reply to comment #1)

Hi Nelson,

I can see that you know a lot about PKCS compared to Jesper and me. I would be happy to "lend" you my PC with VNC/RDP, so you can test it yourself. Mail me.

To access a lot of electronic services (taxes, social security, etc.), the Danish Government requires this certificate, so most people in Denmark has installed it.

When installed (through Firefox), then Firefox won't (auto) update to newer security releases or major versions. And a side-issue is that addons won't update either.
This is kind of critical, but not sure if it's Firefox' "fault" - either way, then we have a bug reference to contact TDC (the certificate provider) with.

Comment 4

[Robert Strong (they/them - no direct email)]

(In reply to comment #1)
> - Mozilla's new addon update security validation logic (see bug 378216) 
> requires that server certs for https servers from which manifests for 
> addons or updates are downloaded must chain up to a root CA in mozilla's
> built-in list of known root CA certs.  (Do I have that right, Dave?)
> This requirement is imposed by Mozilla's addon security code, not by NSS.
The bad cert handler was added in bug 366191 well before the referenced bug (bug 378216) landed and when it was added it utilized the one created for software update.

Comment 5

[Jesper Kristensen]

Would it be possible to change this line:

http://mxr.mozilla.org/mozilla/source/toolkit/mozapps/shared/src/badCertHandler.js#59

from:

if (!issuer || issuer.tokenName != "Builtin Object Token")

to:

if (!issuer || (issuer.tokenName != "Builtin Object Token" && issuer.tokenName != "Crypto API Token"))

?

I have not tested it yet, but I think it will solve this issue, which is a top support request in my country with no good answer.

Comment 6

[Julien Pierre]

I am not sure what that Javascript code does.
However, one should remember that it's possible to change the trust on a built-in certificate. If so, a second copy of the certificate will be made in the NSS softoken database. So, the certificate may no longer appear to come from the "Builtin Object Token". I think it is bogus for FF to depend on that. That JS code will break if any trust setting is modified. And it will also break if another module contains the same certificate. 

NSS has a function to enumerate all the tokens that a given certificate lives in, since there can be more than one. I can't remember its name on the top of my head, but it's there. Perhaps it should be used. I don't know if it's accessible from JS or not.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

The code cited in comment 5 was created in response to bug 366191.  
It represents the enforcement of a Mozilla policy, and as such is not a 
regression.  I believe it is not likely to be changed just because the 
Danish government's add-on to Firefox, which attempts to replace Firefox's 
own security module, is incompatible with it.  

This bug reports that, when the Danish government's add-on is added onto 
Firefox, Firefox starts to have problems.  As such, it appears to me to be 
a bug report about the Danish government's add-on, and not about Firefox.  
So, I recommend that this bug be marked as invalid, or WONTFIX - 
it's not a Firefox bug.

Comment 8

[Jesper Kristensen]

I first thought this was a bug in the addon. Julien Pierre suggesed that it might be a bug in Firefox. I read his comment as: "If the CA certificate lives in both Builtin Object Token and Crypto API Token, then the system could fail when it should not".

I don't know if the bug is in Firefox or TDC Digital Signatur. But no matter what, this is a PR problem for Firefox and a headache for us answering question in support forums.

Comment 9

[Julien Pierre]

The function I was thinking of is called PK11_GetAllSlotsForCert . It is a new API in NSS 3.12, which was recently released, so it is most likely not exposed from JS yet.

As I understand the Mozilla update policy, during the update process, it wants to check that :

a) the root cert is trusted
b) the root cert is one of the built-ins

In order to fully perform b), one would need to invoke that new function, look at the slot list that comes back, and see if any of them is the built-in roots slot.

Otherwise, the check may not work for some users, if they have modified some of the trust settings on that root (which creates a copy in the NSS softoken), or if they have another copy of the cert in another PKCS#11 module.

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

But comment 12 is all based on a supposition, a guess about what this Danish 
browser extension is actually doing.  We just don't know.  No one work has
commented in this bug seems to know.  The NSS team is not going to do the 
work to figure it out (I think I can confidently say that).  

If you can get a developer of that browser extension to participate here, or
can find someone who really understands PKCS#11 well and understands Danish 
(seems to be needed to work with that extension), then I'm personally willing
to help such a person figure out the problem, and make suggestions about how
to fix it.  But I'm not going to take the lead on fixing an extension that 
I didn't help create, is not open source, and that I can't install or use on
my own system (for various reasons: one of which is that I don't read Danish
and don't have a system that is localized for Danish).  I think that goes for
all the NSS team members too (but someone might surprise me).

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

I wrote:  No one work has commented".  That was supposed to say 
"No one WHO has commented".  Sorry.  

Comment 12

[Julien Pierre]

Nelson,

This comment 12 :) You were probably referring to an earlier comment.
Yes, I am just making some guesses here, based on the information available. And the NSS team does not maintain the Firefox updater code, so it is not up to us to fix it.

I think somebody should try the following test case :

1) download an old version of firefox
2) change some trust settings on the root cert that's used by mozilla updater (eg. some changes to S/MIME trust) . I don't know which root that is so I cannot perform this test. This will make an additional copy of the cert in the softoken.
3) try to do an update and see if it still works 

If it does, we won't learn anything new, but if it fails, it will show that the problem exists even with the softoken module, and would be easier to diagnose and fix.

I don't know enough about the Javascript / NSS bindings.

My guess is that the JS expression

issuer.tokenName

is equivalent in C to :

PK11_GetTokenName( ((CERTCertificate*) issuer)->slot) ;

That's the most logical thing I can think of. One of the JS experts should chime in to clarify.

If my guess is correct, then the test is guaranteed to pass only there is a single copy of the certificate in the Built-in Object Token . If any other copies have been made in other tokens, for example by virtue of modifying trust, then the value returned by PK11_GetTokenName will be non-deterministic. One needs to call PK11_GetAllSlotsForCert to examine the list of all slots that the cert exists on.

Comment 13

[Julien Pierre]

Given that bug 366191 was closed well before (one year) before NSS 3.12 was released, I think it's reasonable to say that the test for the certificate's storage token was not implemented using PK11_GetAllSlotsForCert, since it was not available until 3.12.

Before this function existed, it would have been difficult to account for the possibility of a cert existing in multiple tokens. One would need to do an extra call to PK11_TraverseCertsForSubjectInSlot, which might returned a CERTCertificate with a mismatched slot; then do a comparison of the DER certs, to ensure that the cert exists in the built-in slot. I doubt anyone implemented the check in JS that way.

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

No case has been made yet for why any trusted root CA cert needs to be 
duplicated in another module _UNLESS_ that module actually replaces NSS's
own modules, as comment 0 suggests it does.  

If this extension actually does replace NSS's own PKCS#11 modules, then 
that's the problem.  Mozilla has decided on a certain policy, about only
trusting certs in nssckbi.  That's their prerogative, and is not really a
bug.  

Comment 15

[Julien Pierre]

Nelson,

That is true. They should not replace the NSS nssckbi module as that will break the updater. Is this what they are doing or not ?

However, we should support them adding their PKCS#11 module, without removing nssckbi. And right now, I think that any 3rd party module that exposes certs that also exist in nssckbi may cause failures with the update code. That should be recognized as a bug in the update code. Perhaps it should be a separate bug from this one.

Comment 16

[Wan-Teh Chang]

The name of this PKCS #11 module is "Crypto API Token" (see
comment 5), which suggests that this module may be able to
provide to NSS the root CA certs in the Windows system cert
store.  If so, the root CA cert in question would be available
from both the "Builtin Object Token" and this "Crypto API Token".

Comment 17

[Jesper Kristensen]

(In reply to comment #15)
> That is true. They should not replace the NSS nssckbi module as that will break
> the updater. Is this what they are doing or not ?

I don't know. I wrote that in comment 0 because that is what it looks like from the user interface. In Tools->Options->Advanced->Encryption->View Certificates->Authorities the certificates are only listed once with the Crypto API Token name on it. The Builtin Object Token still exists, and a few certificates are labeled with that. So my comment 0 might be misleading.

If someone could help me with the code, I would be happy to help test.

(In reply to comment #12)
> My guess is that the JS expression
> 
> issuer.tokenName
> 
> is equivalent in C to :
> 
> PK11_GetTokenName( ((CERTCertificate*) issuer)->slot) ;
> 
> That's the most logical thing I can think of. One of the JS experts should
> chime in to clarify.

It is reading this attribute:
http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/public/nsIX509Cert.idl#121

> If my guess is correct, then the test is guaranteed to pass only there is a
> single copy of the certificate in the Built-in Object Token . If any other
> copies have been made in other tokens, for example by virtue of modifying
> trust, then the value returned by PK11_GetTokenName will be non-deterministic.
> One needs to call PK11_GetAllSlotsForCert to examine the list of all slots that
> the cert exists on.
> 

Can you point me to an .idl file where this method/attribute is defined?

Comment 18

[Julien Pierre]

Jesper,

Re: comment 17, this confirms what I was thinking. The built-in roots module is still present, but you added the CAPI module also. Some root certs now appear to exist in multiple tokens. In NSS, a cert structure can only have one "primary" slot/token (there is only one slot pointer in the CERTCertificate structure). And now some root certs that were only in the built-in token before appear to come from the CAPI token, even though they are also still in the built-ins also. This would definitely trigger the problem I mentioned. The mozilla updater code should check for all the slots that a root cert is in. I don't know if that's the entirety of the problem when the CAPI module is installed, but that is a problem that needs to be resolved as a pre-requisite. Marking this bug as confirmed.

Comment 19

[Nelson Bolyard (seldom reads bugmail)]

Julien, You're confirming this bug based on an inference drawn from 
the name of a module, which happens to resemble the name of some 
Microsoft Windows code.  I would not confirm the bug on that basis.  

It's certainly not true that "The mozilla updater code should check for 
all the slots that a root cert is in."  There is only one slot that it 
cares about.  It suffices for it to ask "is this cert in nssckbi?". 

Whether Mozilla's updater chooses to check to see if a cert is ALSO in the nssckbi module, or not, and whether the fact that it does not is a "bug" is 
up to the people responsible for that module to decide.  

I observe that One of the two persons most responsible for it has removed 
himself from the CC list for this bug.  I think we can infer from that 
something about the motivation to make any suggested changes.  

Comment 20

[Julien Pierre]

Nelson,

There has been plenty of evidence provided in this bug report already. Just how much more do you need ?

Re: CAPI, I am not inferring anything just from the name of the module. From comment 0 : "another module from TDC, which can read the user's
private certificate from MSCAPI".

Do you think that the mozilla updater code should work ONLY if the root cert they are using exists ONLY in nssckbi, and in no other token ?

Even when PSM's own UI lets you make another copy of a root cert from the built-in token, into the NSS softoken by changing the trust ? This will also break the updater process.

Even when PSM's own UI gives you the ability to install a 3rd party PKCS#11 module, which may contain another copy of root certs, as is the case here ?

I note that the original reporter of the bug has not gone away, and is still responding. I don't know the motivations of the other people. Even if the Danish government completely gave up on supporting firefox, the problem identified in this bug will continue to exist and manifest itself in other situations. I certainly think that warrants confirming the bug against the updater code.
It's their code, so they can decide if or how to fix it, but that doesn't make it any less of a bug.

Comment 21

[Wan-Teh Chang]

This is definitely a bug.  The bug was in the UNCONFIRMED state
because Jesper Kristensen doesn't have the 'canconfirm' privilege
in Bugzilla, not because he wasn't sure if it was a bug.

Comment 22

[Nelson Bolyard (seldom reads bugmail)]

It's a bug if the creators of the product believe the product is not working 
as they intended.  That is not a decision users can make.  

Comment 23

[Jesper Kristensen]

Attachment: Patch

I have created a patch based on Julien's suggestions. It seems to fix the problem. This is how I tested it:

Original reference build based on checkout of mozilla-central:
http://filer.mozilladanmark.dk/tdc-digisign/minefield-o.zip
Test result for the above build:
http://filer.mozilladanmark.dk/tdc-digisign/minefield-o.jpg

Patched test build based on the above checkout with the attached patch applied:
http://filer.mozilladanmark.dk/tdc-digisign/minefield-p.zip
Test result for the above build:
http://filer.mozilladanmark.dk/tdc-digisign/minefield-p.jpg

I have also been able to verify that the list returned by PK11_GetAllSlotsForCert contains the following: ["Builtin Object Token", "CryptoAPI token"]

Requesting review from Julien Pierre for the security/nss part, Wan-Teh Chang for the security/manager part and Robert Strong for the toolkit/mozapps/shared part.

Comment 24

[Julien Pierre]

Comment on attachment 332212 [details] [diff] [review]
Patch

1) pk11pub.h is part of NSS . You should not modify it . If it's missing PK11_GetAllSlotsForCert, then you must not have the right version of NSS in your mozilla tree. The fix is to move up to that version, not just to change one header. The version you want is NSS 3.12 . The trunk of firefox should have it.

2) the code in nsNSSCertificate.cpp seems OK.

3) I am not sure about the JS code. You should run it by someone who knows it.

I suggest you ask Kai Engert for a review since he is the PSM maintainer.

Comment 25

[Jesper Kristensen]

PK11_GetAllSlotsForCert is not part of pk11pub.h currently in mozilla-central. See http://mxr.mozilla.org/mozilla-central/source/security/nss/lib/pk11wrap/pk11pub.h

You say it will be there in NSS 3.12. Is there a bug for updating NSS to that version, which i could mark as a dependency, or should I file a new bug?

Comment 26

[Nick Thomas [:nthomas] (UTC+12)]

Bug 417635 is where NSS was imported into mozilla-central. According to comment 33 on that bug, the CVS tag used was NSS_3_12_RC4.

Comment 27

[Nelson Bolyard (seldom reads bugmail)]

In reply to comment 24,
Julien, 
http://mxr.mozilla.org/security/search?string=PK11_GetAllSlotsForCert
shows that the string PK11_GetAllSlotsForCert appears in exactly two places
in all of NSS & PSM in the trunk:
 - in the .c file where the function lives, and 
 - in the .def file, where the function is exported
but not in any .h file.  My own grep of the trunk confirms this.

Do you have evidence to the contrary, that PK11_GetAllSlotsForCert IS declared
in some NSS header file on the trunk?

Comment 28

[Julien Pierre]

Jasper, Nelson,

Sorry, it appears this function was never added to the .h file. That's a bug in NSS. I will open an NSS bug to fix this omission.

Comment 29

[Nelson Bolyard (seldom reads bugmail)]

Julien,
There already is such a bug.  It has a patch that adds this function to 
this header file.  It was given r+, and the bug is resolved/fixed, but 
it was not checked in. 

Don't file another bug.  Instead, reopen bug 129303, and just commit the
patch that is already approved there.

Comment 30

[Wan-Teh Chang]

Julien: When Bob checked in his patch for bug 129303, he
forgot to check in the change to pk11pub.h in attachment 203479 [details] [diff] [review].

Jesper: you should work around this NSS 3.12 bug by manually
declaring the function in security/manager/ssl/src/nsNSSCertificate.cpp.
Add these two lines to nsNSSCertificate.cpp:

// pk11pub.h in NSS 3.12 is missing the declaration of PK11_GetAllSlotsForCert.
PK11SlotList * PK11_GetAllSlotsForCert(CERTCertificate *c, void *arg);

Comment 31

[Jesper Kristensen]

Attachment: Patch v2

Comment 32

[Wan-Teh Chang]

Comment on attachment 332270 [details] [diff] [review]
Patch v2

Jesper: your patch is missing the new file nsIX509Cert4.idl.

Your patch looks correct to me, but Kai should review it in
details.

Comment 33

[Jesper Kristensen]

Attachment: Patch v2, missing file added

I forgot to run hg add before I ran hg diff.

Comment 34

[Daniel Veditz [:dveditz]]

Comment on attachment 332339 [details] [diff] [review]
Patch v2, missing file added

>     nsIX509Cert2.idl \
>     nsIX509Cert3.idl \
>+    nsIX509Cert4.idl \

Kai: why are these all separate files? if they're just minor variants/evolutions
and inherit couldn't they have all fit into one?

> NS_IMPL_THREADSAFE_ISUPPORTS7(nsNSSCertificate, nsIX509Cert,
>                                                 nsIX509Cert2,
>                                                 nsIX509Cert3,
>+                                                nsIX509Cert4,

Since you added one this needs to use the _ISUPPORTS8() macro.

>+NS_IMETHODIMP
>+nsNSSCertificate::GetAllTokenNames(PRUint32 *aLength, PRUnichar*** aTokenNames)
>+{
...
>+  slots = PK11_GetAllSlotsForCert(mCert, NULL);
>+  if (!slots)
>+    return NS_ERROR_FAILURE;
>+
>+  /* read the token names from slots */
>+  PK11SlotListElement *le;
>+
>+  *aLength = 0;

Why not move "*aLength = 0;" before getting slots, so just in case some
idiot doesn't check the error they hopefully still won't run off into
empty space.

>+  for (le = slots->head; le; le = le->next) {
>+    ++(*aLength);
>+  }
>+
>+  *aTokenNames = (PRUnichar **)nsMemory::Alloc(sizeof(PRUnichar *) * (*aLength));

I don't know NSS -- is PK11_GetAllSlotsForCert() guaranteed to return null
if there are no slots rather than a valid slots struct with a null slots->head?
The latter leads to alloc(0) which will return non-null depending on the platform.
I guess you wouldn't write into it, but you will return NS_OK and 0 entries in
the array which might confuse someone.

If you initialize *aTokenNames to 0 when you initialize aLength you could
put the alloc inside an if (*aLength) and then the "if (!*aTokenNames)" check
would return an error in both cases.

>+  PRUint32 iToken;
>+  for (le = slots->head, iToken = 0; le; le = le->next, ++iToken) {
>+    char *token = PK11_GetTokenName(le->slot);
>+    (*aTokenNames)[iToken] = ToNewUnicode(NS_ConvertUTF8toUTF16(token));
>+  }

Who frees these? If the caller were C/C++ the caller would have to do it, does XPConnect magically take care of this for JS callers?

>-  if (!issuer || issuer.tokenName != "Builtin Object Token")

The original reason for this check was that the old way of handling invalid/self-signed certs was to add them to the database as trusted. A malicious person could create a marginally interesting site with a cert and lure people to accept it, thereby also approving it for any other names included in the cert. This check was meant to defeat that by requiring the AUS/AMO update certs to chain to a built-in root. It was a total hack.

It also means that enterprises who want to set up their own internal update service (to control roll out timing) must use a commercial cert, they can't use one from a corporate CA they probably set up for internal sites (assuming they aren't building their own Firefox, just controlling distribution).

Does the cert exception handling in Firefox 3 eliminate this risk so we can use normal cert validity checks and get rid of this code altogether?

>+  issuer = issuer.QueryInterface(Ci.nsIX509Cert4);
>+  var tokenNames = issuer.getAllTokenNames({});

As mentioned, I'm a bit worried about who frees these. I notice the nsIX509Cert has both getEmailAddresses() and containsEmailAddress() -- maybe because of this? If it turns out we're leaking that might be an alternate approach.

r- for a new patch with the correct macro, I don't really know the answers on the rest (and didn't really review the PSM code)

Comment 35

[Jesper Kristensen]

(In reply to comment #34)
> (From update of attachment 332339 [details] [diff] [review])
> >     nsIX509Cert2.idl \
> >     nsIX509Cert3.idl \
> >+    nsIX509Cert4.idl \
> 
> Kai: why are these all separate files? if they're just minor
> variants/evolutions
> and inherit couldn't they have all fit into one?

I have heard that changing idl files is not good for compatibility, if this is true, changing an idl would make my chances of seeing this bug fixed in Firefox 3.0.x and Thunderbord 2.0.0.x smaller.

> I don't know NSS -- is PK11_GetAllSlotsForCert() guaranteed to return null
> if there are no slots rather than a valid slots struct with a null slots->head?
> The latter leads to alloc(0) which will return non-null depending on the
> platform.
> I guess you wouldn't write into it, but you will return NS_OK and 0 entries in
> the array which might confuse someone.

Do you suggest returning null instead of an empty array? I don't know c++ very well, but from a JavaScript point of view, that would be strange.

> Who frees these? If the caller were C/C++ the caller would have to do it, does
> XPConnect magically take care of this for JS callers?
> >+  issuer = issuer.QueryInterface(Ci.nsIX509Cert4);
> >+  var tokenNames = issuer.getAllTokenNames({});
> As mentioned, I'm a bit worried about who frees these. I notice the nsIX509Cert
> has both getEmailAddresses() and containsEmailAddress() -- maybe because of
> this? If it turns out we're leaking that might be an alternate approach.

The code should be the same as for getEmailAddresses, so if that does not leak, I guess this would not leak either.

> 
> >-  if (!issuer || issuer.tokenName != "Builtin Object Token")
> 
> The original reason for this check was that the old way of handling
> invalid/self-signed certs was to add them to the database as trusted. A
> malicious person could create a marginally interesting site with a cert and
> lure people to accept it, thereby also approving it for any other names
> included in the cert. This check was meant to defeat that by requiring the
> AUS/AMO update certs to chain to a built-in root. It was a total hack.
> 
> It also means that enterprises who want to set up their own internal update
> service (to control roll out timing) must use a commercial cert, they can't use
> one from a corporate CA they probably set up for internal sites (assuming they
> aren't building their own Firefox, just controlling distribution).
> 
> Does the cert exception handling in Firefox 3 eliminate this risk so we can use
> normal cert validity checks and get rid of this code altogether?

That would make things much simpler. I have no idea if that would work.

Comment 36

[Daniel Veditz [:dveditz]]

(In reply to comment #35)
> I have heard that changing idl files is not good for compatibility, if this
> is true, changing an idl would make my chances of seeing this bug fixed in
> Firefox 3.0.x and Thunderbord 2.0.0.x smaller.

You are absolutely right about _interfaces_, but multiple (related) interfaces can happily live in the same .idl file. Here's the ugliness we've forced on 1.8 branch developers, where you can see there were 3 separate extensions to nsIDocShell as part of various security fixes that left the original nsIDocShell alone for compatibility: http://mxr.mozilla.org/mozilla1.8/source/docshell/base/nsIDocShell.idl#415

But it looks like in PSM Kai is putting separate versions in separate files so "when in Rome...".

> Do you suggest returning null instead of an empty array? I don't know c++ very
> well, but from a JavaScript point of view, that would be strange.

null is a perfectly fine JS return value, but I don't care much as long as the method is documented so future users don't have to guess.

> The code should be the same as for getEmailAddresses, so if that does not
> leak, I guess this would not leak either.

Who knows if that one leaks? 1) no one seems to be using it, 2) even if they did if they were C++ callers that wouldn't tell you anything about how JS was supposed to deal with it, and 3) it might leak like a sieve anyway because that code long predates our big mem-leak push and is rare functionality that our leak testing probably doesn't cover.

> > Does the cert exception handling in Firefox 3 eliminate this risk so we can
> > use normal cert validity checks and get rid of this code altogether?
> 
> That would make things much simpler. I have no idea if that would work.

It's probably not a good idea. I can imagine a fair number of people lured into clicking through bad certs in order to look at "interesting" content on hijacked AUS ("what's that?") or AMO, only intending to look around and not do anything without realizing they've just compromised the update mechanism. In any case doing something like that would require a multi-party security review adding more delay so in the short term you still want this "bugfix" approach.

Comment 37

[Jesper Kristensen]

(In reply to comment #36)
> null is a perfectly fine JS return value, but I don't care much as long as the
> method is documented so future users don't have to guess.

I will add a @return to the idl

> Who knows if that one leaks? 1) no one seems to be using it, 2) even if they
> did if they were C++ callers that wouldn't tell you anything about how JS was
> supposed to deal with it, and 3) it might leak like a sieve anyway because that
> code long predates our big mem-leak push and is rare functionality that our
> leak testing probably doesn't cover.

I searched through the code for methods implemented in c++ returning an array of strings. I found a lot in the spell checker and in a few other places. Using that as a base, it seems I need to add an out of memory check for ToNewUnicode.

Comment 38

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 332339 [details] [diff] [review]
Patch v2, missing file added

Here's an unsolicited review comment on this patch.
In the patch to file security/manager/ssl/src/nsNSSCertificate.cpp 
we see:

> #include "pk11func.h"
>+
>+// pk11pub.h in NSS 3.12 is missing the declaration of PK11_GetAllSlotsForCert.
>+PK11SlotList * PK11_GetAllSlotsForCert(CERTCertificate *c, void *arg);
>+
> #include "certdb.h"

NSS 3.12.1 has the missing function declaration added into pk11pub.h.
That was bug 449105.

Firefox 3.0.2 RTM will be built with NSS 3.12.1 (I believe, see bug 450646),
so I'm not sure you really want this declaration in nsNSSCertificate.cpp.

Comment 39

[Jesper Kristensen]

Attachment: Patch v3

Fixed: Handling of certs which does not exist in any slot/token, out of memory handling and use of NS_IMPL_THREADSAFE_ISUPPORTS7/8

I don't know what to set the review flag to, so I leave it blank.

Comment 40

[Dave Townsend [:mossop]]

Comment on attachment 334240 [details] [diff] [review]
Patch v3

You should check the module owners list to find reviewers

Comment 41

[Kai Engert (:KaiE:)]

(In reply to comment #34)
> (From update of attachment 332339 [details] [diff] [review])
> >     nsIX509Cert2.idl \
> >     nsIX509Cert3.idl \
> >+    nsIX509Cert4.idl \
> 
> Kai: why are these all separate files? if they're just minor
> variants/evolutions
> and inherit couldn't they have all fit into one?

It was necessary to produce these separate versions because of the Mozilla 1.8 branch.

At some point it was necessary to add new functionality to the Mozilla 1.8 branch, but the trunk already contained nsIX509Cert2.idl

The solution was to create nsIX509Cert3.idl and add it to both trunk and 1.8 branch.

The 1.8 branch does not have nsIX509Cert2.idl

Only nsIX509Cert.idl is frozen, we are free to change the other files.


There is no need to introduce nsIX509Cert4.idl

I propose to add your new attributes to nsIX509Cert2.idl
If you do, please give the interface a new UUID.

Comment 42

[Kai Engert (:KaiE:)]

@dveditz

Also adding Johnath to cc list, as he was probably involved in the hack that Dan described.


(In reply to comment #34)

(sorry I haven't yet read the later comments)

> >-  if (!issuer || issuer.tokenName != "Builtin Object Token")
> 
> The original reason for this check was that the old way of handling
> invalid/self-signed certs was to add them to the database as trusted. A
> malicious person could create a marginally interesting site with a cert and
> lure people to accept it, thereby also approving it for any other names
> included in the cert. This check was meant to defeat that by requiring the
> AUS/AMO update certs to chain to a built-in root. It was a total hack.
> 
> It also means that enterprises who want to set up their own internal update
> service (to control roll out timing) must use a commercial cert, they can't use
> one from a corporate CA they probably set up for internal sites (assuming they
> aren't building their own Firefox, just controlling distribution).
> 
> Does the cert exception handling in Firefox 3 eliminate this risk so we can use
> normal cert validity checks and get rid of this code altogether?


Firefox 3 has the following behavior:
- if a mismatch occurs, it will display to full set of names
  contained in the certificate
- when adding an exception, it will bind the exception to a single
  name

It is no longer possible to add an old-style exception with the user-assisted dialogs (no more add as you go).

With Firefox 3, the only option to add an old-style exception is using certificate manager and doing a manual import from a local file, then editing trust.

However, for users who have migrated from Firefox 2 (or earlier) and still have old-style exceptions, those exceptions are still valid (for any hostnames).

Comment 43

[Kai Engert (:KaiE:)]

I just wrote:

(In reply to comment #41)
> The solution was to create nsIX509Cert3.idl and add it to both trunk and 1.8
> branch.
> 
> The 1.8 branch does not have nsIX509Cert2.idl
> 
> I propose to add your new attributes to nsIX509Cert2.idl
> If you do, please give the interface a new UUID.


but then I read:


(In reply to comment #36)
> > I have heard that changing idl files is not good for compatibility, if this
> > is true, changing an idl would make my chances of seeing this bug fixed in
> > Firefox 3.0.x and Thunderbord 2.0.0.x smaller.
> 
> You are absolutely right about _interfaces_, but multiple (related) interfaces
> can happily live in the same .idl file. Here's the ugliness we've forced on 1.8
> branch developers, where you can see there were 3 separate extensions to
> nsIDocShell as part of various security fixes that left the original
> nsIDocShell alone for compatibility:
> http://mxr.mozilla.org/mozilla1.8/source/docshell/base/nsIDocShell.idl#415


Given that you want to target multiple branches, my new recommendation is to add your new function to nsIX509Cert3.idl which lives on all those branches.


> But it looks like in PSM Kai is putting separate versions in separate files so
> "when in Rome...".

I was given this preference at some time in the past by another developer and I like it in general, but I've diverted from that rule myself.

I would like to avoid additional interfaces, unless necessary.

Comment 44

[Kai Engert (:KaiE:)]

Comment on attachment 334240 [details] [diff] [review]
Patch v3

>+    nsIX509Cert4.idl \

not necessary


>diff --git a/security/manager/ssl/public/nsIX509Cert4.idl b/security/manager/ssl/public/nsIX509Cert4.idl
>new file mode 100644

not necessary


>+[scriptable, uuid(9cebfeb5-3464-42ac-be49-6fd491c3fc3d)]

you can use that as a new UUID for nsIX509Cert3


>+    /**
>+     * Human readable names identifying all hardware or
>+     * software tokens the certificate is stored on.
>+     *
>+     * @return An array containing the names of all tokens the certificate is stored on

nit: you might want to avoid very long lines in IDL files (limit to 80)


>+     */
>+    void getAllTokenNames(out unsigned long length, 
>+                         [retval, array, size_is(length)] out wstring tokenNames);

please move to nsIX509Cert3


> 
>-NS_IMPL_THREADSAFE_ISUPPORTS7(nsNSSCertificate, nsIX509Cert,
>+NS_IMPL_THREADSAFE_ISUPPORTS8(nsNSSCertificate, nsIX509Cert,
>                                                 nsIX509Cert2,
>                                                 nsIX509Cert3,
>+                                                nsIX509Cert4,
>                                                 nsIIdentityInfo,
>                                                 nsISMimeCert,
>                                                 nsISerializable,
>                                                 nsIClassInfo)

not necessary


>diff --git a/security/manager/ssl/src/nsNSSCertificate.h b/security/manager/ssl/src/nsNSSCertificate.h

all changes to this file not necessary


>diff --git a/toolkit/mozapps/shared/src/badCertHandler.js b/toolkit/mozapps/shared/src/badCertHandler.js


I am not a module owner to that function.
I propose that dveditz or johnath review it, or whoever touched this code last.
I will give you comments though:


>-  if (!issuer || issuer.tokenName != "Builtin Object Token")
>+  if (!issuer)
>+    throw "cert issuer is not built-in";
>+
>+  issuer = issuer.QueryInterface(Ci.nsIX509Cert4);

obviously, please change to 3


>+  var tokenNames = issuer.getAllTokenNames({});
>+
>+  function isBuiltinToken(tokenName)
>+    tokenName == "Builtin Object Token";

I personally avoid defining functions inside another function, I'd recommend, please move it to the top level.

Also, please wrap the function body in {}


>+
>+  if (!tokenNames.some(isBuiltinToken))
>     throw "cert issuer is not built-in";
> }

Comment 45

[Kai Engert (:KaiE:)]

Please document the behaviour of the output values of your function in the failure scenario.

For example, you set the length output variable and keep it, even if a failure occurs later in your code.

It should be clear which return variable declares success or failure of the function, and when the length result can be relied on (or is undefined).

You could write something like:

@param length On success, the number of entries in the returned array.

@return On success, an array containing the names of all tokens 
        the certificate is stored on.
        On failure the function returns NULL.

Comment 46

[Kai Engert (:KaiE:)]

>+      NS_FREE_XPCOM_ALLOCATED_POINTER_ARRAY(iToken, *aTokenNames);

Is this call sufficient, or should you add |*aTokenNames=0;| ?
Yes, it's true that the JS caller will run into an exception, but still, the caller has obtained a dangling pointer to some output variable. I'd prefer if the out parameter gets set to zero.


completely optional proposal:

You can avoid using PK11_FreeSlotList three times in your code, if you do the following instead, which uses an automatic cleanup at the time the scope is left:

at the top of the file, next to the other cleaners, declare 
NSSCleanupAutoPtrClass(PK11SlotList, PK11_FreeSlotList)

and use
+  PK11SlotList *slots = NULL;
+  PK11SlotListCleaner slotCleaner(slots);

Comment 47

[Jesper Kristensen]

(In reply to comment #44)
> please move to nsIX509Cert3 [and related]

Ok, I will do that.

> >+  var tokenNames = issuer.getAllTokenNames({});
> >+
> >+  function isBuiltinToken(tokenName)
> >+    tokenName == "Builtin Object Token";
> 
> I personally avoid defining functions inside another function, I'd recommend,
> please move it to the top level.
> 
> Also, please wrap the function body in {}

Ok

(In reply to comment #45)
> You could write something like:
> 
> @param length On success, the number of entries in the returned array.

OK

> 
> @return On success, an array containing the names of all tokens 
>         the certificate is stored on.
>         On failure the function returns NULL.

The failure note might be misleading. In JS, the function throws an exception and in C++ the function returns an error code. Also NULL is the same as the empty array, so it might return NULL on success.

(In reply to comment #46)
I will do both, and I will also reset aLength to 0

Comment 48

[Jesper Kristensen]

Attachment: Patch v4

New patch addressing kaie's comments except I tweaked the suggested idl documentation.

Comment 49

[Jesper Kristensen]

Comment on attachment 337845 [details] [diff] [review]
Patch v4

(In reply to comment #44)
> I am not a module owner to that function.
> I propose that dveditz or johnath review it, or whoever touched this code last.

Adding dveditz then as it seems he created the code

Comment 50

[Jesper Kristensen]

In case you are interested in reproducing, I finally found a way of doing so without applying for a certificate:

Steps to reproduce:
1. Download http://temp.jesperkristensen.dk/mozilla/capi_pkcs11.dll
2. Add the downloaded file in Tools -> Options -> Advanced -> Encryption -> Security Modules -> Load (My translation - English names might differ)
3. Restart Firefox (Before the restart of Firefox, Builtin Object Token takes preference; after restart, CryptoAPI token takes preference)
4. Click Help -> Check for updates

Comment 51

[Jesper Kristensen]

Can you give any indication on when you are going to review this? For the users which experience this bug, some features of Firefox are completely broken (application update and addon update), and some are partly broken (addon installation). I get more and more private e-mails on this issue, and we continue to get some questions about this in our support forum even through detailed information is available only one click away on our forum entry page. This is the only of our frequently asked questions, where I cannot give a good answer when people ask. I can only explain the problem, I cannot give a solution to them.

Comment 52

[Daniel Veditz [:dveditz]]

Comment on attachment 337845 [details] [diff] [review]
Patch v4

r=dveditz for the badCertHandler.js change. Kai needs to r+ the PSM part.

Comment 53

[Kai Engert (:KaiE:)]

Comment on attachment 337845 [details] [diff] [review]
Patch v4

I'd like to propose some improvements:


>+  /**
>+   * Human readable names identifying all hardware or
>+   * software tokens the certificate is stored on.
>+   *
>+   * @param On success, the number of entries in the returned array.
>+   * @return On success, an array containing the names of all tokens 
>+   *         the certificate is stored on (may be empty).
>+   *         On failure the function throws/returns an error.
>+   */
>+  void getAllTokenNames(out unsigned long length, [retval, array,
>+                        size_is(length)] out wstring tokenNames);

I'd prefer each parameter on its own line.


>-  if (!issuer || issuer.tokenName != "Builtin Object Token")
>+  if (!issuer)
>     throw "cert issuer is not built-in";
>+
>+  issuer = issuer.QueryInterface(Ci.nsIX509Cert3);
>+  var tokenNames = issuer.getAllTokenNames({});
>+
>+  if (!tokenNames.some(isBuiltinToken))
>+    throw "cert issuer is not built-in";
>+}

Could you declare a variable 
  var errorstring = "cert issuer is not built-in";
and use
  throw errorstring;
? It would avoid having the string twice.

r=kaie and my apologies for the delay

Comment 54

[Jesper Kristensen]

Ok, I will upload a patch with these two changes when I get home from Barcelona, as I don't have a computer with build setup with me here.

Comment 55

[Jesper Kristensen]

Attachment: Patch v4 with the two changes from kaie [Checkin: Comment 57]

Comment 56

[Kai Engert (:KaiE:)]

Comment on attachment 344902 [details] [diff] [review]
Patch v4 with the two changes from kaie
[Checkin: Comment 57]

Thanks for the new patch.


>+  /**
>+   * Human readable names identifying all hardware or
>+   * software tokens the certificate is stored on.
>+   *
>+   * @param On success, the number of entries in the returned array.

I just see, you forgot to add the parameter name "length" in the comment.
please fix this when you check in. Thanks.

Comment 57

[Serge Gautherie (:sgautherie)]

Comment on attachment 344902 [details] [diff] [review]
Patch v4 with the two changes from kaie
[Checkin: Comment 57]

http://hg.mozilla.org/mozilla-central/rev/102069a4c1b8

Comment 58

[Jesper Kristensen]

Attachment: Missing "length" name

(In reply to comment #56)
> I just see, you forgot to add the parameter name "length" in the comment.
> please fix this when you check in. Thanks.

Oh, stupid mistake.

Too late. It is already in. Should I mark this for checkin-needed?

Comment 59

[Dave Townsend [:mossop]]

I suspect there is still a problem here when the xpi itself is served over https since the CheckCert implementation in nsXPinstallManager wasn't changed in the same way, or am I missing something?

Comment 60

[Jesper Kristensen]

(In reply to comment #59)

You are right. I did not know there were copies of that function.

Steps to Reproduce:

1. Use new trunk build
2. Add cspi_pkcs11.dll security module
3. Restart and confirm presence of security module
4. Download http://download.mcafee.com/products/sa/firefox/safe.xpi
5. Modify the xpi: decrease em:version in install.rdf and remove META-INF
6. Install changed SiteAdvisor xpi
7. Restart to complete installation
8. Check for updates
9. New update is found
10. Install the update

Expected: Installation succeeded

Actual: The following dialog:

<<<<
Minefield could not install the file at 

https://sadownload.mcafee.com/products/sa/firefox/safe.xpi?upgrade

because: Download error
-228
>>>>

11. Remove security module
12. Retry update
13. Success

Comment 61

[Jesper Kristensen]

Attachment: Second patch v1 [Checkin: Comment 64 & 69]

Comment 62

[Daniel Veditz [:dveditz]]

Comment on attachment 349981 [details] [diff] [review]
Second patch v1
[Checkin: Comment 64 & 69]

r=dveditz

There may not be any need for this code anymore. Starting with Firefox 3 SSL cert exceptions do not get installed in the cert database so there won't be any confusion between an exception and an installed root. The current code prevents the reasonable case of an institution with its own root cert that is pre-installed by all students or employees. As long as the XPI or update site certs validate to an installed root and not an exception that's all we really want.

Comment 63

[Jesper Kristensen]

(In reply to comment #62)
> There may not be any need for this code anymore. Starting with Firefox 3 SSL
> cert exceptions do not get installed in the cert database so there won't be any
> confusion between an exception and an installed root. The current code prevents
> the reasonable case of an institution with its own root cert that is
> pre-installed by all students or employees. As long as the XPI or update site
> certs validate to an installed root and not an exception that's all we really
> want.

1: See comment 42 (last sentence)
2: That is another bug

Comment 64

[Serge Gautherie (:sgautherie)]

Comment on attachment 349981 [details] [diff] [review]
Second patch v1
[Checkin: Comment 64 & 69]

http://hg.mozilla.org/mozilla-central/rev/7482683c532a

Comment 65

[Serge Gautherie (:sgautherie)]

(In reply to comment #64)
> (From update of attachment 349981 [details] [diff] [review])

(I guess you'll want this in 1.9.1 too...)

Comment 66

[Jesper Kristensen]

(In reply to comment #65)
> (I guess you'll want this in 1.9.1 too...)

Yes. How should I do that? Should I just set approval1.9.1 to a question mark for the attachment? I have just checked with the latest nightly from mozilla-central that my Steps To Reproduce from comment 60 does not give the failure anymore.

An off-topic question: I have filed bug 465528 which is probably filed in the wrong component, but I don't know where to put it. It is related to SSL certificates and I guess some people cc'ed to this bug would know where that bug belongs, so if you could tell me, I would be happy.

Comment 67

[Jesper Kristensen]

Comment on attachment 349981 [details] [diff] [review]
Second patch v1
[Checkin: Comment 64 & 69]

No answer. Then I will just try and see if it works.

Comment 68

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 349981 [details] [diff] [review]
Second patch v1
[Checkin: Comment 64 & 69]

a191=beltzner

Comment 69

[Serge Gautherie (:sgautherie)]

Comment on attachment 349981 [details] [diff] [review]
Second patch v1
[Checkin: Comment 64 & 69]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/b884fd68ff49

Comment 70

[Serge Gautherie (:sgautherie)]

(In reply to comment #66)
> Should I just set approval1.9.1 to a question mark for the attachment?

Yes (after verifying the patch applies to branch).

(In reply to comment #67)
> No answer.

Sorry, I'm not cc'ed to this bug.