406755 - EV certs not recognized as EV with some cross-certification scenarios

Status: RESOLVED FIXED

(Core :: Security: PSM, defect, P1)

Description

[Rob Stradling]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b2pre) Gecko/2007120410 Minefield/3.0b2pre
Build Identifier: Trunk

Most EV CAs (who have been accepted as such by Microsoft already, and anticipate being accepted by Mozilla too) have issued a "New" Root CA Certificate, to which the EV SSL Certificates they issue are chained.  Each "New" Root CA Certificate is typically cross-certified by at least one "Legacy" Root CA Certificate.
(The CAs needed to do this to ensure that WinXP/IE7 would display its EV UI indicators).

Case A: The "New" and "Legacy" Root CA Certificates belong to the same CA, and are associated with the same EV Policy OID(s) (e.g. VeriSign, Entrust, Comodo, etc).

Case B: The "New" Root CA Certificate belongs to a different CA (e.g. DigiCert, SecureTrust, etc) than the "Legacy" Root CA Certificate (e.g. Entrust).

Bug 404592 has temporarily enabled EV testing for VeriSign's EV SSL Certificates. This only allows testing of one Case A example.
I've done my own testing by adding a few Case B examples to myTrustedEVInfos[] (and adding the appropriate "New" Case B Root CA Certificates to the Certificate Manager "Authorities"), and it looks like *all* Case B CAs would have problems with the current PSM/NSS code.

Problem 1:
nsNSSCertificate::hasValidEVOidTag() calls CERT_PKIXVerifyCert(), which selects and returns 1 certificate chain. hasValidEVOidTag() then checks if there is an EV Policy OID match between the site certificate's Certificate Policies extension and the entry in the myTrustedEVInfos[] structure for that chain's Root CA Certificate.
I've observed that CERT_PKIXVerifyCert() often prefers to pick a "Legacy" certificate chain instead of a "New" certificate chain, even though the appropriate EV Policy OID is only associated with the "New" Root CA Certificate. This choice of certificate chain prevents the EV UI indicators from being displayed.

Problem 2:
Once CERT_PKIXVerifyCert() has chosen a "Legacy" certificate chain (Problem 1), it appears that this choice is cached (for the rest of the browser session?) and reused for all other sites that use EV certs from the same Issuer. None of these sites will get the EV UI treatment.
(The caching can sometimes help though: if CERT_PKIXVerifyCert() picks a "New" certificate chain, this is cached and reused for all other sites certified by that Issuer).

Problem 3:
The "Legacy"->"New" cross-certificate sometimes gets installed automatically in the Certificate Manager "Authorities" (is this an Authority Info Access "caIssuers" lookup?)
When this happens, I've spotted at least one occasion when CERT_PKIXVerifyCert() was even less likely to pick a "New" certificate chain.


Reproducible: Always

Steps to Reproduce:
1. Add some Case B Root CA Certificates to myTrustedEVInfos[].
2. Add those Case B Root CA Certificates to the Certificate Manager "Authorities".
3. Browse to any HTTPS URL that uses an EV SSL Certificate from a Case B CA, for which the server is configured to return the "Legacy" certificate chain.
Actual Results:  
EV UI indicators not displayed.

Expected Results:  
EV UI indicators displayed.

Here are some example URLs (with Case B EV certs) that exhibit problems:
  https://www.digicert.com
  https://www.securetrust.com
  https://www.trustwave.com

Comment 1

[Rob Stradling]

Attachment: Patch for testing the example URLs

The attached .diff adds 3 EV Policy OID entries to myTrustedEVInfos[]. This is for testing the example URLs I quoted.

Comment 2

[Rob Stradling]

Of course, you also need to set "NSS_EV_TEST_HACK=USE_PKIX" (as per Bug 404592 Comment #2) to test this.

Also, when Problem 3 occurs, even setting "NSS_EV_TEST_HACK=EV_HACK_INSECURE_OID_CHECK" is not enough to make the EV UI indicators show.

Comment 3

[Kai Engert [:KaiE:]]

> Problem 1:
> nsNSSCertificate::hasValidEVOidTag() calls CERT_PKIXVerifyCert(), which selects
> and returns 1 certificate chain. hasValidEVOidTag() then checks if there is an
> EV Policy OID match between the site certificate's Certificate Policies
> extension and the entry in the myTrustedEVInfos[] structure for that chain's
> Root CA Certificate.
> I've observed that CERT_PKIXVerifyCert() often prefers to pick a "Legacy"
> certificate chain instead of a "New" certificate chain, even though the
> appropriate EV Policy OID is only associated with the "New" Root CA
> Certificate. This choice of certificate chain prevents the EV UI indicators
> from being displayed.

Rob, in your test case, did PSM pass the correct policy OID to NSS?

That OID is designed to be a hint for NSS to prefer the chain going to the "New" cert.

If NSS prefers the root without that OID, I'd call that a bug in NSS.


> The "Legacy"->"New" cross-certificate sometimes gets installed automatically in
> the Certificate Manager "Authorities" (is this an Authority Info Access
> "caIssuers" lookup?)

AFAIK NSS doesn't do AIA lookups (yet).

You probably have visited a web server that sent the valid intermediate CA certs as part of the server cert chain during the SSL handshake, and PSM permanently caches such certs by importing them into NSS' cert db.

Comment 4

[Rob Stradling]

> Rob, in your test case, did PSM pass the correct policy OID to NSS?

Yes.

> That OID is designed to be a hint for NSS to prefer the chain going to
> the "New" cert.

I'd guessed that this was what...
  CERTValInParam cvin[3];
  cvin[0].type = cert_pi_policyOID;
  cvin[0].value.arraySize = 1; 
  cvin[0].value.array.oids = &oid_tag;
...in hasValidEVOidTag() was trying to achieve.

> If NSS prefers the root without that OID, I'd call that a bug in NSS.

I'm not convinced it's that simple.

Does NSS look at myTrustedEVInfos[] to see which OIDs are associated with which Roots?  I don't think it does.  I think NSS only looks at Policy OIDs within the Certificate Policies extensions of each certificate in each candidate certificate chain.

For all the test cases I mentioned, there is no Certificate Policies extension in any of the "Legacy" Root CA Certificates, "New" Root CA Certificates or "Legacy"->"New" cross-certificates.
Also, in a number of other cases I haven't mentioned, "anyPolicy" is included in the Certificate Policies extension instead of a CA-specific EV Policy OID.

If my understanding is correct, this must mean that NSS does not have enough information to know that it should choose the "New" chain.

I can imagine that to fix this problem might require changes to both PSM and NSS.

> AFAIK NSS doesn't do AIA lookups (yet).
> You probably have visited a web server that sent the valid intermediate CA
> certs as part of the server cert chain during the SSL handshake, and PSM
> permanently caches such certs by importing them into NSS' cert db.

That's probably it. Thanks.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

In comment 4, Rob Stradling wrote:
> If my understanding is correct, this must mean that NSS does not have 
> enough information to know that it should choose the "New" chain.

Rob, your analysis is exactly correct. 

Some CABForum members believe that RFC 3280 prohibits the inclusion of 
policy extensions in root CA certs.  IIRC, the Forum's guidelines 
prohibit or discourage the inclusion of policy extensions in root certs.
Consequently, none of the new EV roots have policy extensions, IINM.

We have several options of how to deal with the absence of policy 
information in the root certs, and are working on a resolution.
One of those options is to put the legacy roots into PSM's table of 
"trusted EV info" and associate EV policy OIDs with them, also. 
As things stand now, I believe that potential solution is the one most 
likely to be chosen, but that's just my opinion.

Comment 6

[Rob Stradling]

> Rob, your analysis is exactly correct.

Thanks Nelson.

> Some CABForum members believe that RFC 3280 prohibits the inclusion of
> policy extensions in root CA certs.  IIRC, the Forum's guidelines
> prohibit or discourage the inclusion of policy extensions in root certs.
> Consequently, none of the new EV roots have policy extensions, IINM.

Yes, I haven't seen a Certificate Policies extension in any of the new EV roots either...but I doubt that this is primarily because some CABForum members believe it to be prohibited or discouraged.
Many CAs prefer to have generic trust anchors rather than purpose-specific trust anchors. They may want to use their "new EV roots" for non-EV(-SSL) purposes in the future, using Policy OIDs they may have not even defined yet.
Therefore, IMHO, it's entirely legitimate (and I would even say sensible) to have omitted the Certificate Policies extension from these Root Certificates.

> We have several options of how to deal with the absence of policy information
> in the root certs, and are working on a resolution.

Can I assume that this is a blocker for FF3 ?

> One of those options is to put the legacy roots into PSM's table of
> "trusted EV info" and associate EV policy OIDs with them, also.
> As things stand now, I believe that potential solution is the one most
> likely to be chosen, but that's just my opinion.

How would that fix the problem for the test cases I've mentioned?

To be specific, are you suggesting that PSM's myTrustedEVInfos[] would need to associate the "Legacy" Entrust Root Certificate with the EV Policy OIDs of Entrust, SecureTrust and DigiCert?

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Rob, When I test the 3 URLs you supplied in comment 0 with a recent trunk
browser (unpatched), I find that the Digicert URL chains up to Digicert's
new EV root.  So, I will ignore Digicert from the rest of this discussion
since it seems not to be relevant to the cases and problems you enumerated
in comment 0.

The other two URLs you supplied seem to have server certs issued by the 
same intermediate CA which, in both cases, chains up to an Entrust root.
Both of the server certs seem to bear the same policy OID, namely,
2.16.840.1.114404.1.1.2.4.1 which appears to be the OID associated with 
"CN=SecureTrust CA,O=SecureTrust Corporation,C=US" in your patch.

So, in the examples given in comment 0 (ignoring Digicert), there seem to 
be one policy OID belonging to SecureTrust that chains up to Entrust.  In 
that light, I think your question in comment 6 would be: "Are you suggesting 
that the SecureTrust policy OID would also be associated with Entrust's root?"

My answer is: that is one of the alternatives we are considering, yes. 
That alternative would associate multiple EV policy OIDs with Entrust's root, 
and would associate SecureTrust's policy OID with multiple Roots.  BTW,
it is my understanding that this is exactly what happens with IE7 in Vista.

Another alternative we may consider is to, in essence, promote the 
Entrust-issued intermediate CA cert for SecureTrust to be treated as a 
root CA cert in its own right, for purposes of determining EV path validity, 
(I realize this is heresy :) and associate SecureTrust's EV policy OID with 
that cert (also), rather than with Entrust's root.  This is technically easy
but perhaps politically more difficult.  There are pros and cons to each of those two approaches, and they may not be the only ones to be considered.  

Comment 8

[Rob Stradling]

> Rob, When I test the 3 URLs you supplied in comment 0 with a recent trunk
> browser (unpatched), I find that the Digicert URL chains up to Digicert's
> new EV root.

When I browse to https://www.digicert.com in a recent trunk browser, "Page Info->View Certificate" does show a chain up to the "DigiCert High Assurance EV Root CA". I presume that this is what you mean by "chains up to Digicert's new EV root".

> So, I will ignore Digicert from the rest of this discussion since it seems
> not to be relevant to the cases and problems you enumerated in comment 0.

"openssl s_client -connect www.digicert.com:443" tells me that DigiCert's server is returning the Entrust->DigiCert cross-certificate. This (apparently) causes hasValidEVOidTag()'s call to CERT_PKIXVerifyCert() to pick the "Legacy" Entrust->DigiCert chain...and so the EV UI features are *not* shown.

So, IMHO, the DigiCert example *is* relevant. It seems that "Page Info->View Certificate" doesn't necessarily show the same certificate chain that hasValidEVOidTag() uses!

> The other two URLs you supplied seem to have server certs issued by the
> same intermediate CA which, in both cases, chains up to an Entrust root.
> Both of the server certs seem to bear the same policy OID, namely,
> 2.16.840.1.114404.1.1.2.4.1 which appears to be the OID associated with
> "CN=SecureTrust CA,O=SecureTrust Corporation,C=US" in your patch.

Yes.

> So, in the examples given in comment 0 (ignoring Digicert), there seem to
> be one policy OID belonging to SecureTrust that chains up to Entrust.  In
> that light, I think your question in comment 6 would be: "Are you suggesting
> that the SecureTrust policy OID would also be associated with Entrust's root?"

If we did ignore DigiCert, then yes, that would be my question.

> My answer is: that is one of the alternatives we are considering, yes.
> That alternative would associate multiple EV policy OIDs with Entrust's
> root, and would associate SecureTrust's policy OID with multiple Roots.

That approach would work...to a point. But imagine if, sometime in the future, SecureTrust decided to switch "supplier" for their legacy ubiquity. This would involve their customers' webservers ceasing to serve their Entrust-signed cross-certificate, and instead starting to serve a cross-certificate issued by a different "Legacy" CA (GlobalSign, for example). As soon as any FF3 instances encountered this new cross-certificate, they would no longer show the EV UI (since only the Entrust Root would have SecureTrust's EV Policy OID associated with it).
(I chose GlobalSign as the example deliberately, since they also have a cross-certification relationship with SecureTrust. For an example of this, look no further than the current SSL Certificate for bugzilla.mozilla.org. FYI, XRamp was a former name for SecureTrust, which was itself a former name for AmbironTrustWave).
The caching behaviour described by Kai in comment #3 would make the problem even worse, because it would potentially affect all EV sites certified by SecureTrust.

