764393 - Quick fix required for the chain-building looping bug

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Rob Stradling]

Bug 479508 and Bug 634074 were filed well over 3 years ago.  Back then, it was decided that the proper fix was to wait for Firefox to migrate to libpkix by default.  We (COMODO) and our customers have been waiting patiently for this fix, but the effects of the bug have apparently been getting worse over time and we don't believe that we can tolerate this bug for very much longer.

Bug 651246 suggests that "libpkix by default" is still some way off, so we've concluded that we _really need_ a quick fix to be applied to the "old" chain building code.

I'm about to attach 2 patches, which take different approaches to fixing this bug.

Comment 1

[Rob Stradling]

Attachment: Nelson's "disgusting hack"

This patch combines Nelson's patches from Bug 479508 and Bug 482153, with minor edits to make it apply cleanly and compile.
I've tested this patch with the various COMODO certificate chains affected by the bug, and AFAICT this patch is an effective fix.

Comment 2

[Rob Stradling]

Attachment: Blacklist "never best certs"

This patch causes the nssCertificateArray_FindBestCertificate() function to consult a blacklist of certificates that should never be regarded as "best".
To avoid having to list lots of similar AddTrust<-->UTN cross-certificates, it does a substring match on the Issuer and Subject names.
The optional serial number field makes it possible to blacklist only certain "versions" of a Root Certificate.

This patch also causes PSM's Certificate Viewer to display a sensible chain, whereas with Nelson's patch the 20-cert-long chain described in Bug #479029 Comment 16 still gets shown.
For this reason, I prefer my patch.

Comment 3

[Rob Stradling]

(In reply to Rob Stradling from comment #0)
> Bug 479508 and Bug 634074 were filed well over 3 years ago.

Sorry, I meant to say "Bug 479508 and Bug 482153".

Comment 4

[Wan-Teh Chang]

Comment on attachment 632704 [details] [diff] [review]
Blacklist "never best certs"

Review of attachment 632704 [details] [diff] [review]:
-----------------------------------------------------------------

r=wtc.  Thank you for the patch.  Please attach a new patch with my
suggested changes.

High-level comments:

1. Please confirm that the certs in the "never best" blacklist really
are never best, not even when they are the only cert in the |certs| array.

In other words, I am not sure whether those certs are "truly never best" or
"never best if there are other options".

2. It would be nice to move your code, including the "never best" blacklist,
into a new function.

::: security/nss/lib/pki/pkibase.c
@@ +460,5 @@
>  
> +struct NeverBestCertificate {
> +    const char *issuerName_needle;
> +    const char *subjectName_needle;
> +    const char *serialNumber_base64;

Please add comments to document the three fields of the structure.  In particular,
there are two non-obvious things about the first two fields:

1. They are used for substring match (using strstr as opposed to strcmp).
2. The "_needle" in their names is not clear.

It may make sense to store the serial number in binary, which would represent
the need to call NSSBase64_DecodeBuffer and SECITEM_FreeItem every time such a
never-best certificate is encountered.  Since that is expected to be uncommon,
I guess it's not worth optimizing for that?

@@ +473,5 @@
> +    "IKTEf93f4cdTYwcTiHdgEg==" }	/* 0x20A4C47FDDDFE1C75363071388776012 */
> +};
> +
> +static const unsigned int numNeverBestCerts =
> +    (sizeof neverBestCerts) / (sizeof neverBestCerts[0]);

You can use PR_ARRAY_SIZE(neverBestCerts).  It's not necessary to define
this constant.

@@ +497,5 @@
>      }
>      if (!certs) {
>  	return (NSSCertificate *)NULL;
>      }
> +    int i = 0;

Delete this unused variable.  (The 'i' inside the for loop is used.)

