Update NSS to NSS 3.14 RC1.

Update NSS to NSS 3.14 RC1.

R=rsleevi@chromium.org
BUG=151692
TEST=none
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=162852

[wtc, 2012-10-18 21:20:42]

Please review.

I reviewed the new code in this update and wrote my comments
below.

https://codereview.chromium.org/11193042/diff/1/nss/mozilla/security/nss/lib/...
File nss/mozilla/security/nss/lib/certhigh/certvfypkix.c (right):

https://codereview.chromium.org/11193042/diff/1/nss/mozilla/security/nss/lib/...
nss/mozilla/security/nss/lib/certhigh/certvfypkix.c:1712: break;

This break statement should be inside the {} block. But I
saw a placement at this location on line 1653.

https://codereview.chromium.org/11193042/diff/1/nss/mozilla/security/nss/lib/...
File nss/mozilla/security/nss/lib/nss/nssinit.c (right):

https://codereview.chromium.org/11193042/diff/1/nss/mozilla/security/nss/lib/...
nss/mozilla/security/nss/lib/nss/nssinit.c:644: * to init with not noCertDB and
noModDB */

I believe "not" should be removed from this comment.

https://codereview.chromium.org/11193042/diff/1/nss/mozilla/security/nss/lib/...
nss/mozilla/security/nss/lib/nss/nssinit.c:646: /* we always try to initialize
the modules */

This comment should be removed because nss_InitModules is now
called only under certain conditions.

https://codereview.chromium.org/11193042/diff/1/nss/mozilla/security/nss/lib/...
File nss/mozilla/security/nss/lib/util/SECerrs.h (right):

https://codereview.chromium.org/11193042/diff/1/nss/mozilla/security/nss/lib/...
nss/mozilla/security/nss/lib/util/SECerrs.h:552: "The certificate was rejected
by extra checks in the application.")

The error code is a generic application callback failure,
but the error message is specific to the cert verify callback.

https://codereview.chromium.org/11193042/diff/1/nss/mozilla/security/nss/lib/...
File nss/mozilla/security/nss/lib/util/secoid.c (right):

https://codereview.chromium.org/11193042/diff/1/nss/mozilla/security/nss/lib/...
nss/mozilla/security/nss/lib/util/secoid.c:1924:
xOids[SEC_OID_PKCS5_PBE_WITH_MD5_AND_DES_CBC].notPolicyFlags = ~0;

Do we need to add the calls to allow MD5 signatures?

We should make sure we are mapping SEC_ERROR_CERT_SIGNATURE_ALGORITHM_DISABLED
to net::ERR_CERT_WEAK_SIGNATURE_ALGORITHM or
net::ERR_CERT_INVALID.

[Ryan Sleevi, 2012-10-18 21:28:51]

LGTM. I'd like to make sure the ChromeOS team upgrades to 3.14 soon as well.
They're currently on 3.12

https://codereview.chromium.org/11193042/diff/1/nss/mozilla/security/nss/lib/...
File nss/mozilla/security/nss/lib/certhigh/certvfypkix.c (right):

https://codereview.chromium.org/11193042/diff/1/nss/mozilla/security/nss/lib/...
nss/mozilla/security/nss/lib/certhigh/certvfypkix.c:1712: break;
On 2012/10/18 21:20:42, wtc wrote:
> 
> This break statement should be inside the {} block. But I
> saw a placement at this location on line 1653.

I don't think it matters, and Chromium style actually keeps the break outside of
the brackets as well. In libpkix, I've seen this style as well.

https://codereview.chromium.org/11193042/diff/1/nss/mozilla/security/nss/lib/...
File nss/mozilla/security/nss/lib/nss/nssinit.c (right):

https://codereview.chromium.org/11193042/diff/1/nss/mozilla/security/nss/lib/...
nss/mozilla/security/nss/lib/nss/nssinit.c:644: * to init with not noCertDB and
noModDB */
On 2012/10/18 21:20:42, wtc wrote:
> 
> I believe "not" should be removed from this comment.

I think you're right

https://codereview.chromium.org/11193042/diff/1/nss/mozilla/security/nss/lib/...
nss/mozilla/security/nss/lib/nss/nssinit.c:646: /* we always try to initialize
the modules */
On 2012/10/18 21:20:42, wtc wrote:
> 
> This comment should be removed because nss_InitModules is now
> called only under certain conditions.

I agree

https://codereview.chromium.org/11193042/diff/1/nss/mozilla/security/nss/lib/...
File nss/mozilla/security/nss/lib/util/SECerrs.h (right):