Furthermore, imagine if a "rogue" CA (any member of the Mozilla Root Program - EV or not) decided to take advantage of this behaviour. They could mount an "EV UI denial-of-service" attack by issuing an unrequested cross-certificate to a competitor EV CA. The rogue would then need to obtain an SSL Certificate (EV or non-EV) from that competitor CA, install it on an SSL server, and get that SSL server to serve the unrequested cross-certificate. FF3 clients would then cache this (as per Kai's comment #3) and quite possibly never show the EV UI for any EV sites certified by that competitor EV CA!

> BTW, it is my understanding that this is exactly what happens with IE7 in
> Vista.

Vista/IE7 could work that way, but it's typically not the case. My investigations have found that each "New" Root in IE7 (on XP and Vista) is associated with exactly 1 EV Policy OID, except for 1 VeriSign Root (which also has Thawte's EV Policy OID associated with it) and GoDaddy's Roots (which also have Starfield's EV Policy ID associated with them). VeriSign/Thawte are the same company; GoDaddy/Starfield are the same company.

> Another alternative we may consider is to, in essence, promote the
> Entrust-issued intermediate CA cert for SecureTrust to be treated as a root
> CA cert in its own right, for purposes of determining EV path validity,
> (I realize this is heresy :) and associate SecureTrust's EV policy OID with
> that cert (also), rather than with Entrust's root.

If you did it that way, I presume you'd need to hard-code it in 
myTrustedEVInfos[], in which case I think it would still be prone to the problem scenarios I mentioned.
Also, there would be an added problem when the current Entrust->SecureTrust and Entrust->DigiCert cross-certificates expire in 2013 and 2014 respectively. SecureTrust's "New" Root expires in 2029, and DigiCert's expires in 2031. So both CAs might well want to obtain a new cross-certificate from Entrust, or GlobalSign, or some other "Legacy" CA. These new cross-certificates would not be "heretical" pseudo-Roots in all the versions of FF3+ that have been released up to that point in time, and so the EV UI indicators would again start to disappear on those browsers.

> There are pros and cons to each of those two approaches, and they may not be
> the only ones to be considered.

Most EV CAs have specified earlier "notBefore" and "notAfter" dates in their "Legacy->New" cross-certificates, compared to the "notBefore" and "notAfter" dates in their "New" Root Certificates. Microsoft's representatives to the CABForum advised the EV CAs to do this in order to ensure that XP/Vista would pick the "New" certificate chain instead of the "Legacy" chain.
On a side note...Microsoft also mentioned to the CABForum that "Whether the certificate is EV or not is not part of the core certificate validation behavior". I suppose this is similar to the current relationship between PSM (knows the "is EV or not" bit) and NSS (contains the core certificate validation behaviour).

If it were up to me, I would want to solve this problem by somehow ensuring that each CA's "New" Root is *always* used (regardless of which cross-certificate(s) exist at whichever points in time) by PSM/NSS to determine if a site certificate is EV or not.
My second choice would be to get PSM/NSS to use the same "notBefore"/"notAfter" logic that Microsoft have used.

Comment 9

[Rob Stradling]

I've had a couple more thoughts on how to tackle this issue:

1. NSS's CERT_PKIXVerifyCert(), called by PSM's hasValidEVOidTag(), does the following:
>    PKIX_List *           anchors = NULL;
...
>    /* The 'anchors' parameter must be supplied, but it can be an 
>       empty list. PKIX will use the NSS trust database to form
>       the anchors list */
>    PKIX_List_Create(&anchors, plContext);
>    error = PKIX_ProcessingParams_Create(anchors, &procParams, plContext);

Would it be possible for PSM to supply an 'anchors' list to NSS, so that NSS doesn't default to using the full "NSS trust database" ?
If yes, then PSM could supply an 'anchors' list that contained just the "New" SecureTrust Root, for example. This would presumably guarantee that the "New" chain would be picked rather than the "Legacy" chain, and so the EV UI would always be seen.