@@ +502,5 @@
>      for (; *certs; certs++) {
>  	nssDecodedCert *dc, *bestdc;
>  	NSSCertificate *c = *certs;
>  	dc = nssCertificate_GetDecoding(c);
>  	if (!dc) continue;

Since your code doesn't depend on 'dc', is it OK to move your code
before the "dc = nssCertificate_GetDecoding(c);" line?

Or is there actually an implicit dependency on 'dc'?

@@ +507,5 @@
> +
> +	/* don't take this cert if it matches an entry in the "never best"
> +	 * blacklist
> +	 */
> +	CERTCertificate *cert = STAN_GetCERTCertificate(c);

'cc' is the common name for a variable of the CERTCertificate type in this file.

@@ +509,5 @@
> +	 * blacklist
> +	 */
> +	CERTCertificate *cert = STAN_GetCERTCertificate(c);
> +	if (cert) {
> +	    int i;

It would be nice to point that i = -1 means there is a match.

@@ +521,5 @@
> +			 */
> +			i = -1;
> +			break;
> +		    }
> +		    SECItem *serial = NSSBase64_DecodeBuffer(NULL, NULL,

"SECItem *serial" needs to be declared at the beginning of this lexical scope.
Some older compilers still require that.

Comment 5

[Wan-Teh Chang]

(In reply to Wan-Teh Chang from comment #4)
>
> Review of attachment 632704 [details] [diff] [review]:
...
> It may make sense to store the serial number in binary, which would represent
> the need to call NSSBase64_DecodeBuffer and SECITEM_FreeItem every time such
> a
> never-best certificate is encountered.

I have a typo: s/represent/remove

Comment 6

[Rob Stradling]

(In reply to Wan-Teh Chang from comment #4)
> r=wtc.  Thank you for the patch.  Please attach a new patch with my
> suggested changes.

Wan-Teh, thanks for the review.  I've prepared a new patch.

> 1. Please confirm that the certs in the "never best" blacklist really
> are never best, not even when they are the only cert in the |certs| array.
> 
> In other words, I am not sure whether those certs are "truly never best" or
> "never best if there are other options".

The cross-certificates in the blacklist are "truly never best", because there will always be an NSS built-in root certificate in the |certs| array that can (and should!) be used instead.

The root certificate in the blacklist is also "truly never best", because it is not an NSS built-in root, and there exists another root certificate with the same name/key that is an NSS built-in root.

> 2. It would be nice to move your code, including the "never best" blacklist,
> into a new function.

Done.

<snip>
> > +struct NeverBestCertificate {
<snip>
> Please add comments to document the three fields of the structure.  In
> particular, there are two non-obvious things about the first two fields:
> 
> 1. They are used for substring match (using strstr as opposed to strcmp).

Comments added.

> 2. The "_needle" in their names is not clear.

"man strstr" says:
char *strstr(const char *haystack, const char *needle);

> It may make sense to store the serial number in binary, which would represent
> the need to call NSSBase64_DecodeBuffer and SECITEM_FreeItem every time such
> a never-best certificate is encountered.  Since that is expected to be
> uncommon, I guess it's not worth optimizing for that?

It's not worth optimizing for this case.  The serial number field is only used for the blacklisted root certificate, which I don't expect to be encountered very often.

<snip>
> > +static const unsigned int numNeverBestCerts =
> > +    (sizeof neverBestCerts) / (sizeof neverBestCerts[0]);
> 
> You can use PR_ARRAY_SIZE(neverBestCerts).  It's not necessary to define
> this constant.

Changed.

<snip>
> > +    int i = 0;
> 
> Delete this unused variable.  (The 'i' inside the for loop is used.)

Done.

<snip>
> >  	dc = nssCertificate_GetDecoding(c);
> >  	if (!dc) continue;
> 
> Since your code doesn't depend on 'dc', is it OK to move your code
> before the "dc = nssCertificate_GetDecoding(c);" line?
> Or is there actually an implicit dependency on 'dc'?

Moved.

<snip>
> > +	CERTCertificate *cert = STAN_GetCERTCertificate(c);
> 
> 'cc' is the common name for a variable of the CERTCertificate type in this
> file.

Changed.

<snip>
> It would be nice to point that i = -1 means there is a match.

My new patch has a function which returns PR_TRUE instead of setting i = -1, but I've added a comment that PR_TRUE means there is a match.

<snip>
> "SECItem *serial" needs to be declared at the beginning of this lexical
> scope.
> Some older compilers still require that.

Changed.

Comment 7

[Rob Stradling]

Attachment: Blacklist "never best certs" V2

V2 patch, addressing wtc's review comments.

Comment 8

[Wan-Teh Chang]

Comment on attachment 638331 [details] [diff] [review]
Blacklist "never best certs" V2

Review of attachment 638331 [details] [diff] [review]:
-----------------------------------------------------------------

r=wtc.  I suggest some fixes for coding style nits.

::: security/nss/lib/pki/pkibase.c
@@ +462,5 @@
> + * nssCertificateArray_FindBestCertificate() should never return.
> + */
> +struct NeverBestCertificate {
> +    /* A substring that must be present in cert->issuerName */
> +    const char *issuerName_needle;

The 'haystack' and 'needle' argument names are not used in all strstr man
pages.  For example, this strstr man page uses the nondescriptive 's1' and
's2' argument names:
http://pubs.opengroup.org/onlinepubs/009604599/functions/strstr.html

@@ +480,5 @@
> +   */
> +  { "CN=AddTrust External CA Root", "CN=UTN", NULL },
> +  { "CN=UTN", "CN=AddTrust External CA Root", NULL },
> +  { "CN=AddTrust External CA Root", "CN=COMODO Certification Authority", NULL },
> +  /* Root Certificates that are not built-in but which match the Names/Keys of

Nit: we should use either "that" or "which" consistently in "that are not built-in"
and "but which match ..."?

@@ +489,5 @@
> +  { "CN=COMODO Certification Authority", "CN=COMODO Certification Authority",
> +    "IKTEf93f4cdTYwcTiHdgEg==" }	/* 0x20A4C47FDDDFE1C75363071388776012 */
> +};
> +
> +static PRBool is_never_best_certificate (

The function name should be IsNeverBestCertificate of
nssCertificate_IsNeverBestCertificate.

@@ +500,5 @@
> +	for (i = 0; i < PR_ARRAY_SIZE(neverBestCerts); i++) {
> +	    if (strstr(cc->issuerName,
> +			neverBestCerts[i].issuerName_needle)
> +		    && strstr(cc->subjectName,
> +				neverBestCerts[i].subjectName_needle)) {

Nit: I think it's better to align the second argument of strstr() with the
first argument, for example,
    if (strstr(cc->issuerName,
               neverBestCerts[i].issuerName_needle) &&
        strstr(cc->subjectName,
               neverBestCerts[i].subjectName_needle)) {

@@ +503,5 @@
> +		    && strstr(cc->subjectName,
> +				neverBestCerts[i].subjectName_needle)) {
> +		SECItem *serial;
> +		/* any serial number for this Issuer/Subject is a match
> +		 */

Nit: format one-line comments like this:
  /* any serial number for this Issuer/Subject is a match */

Please make the same change to the other one-line comments in your patch.

@@ +504,5 @@
> +				neverBestCerts[i].subjectName_needle)) {
> +		SECItem *serial;
> +		/* any serial number for this Issuer/Subject is a match
> +		 */
> +		if (!(neverBestCerts[i].serialNumber_base64))

Nit: the inner parantheses aren't necessary.

Similarly, the parentheses in &(cc->serialNumber) below aren't necessary.

Comment 9

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 638331 [details] [diff] [review]
Blacklist "never best certs" V2

Rob, the idea of your patch seems reasonable as far as minimizing risk and being easy to review and understand, considering the positive impact it has. Thanks for writing it.

I moved wtc's sr+ to an r+ so that I can request super-review from Bob, the NSS module owner. If Bob feels uncomfortable with having this patch in the official NSS release, then Mozilla should maintain it as a private patch just for Firefox (and Thunderbird). But, I think we're better off having the patch in the NSS trunk.

Comment 10

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Kai and Kathleen, please review the discussion above.

Comment 11

[Kathleen Wilson]

(In reply to Brian Smith (:bsmith) from comment #10)
> Kai and Kathleen, please review the discussion above.

Done. It'll be nice to get this fixed. Thanks!

Comment 12

[Rob Stradling]

Attachment: Blacklist "never best certs" V3

(In reply to Wan-Teh Chang from comment #8)
<snip>
> r=wtc.  I suggest some fixes for coding style nits.

Thanks Wan-Teh.  V3 of the patch addresses these coding style nits as follows...

<snip>
> The 'haystack' and 'needle' argument names are not used in all strstr man
> pages.

I've changed "needle" to "substring" in these argument names.

<snip>
> > +  /* Root Certificates that are not built-in but which match the Names/Keys
> > of
> 
> Nit: we should use either "that" or "which" consistently in "that are not
> built-in" and "but which match ..."?

I've changed "which" to "that" in the comment.

<snip>
> > +static PRBool is_never_best_certificate (
> 
> The function name should be IsNeverBestCertificate of
> nssCertificate_IsNeverBestCertificate.

Changed to "IsNeverBestCertificate".

<snip>
> Nit: I think it's better to align the second argument of strstr() with the
> first argument,

Done.

<snip>
> Nit: format one-line comments like this:
>   /* any serial number for this Issuer/Subject is a match */
> 
> Please make the same change to the other one-line comments in your patch.

Done.

<snip>
> > +		if (!(neverBestCerts[i].serialNumber_base64))
> 
> Nit: the inner parantheses aren't necessary.
>
> Similarly, the parentheses in &(cc->serialNumber) below aren't necessary.

I've removed them.

Comment 13

[Rob Stradling]

Brian, I see that the next Aurora merge is scheduled for Monday.  If Bob is uncomfortable with having my patch in NSS trunk or if he is unavailable to make a quick decision, would you be happy to commit it and "maintain it as a private patch just for Firefox (and Thunderbird)", so that we can at least get a working fix out to our users in FF16?

If a better solution comes along in the future, you can always remove my code at that point!

Comment 14

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

I am fine with that but it still depends on getting some agreement from Kai or Bob.

Most likely, the patch will have to land on nightly after the merge and then we'll try to backport it to the Aurora branch after the merge--assuming we reach some agreement (which then also requires agreement from release drivers).

Comment 15

[Robert Relyea]

> 1. Please confirm that the certs in the "never best" blacklist really
> are never best, not even when they are the only cert in the |certs| array.

This is also my main issue. Are these certs blacklisted because they are actually invalid, or are they black listed to prefer a root cert. Do they actually chain to a trusted root cert currently?

> The cross-certificates in the blacklist are "truly never best", because there will always be
> an NSS built-in root certificate in the |certs| array that can (and should!) be used instead.

Also, is the intermediates in this list replacements for actual trusted root certs in the store? That is would we expect to find the trusted root cert in this list? If so I would rather white list the trusted cert than black list a random intermediate.

Not all NSS apps use the builtins, I'm not sure I want to have code that effectively hides random certs hard coded into NSS without any ability of the user to turn it off or on.

If these certs are truly mirrored in the builtin's I would rather and (a much simplier and safer) patch to prefer a trusted cert if it's in the list.

bob

Comment 16

[Rob Stradling]

(In reply to Robert Relyea from comment #15)
> > 1. Please confirm that the certs in the "never best" blacklist really
> > are never best, not even when they are the only cert in the |certs| array.
> 
> This is also my main issue. Are these certs blacklisted because they are
> actually invalid,

They are not actually invalid.

> or are they black listed to prefer a root cert.

Yes.

> Do they actually chain to a trusted root cert currently?

Yes.

> > The cross-certificates in the blacklist are "truly never best", because there will always be
> > an NSS built-in root certificate in the |certs| array that can (and should!) be used instead.
> 
> Also, is the intermediates in this list replacements for actual trusted root
> certs in the store?

Yes.

> That is would we expect to find the trusted root cert in this list?

Yes.

> If so I would rather white list the trusted cert than black list
> a random intermediate.
> 
> Not all NSS apps use the builtins, I'm not sure I want to have code that
> effectively hides random certs hard coded into NSS without any ability of
> the user to turn it off or on.
> 
> If these certs are truly mirrored in the builtin's I would rather and (a
> much simplier and safer) patch to prefer a trusted cert if it's in the list.
> 
> bob

OK, I agree that whitelisting the trusted roots would be a better/safer approach.

Do all NSS apps have the builtins available (even if they choose not to trust them)?  If yes, then would it be better for nssCertificateArray_FindBestCertificate to:
a. have its own list of whitelisted roots?
or
b. check to see if each candidate cert is a builtin, and if it is, prefer it?

I believe you're suggesting option a, but I'd like to rule out option b before I have a go at writing a whitelist-based patch.  Thanks.

Comment 17

[Robert Relyea]

> OK, I agree that whitelisting the trusted roots would be a better/safer approach.
> 
> Do all NSS apps have the builtins available (even if they choose not to trust them)?
>  If yes, then would it be better for nssCertificateArray_FindBestCertificate to:

No, that's part of the point. The white listing should also *NOT* be by putting code in saying "I prefer this specific cert", but by putting code in that says "If one of the certs in the list is trusted for the requested usage, prefer it".

> a. have its own list of whitelisted roots?
> or
> b. check to see if each candidate cert is a builtin, and if it is, prefer it?

Neither. We check to see if the cert is trusted. This handles the case where you may have an old builtins, you may have your one list of trusted certsm etc. The user can still control trust or not trust by tweaks to his own database.

NOTE: the builtins are not the only source of trust (or even the primary source of trust).

bob

Comment 18

[Rob Stradling]

(In reply to Robert Relyea from comment #17)
<snip>
> > Do all NSS apps have the builtins available (even if they choose not to trust
> > them)?
<snip>
> 
> No, that's part of the point. The white listing should also *NOT* be by
> putting code in saying "I prefer this specific cert", but by putting code in
> that says "If one of the certs in the list is trusted for the requested
> usage, prefer it".

That's Bug 482153 (isn't it?), which it seems even some experienced NSS developers have struggled to understand!  If I could fix that bug, I would.

I raised this bug because I can't fix Bug 482153, nobody else is working on it, and we (Comodo and our customers) urgently need the problematic behaviour to be fixed in Firefox!

Comment 19

[Wan-Teh Chang]

Attachment: Nelson's approach

Rob: I cleaned up Nelson's "disgusting hack" (attachment 632700 [details] [diff] [review]).

Please review and test this patch.  Could you also tell me how I can test
it myself?  Thanks.

Comment 20

[Wan-Teh Chang]

Comment on attachment 644136 [details] [diff] [review]
Nelson's approach

Bob: please also review this patch.

The nss3certificate_isTrustedForUsage function is based on the
pkix_pl_Cert_GetTrusted function that you wrote for libpkix.
There are two places in nss3certificate_isTrustedForUsage that
I am not sure about.

>+    /* This is for NSS 3.3 functions that do not specify a usage */
>+    if (usage->anyUsage) {
>+	return PR_FALSE;  /* XXX is this right? */
>+    }

This is the first place: what to do if usage->anyUsage is true.
My code essentially ignores cert trust in that case, falling back
on the original behavior.

>+    if (trustType == trustTypeNone) {
>+	/* normally trustTypeNone usages accept any of the given trust bits
>+	 * being on as acceptable. */
>+	trustFlags = trust.sslFlags | trust.emailFlags |
>+		     trust.objectSigningFlags;
>+    } else {
>+	trustFlags = SEC_GET_TRUST_FLAGS(&trust, trustType);
>+    }

This is the second place: what to do if trustType == trustTypeNone.

Comment 21

[Rob Stradling]

(In reply to Wan-Teh Chang from comment #19)
<snip>
> Please review and test this patch.  Could you also tell me how I can test
> it myself?  Thanks.

Wan-Teh, thanks for this patch!  I've tested it and it works OK.  r+

Here are a few tests that you can do:

TEST 1: Different "version" of a builtin
Browse to: https://evenhancer.comodo.com
This TLS server serves a different "version" of the NSS builtin "COMODO Certification Authority".  I believe that every unpatched Firefox client will exhibit the chain-building looping bug when attempting to perform a TLS handshake with evenhancer.comodo.com.

TEST 2: Cross-certified Roots
Install both of the following intermediates as "Software Security Device"s:
http://crt.usertrust.com/AddTrustUTNServerCA.crt
http://crt.usertrust.com/UTNAddTrustServerCA.crt
Then, when browsing (with an unpatched Firefox client) to any site with a cert that chains to "AddTrust External CA Root" and/or "UTN-USERFirst-Hardware", the chain-building looping bug will occur.
e.g.
https://www.icloud.com
https://www.gandi.net
https://tigerbags.co.uk

Comment 22

[Rob Stradling]

With Wan-Teh's patch I still see 20-cert-long chains in the PSM Certificate Viewer, whereas (as I noted in comment 2) my patch happens to fix this for Comodo certificate chains.  But I'm happy to let my patch die if you'll take Wan-Teh's patch.

Comment 23

[Wan-Teh Chang]

Rob: thank you for testing my patch.  With your instructions, I
have also verified that it works with the NSS 'vfyserv' command.
I will test with Firefox next.  I'd like to find out why the PSM
certificate viewer still displays a 20-cert-long chain.

Comment 24

[Wan-Teh Chang]

Attachment: Pass usage=certUsageSSLServer in nsNSSCertificate::GetChain

Rob: I tracked down the 20-cert-long chain problem in the PSM
certificate viewer.  I can only reproduce it with https://www.gandi.net.
The PSM certificate viewer calls CERT_GetCertChainFromCert with
usage=certUsageSSLClient.  The call stack is:

#0  nssCertificateArray_FindBestCertificate (certs=0x7fcb95aca830, 
    timeOpt=0x7fcb990f5090, usage=0x7fffe47aa260, policiesOpt=0x0)
    at pkibase.c:468
#1  0x00007fcbaefe4154 in find_cert_issuer (c=0x7fcb9148b888, 
    timeOpt=0x7fcb990f5090, usage=0x7fffe47aa260, policiesOpt=0x0, 
    td=0x7fcb912f0030, cc=0x7fcb912f7830) at certificate.c:463
#2  0x00007fcbaefe42a1 in nssCertificate_BuildChain (c=0x7fcb9148b888, 
    timeOpt=0x7fcb990f5090, usage=0x7fffe47aa390, policiesOpt=0x0, 
    rvOpt=0x7fffe47aa370, rvLimit=2, arenaOpt=0x0, statusOpt=0x7fffe47aa3cc, 
    td=0x7fcb912f0030, cc=0x7fcb912f7830) at certificate.c:517
#3  0x00007fcbaefe440e in NSSCertificate_BuildChain (c=0x7fcb9148b888, 
    timeOpt=0x7fcb990f5090, usage=0x7fffe47aa390, policiesOpt=0x0, 
    rvOpt=0x7fffe47aa370, rvLimit=2, arenaOpt=0x0, statusOpt=0x7fffe47aa3cc, 
    td=0x7fcb912f0030, cc=0x7fcb912f7830) at certificate.c:563
#4  0x00007fcbaef88f7f in CERT_FindCertIssuer (cert=0x7fcb9148bc20, 
    validTime=1342831507694507, usage=certUsageSSLClient) at certvfy.c:186
#5  0x00007fcbaef8bbc5 in CERT_GetCertChainFromCert (cert=0x7fcb9148bc20, 
    time=1342831507694507, usage=certUsageSSLClient) at certvfy.c:1849
#6  0x00007fcbac80d3b4 in nsNSSCertificate::GetChain (this=0x7fcb989b3560, 
    _rvChain=0x7fffe47aa720)
    at /usr/local/google/home/wtc/mozilla-inbound/src/security/manager/ssl/src/nsNSSCertificate.cpp:827
#7  0x00007fcbace226fa in NS_InvokeByIndex_P (that=0x7fcb989b3560, 
    methodIndex=23, paramCount=1, params=0x7fffe47aa720)
    at /usr/local/google/home/wtc/mozilla-inbound/src/xpcom/reflect/xptcall/src/md/unix/xptcinvoke_x86_64_unix.cpp:163
...

Since the built-in COMODO root CA certificate is not trusted
for SSL client certificates, my patch has no effect.  This patch
changes nsNSSCertificate::GetChain to pass usage=certUsageSSLServer
instead.  This fixes the 20-cert-long chain problem in the PSM
certificate viewer.  Since I don't know what other PSM code also
uses nsNSSCertificate::GetChain, I am not sure if this patch needs
more work.  Kai, do you know?

Rob, your blacklist fixes the 20-cert-long chain problem because
it ignores the certificate usage.  Could you test this PSM patch?

Comment 25

[Wan-Teh Chang]

Comment on attachment 644136 [details] [diff] [review]
Nelson's approach

Brian: please also review this patch.

Note that bestCertIsValidAtTime and bestCertIsTrusted are for
optimizations.  Please review them carefully.  I am wondering
if we should reset bestCertIsValidAtTime and bestCertIsTrusted
to PR_FALSE when we change bestCert and bestdc.  I think not.

Comment 26

[Robert Relyea]

Comment on attachment 644136 [details] [diff] [review]
Nelson's approach

r+ rrelyea

Comment 27

[Robert Relyea]

Comment on attachment 641773 [details] [diff] [review]
Blacklist "never best certs" V3

r- 
I've already r+'ed wtc's proposal.

Comment 28

[Rob Stradling]

Comment on attachment 644565 [details] [diff] [review]
Pass usage=certUsageSSLServer in nsNSSCertificate::GetChain

Review of attachment 644565 [details] [diff] [review]:
-----------------------------------------------------------------

Hi Wan-Teh.  Thanks for this patch.  With the patch applied, I can no longer reproduce the 20-cert-long chain problem.

Would it make sense to move this patch to Bug 651994?

Comment 29

[Wan-Teh Chang]

Comment on attachment 644136 [details] [diff] [review]
Nelson's approach

Patch checked in on the NSS trunk (NSS 3.14).

Checking in pki3hack.c;
/cvsroot/mozilla/security/nss/lib/pki/pki3hack.c,v  <--  pki3hack.c
new revision: 1.109; previous revision: 1.108
done
Checking in pkibase.c;
/cvsroot/mozilla/security/nss/lib/pki/pkibase.c,v  <--  pkibase.c
new revision: 1.36; previous revision: 1.35
done
Checking in pkitm.h;
/cvsroot/mozilla/security/nss/lib/pki/pkitm.h,v  <--  pkitm.h
new revision: 1.17; previous revision: 1.16
done

Patch checked in on the NSS_3_13_4_BRANCH (NSS 3.13.6).

Checking in pki3hack.c;
/cvsroot/mozilla/security/nss/lib/pki/pki3hack.c,v  <--  pki3hack.c
new revision: 1.106.2.2; previous revision: 1.106.2.1
done
Checking in pkibase.c;
/cvsroot/mozilla/security/nss/lib/pki/pkibase.c,v  <--  pkibase.c
new revision: 1.33.6.2; previous revision: 1.33.6.1
done
Checking in pkitm.h;
/cvsroot/mozilla/security/nss/lib/pki/pkitm.h,v  <--  pkitm.h
new revision: 1.15.64.1; previous revision: 1.15
done

Comment 30

[Wan-Teh Chang]

Comment on attachment 644565 [details] [diff] [review]
Pass usage=certUsageSSLServer in nsNSSCertificate::GetChain

I moved this patch to bug 651994 as Rob suggested.

Comment 31

[Rob Stradling]

(In reply to Brian Smith (:bsmith) from comment #14)
> I am fine with that but it still depends on getting some agreement from Kai
> or Bob.
> 
> Most likely, the patch will have to land on nightly after the merge and then
> we'll try to backport it to the Aurora branch after the merge--assuming we
> reach some agreement (which then also requires agreement from release
> drivers).

Brian, what's the next step in getting this fix to our users?

Shall I set the "approval‑mozilla‑aurora=?" flag for Wan-Teh's committed patch?
Or, since Wan-Teh's patch is in the official NSS code (instead of being a private patch just for Firefox and Thunderbird), are we going to have to wait for the next NSS release (3.13.6) and then file a new bug that requests mozilla-aurora to pull in NSS 3.13.6?

Is there any chance of getting this fix into mozilla-beta so that our users will get the fix when Firefox 15 is released in ~4 weeks time?

Comment 32

[Wan-Teh Chang]

Comment on attachment 644136 [details] [diff] [review]
Nelson's approach

I created the NSS_3_13_6_BETA1 CVS tag and pushed it to mozilla-inbound
in changeset 25ec9ccddcb5:
https://hg.mozilla.org/integration/mozilla-inbound/rev/25ec9ccddcb5

Comment 33

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Rob Stradling from comment #31)
> Brian, what's the next step in getting this fix to our users?

This problem in Firefox is tracked in bug 634074. I have commented in that bug regarding my recommended course of action and I will talk with akeybl about what we need to do for approval.

> Are we going to have to wait for the next NSS release (3.13.6) and then
> file a new bug that requests mozilla-aurora to pull in NSS 3.13.6?

We should be able to make an NSS release soon, once the fix has been tested on mozilla-central for a week or so, and then we can (with release-drivers approval) upgrade mozilla-aurora and/or mozilla-beta with that release.

Comment 34

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/25ec9ccddcb5