Update NSS libSSL to NSS_3_15_BETA2.

Update NSS libSSL to NSS_3_15_BETA2.

The OCSP stapling patch has been accepted by the NSS upstream, but the
SSL_GetStapledOCSPResponse function is renamed SSL_PeerStapledOCSPResponses
and the function prototype changed to use the new SECItemArray type.

Many source files contain only a trivial CVS keyword change because the
NSS upstream repository was migrated from CVS to hg (Mercurial).

R=agl@chromium.org,rsleevi@chromium.org
BUG=233732
TEST=no build errors or test failures

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=197918

[wtc, 2013-04-29 17:44:36]

These comments are notes to myself. You can ignore them.

https://chromiumcodereview.appspot.com/14522022/diff/1/net/third_party/nss/ss...
File net/third_party/nss/ssl/ssl3ext.c (left):

https://chromiumcodereview.appspot.com/14522022/diff/1/net/third_party/nss/ss...
net/third_party/nss/ssl/ssl3ext.c:711: return SECFailure;

Lines 709-711 should not be deleted.

https://chromiumcodereview.appspot.com/14522022/diff/1/net/third_party/nss/ss...
File net/third_party/nss/ssl/sslauth.c (right):

https://chromiumcodereview.appspot.com/14522022/diff/1/net/third_party/nss/ss...
net/third_party/nss/ssl/sslauth.c:308: now, &certStatusArray->items[i], arg);

The last argument should be ss->pkcs11PinArg.

This for loop should only be executed for i=0 (leaf cert), which
corresponds to ss->sec.peerCert.

[wtc, 2013-04-29 18:09:58]

This CL is ready for review.

Please ignore the try bot errors for now.

I have tested the CL on Linux. It should only need minor
changes when I test it on Mac and Windows, which should not
invalidate your review.

https://chromiumcodereview.appspot.com/14522022/diff/30001/net/third_party/ns...
File net/third_party/nss/README.chromium (left):

https://chromiumcodereview.appspot.com/14522022/diff/30001/net/third_party/ns...
net/third_party/nss/README.chromium:50: patches/restartclientauth.patch

This patch still exists. I merely moved it to match the
order of patches in the applypatches.sh script.

https://chromiumcodereview.appspot.com/14522022/diff/30001/net/third_party/ns...
File net/third_party/nss/ssl/bodge/secitem_array.c (right):

https://chromiumcodereview.appspot.com/14522022/diff/30001/net/third_party/ns...
net/third_party/nss/ssl/bodge/secitem_array.c:22: SECITEM_AllocArray(PLArenaPool
*arena, SECItemArray *array, unsigned int len)

This file is a subset of the nss/lib/util/secitem.c file from
NSS upstream.

I will need to add this file to SVN first so that I can use
the try bots for this CL. (I generated this CL using SVN.
Try bots no longer support SVN-generated CLs that add new
files.)

https://chromiumcodereview.appspot.com/14522022/diff/30001/net/third_party/ns...
File net/third_party/nss/ssl/ssl3con.c (left):

https://chromiumcodereview.appspot.com/14522022/diff/30001/net/third_party/ns...
net/third_party/nss/ssl/ssl3con.c:4190: }

This is rsleevi's CL:
https://chromiumcodereview.appspot.com/12327032

I studied the new code and believe it doesn't need this fix.
The new code uses a new handshake wait state
(ss->ssl3.hs.ws == wait_certificate_status) to determine
if it may get the CertificateStatus message.

https://chromiumcodereview.appspot.com/14522022/diff/30001/net/third_party/ns...
net/third_party/nss/ssl/ssl3con.c:9041: }

I noticed that the new code is no longer deleting
ss->sec.peerCert on the error path.  I will ask the NSS
team if this was removed intentionally.

https://chromiumcodereview.appspot.com/14522022/diff/30001/net/third_party/ns...
File net/third_party/nss/ssl/sslimpl.h (left):

https://chromiumcodereview.appspot.com/14522022/diff/30001/net/third_party/ns...
net/third_party/nss/ssl/sslimpl.h:1690: PRUint32 maxBytes);

This function and ssl3_ClientHandleStatusRequestXtn (lines
1678 - 1679) still exist. They became static functions in
ssl3ext.c.

[wtc, 2013-05-01 16:39:23]