https://codereview.chromium.org/11193042/diff/1/nss/mozilla/security/nss/lib/...
nss/mozilla/security/nss/lib/util/SECerrs.h:552: "The certificate was rejected
by extra checks in the application.")
On 2012/10/18 21:20:42, wtc wrote:
> 
> The error code is a generic application callback failure,
> but the error message is specific to the cert verify callback.

I don't read the message that way. It's exactly what it says - extra checks in
the application, provided by the application callback.

https://codereview.chromium.org/11193042/diff/1/nss/mozilla/security/nss/lib/...
File nss/mozilla/security/nss/lib/util/secoid.c (right):

https://codereview.chromium.org/11193042/diff/1/nss/mozilla/security/nss/lib/...
nss/mozilla/security/nss/lib/util/secoid.c:1924:
xOids[SEC_OID_PKCS5_PBE_WITH_MD5_AND_DES_CBC].notPolicyFlags = ~0;
On 2012/10/18 21:20:42, wtc wrote:
> 
> Do we need to add the calls to allow MD5 signatures?

We already block them. I'd be more concerned for the unit test results to make
sure they're being handled appropriately. I suspect we may get different error
codes back from libpkix

> 
> We should make sure we are mapping SEC_ERROR_CERT_SIGNATURE_ALGORITHM_DISABLED
> to net::ERR_CERT_WEAK_SIGNATURE_ALGORITHM or
> net::ERR_CERT_INVALID.

[wtc, 2012-10-18 21:53:04]

Thank you for the review. Do you know how to update the
NSS package in Chrome OS? Is cmasone or gspencer the NSS
package owner?

https://codereview.chromium.org/11193042/diff/1/nss/mozilla/security/nss/lib/...
File nss/mozilla/security/nss/lib/certhigh/certvfypkix.c (right):

https://codereview.chromium.org/11193042/diff/1/nss/mozilla/security/nss/lib/...
nss/mozilla/security/nss/lib/certhigh/certvfypkix.c:1712: break;

The code example in the Google C++ Style Guide puts the
break statements inside the brackets:

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Loops_and_Swit...

https://codereview.chromium.org/11193042/diff/1/nss/mozilla/security/nss/lib/...
File nss/mozilla/security/nss/lib/util/SECerrs.h (right):

https://codereview.chromium.org/11193042/diff/1/nss/mozilla/security/nss/lib/...
nss/mozilla/security/nss/lib/util/SECerrs.h:552: "The certificate was rejected
by extra checks in the application.")

On 2012/10/18 21:28:51, Ryan Sleevi wrote:
> 
> I don't read the message that way. It's exactly what it says - extra checks in
> the application, provided by the application callback.

The problem is that the error code name,
SEC_ERROR_APPLICATION_CALLBACK_ERROR, does not contain
any word related to a certificate, but the error message
says "The certificate was rejected ..."

[Ryan Sleevi, 2012-10-18 21:57:09]

On 2012/10/18 21:53:04, wtc wrote:
> Thank you for the review. Do you know how to update the
> NSS package in Chrome OS? Is cmasone or gspencer the NSS
> package owner?

http://git.chromium.org/gitweb/?p=chromiumos/overlays/chromiumos-overlay.git;...

Suggests gspencer, benchan, or cmasone are all good candidates.

(I'm working on adding the TPM patch to this code, so I'll be having to nag them
soon as well)

> 
>
https://codereview.chromium.org/11193042/diff/1/nss/mozilla/security/nss/lib/...
> File nss/mozilla/security/nss/lib/certhigh/certvfypkix.c (right):
> 
>
https://codereview.chromium.org/11193042/diff/1/nss/mozilla/security/nss/lib/...
> nss/mozilla/security/nss/lib/certhigh/certvfypkix.c:1712: break;
> 
> The code example in the Google C++ Style Guide puts the
> break statements inside the brackets:
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Loops_and_Swit...
> 
>
https://codereview.chromium.org/11193042/diff/1/nss/mozilla/security/nss/lib/...
> File nss/mozilla/security/nss/lib/util/SECerrs.h (right):
> 
>
https://codereview.chromium.org/11193042/diff/1/nss/mozilla/security/nss/lib/...
> nss/mozilla/security/nss/lib/util/SECerrs.h:552: "The certificate was rejected
> by extra checks in the application.")
> 
> On 2012/10/18 21:28:51, Ryan Sleevi wrote:
> > 
> > I don't read the message that way. It's exactly what it says - extra checks
in
> > the application, provided by the application callback.
> 
> The problem is that the error code name,
> SEC_ERROR_APPLICATION_CALLBACK_ERROR, does not contain
> any word related to a certificate, but the error message
> says "The certificate was rejected ..."

Ah, right, the concern being application callbacks other than certificate? Fair
enough.