2. Alternatively, how about changing myTrustedEVInfos[] so that it doesn't list specific Root (or pseudo-Root) Certificates. Instead, it could list each CA's Subject DN, Public Key (or a hash of it) and (optionally) Subject Key Identifier - this trust anchor information is shared by each pair of "New" Root Certificate / "Legacy"->"New" cross-certificate.
(This is only half an idea. I'm no expert in the PSM and NSS codebases, but I have a hunch that this approach might have ramifications that would be infeasibly complex to implement, especially if an alternative workable solution is available).

Comment 10

[Robert Relyea]

It turns out the base problem was a bug in the Explicit code.

The path constructed through the cross cert should not validate against a specific policy with initExplicit set, and therefore the path should be rejected in favor one a path that does validate through the policy oid.

Bug 411614 fixed addresses this issue. With that patch applied, Mozilla correctly identifies the above sites as EV. Rob, can you verify that you get the same results?

Thanks,

bob

(and thanks for proactively testing your EV chains!).

Comment 11

[Rob Stradling]

> Rob, can you verify that you get the same results?

Thanks Bob.  I tested the patch from Bug 411614 a few days ago, and I can confirm that the 3 example URLs (DigiCert and SecureTrust) that I originally cited did receive the EV UI treatment.  However...

Problem 1: I've recompiled the latest CVS code today, and I am no longer shown the EV UI for any site at all, even though I've set NSS_EV_TEST_HACK=USE_PKIX.
(I notice that CERT_PKIXVerifyCert() now only seems to get called if/when I go to "Page Info->Security->View Certificate".  Does this indicate a problem?  If yes, might it be caused by the recently committed patch from Bug #406999?)

Problem 2: The Entrust->DigiCert and Entrust->SecureTrust cross-certificates just happen to not include a Certificate Policies extension.  IIUC, the patch in Bug #411614 relies on this fact.  However, other CAs' "Legacy->New" cross-certificates do legitimately contain the Certificate Policies extension.
  Example: https://www.networksolutions.com/manage-it/index.jsp
Whilst I had the DigiCert and SecureTrust examples working, I tested this example (where Network Solutions are both the EV CA and the EV Certificate Holder).  CERT_PKIXVerifyCert() chose the "Legacy->New" cross-certificate, so no EV UI was displayed. In other words, the patch from Bug #411614 is not effective enough at ensuring that the relevant EV Root is picked.
(I can't replicate this problem now, due to Problem 1).

> (and thanks for proactively testing your EV chains!).

No problem at all.  It'll be much better for everyone if we get this all working before FF3 gets launched.  Hooray for open source!  :-)

Comment 12

[Kai Engert [:KaiE:]]

(In reply to comment #11)
> 
> Problem 1: I've recompiled the latest CVS code today, and I am no longer shown
> the EV UI for any site at all, even though I've set NSS_EV_TEST_HACK=USE_PKIX.
> (I notice that CERT_PKIXVerifyCert() now only seems to get called if/when I go
> to "Page Info->Security->View Certificate".  Does this indicate a problem?  If
> yes, might it be caused by the recently committed patch from Bug #406999?)

Indeed, I confirm this regressed, and I confirm backing out the patch from bug 406999 caused this problem :-/

Thanks for your report and your analysis, this will save me some time!

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

(In reply to comment #11)
> other CAs' "Legacy->New" cross-certificates do legitimately contain the 
> Certificate Policies extension.
>   Example: https://www.networksolutions.com/manage-it/index.jsp

> CERT_PKIXVerifyCert() chose the "Legacy->New" cross-certificate, 

... which is correct behavior ...

> so no EV UI was displayed. In other words, the patch from Bug #411614 is 
> not effective enough at ensuring that the relevant EV Root is picked.

Given that both certification paths are valid for that CA's EV policy, there
is NO SENSE in which picking one of those paths is more correct than the 
other.  Nothing in RFC 3280 gives preference to one over the other.
Both paths are valid.  That was the choice made by the CAs when they put
a policy extension into their "Legacy->New" cross-certificates.  

I'll avoid repeating comment 7 here.  :)

Comment 14

[Robert Relyea]

sigh,

The cross certificate would have been fine, except it includes the EV policy.
(it's a good cross cert in that it doesn't have the any policy).
It seems wrong to me for Network Solutions to actually get this particular cross certificate, the whole point of pkix is to help us choose the correct patch without relying on such things as certificate date manipulation (the relative start dates of certificates is not part of the PKIX processing spec, and should not be relied upon as the primary means of forcing correct certificate path selection).

For network solutions we may have to go with the comment 7 suggestions which Nelson make.

bob

Comment 15

[Rob Stradling]

(in reply to comment 13)
> Nothing in RFC 3280 gives preference to one over the other. Both paths are
> valid. 

I agree. Both paths are valid, and therefore neither should be rejected.

> Given that both certification paths are valid for that CA's EV policy, there
> is NO SENSE in which picking one of those paths is more correct than the
> other.

I agree that RFC 3280 does not mandate any particular algorithm for how to prefer one valid path over another.
However, I disagree with your conclusion that "there is NO SENSE in which picking one of those paths is more correct than the other".

Both paths are valid according to RFC 3280 section 6.1, which "defines the minimum conditions for a path to be considered valid".
However, we have additional application-specific conditions to consider for each candidate path as well: Does the path terminate with a Root Certificate listed in myTrustedEVInfos[]? If yes, is the path valid for (one of) the EV Policy OID(s) listed for that Root in myTrustedEVInfos[]?

RFC 3280 section 6.2 allows us to "augment the algorithm presented in section 6.1 to further limit the set of valid certification paths which begin with a particular trust anchor". It goes on to say that the "selection of one or more trusted CAs is a local decision...The inputs used to process a path may reflect application-specific requirements or limitations in the trust accorded a particular trust anchor. For example, a trusted CA may only be trusted for a particular certificate policy. This restriction can be expressed through the inputs to the path validation procedure".

IMHO, it would be both common sense, and entirely within the spirit of RFC 3280, to solve the problem via "the inputs to the path validation procedure". I recommend (as I tried to express in comment 9, idea 1) that only the Trusted-for-EV-with-this-Policy-OID Root(s) should be passed into PSM/NSS's "Is this a valid EV path?" RFC 3280-based path validation algorithm.

What makes NO SENSE to me is the idea of arbitrarily rejecting all but one of the valid paths *before* PSM applies its application-specific "Does this match anything in myTrustedEVInfos[]?" check. In fact, if I'm not mistaken, this arbitrary path rejection actually violates RFC 3280 section 6 (I'm assuming that we want PSM/NSS's entire "Is this a valid EV path?" algorithm to be classed as RFC 3280-compliant).
(Actually, I suppose "violates" is too strong, since you could say that arbitrary path rejection is a way to "augment the algorithm...to further limit the set of valid certification paths..." :-) ).

Comment 16

[Rob Stradling]

(in reply to comment 14)
> It seems wrong to me for Network Solutions to actually get this particular
> cross certificate

I disagree.

RFC 3280 section 4.2.1.5 says that "In a CA certificate, these policy information terms limit the set of policies for certification paths which include this certificate".

In other words, this cross-certificate permits Network Solutions to issue certificates under their EV Policy, but prevents them from issuing certificates under a myriad of other policies.

In contrast, the Entrust->SecureTrust and Entrust->DigiCert cross-certificates permit SecureTrust and Entrust to issue certificates under their EV Policy and any other policy they want to.

I see nothing objectionable in any of these cross-certificates.

> For network solutions we may have to go with the comment 7 suggestions which
> Nelson make.

I identified some problems with Nelson's suggestions in comment 8 (i.e. "EV
UI denial-of-service" by a Rogue CA, and early-expiring "pseudo EV Root" cross-certificates).

I believe that what I've just recommended in comment 15 would solve these problems and avoid the need to treat the Network Solutions example (or any similar examples that might crop up in the future) as a special case.

Comment 17

[Robert Relyea]

>> It seems wrong to me for Network Solutions to actually get this particular
>> cross certificate
>
> I disagree.
>
> RFC 3280 section 4.2.1.5 says that "In a CA certificate, these policy
> information terms limit the set of policies for certification paths which
> include this certificate".
> 
> In other words, this cross-certificate permits Network Solutions to issue
> certificates under their EV Policy, but prevents them from issuing
> certificates under a myriad of other policies.

However the EV policy is never valid across the cross-certificate as the root is not trusted for EV.

Part of the problem here is EV is sort of hijacking the RFC 3280 model in a non-standard way (Mostly because the world is non-standard wrt 3280).

The PKIX processing code is 3280. It does not understand EV directly (which is as it should be EV should not require the generic 3280 processing code to understand it). EV however adds the additional requirement at certain policy oids must map to specific roots. That means EV requires the path through the cross certificate to be invalid. If there isn't a 3280 way of determining this, then the path itself is ambiguous and there is no way for the generic PKIX code to determine which is the correct path.

That is way including your EV policy in your cross certificate is problematic.

bob

Comment 18

[Rob Stradling]

> Part of the problem here is EV is sort of hijacking the RFC 3280 model in a
> non-standard way

I disagree that EV is "hijacking" anything "in a non-standard way".

EV certainly augments RFC 3280's path validation algorithm, but then RFC 3280 section 6.1 anticipates and allows this kind of augmentation:
  "Information about trust anchors are provided as inputs to the certification path validation algorithm (section 6.1.1).  A particular certification path may not, however, be appropriate for all applications.  Therefore, an application MAY augment this algorithm to further limit the set of valid paths."

Clearly, the path that includes the cross-certificate issued to Network Solutions is "...not...appropriate for" applications that use EV Certificates.

> The PKIX processing code is 3280. It does not understand EV directly (which is
> as it should be EV should not require the generic 3280 processing code to
> understand it).

Agreed.

> EV however adds the additional requirement at certain policy oids must map to
> specific roots. That means EV requires the path through the cross certificate
> to be invalid.

Agreed.

> If there isn't a 3280 way of determining this, then the path itself is
> ambiguous and there is no way for the generic PKIX code to determine which is
> the correct path.

RFC 3280 says *nothing* about how to "determine which is the correct path". Rather, it specifies an algorithm for analyzing ONE path at a time, in order to determine if that path is valid or invalid.

So, IMHO, it should not be the responsibility of "the generic PKIX code" to "determine which is the correct path" !

I think that the best way for nsNSSCertificate::hasValidEVOidTag() to "determine which is the correct path" would be...
  Step 1. Extract the Policy OID from the end-entity certificate.
  Step 2. Create a "temporary trust database", consisting of only those entries in the "NSS trust database" that are also identified by myTrustedEVInfos[] as being valid for EV with that Policy OID.
  Step 3. Call CERT_PKIXVerifyCert(), passing in that "temporary trust database" somehow. (I imagine this would require extending the capabilities of cvin[].type, etc).
  Step 4. In CERT_PKIXVerifyCert() itself, override the current "PKIX will use the NSS trust database to form the anchors list" behaviour with "PKIX will use the 'temporary trust database' to form the anchors list".

Taking this approach would...
  (i) Resolve the problem with the Network Solutions cross-certificate (which, IMHO, appears to violate neither EV nor RFC 3280).
  (ii) Resolve similar problems with any other/future cross-certificates that might happen to include the Certificate Policies extension.
  (iii) Thwart any attempt to mount an "EV UI denial-of-service" attack (see comment #8).

As far as I can tell, the other options being considered (see Nelson's comment #7) would achieve (i), but not (ii) or (iii).

Comment 19

[Nelson Bolyard (seldom reads bugmail)]

Shortly after comment 17 was written, the NSS team met and discussed this 
issue.  Bob Relyea identified a solution that is perfect, one that uses
libPKIX the way that RFC 3280 intended that it be used.  It was not recorded
in this bug (until now) and was apparently forgotten by some.  Let me describe
it here.  

Background:
After NSS has initially validated the cert as being a valid SSL server cert,
without regard to EV, PSM then checks the server cert for the presence of a 
known EV policy OID.  If found, PSM then does a second validation of the 
server cert, using libPKIX, which understands policy extensions and which 
implements RFC 3280 rather exactly.  PSM invokes libPKIX through a wrapper
function that exists for that very purpose.  

At the present time, PSM allows libPKIX to use the entire set of trusted 
(root) CA certs known to NSS when doing that second validation.  That is the 
cause of the problem.  By allowing libPKIX to construct a chain to ANY of
the trusted CAs, libPKIX has the opportunity to construct a chain to a non-EV
legacy root.  

The solution is to give libPKIX a smaller set of trusted (root) CA certs 
against which to validate the server cert.  The smaller set would consist
of ONLY the CA cert (or certs) that are authorized to use the particular 
EV policy OID found in the server cert.  Then, libPKIX will either construct
a chain to an EV root, or will fail, but it will NOT construct a chain to a
non-EV root.  

With this solution, it is not necessary to give non-EV roots any ability 
to anchor EV cert chains, and it is not necessary to give competing CAs' 
roots ability to issue EV policy OIDs for their competitors.  

This approach is both relatively simple to implement, and keeps the lid on
Pandora's box, which would surely be opened if we let CAs issue certs with 
the EV policy OIDs of their competitors.

Comment 20

[Nelson Bolyard (seldom reads bugmail)]

Rob, I have been told that, since bug 411614 was fixed, the issue reported 
in THIS bug is no longer reproducible.  Can you confirm that?

(You may need a very recent nightly Firefox trunk build to test that.)

Comment 21

[Nelson Bolyard (seldom reads bugmail)]

I guess we're waiting on the issue in comment 11 to be resolved. 

Comment 22

[Rob Stradling]

(in reply to comment #19)
> Shortly after comment 17 was written, the NSS team met and discussed this
> issue.  Bob Relyea identified a solution that is perfect...to give libPKIX...
> ONLY the CA cert (or certs) that are authorized to use the particular
> EV policy OID found in the server cert

Thanks Nelson.  This is exactly the same solution that I proposed in comment #9 (idea 1) and for which I have argued the case in my more recent comments.  I'm very glad that we've all reached the same conclusion on this issue.  :-)

(in reply to comment #20)
> Rob, I have been told that, since bug 411614 was fixed, the issue reported
> in THIS bug is no longer reproducible.  Can you confirm that?

Confirmed.  The 3 examples I cited originally (for DigiCert and SecureTrust) now receive the EV UI treatment consistently for me (using the latest CVS trunk code).

(in reply to comment #21)
> I guess we're waiting on the issue in comment 11 to be resolved.

Yes.  The Network Solutions example still fails to receive the EV UI treatment for me.  This should be resolved by the implementation of comment #19.

(in reply to comment #19 again)
Nelson, you've said that "This approach is...relatively simple to implement".  Will this be done in time for Firefox 3.0 ?

Comment 23

[Kai Engert [:KaiE:]]

> Nelson, you've said that "This approach is...relatively simple to implement". 
> Will this be done in time for Firefox 3.0 ?

Very likely, I'm working on that now.

Comment 24

[Kai Engert [:KaiE:]]

Attachment: Patch v1

This patch appears to fix the issue we see in bug 403665.

Comment 25

[Kai Engert [:KaiE:]]

Attachment: Patch v2

must cleanup certs before shutting down nss

Comment 26

[Kai Engert [:KaiE:]]

Rob, I assume it's necessary to add more root CA certs to the NSS builtin module. Using the latest trunk and your patch, I get EV UI only for the first of your three test sites.

Are you interested to test this patch with your own roots added locally?

You can use the patch in bug 421989 to compile a pp utility that helps to dump the required base64 encodings of issuer and serial. Use "pp -t certificate -x -a -i file.pem".

For example, for the Entrust cert you listed in your patch, I used the following new snippet:

  {
    // CN=Entrust.net Secure Server Certification Authority,OU=(c) 1999 Entrust.net Limited,OU=www.entrust.net/CPS incorp. by ref. (limits liab.),O=Entrust.net,C=US
    "2.16.840.1.114028.10.1.2",
    "Entrust EV OID",
    SEC_OID_UNKNOWN,
    "99:A6:9B:E6:1A:FE:88:6B:4D:2B:82:00:7C:B8:54:FC:31:7E:15:39",
    "MIHDMQswCQYDVQQGEwJVUzEUMBIGA1UEChMLRW50cnVzdC5uZXQxOzA5BgNVBAsT"
    "Mnd3dy5lbnRydXN0Lm5ldC9DUFMgaW5jb3JwLiBieSByZWYuIChsaW1pdHMgbGlh"
    "Yi4pMSUwIwYDVQQLExwoYykgMTk5OSBFbnRydXN0Lm5ldCBMaW1pdGVkMTowOAYD"
    "VQQDEzFFbnRydXN0Lm5ldCBTZWN1cmUgU2VydmVyIENlcnRpZmljYXRpb24gQXV0"
    "aG9yaXR5",
    "N0rSQw==",
    nsnull
   },

But still, I only got EV status for your first test site.
I could not find the Ambiron root referenced in your patch.



For example, for the Entrust cert you listed in your patch, I added the following new snippet to 

Comment 27

[Rob Stradling]

(in reply to comment #26)
> Rob, I assume it's necessary to add more root CA certs to the NSS builtin
> module. Using the latest trunk and your patch, I get EV UI only for the first
> of your three test sites.

Yes, that's correct.  Sorry I forgot to mention that.

> Are you interested to test this patch with your own roots added locally?

Yes, I'm very interested.  :-)

I've just applied your v2 patch to the latest CVS code.  However, I cannot get it to compile.  I get...
"In member function ‘nsresult nsNSSCertificate::hasValidEVOidTag(SECOidTag&, PRBool&)’:
/home/rob/cvs/HEAD/mozilla/security/manager/ssl/src/nsIdentityChecking.cpp:767: error: ‘cert_pi_trustAnchors’ was not declared in this scope"

I can't find any other references to cert_pi_trustAnchors with grep.
Perhaps your patch should have included some changes to nss/lib/certdb/certt.h ?
I would also have expected to see some changes to nss/lib/certhigh/certvfypkix.c.

> I could not find the Ambiron root referenced in your patch.

It's the "SecureTrust CA" referenced in Bug #418907 and Bug #418910 (and Bug #409837).

Comment 28

[Kai Engert [:KaiE:]]

(In reply to comment #27)
> I've just applied your v2 patch to the latest CVS code.  However, I cannot get
> it to compile.  I get...
> "In member function ‘nsresult nsNSSCertificate::hasValidEVOidTag(SECOidTag&,
> PRBool&)’:
> /home/rob/cvs/HEAD/mozilla/security/manager/ssl/src/nsIdentityChecking.cpp:767:
> error: ‘cert_pi_trustAnchors’ was not declared in this scope"

You need to fetch the very latest trunk of NSS.

In mozilla/client.mk, change both NSPR_CO and NSS_CO to HEAD

Comment 29

[Kai Engert [:KaiE:]]

I added the new root CA certs from bug 418907 locally, and used code to enable the 3 trustwave roots for EV locally (bug 418902, bug 418910), and for each of the provided test sites I get EV with the expected trust anchor chosen.

Comment 30

[Kai Engert [:KaiE:]]

Regarding comment 11:

https://www.networksolutions.com uses a server which contains policy OID 1.3.6.1.4.1.782.1.2.1.8.1

Which CA is supposed to be blessed for this OID?

Comment 31

[Rob Stradling]

(in reply to comment #28)
> You need to fetch the very latest trunk of NSS.

Thanks Kai.  That's much better.  Your patch now applies without fuzz and compiles successfully.  :-)

(in reply to comment #26)
> You can use the patch in bug 421989 to compile a pp utility...

I'm afraid I can't figure out how to build pp, but I've managed to generate the required Base64 data pretty easily with OpenSSL's asn1parse and base64 utilities.

(in reply to comment #29)
> I added the new root CA certs from bug 418907 locally, and used code to enable
> the 3 trustwave roots for EV locally (bug 418902, bug 418910), and for each of
> the provided test sites I get EV with the expected trust anchor chosen.

I've done a similar test. I've enabled just the "SecureTrust CA" locally for EV, and I too now get the EV UI consistently for both https://www.securetrust.com and https://www.trustwave.com

The relevant DigiCert root has already been added to myTrustedEVInfos[] in trunk, so I didn't need to add this locally.  I too now get the EV UI consistently for https://www.digicert.com

(in reply to comment #30)
> https://www.networksolutions.com uses a server which contains policy OID
> 1.3.6.1.4.1.782.1.2.1.8.1
> Which CA is supposed to be blessed for this OID?

The "Network Solutions Certificate Authority" root certificate in Bug #403915. That bug mentions that you can download it from:
http://www.netsolssl.com/NetworkSolutionsCertificateAuthority.crt

I have added this root locally and enabled it for EV.  I now get the EV UI consistently for https://www.networksolutions.com/manage-it/index.jsp

Here is the myTrustedEVInfos[] element I used for the Network Solutions Certificate Authority:

  {
    // CN=Network Solutions Certificate Authority,O=Network Solutions L.L.C.,C=US
    "1.3.6.1.4.1.782.1.2.1.8.1",
    "Network Solutions EV OID",
    SEC_OID_UNKNOWN,
    "74:F8:A3:C3:EF:E7:B3:90:06:4B:83:90:3C:21:64:60:20:E5:DF:CE",
    "MGIxCzAJBgNVBAYTAlVTMSEwHwYDVQQKExhOZXR3b3JrIFNvbHV0aW9ucyBMLkwu"
    "Qy4xMDAuBgNVBAMTJ05ldHdvcmsgU29sdXRpb25zIENlcnRpZmljYXRlIEF1dGhv"
    "cml0eQ==",
    "V8szb8JcFuZHFhfjkDFo4A==",
    nsnull
  },

Comment 32

[Rob Stradling]

Kai, I believe that your "Patch v2" resolves all of the existing/potential problems that I have mentioned/imagined in this bug so far.  I hope that this patch will now be reviewed and checked in.

However, I've just noticed issues with a different cross-certification scenario.  I'll describe this as fully as I can in my next comment...

Comment 33

[Rob Stradling]

(further to comment #32)
> However, I've just noticed issues with a different cross-certification
> scenario.  I'll describe this as fully as I can in my next comment...

Firstly, I'd better give you some context...

In response to the technical hurdles involved in ensuring that WinXP/IE7 will consistently show its EV UI when it encounters an EV SSL Certificate, Comodo has developed a technology called EV AUTO-Enhancer(tm).

Please note: Our technology is very different to other CAs' "EV beacon" solutions (e.g. VeriSign's EV Upgrader(tm), Entrust's EV Updater(tm), etc) that attempt to overcome some (but not all!) of the same technical hurdles.

Our technology works by getting an SSL Server to send an apparently strange set of certificates in each SSL handshake:
  - First, the SSL site certificate, issued by CA X.
  - Next, one or more CA cross-certificates, each issued to CA X by a "Legacy" Root.
  - Next, one cross-certificate issued by the "New" Root (created primarily to get EV working in IE7/WinXP).
  - Finally, the "New" Root itself.

For a working example, look no further than Comodo's own secure site.  Look at the output from "openssl s_client -connect secure.comodo.com:443".
This SSL server returns: cross-certificates issued by 2 "Legacy" Roots ("AddTrust External CA Root" and "UTN - DATACorp SGC"); a cross-certificate issued by our "New" Root ("COMODO Certification Authority"); the "New" Root itself.

I won't go into any more detail right now of precisely how or why it works.  Instead, let me jump straight to what it achieves compared to other CAs' "EV beacon" solutions:
1. EV AUTO-Enhancer(tm) will enable the EV UI in WinXP/IE7 even when Javascript is disabled.  "EV beacon" solutions cannot achieve this.
2. EV AUTO-Enhancer(tm) will always enable the EV UI in WinXP/IE7 on the first visit to an https page.  "EV beacon" solutions cannot achieve this in cases where the relevant "New" Root is not already installed.
3. EV AUTO-Enhancer(tm) makes installing an EV SSL Certificate on a webserver as easy as installing a non-EV SSL Certificate.  With an "EV beacon" solution, there is the additional step of inserting some HTML/Javascript into (ideally) all of a website's http and https webpages, in order to improve the chances of seeing the EV UI in WinXP/IE7.
4. EV AUTO-Enhancer(tm) only requires a webmaster's involvement to install an EV SSL Certificate.  "EV beacon" solutions typically require both a webmaster and webdesigner (due to point 3).
5. In situations where a computer cannot connect to Microsoft's Windows Update website (in order to download a "New" Root Certificate), EV AUTO-Enhancer(tm) avoids causing browser warnings on http/https pages.  In the same situations, "EV beacon" solutions cause WinXP/IE7 to display the warning: ""To help protect your security, Internet Explorer has blocked this website from displaying content with security certificate errors", because WinXP will not trust the EV beacon's certificate.

We have tested EV AUTO-Enhancer(tm) fairly extensively, and found it to be compatible with all the main browsers.
We have found that some customers were unwilling to use EV SSL Certificates at all, when faced with the prospect of having to use an "EV beacon" solution.  However, by explaining to them what EV AUTO-Enhancer(tm) can do, we have managed to persuade a number of those customers to adopt EV.

Currently, only Comodo are using this technology.  But, I think it is very possible that other CAs might want to (and we might allow them to) use it in the future.  This could potentially lead to wider adoption of EV, less "Why can't I see the EV UI?" support requests sent to those CAs and to Microsoft, less "Why do I see this browser warning?" support requests sent to those CAs and to Microsoft, etc.

Now, you'd be forgiven for thinking "Why is any of this a concern for Mozilla?  Isn't it all about working around problems with a Microsoft product?".

I'll post a follow-up message shortly to answer those questions.

Comment 34

[Rob Stradling]

(further to comment #33)
> I'll post a follow-up message shortly to answer those questions.

With Kai's Patch v2 applied, and with Comodo's "New" Root (COMODO Certification Authority) locally added and trusted for EV, I do *not* see the EV UI for https://secure.comodo.com, even though all the certificates in a suitable trusted-for-EV chain are sent by the server during the SSL handshake.

I think PSM/NSS must be filtering the list of certificates sent during the SSL handshake - picking all the certificates from one certificate chain (where that chain is chosen fairly arbitrarily), and discarding any other certificates (even if they might be useful in forming trusted chain(s)) - well before PSM/NSS evaluate whether the site certificate is a valid-for-EV certificate.
I suppose this behaviour is understandable, since RFC4346 (TLS 1.1) section 7.4.2 expects only the certificates in 1 certificate chain to be sent (although I note that it says "must" rather than "MUST").

In practice, this current PSM/NSS behaviour is probably not too big a deal for Comodo.  If/when Frank Hecker approves our pending requests to have both our "New" and "Legacy" Roots enabled for EV in the Mozilla Root Program, we should see the EV UI in Firefox 3.  We would only have a problem in the unlikely (I hope) event of Frank agreeing to enable our "New" Root for EV, but refusing to enable our "Legacy" Roots for EV.

Also, the current PSM/NSS behaviour is probably not a problem for any other CA (which has both "Legacy" and "New" Roots enabled for EV in Firefox 3.0.0.0 with the *same* EV Policy OID) who might ever want to use our EV AUTO-Enhancer(tm) technology.

Now, the issue/problem is...and I'm sorry it's taken so many words to reach this point...
Any CA, for which the "New" Root and "Legacy" Root(s) are enabled for EV in Firefox 3.0.0.0 with *different* EV Policy OIDs (e.g. DigiCert, Trustwave, Network Solutions, etc), would not see the EV UI in Firefox 3.0.0.0 if they ever attempted to use our EV AUTO-Enhancer(tm) technology.  This would effectively turn AUTO-Enhancer into AUTO-Suppressor! :-(

I would really like to leave open the option for these CAs to use EV AUTO-Enhancer(tm) technology at some stage in the future (although, of course, there is no guarantee that any other CA will ever decide to use it).  IE7+ on WinXP will not be dying out any time soon (and so EV AUTO-Enhancer(tm) will remain useful for some time to come), and I think it's in everybody's interests to encourage wider adoption of EV.  However, I'm sure that the suppression of the EV UI in Firefox 3.0.0.0 would be too much of a caveat for these CAs to adopt EV AUTO-Enhancer(tm).

I've spent a few hours delving into what I think are the relevant parts of the PSM/NSS code.  I'm no expert, so please forgive me if the following assessment is completely wrong...
I think the current behaviour I've described occurs in the AuthCertificateCallback() function in PSM's nsNSSCallbacks.cpp.  The comments in that function mention a "temp db".  I think (after looking at NSS's SSL handshake code) that all of the certs from the SSL handshake are in the "temp db".  Then, CERT_GetCertChainFromCert() appears to effectively filter out all but 1 certificate chain.  Then, the certificates in that 1 certificate chain are added to the PKCS#11 slot.  Later, I think CERT_PKIXVerifyCert() only evaluates certificates that are present in the PKCS#11 slot when it checks if a site certificate is trusted-for-EV.

If I've got that right...
Would it be possible to make AuthCertificateCallback() put *all* of the certs from the "temp db" into the PKCS#11 slot?

Or, if my assessment is incorrect...
Would it be possible to resolve the issue I've described in any other way?

And finally...
What are the chances of resolving this in time for Firefox 3.0.0.0 ?

(If it's not resolved in time for Firefox 3.0.0.0, it's probably not worth addressing at all, for reasons already mentioned :-( ).

If you've read this far, it's probably now time to get some (more) coffee!

Comment 35

[Rob Stradling]

(further to comment #34)
The "COMODO Certification Authority" root certificate is the one mentioned in Bug #401587.  You can download it at: http://crt.comodoca.com/COMODOCertificationAuthority.crt

The required myTrustedEVInfos[] element to enable this root for EV is...

  {
    // CN=COMODO Certification Authority,O=COMODO CA Limited,L=Salford,ST=Greater Manchester,C=GB
    "1.3.6.1.4.1.6449.1.2.1.5.1",
    "COMODO EV OID",
    SEC_OID_UNKNOWN,
    "66:31:BF:9E:F7:4F:9E:B6:C9:D5:A6:0C:BA:6A:BE:D1:F7:BD:EF:7B",
    "MIGBMQswCQYDVQQGEwJHQjEbMBkGA1UECBMSR3JlYXRlciBNYW5jaGVzdGVyMRAw"
    "DgYDVQQHEwdTYWxmb3JkMRowGAYDVQQKExFDT01PRE8gQ0EgTGltaXRlZDEnMCUG"
    "A1UEAxMeQ09NT0RPIENlcnRpZmljYXRpb24gQXV0aG9yaXR5",
    "ToEtioJl4AsC7j41AkblPQ==",
    nsnull
  },

Comment 36

[Nelson Bolyard (seldom reads bugmail)]

In reply to comments 33 and 34, 
There certainly should not be any requirement that all the certs, or even
ANY of the certs, that form the final chain to a trust anchor must be in
some single PKCS#11 slot.  The code is not intended to require any certs 
to be in any slot before, or during, cert chain evaluation.  If the code
actually does impose such a requirement, that would be a bug, pure and 
simple.  The solution is not to try to put more certs into some slot.

Let me say a few words about why PSM puts some certs into a PKCS#11 slot.
During the course of cert chain evaluation, as the code attempts to 
construct various certification paths for validation, it draws certs from 
any of the following sources: the "temp cert db" (an in-memory pool of 
certs that are held only for the lifetime of an SSL handshake), any 
relevant local PKCS#11 slots (where certs may have been stored "permanently")
and finally from remote servers (indicated by AIA extensions).  

Because so many servers are misconfigured, and fail to send out any intermediate CA certs, FF3 has adopted the defensive strategy )already used 
by some versions of IE, IINM) of saving (making "permanent") a local copy of
the intermediate CA certs found in validated cert chains.  The certs in the validated chain are copied from the temp cert DB into a PKCS#11 slot from 
which they may later be drawn.  This way, the valid intermediate CA certs 
from one correctly configured server may be used in a subsequent cert chain 
validation for an incorrectly configured server.  Only the certs from the
ONE path that is validated are added to permanent storage.  

I think what you've found is that the certs that were sent by the server 
that are NOT part of the chain initially validated during the SSL handshake
are discarded from the temp cert DB before the second cert chain validation
is performed.  I suspect that the second cert chain validation is not being
performed until after the SSL handshake has completed, by which time the 
contents of the temp cert DB that were not part of the validated chain have
been evicted.  

This problem is a consequence of the fact that we do two separate cert chain
validations, one without regard to EV, and a second with regard to EV. 
It may be that we are performing the second validation when it's too late.

I agree that there's a bug here.  The chain that we should be putting into 
the PKCS#11 slot for permanent keeping should be the chain constructed from
the EV validation, when an EV validation succeeds.  There are several ways 
we can approach this issue.  We'll figure something out. I think this should
be the subject of a second bug.  (This one's just getting too long).

There are several potential solutions

Comment 37

[Kai Engert [:KaiE:]]

I have not followed the previous discussions in all detail.

During a conference call we discussed this issue.
My understanding is:
Site secure.comodo.com does not get EV status, because at the time we attempt the verification, the NSS temp db misses some certs.

It has been proposed to solve this problem by doing the EV verification right inside the authcallback function. At this time, all certs obtained from the server are supposed to be still contained in the temp db.

So, I tested that, but don't get success.
- I added the root from comment 35
- I added the EV blessing from comment 35
- during execution, we find the OID, and we find the single candidate trust anchor, and pass that into NSS

But still, when CERT_PKIXVerifyCert returns, it still returns the wrong root
(CN=UTN - DATACorp SGC) which is not blessed for EV.

I tried to execute the EV verification during both the AuthCallback and the HandshakeCallback, with identical results.

Comment 38

[Nelson Bolyard (seldom reads bugmail)]

> - during execution, we find the OID, and we find the single candidate trust
> anchor, and pass that into NSS
>
> But still, when CERT_PKIXVerifyCert returns, it still returns the wrong root
> (CN=UTN - DATACorp SGC) which is not blessed for EV.

Kai, please file an NSS bug about this, P1, assign to Alexei.  

It would be good if you could provide a way to test this that does not 
involve the browser, e.g., a patch to vfyserv that can reproduce this.  

Comment 39

[Nelson Bolyard (seldom reads bugmail)]

Kai, 
I'm pretty sure that the issue with certs in the temp cert db is a real issue, 
and relevant to this problem.  It may be masked, in the short term, by the 
issue with libPKIX validating a chain to a an anchor not in the list, but 
once that issue is resolved, I expect the temp cert DB issue will again need
to be solved.  So, hold on to the patch you developed to do the EV evaluation
in the auth-cert callback.  You might even want to attach it here.

Comment 40

[Johnathan Nightingale [:johnath]]

Note to drivers - in my opinion, this should block beta 5.  It fixes our certificate verification code to handle some complicated cross-signing cases, and I'd want a beta to make sure it gets some exposure to real-world configurations.

Comment 41

[Kai Engert [:KaiE:]]

Attachment: Patch v3

Slightly updated patch that resolves a merge conflict after today's check in for bug 374336.

Comment 42

[Kai Engert [:KaiE:]]

I'm making this dependent on bug 294531.
I'm updating this patch to include the PSM changes that are required on top of the changes to the EV revocation checking API.

Comment 43

[Kai Engert [:KaiE:]]

Attachment: Patch v4

You can use the interdiff feature to compare v3 with v4, and you'll see the PSM changes that are based on the latest API in bug 294531.

Comment 44

[Kai Engert [:KaiE:]]

Attachment: Patch v5

Fix. Now tested.

Comment 45

[Robert Relyea]

Comment on attachment 309497 [details] [diff] [review]
Patch v5

r+ with the caveat we need to fix the 'Require_info_onMissing' and 'Fail_on_missing_fresh_info'

as revocation independent flags..

Comment 46

[Robert Relyea]

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

V5 obsoletes this patch

Comment 47

[Kai Engert [:KaiE:]]

Attachment: Patch v6

This patch has the flag method independent flag
  CERT_REV_MI_REQUIRE_SOME_FRESH_INFO_AVAILABLE
added, which we just defined in bug 294531.

Comment 48

[Kai Engert [:KaiE:]]

Attachment: real Patch v6 [checked in]

sorry, this is the correct patch v6

Comment 49

[Robert Relyea]

Comment on attachment 309526 [details] [diff] [review]
real Patch v6 [checked in]

r+ rrelyea

Comment 50

[Damon Sicore (:damons)]

Blocking B5 based on comment #40.

Comment 51

[Johnathan Nightingale [:johnath]]

Comment on attachment 309526 [details] [diff] [review]
real Patch v6 [checked in]

Clearing approval flag - this is now a P1 blocker, and tree rules (
http://wiki.mozilla.org/TreeStatus ) say it can land without patch-level
approval.  Assuming you have all the necessary reviews, you're good to go.

Comment 52

[Kai Engert [:KaiE:]]

checked in

Comment 53

[Johnathan Nightingale [:johnath]]

On Mac trunk with latest code, I crash when loading an EV site.  Not positive it's this bug, but it's the most obvious candidate.  Re-opening.

Stack:

Exception Type:  EXC_BAD_ACCESS (SIGBUS)
Exception Codes: KERN_PROTECTION_FAILURE at 0x0000000000000048
Crashed Thread:  0

Thread 0 Crashed:
0   libnss3.dylib                 	0x12f9e320 cert_pkixSetParam + 340
1   libnss3.dylib                 	0x12f9e6bc CERT_PKIXVerifyCert + 232
2   libpipnss.dylib               	0x12efeea9 nsNSSCertificate::hasValidEVOidTag(SECOidTag&, int&) + 825
3   libpipnss.dylib               	0x12eff153 nsNSSCertificate::getValidEVOidTag(SECOidTag&, int&) + 83
4   libpipnss.dylib               	0x12eff2e6 nsNSSCertificate::GetIsExtendedValidation(int*) + 134
5   libpipnss.dylib               	0x12efe815 nsNSSSocketInfo::GetIsExtendedValidation(int*) + 149
6   libpipboot.dylib              	0x12eb2db1 nsSecureBrowserUIImpl::EvaluateAndUpdateSecurityState(nsIRequest*) + 401
7   libpipboot.dylib              	0x12eb3182 nsSecureBrowserUIImpl::OnLocationChange(nsIWebProgress*, nsIRequest*, nsIURI*) + 594
8   libdocshell.dylib             	0x00c27c9c nsDocLoader::FireOnLocationChange(nsIWebProgress*, nsIRequest*, nsIURI*) + 172
9   libdocshell.dylib             	0x00c15745 nsDocShell::CreateContentViewer(char const*, nsIRequest*, nsIStreamListener**) + 965
10  libdocshell.dylib             	0x00c201b3 nsDSURIContentListener::DoContent(char const*, int, nsIRequest*, nsIStreamListener**, int*) + 163
11  libdocshell.dylib             	0x00c24e67 nsDocumentOpenInfo::TryContentListener(nsIURIContentListener*, nsIChannel*) + 375
12  libdocshell.dylib             	0x00c251df nsDocumentOpenInfo::DispatchContent(nsIRequest*, nsISupports*) + 559
13  libdocshell.dylib             	0x00c25bf5 nsDocumentOpenInfo::OnStartRequest(nsIRequest*, nsISupports*) + 181
14  libnecko.dylib                	0x12dc93bb nsHttpChannel::CallOnStartRequest() + 251
15  libnecko.dylib                	0x12dcd032 nsHttpChannel::ProcessNormal() + 226
16  libnecko.dylib                	0x12d51edc nsInputStreamPump::OnStateStart() + 76
17  libnecko.dylib                	0x12d52258 nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*) + 184
18  libxpcom_core.dylib           	0x00181e6d nsInputStreamReadyEvent::Run() + 45
19  libxpcom_core.dylib           	0x0019f7ea nsThread::ProcessNextEvent(int, int*) + 218
20  libxpcom_core.dylib           	0x0015fe27 NS_ProcessPendingEvents_P(nsIThread*, unsigned int) + 71
21  libwidget_mac.dylib           	0x133dec6f nsBaseAppShell::NativeEventCallback() + 95
22  libwidget_mac.dylib           	0x133b283a nsAppShell::ProcessGeckoEvents(void*) + 634
23  com.apple.CoreFoundation      	0x9145062e CFRunLoopRunSpecific + 3166
24  com.apple.CoreFoundation      	0x91450d18 CFRunLoopRunInMode + 88
25  com.apple.HIToolbox           	0x929566a0 RunCurrentEventLoopInMode + 283
26  com.apple.HIToolbox           	0x929564b9 ReceiveNextEventCommon + 374
27  com.apple.HIToolbox           	0x9295632d BlockUntilNextEventMatchingListInMode + 106
28  com.apple.AppKit              	0x91e6d7d9 _DPSNextEvent + 657
29  com.apple.AppKit              	0x91e6d08e -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] + 128
30  com.apple.AppKit              	0x91e660c5 -[NSApplication run] + 795
31  libwidget_mac.dylib           	0x133b1b9a nsAppShell::Run() + 186
32  libtoolkitcomps.dylib         	0x1324ba67 nsAppStartup::Run() + 71
33  XUL                           	0x0004fe08 XRE_main + 10040
34  org.mozilla.firefox           	0x00002d17 main + 231
35  org.mozilla.firefox           	0x0000259c _start + 210
36  org.mozilla.firefox           	0x000024c9 start + 41

Comment 54

[Jim Jeffery not reading bug-mail 1/2/11]

Also crashes on Windows Vista HP Sorry no Breakpad 
Stifu in the forums says he submitted BP reports.

http://forums.mozillazine.org/viewtopic.php?p=3298643#3298643

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b5pre) Gecko/2008031705 Minefield/3.0b5pre Firefox/3.0 Firefox/2.0.0.12 ID:2008031705

Comment 55

[Johnathan Nightingale [:johnath]]

Confirmed, here's a Vista crash-report of my own.

http://crash-stats.mozilla.com/report/index/5fbc7978-f433-11dc-b2d7-001a4bd43ef6

https://www.quovadis.bm is a consistent site for reproducing this bug, for me.

Comment 56

[Kai Engert [:KaiE:]]

The crash is a regression caused by the code from this bug, and it's now being tracked in bug 423475.

Comment 57

[Johnathan Nightingale [:johnath]]

(In reply to comment #56)
> The crash is a regression caused by the code from this bug, and it's now being
> tracked in bug 423475.

And if I were behaving myself, I would have just opened a new bug anyhow, since that's cleaner.  That one is marked as blocking now, so I'm happy to re-close this one.

Comment 58

[Kai Engert [:KaiE:]]

Attachment: regression fix for debug code

When I produced the fix for this bug, I had not tested the debug-only code that allows one to load EV blessings from an external text file, and I've broken it.

Here's a fix.

Comment 59

[Kai Engert [:KaiE:]]

Attachment: test temp db

(In reply to comment #38)
> Kai, please file an NSS bug about this, P1, assign to Alexei.  

For the record, Nelson has filed bug 422859.


(In reply to comment #39)
> I expect the temp cert DB issue will again need
> to be solved.  So, hold on to the patch you developed to do the EV evaluation
> in the auth-cert callback.  You might even want to attach it here.

Attached.
It's just what I used for testing.
Note it has two lines with a call to GetIsExtendedValidation, one disabled, one enabled.
If the current patch doesn't help, you might try to switch which calls is enabled and disabled.

Comment 60

[Kai Engert [:KaiE:]]

Attachment: test patch for comment 33, 34, 35

Comment 61

[Nelson Bolyard (seldom reads bugmail)]

The issue which is discussed in comment 33 - comment 39 above, and also in 
bug 422859 comment 15 is now understood, and is really part of this bug,
so I am reopening this bug.  

If you disable http keep-alive, then with an empty cert DB, visit https://secure.comodo.com, it shows as non-EV.  We know why now.  
The set of certs received from a server is kept around (in the "temp cert db")
for the lifetime of the SSL (https) connection.  That's a long time if 
keep-alive is enabled, and a very short time if not.  It's long enough that
when keep-alive is in use, the certs are still around, after the handshake, 
when the EV test is done, because the SSL connection is not closed immediately  when the https response is received.  But when keep-alive is not used, either
because the client or the server has disabled it, then the connection gets 
closed immediately, and the certs are discarded before the EV test is done.

To test this: 
1) go to about:config
2) Type network.http.keep-alive into the filter text box
3) If that pref is set to "true" (the default) double click it to toggle it.
4) exit the browser
5) move aside the cert DB
6) restart the browser
7) visit https://secure.comodo.com

Kai has a patch for this problem already attached to this bug, named 
"test temp db".

Also, there is another patch attached to this bug that seems to be awaiting
review.  What is its status?

Comment 62

[Kai Engert [:KaiE:]]

(In reply to comment #61)
> Kai has a patch for this problem already attached to this bug, named 
> "test temp db".

While this patch helps in the immediate tests, I can not guarantee that this patch is always sufficient.

The EV verification result, which PSM attempts to cache, might get lost.
If it got lost, and PSM attempt to repeat the EV verification, then it will fail.
It will fail, because the required intermediates are gone.


> Also, there is another patch attached to this bug that seems to be awaiting
> review.  What is its status?

You are referring to the "regression fix for debug code".
Yes, this patch is still waiting for review and we must check it in, once reviewed.
It fixes code that only gets built in debug builds.

Comment 63

[Kai Engert [:KaiE:]]

Attachment: Earlier EV verification, patch 7

Cleaned up version of the earlier "test temp db" patch.

As Nelson said, this patch helps in the scenario where keep alive is turned off.

It has the following performance disadvantage:
We'll perform an EV verification for every single SSL handshake.

(As of today, without this patch, EV verification only happens for the SSL connection associated to the toplevel document of a window.)


As I said in the previous comment, I'm worried about a scenario where we'd have to repeat the EV verification at a later time, and we end up switching to non-EV because of the missing intermediates.

However, so far I've been unable to reproduce such a scenario. This patch might be sufficient.

If we happen to run into that scenario, we'll have to work harder to get it right. Then we'll require that NSS provides the list of handshake certificates into PSM, allowing PSM to hold on to them

Comment 64

[Kai Engert [:KaiE:]]

Attachment: Earlier EV verification, patch 8

This improved patch could avoid some verification calls.

Comment 65

[Rob Stradling]

(in reply to comment #63)
> As I said in the previous comment, I'm worried about a scenario where we'd
> have to repeat the EV verification at a later time, and we end up switching to
> non-EV because of the missing intermediates.

Kai, if you're worried, then I'm worried.  :-(

> However, so far I've been unable to reproduce such a scenario. This patch
> might be sufficient.

Whilst this patch definitely improves the situation when keepalive is disabled, I'm afraid that "might be sufficient" still leaves me feeling worried.

> If we happen to run into that scenario, we'll have to work harder to get it
> right. Then we'll require that NSS provides the list of handshake certificates
> into PSM, allowing PSM to hold on to them

Is there anything to stop you from taking this approach now?
That would make me much less worried.  :-)

Comment 66

[Johnathan Nightingale [:johnath]]

(In reply to comment #65)
> (in reply to comment #63)
> > As I said in the previous comment, I'm worried about a scenario where we'd
> > have to repeat the EV verification at a later time, and we end up switching to
> > non-EV because of the missing intermediates.
> 
> Kai, if you're worried, then I'm worried.  :-(
> 
> > However, so far I've been unable to reproduce such a scenario. This patch
> > might be sufficient.
> 
> Whilst this patch definitely improves the situation when keepalive is disabled,
> I'm afraid that "might be sufficient" still leaves me feeling worried.
> 
> > If we happen to run into that scenario, we'll have to work harder to get it
> > right. Then we'll require that NSS provides the list of handshake certificates
> > into PSM, allowing PSM to hold on to them
> 
> Is there anything to stop you from taking this approach now?
> That would make me much less worried.  :-)

Not that I don't support being proactive here, but I also think that if we can't come up with such a scenario up front, and if it is the kind of thing we can fix in a point release if it comes up, then I would hope we wouldn't hold back a P1 blocker to work a larger change like this.  PSM is Kai's domain, not mine, but my vote would be that we cover that case independently of this one, unless I misunderstand the likelihood of failures there? 

Comment 67

[Nelson Bolyard (seldom reads bugmail)]

(In reply to comment #63)
> If we happen to run into that scenario, we'll have to work harder to get it
> right. Then we'll require that NSS provides the list of handshake 
> certificates into PSM, allowing PSM to hold on to them

Bug 324867 proposes an API by which an app (PSM) could get the peer's list 
of certs.  

(In reply to comment #66)
> Not that I don't support being proactive here, but I also think that if we
> can't come up with such a scenario up front, and if it is the kind of thing we
> can fix in a point release if it comes up, then I would hope we wouldn't hold
> back a P1 blocker to work a larger change like this.  

Yes, I think that we can do this in a point release, if necessary.  
I agree that this hypothetical problem isn't a P1 release blocker.  

However, the situation that occurs in the absence of http keep-alive is not
merely hypothetical.  If & when Kai's "Earlier EV verification" patch lands, 
I think this bug will be resolved adequately for FF 3.000.

Kai, are you going to request review for that patch?

Comment 68

[Rob Stradling]

(in reply to comment #67)
> Kai, are you going to request review for that patch?

Kai, sorry to nag you, but I'd really like to see your "Earlier EV verification" patch make it into Firefox 3.0.0.0, and I understand that the code freeze is very near.

Are you going to request review for this patch?

(I hope the answer is "yes". This bug is blocking1.9+ after all :-) ).

Comment 69

[Kai Engert [:KaiE:]]

(In reply to comment #67)
> Kai, are you going to request review for that patch?

(In reply to comment #68)
> 
> Are you going to request review for this patch?

You should see that by now there is a review request for this patch, but we are all swamped with work.

Comment 70

[Kai Engert [:KaiE:]]

I agree with Nelson's and Johnathan's latest comments.

Yes, at this point my worries are hypothetical, given that I'm unable to reproduce with our default settings.

However, my worries are real if someone decides to disable the http-keep-alive feature.

I agree that we should start with the current patch and hope it's sufficient for the immediate release.

Should we learn at later time that my worries were justified, we'll try to fix it then, and I'm thankful that Nelson has began to work on bug 324867.

Comment 71

[Nelson Bolyard (seldom reads bugmail)]

> my worries are real if someone decides to disable the http-keep-alive 

It is a server admin who is likely to do this, not the browser user, IMO.
Either way, the effect is the same.

Comment 72

[Kai Engert [:KaiE:]]

Comment on attachment 312419 [details] [diff] [review]
Earlier EV verification, patch 8

Let me explain how this patch works:

The code gets added in function AuthCertificateCallback, which is a callback function, called by NSS during SSL connections.

The context which initiated the current socket needs to know whether the server cert used by the current SSL socket has EV status. But the code which consumes the EV status gets executed much later, from a completely different stack. However, that other code will still have access to information we "attach" to that socket.

In fact, the objects which can carry that information are already in place. We just need to make sure the information is retrieved. The EV status will be cached, once obtained.

How does it work?

There is a chain of objects.

fd points to a SocketInfo object, maintained by PSM, in fd->higher->secret.

The SocketInfo object contains a SSLstatus object, which is available to the rest of Mozilla (PSM provides the SSLStatus object to the outside world, when asked for it - per socket).

The SSLStatus object contains a member: a server cert. The server cert is a wrapper object, maintained by PSM, around a CERTCertificate (named nsNSSCertificate).

And nsNSSCertificate is the one who caches the result of a EV verification.


As you can see in the patch, the callback function already creates a wrapper object nsNSSCertificate (if necessary). Most of the patch moves creation of that object to an earlier point of the function.

Because as soon as we found the file descriptor and NSS default implementation of SSL_AuthCertificate was able to verify the cert (indicated by rv==SECSuccess), we want to check the EV status and have the wrapper object to cache the status.

And further down we'll store that cert wrapper object into the SSLStatus object associated to the current context for later retrieval by other consumers (like the Firefox security indicators).

Comment 73

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 312419 [details] [diff] [review]
Earlier EV verification, patch 8

This patch consists mostly of reordering certain existing steps in the function, 
and adding one new step, namely this one:

>+        nsc->GetIsExtendedValidation(&dummyIsEV); // the nsc object will cache the status

I'm convinced that the newly ordered code is at least as correct as the old
code.  I have not reviewed method GetIsExtendedValidation, but it is not new code.

Comment 74

[Wan-Teh Chang]

Comment on attachment 312419 [details] [diff] [review]
Earlier EV verification, patch 8

r=wtc.

If possible, the code that uses |status| should be moved
up along with the declaration of |status| so that all code
that uses |status| stays together.

Comment 75

[Kai Engert [:KaiE:]]

I'd rather avoid moving that code in order to avoid unwanted side effects by communicating the SSL status to the outside world, when we have not yet executed the code in the middle of that function.

Comment 76

[Kai Engert [:KaiE:]]

Checked in, marking fixed again.

I'll open another bug for an issue that Nelson noticed.
The issue is not with the patches, but applies to the callback functions.
In theory the server cert used on a single SSL socket might change in a secondary handshake, but PSM's callback functions are currently not prepared to deal with that.
(But it appears unlikely that would happen.)

Comment 77

[Kai Engert [:KaiE:]]

I must reopen this bug.
The patch caused our test suites to fail.

While the patch looks really unsuspicious, it had the side effect that function GetIsExtendedValidation is now being executed on a different thread.

Apparently for the time we are accessing functions from class nsNSSCertificateDB from different threads.

That code is currently being declared as "not thread safe", and as soon as some self protection code detects multiple thread access, it triggers an assertion. Mozilla's test suite treated that as fatal and aborted execution.

So, at first sight I suspected the fix would be really complicated.

However, after looking at it a bit, I'm now very optimistic.

It turns out that class nsNSSCertificateDB doesn't have any state, no member variables at all. It's simply a collection of functions that call into NSS. Which should be thread safe.

So, I think we can risk to declare class nsNSSCertificateDB as threadsafe, which immediately avoids the fatal assertion.

For example, the functionality we access from within GetIsExtendedValidation is really simply: We query whether OCSP is turned on or turned off.

Comment 78

[Kai Engert [:KaiE:]]

Attachment: Earlier EV verification, patch 9

Comment 79

[Kai Engert [:KaiE:]]

Look here:

http://lxr.mozilla.org/seamonkey/source/security/manager/ssl/src/nsNSSCertificateDB.h

As a test I declared the function getCertNames, handleCACertDownload and getCertsFromPackage as static function. As expected, this builds fine, too.

You'll see this class doesn't have any member variables. The nsI* base classes are interfaces and only contain functions as well.


I think it's ok to declare the class as threadsafe, which is the single change from patch 8 to patch 9.

Comment 80

[Robert Relyea]

Comment on attachment 314248 [details] [diff] [review]
Earlier EV verification, patch 9

r+

Comment 81

[Robert Relyea]

Comment on attachment 311692 [details] [diff] [review]
regression fix for debug code

r+

Comment 82

[Kai Engert [:KaiE:]]

I checked in both missing patches 2 hours ago.
I've not yet marked the patch as fixed, because I had intended to wait for a green cycle. But let's move this off your radar, marking fixed.

Comment 83

[Kai Engert [:KaiE:]]

we're good, all automated tests passed (the usual ones, not related to this bug)