agl, rsleevi: have you had a chance to look at this CL?
Thanks.

[Ryan Sleevi, 2013-05-01 19:06:08]

Mostly rants here about certain style decisions. I'm not fully sure I grok why
Kai decided to make the server status array a pointer, but the client/peer
status array a struct. Was he trying to save on the stack the 4-8 bytes of
storing the length of the array when you weren't stapling?

Whatever.

A few REAL comments below though (those not prefixed as rant:)

Otherwise, LGTM.

https://codereview.chromium.org/14522022/diff/60075/net/socket/ssl_client_soc...
File net/socket/ssl_client_socket_nss.cc (right):

https://codereview.chromium.org/14522022/diff/60075/net/socket/ssl_client_soc...
net/socket/ssl_client_socket_nss.cc:1858: const SECItemArray *ocsp_responses =
nit: "const SECItemArray *" -> "const SECItemArray* "

https://codereview.chromium.org/14522022/diff/60075/net/third_party/nss/READM...
File net/third_party/nss/README.chromium (right):

https://codereview.chromium.org/14522022/diff/60075/net/third_party/nss/READM...
net/third_party/nss/README.chromium:85: patches/suitebonly.patch
Could you explain this more (if not here, then just for my own understanding)?

What does it mean to "report an incorrect EC key size range" - that is, how
would I measure whether or not I've got a bugged token?

https://codereview.chromium.org/14522022/diff/60075/net/third_party/nss/patch...
File net/third_party/nss/patches/secitemarray.patch (right):

https://codereview.chromium.org/14522022/diff/60075/net/third_party/nss/patch...
net/third_party/nss/patches/secitemarray.patch:31: +#if NSSUTIL_VMINOR < 15
BUG?: Shouldn't you also be looking at NSSUTIL_VMAJOR here, and not just
NSSUTIL_VMINOR

Or is the assumption we'll "never" bump to NSS 4?

https://codereview.chromium.org/14522022/diff/60075/net/third_party/nss/ssl/s...
File net/third_party/nss/ssl/ssl3con.c (right):

https://codereview.chromium.org/14522022/diff/60075/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:8671: if (!ss->certStatusArray)
nit: It seems to me that ss->certStatusArray->len should be checked here for
correctness. Namely, if ss->certStatusArray was freed, ->len will be 0, and
items[0] will read off into the nether.

This is more about correctness though - the existing code does appear to be
NULLing certStatusArray when it's freed, but I absolutely hate when code follows
two different patterns for validity checking, and assumes it can only check one.

https://codereview.chromium.org/14522022/diff/60075/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:8776: #undef MAX_CERTSTATUS_LEN
rant: I have no idea why Kai did this.

The NSS code here (as far as I've seen) follows the spec, which is to allow any
spec-valid form. OCSPResponse is 2^24-1, and this is continued in the
multi-response.

I don't see why this limit is imposed here, given that we already have limits
for other areas (like certificates) that would handle this fine.

I would recommend we fix this upstream, but that we not take this bit in
Chromium.

https://codereview.chromium.org/14522022/diff/60075/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:8786:
SECITEM_FreeArray(&ss->sec.ci.sid->peerCertStatus, PR_FALSE);
See how this does not zero-out peerCertStatus, but just leaves ->len = 0? This
is why my comments in the server send code apply.

https://codereview.chromium.org/14522022/diff/60075/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:9969: } else switch (ss->ssl3.hs.msg_type) {
totally unnecessary rant: I really dislike this style of unbraced-else with
braced-switch.

https://codereview.chromium.org/14522022/diff/60075/net/third_party/nss/ssl/s...
File net/third_party/nss/ssl/sslauth.c (right):

https://codereview.chromium.org/14522022/diff/60075/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/sslauth.c:294: SECItemArray *certStatusArray;
rant: inconsistent style = sad.

[wtc, 2013-05-01 21:52:16]

rsleevi: thank you for the code review. I made the changes
you suggested. You can review the diffs between patch sets
7 and 8.

https://codereview.chromium.org/14522022/diff/60075/net/socket/ssl_client_soc...
File net/socket/ssl_client_socket_nss.cc (right):

https://codereview.chromium.org/14522022/diff/60075/net/socket/ssl_client_soc...
net/socket/ssl_client_socket_nss.cc:1858: const SECItemArray *ocsp_responses =
On 2013/05/01 19:06:08, Ryan Sleevi wrote:
> nit: "const SECItemArray *" -> "const SECItemArray* " 

Done.

https://codereview.chromium.org/14522022/diff/60075/net/third_party/nss/READM...
File net/third_party/nss/README.chromium (right):

https://codereview.chromium.org/14522022/diff/60075/net/third_party/nss/READM...
net/third_party/nss/README.chromium:85: patches/suitebonly.patch
On 2013/05/01 19:06:08, Ryan Sleevi wrote:
> Could you explain this more (if not here, then just for my own understanding)?

Done. I added an explanation to the README.chromium file.

You can see the bug in this changeset:
https://hg.mozilla.org/projects/nss/rev/2380cf2250c4

Look at the original code in lib/softoken/pkcs11.c. The
key size range of (112, 571) is used whether
NSS_ECC_MORE_THAN_SUITE_B is defined or not.

> What does it mean to "report an incorrect EC key size range" - that is, how
> would I measure whether or not I've got a bugged token?

The only reliable test, I think, is to query the NSS softoken
for its version. There is a PKCS #11 function to do that
(C_GetSlotInfo or C_GetTokenInfo), and I remember the
softoken reports an <major>.<minor> version, which in this
case is enough to disambiguate (the bug is fixed in 3.15).

https://codereview.chromium.org/14522022/diff/60075/net/third_party/nss/patch...
File net/third_party/nss/patches/secitemarray.patch (right):

https://codereview.chromium.org/14522022/diff/60075/net/third_party/nss/patch...
net/third_party/nss/patches/secitemarray.patch:31: +#if NSSUTIL_VMINOR < 15
On 2013/05/01 19:06:08, Ryan Sleevi wrote:
> BUG?: Shouldn't you also be looking at NSSUTIL_VMAJOR here, and not just
> NSSUTIL_VMINOR
> 
> Or is the assumption we'll "never" bump to NSS 4?

Yes. Once a shared library is widely used, you can't let
libfoo.so.3 and libfoo.so.4 coexist in the same process
without renaming the functions.

But I added a test for NSSUTIL_VMAJOR to avoid this question.

https://codereview.chromium.org/14522022/diff/60075/net/third_party/nss/ssl/s...
File net/third_party/nss/ssl/ssl3con.c (right):

https://codereview.chromium.org/14522022/diff/60075/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:8671: if (!ss->certStatusArray)
On 2013/05/01 19:06:08, Ryan Sleevi wrote:
> nit: It seems to me that ss->certStatusArray->len should be checked here for
> correctness. Namely, if ss->certStatusArray was freed, ->len will be 0, and
> items[0] will read off into the nether.

I can fix this problem in the NSS upstream.

https://codereview.chromium.org/14522022/diff/60075/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:8776: #undef MAX_CERTSTATUS_LEN

On 2013/05/01 19:06:08, Ryan Sleevi wrote:
> rant: I have no idea why Kai did this.
> 
> The NSS code here (as far as I've seen) follows the spec, which is to allow
any
> spec-valid form. OCSPResponse is 2^24-1, and this is continued in the
> multi-response.

I looked into this. bsmith suggested using a smaller max
size in his code review. See these three comments:
https://bugzilla.mozilla.org/show_bug.cgi?id=360420#c45
https://bugzilla.mozilla.org/show_bug.cgi?id=360420#c63
https://bugzilla.mozilla.org/show_bug.cgi?id=360420#c110

(Search for "sane" and "max" in that NSS bug report to
find the relevant comments.)

I think the 2^24-1 max size in the OCSP stapling and
multi-stapling specs is to allow maximum variable-length
vectors that can be encoded with a three-byte length. It
seems reasonable for NSS to refuse to call PORT_Alloc with
a 16MB length (line 8783).

https://codereview.chromium.org/14522022/diff/60075/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:9969: } else switch (ss->ssl3.hs.msg_type) {
On 2013/05/01 19:06:08, Ryan Sleevi wrote:
> totally unnecessary rant: I really dislike this style of unbraced-else with
> braced-switch.

I also hate this. I know why the author did this: to avoid
indenting the big switch statement.

I will figure out a solution upstream.

https://codereview.chromium.org/14522022/diff/60075/net/third_party/nss/ssl/s...
File net/third_party/nss/ssl/sslauth.c (right):

https://codereview.chromium.org/14522022/diff/60075/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/sslauth.c:294: SECItemArray *certStatusArray;
On 2013/05/01 19:06:08, Ryan Sleevi wrote:
> rant: inconsistent style = sad.

I can fix this upstream.

[Ryan Sleevi, 2013-05-01 23:22:57]

https://codereview.chromium.org/14522022/diff/60075/net/third_party/nss/ssl/s...
File net/third_party/nss/ssl/ssl3con.c (right):

https://codereview.chromium.org/14522022/diff/60075/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:8776: #undef MAX_CERTSTATUS_LEN
On 2013/05/01 21:52:16, wtc wrote:
> 
> On 2013/05/01 19:06:08, Ryan Sleevi wrote:
> > rant: I have no idea why Kai did this.
> > 
> > The NSS code here (as far as I've seen) follows the spec, which is to allow
> any
> > spec-valid form. OCSPResponse is 2^24-1, and this is continued in the
> > multi-response.
> 
> I looked into this. bsmith suggested using a smaller max
> size in his code review. See these three comments:
> https://bugzilla.mozilla.org/show_bug.cgi?id=360420#c45
> https://bugzilla.mozilla.org/show_bug.cgi?id=360420#c63
> https://bugzilla.mozilla.org/show_bug.cgi?id=360420#c110
> 
> (Search for "sane" and "max" in that NSS bug report to
> find the relevant comments.)
> 
> I think the 2^24-1 max size in the OCSP stapling and
> multi-stapling specs is to allow maximum variable-length
> vectors that can be encoded with a three-byte length. It
> seems reasonable for NSS to refuse to call PORT_Alloc with
> a 16MB length (line 8783).

I'm not sure I agree. |b| (SSL3Opaque) already contains the fully decoded
handshake message, and the code on lines 8769 guarantee that length == len,
meaning that |b| contains at least that many bytes.

That's why I think this check is silly/unnecessary/at the wrong layer.

If we're trying to avoid strange allocs, there are plenty of other ways - since
the code assumes spec compliance throughout (that I've seen).

That's why this didn't make sense.

[agl, 2013-05-01 23:28:08]

On Wed, May 1, 2013 at 12:39 PM,  <wtc@chromium.org> wrote:
> agl, rsleevi: have you had a chance to look at this CL?

I thought that I already had reviewed it but I see that issue 13898013
is not the same as this one.

Sorry about that. Please feel free to land, otherwise I'll review tomorrow.


Cheers

AGL

[wtc, 2013-05-01 23:58:37]

https://chromiumcodereview.appspot.com/14522022/diff/1/net/third_party/nss/ss...
File net/third_party/nss/ssl/ssl3ext.c (left):

https://chromiumcodereview.appspot.com/14522022/diff/1/net/third_party/nss/ss...
net/third_party/nss/ssl/ssl3ext.c:711: return SECFailure;

On 2013/04/29 17:44:36, wtc wrote:
> 
> Lines 709-711 should not be deleted.

It is correct to delete this check.
ssl3_HandleHelloExtensions does this check for all extensions.

https://chromiumcodereview.appspot.com/14522022/diff/30001/net/third_party/ns...
File net/third_party/nss/ssl/ssl3con.c (left):

https://chromiumcodereview.appspot.com/14522022/diff/30001/net/third_party/ns...
net/third_party/nss/ssl/ssl3con.c:9041: }

On 2013/04/29 18:09:59, wtc wrote:
> 
> I noticed that the new code is no longer deleting
> ss->sec.peerCert on the error path.  I will ask the NSS
> team if this was removed intentionally.

Looking at this code more, I think it seems reasonable to
preserve ss->sec.peerCert after a certificate verification
failure. So it seems correct to remove the
CERT_DestroyCertificate(ss->sec.peerCert) call under |loser|.

[commit-bot: I haz the power, 2013-05-01 23:59:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wtc@chromium.org/14522022/78001

[commit-bot: I haz the power, 2013-05-02 02:58:53]

Retried try job too often on win7_aura for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...

[commit-bot: I haz the power, 2013-05-02 13:03:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wtc@chromium.org/14522022/78001

[commit-bot: I haz the power, 2013-05-02 14:22:05]

Change committed as 197918

[agl, 2013-05-02 14:43:57]

LGTM

https://chromiumcodereview.appspot.com/14522022/diff/78001/net/third_party/ns...
File net/third_party/nss/ssl/ssl3con.c (right):

https://chromiumcodereview.appspot.com/14522022/diff/78001/net/third_party/ns...
net/third_party/nss/ssl/ssl3con.c:8778: /* Array size 1, because we currently
implement single-stapling only*/
nit: missing space before "*/"

https://chromiumcodereview.appspot.com/14522022/diff/78001/net/third_party/ns...
File net/third_party/nss/ssl/ssl3ecc.c (right):

https://chromiumcodereview.appspot.com/14522022/diff/78001/net/third_party/ns...
net/third_party/nss/ssl/ssl3ecc.c:1061: int ECListSize = 0;
nit: starting with a capital letter is odd for a local variable.

https://chromiumcodereview.appspot.com/14522022/diff/78001/net/third_party/ns...
File net/third_party/nss/ssl/ssl3ext.c (right):

https://chromiumcodereview.appspot.com/14522022/diff/78001/net/third_party/ns...
net/third_party/nss/ssl/ssl3ext.c:69: PRBool      append, PRUint32    maxBytes);
nit: odd spacing here.

https://chromiumcodereview.appspot.com/14522022/diff/78001/net/third_party/ns...
net/third_party/nss/ssl/ssl3ext.c:73: PRUint16 ex_type,
This seems to be one space too few (and in the next one). It may just be a
tabs/spaces thing in codereview however.

https://chromiumcodereview.appspot.com/14522022/diff/78001/net/third_party/ns...
File net/third_party/nss/ssl/sslauth.c (right):

https://chromiumcodereview.appspot.com/14522022/diff/78001/net/third_party/ns...
net/third_party/nss/ssl/sslauth.c:11: #include "ocsp.h"
nit: these headers are no longer sorted.

[wtc, 2013-05-02 22:32:00]

agl: thanks for the review. I will fix the problems you
pointed out in the NSS upstream.

https://chromiumcodereview.appspot.com/14522022/diff/78001/net/third_party/ns...
File net/third_party/nss/ssl/ssl3con.c (right):

https://chromiumcodereview.appspot.com/14522022/diff/78001/net/third_party/ns...
net/third_party/nss/ssl/ssl3con.c:8778: /* Array size 1, because we currently
implement single-stapling only*/
On 2013/05/02 14:43:57, agl wrote:
> nit: missing space before "*/"

Done.

https://chromiumcodereview.appspot.com/14522022/diff/78001/net/third_party/ns...
File net/third_party/nss/ssl/ssl3ecc.c (right):

https://chromiumcodereview.appspot.com/14522022/diff/78001/net/third_party/ns...
net/third_party/nss/ssl/ssl3ecc.c:1061: int ECListSize = 0;
On 2013/05/02 14:43:57, agl wrote:
> nit: starting with a capital letter is odd for a local variable.

Done.

https://chromiumcodereview.appspot.com/14522022/diff/78001/net/third_party/ns...
File net/third_party/nss/ssl/ssl3ext.c (right):

https://chromiumcodereview.appspot.com/14522022/diff/78001/net/third_party/ns...
net/third_party/nss/ssl/ssl3ext.c:69: PRBool      append, PRUint32    maxBytes);
On 2013/05/02 14:43:57, agl wrote:
> nit: odd spacing here.

Done.

https://chromiumcodereview.appspot.com/14522022/diff/78001/net/third_party/ns...
net/third_party/nss/ssl/ssl3ext.c:73: PRUint16 ex_type,
On 2013/05/02 14:43:57, agl wrote:
> This seems to be one space too few (and in the next one). It may just be a
> tabs/spaces thing in codereview however.

Done. It is one space too few.

https://chromiumcodereview.appspot.com/14522022/diff/78001/net/third_party/ns...
File net/third_party/nss/ssl/sslauth.c (right):

https://chromiumcodereview.appspot.com/14522022/diff/78001/net/third_party/ns...
net/third_party/nss/ssl/sslauth.c:11: #include "ocsp.h"
On 2013/05/02 14:43:57, agl wrote:
> nit: these headers are no longer sorted.

NSS doesn't require the headers to be sorted, so I'll leave
this unchanged.