Issue 64553002: Certificate Transparency TLS extension patch for NSS

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+631 lines, -37 lines)			Patch
M	net/third_party/nss/README.chromium	View	1 2 3 4	1 chunk	+4 lines, -0 lines	0 comments	Download
M	net/third_party/nss/patches/applypatches.sh	View	1 2 3 4	1 chunk	+2 lines, -0 lines	0 comments	Download
A	net/third_party/nss/patches/signedcertificatetimestamps.patch	View	1 2 3 4 5 6 7	1 chunk	+421 lines, -0 lines	0 comments	Download
M	net/third_party/nss/ssl/ssl.h	View	1 2 3 4 5 6 7	2 chunks	+19 lines, -0 lines	1 comment	Download
M	net/third_party/nss/ssl/ssl3con.c	View	1 2 3 4	2 chunks	+15 lines, -0 lines	0 comments	Download
M	net/third_party/nss/ssl/ssl3ext.c	View	1 2 3 4 5 6 7	4 chunks	+73 lines, -1 line	1 comment	Download
M	net/third_party/nss/ssl/sslimpl.h	View	1 2 3 4	3 chunks	+41 lines, -23 lines	0 comments	Download
M	net/third_party/nss/ssl/sslnonce.c	View	1 2 3 4 5 6 7	2 chunks	+15 lines, -10 lines	0 comments	Download
M	net/third_party/nss/ssl/sslsock.c	View	1 2 3 4 5 6 7	7 chunks	+39 lines, -2 lines	0 comments	Download
M	net/third_party/nss/ssl/sslt.h	View	1 2 3 4	2 chunks	+2 lines, -1 line	0 comments	Download

Messages

Total messages: 9 (0 generated)

Expand Messages | Collapse Messages

ekasper

This is the lib/ssl part of the patch replayed (pretty straightforward) on top of the ...

7 years, 1 month ago (2013-11-07 14:56:12 UTC) #1

wtc

Review comments on patch set 1: Overall the CL is in good shape. Most of ...

7 years, 1 month ago (2013-11-08 19:51:31 UTC) #2

ekasper

PTAL https://codereview.chromium.org/64553002/diff/1/net/third_party/nss/patches/applypatches.sh File net/third_party/nss/patches/applypatches.sh (right): https://codereview.chromium.org/64553002/diff/1/net/third_party/nss/patches/applypatches.sh#newcode67 net/third_party/nss/patches/applypatches.sh:67: patch -p5 < $patches_dir/signedcertificatetimestamps.patch On 2013/11/08 19:51:31, wtc ...

7 years, 1 month ago (2013-11-18 17:47:18 UTC) #3

PTAL

https://codereview.chromium.org/64553002/diff/1/net/third_party/nss/patches/a...
File net/third_party/nss/patches/applypatches.sh (right):

https://codereview.chromium.org/64553002/diff/1/net/third_party/nss/patches/a...
net/third_party/nss/patches/applypatches.sh:67: patch -p5 <
$patches_dir/signedcertificatetimestamps.patch
On 2013/11/08 19:51:31, wtc wrote:
> 
> Please document this patch in net/third_party/nss/README.chromium.

Done.

https://codereview.chromium.org/64553002/diff/1/net/third_party/nss/ssl/ssl.h
File net/third_party/nss/ssl/ssl.h (right):

https://codereview.chromium.org/64553002/diff/1/net/third_party/nss/ssl/ssl.h...
net/third_party/nss/ssl/ssl.h:475: * extension data provided by the TLS server.
The return value is a pointer
On 2013/11/08 19:51:31, wtc wrote:
> 
> Should we mention the extension data is a SignedCertificateTimestampList?

Can't hurt! Done.

https://codereview.chromium.org/64553002/diff/1/net/third_party/nss/ssl/ssl.h...
net/third_party/nss/ssl/ssl.h:477: * until the callback function that calls
SSL_PeerSignedCertTimestamps returns.
On 2013/11/08 19:51:31, wtc wrote:
> 
> Please be explicit which callback function you are referring to. I guess it is
> the SSLAuthCertificate callback?

I believe the precise callback doesn't matter but normally you'd call it from
AuthCertificate or HandshakeCallback so I've made that explicit. This is similar
to how PeerStapledOCSPResponses are handled (tstclnt calls this from both
hooks).

https://codereview.chromium.org/64553002/diff/1/net/third_party/nss/ssl/ssl.h...
net/third_party/nss/ssl/ssl.h:483: * Certificate Timestamps to be provided by a
server.
On 2013/11/08 19:51:31, wtc wrote:
> 
> Nit: change "to enable Signed Certificate Timestamps to be provided by a
server"
> to "to request Signed Certificate Timestamps from a server" or "to indicate
> support for Signed Certificate Timestamps to a server".

That's much better, thanks. Done. I chose "indicate support" because the server
is not obliged to honour the request.

https://codereview.chromium.org/64553002/diff/1/net/third_party/nss/ssl/ssl3c...
File net/third_party/nss/ssl/ssl3con.c (right):

https://codereview.chromium.org/64553002/diff/1/net/third_party/nss/ssl/ssl3c...
net/third_party/nss/ssl/ssl3con.c:6615: /* Copy Signed Certificate Timestamps,
if any. */
On 2013/11/08 19:51:31, wtc wrote:
> 
> So if session resumption succeeds, we ignore any Signed Certificate Timestamps
> that we have received, correct?
> 
> Update: RFC 6962 Sec. 3.3.1 answers my question:
> 
>    Session resumption uses the original session information: clients
>    SHOULD include the extension type in the ClientHello, but if the
>    session is resumed, the server is not expected to process it or
>    include the extension in the ServerHello.
> 
> I guess this implies if the server still includes the extension in the
> ServerHello, the extension is ignored by the client.

Correct.

https://codereview.chromium.org/64553002/diff/1/net/third_party/nss/ssl/ssl3c...
net/third_party/nss/ssl/ssl3con.c:6625: ss->xtnData.signedCertTimestamps.len =
0;
On 2013/11/08 19:51:31, wtc wrote:
> 
> Nit: these three lines can be moved into the preceding if statement.

Actually, uh, they belong to the winner: block below, otherwise the goto winner;
above misses this.

https://codereview.chromium.org/64553002/diff/1/net/third_party/nss/ssl/ssl3e...
File net/third_party/nss/ssl/ssl3ext.c (right):

https://codereview.chromium.org/64553002/diff/1/net/third_party/nss/ssl/ssl3e...
net/third_party/nss/ssl/ssl3ext.c:300: &ssl3_ClientSendSignedCertTimestampXtn },
On 2013/11/08 19:51:31, wtc wrote:
> 
> Nit: omit the comma (,).

Done.

https://codereview.chromium.org/64553002/diff/1/net/third_party/nss/ssl/ssl3e...
net/third_party/nss/ssl/ssl3ext.c:2322: 2 /* length(extension_data) */;
On 2013/11/08 19:51:31, wtc wrote:
> 
> IMPORTANT: move the variable declaration to the beginning of the function.
> Unfortunately Visual C++ still doesn't support this C99 feature.

Done, thanks for catching this.

https://codereview.chromium.org/64553002/diff/1/net/third_party/nss/ssl/ssl3e...
net/third_party/nss/ssl/ssl3ext.c:2352: /* We do not yet know whether we'll be
reusing a session or creating
On 2013/11/08 19:51:31, wtc wrote:
> 
> Nit: reusing => resuming

I actually meant "reusing" (as in reusing the structure). But both work, so
done.

https://codereview.chromium.org/64553002/diff/1/net/third_party/nss/ssl/ssl3e...
net/third_party/nss/ssl/ssl3ext.c:2356: * data is made once a new session has
been set up.
On 2013/11/08 19:51:31, wtc wrote:
> 
> Nit: "a copy of the data is made" => "the data is copied"

Done.

https://codereview.chromium.org/64553002/diff/1/net/third_party/nss/ssl/sslim...
File net/third_party/nss/ssl/sslimpl.h (right):

https://codereview.chromium.org/64553002/diff/1/net/third_party/nss/ssl/sslim...
net/third_party/nss/ssl/sslimpl.h:702: /* Signed certificate timestamps received
in a TLS extension.
On 2013/11/08 19:51:31, wtc wrote:
> 
> Nit: add a blank line before this line.

Done.

https://codereview.chromium.org/64553002/diff/1/net/third_party/nss/ssl/sslim...
net/third_party/nss/ssl/sslimpl.h:797: /* Signed Certificate Timestamps
extracted from the TLS extension.
On 2013/11/08 19:51:31, wtc wrote:
> 
> Nit: add a blank line before this line.

Done.

https://codereview.chromium.org/64553002/diff/1/net/third_party/nss/ssl/sslim...
net/third_party/nss/ssl/sslimpl.h:800: * until a session is setup that can hold
a permanent copy of the data.
On 2013/11/08 19:51:31, wtc wrote:
> 
> Is the 'signedCertTimestamps' SECItem on line 705 the permanent copy of the
> data?

Yup.

> 
> Nit: change "a session" to "a session ID" (which means the NSS sslSessionID
> structure). Or we can be precise and use "ss->sec.ci.sid".

Changed to more precise wording.

https://codereview.chromium.org/64553002/diff/1/net/third_party/nss/ssl/sslno...
File net/third_party/nss/ssl/sslnonce.c (right):

https://codereview.chromium.org/64553002/diff/1/net/third_party/nss/ssl/sslno...
net/third_party/nss/ssl/sslnonce.c:146: if (sid->version >=
SSL_LIBRARY_VERSION_3_0) {
On 2013/11/08 19:51:31, wtc wrote:
> 
> We are planning to remove the SSL 2.0 code within twelve months, so I suggest
we
> not add this sid->version check.

u is a union and u.ssl3 is only valid for a 3.0 session, so I think we need this
check. (In practice this is tail end of the union so should be zeroed for ssl2,
but still.)

https://codereview.chromium.org/64553002/diff/1/net/third_party/nss/ssl/sslso...
File net/third_party/nss/ssl/sslsock.c (right):

https://codereview.chromium.org/64553002/diff/1/net/third_party/nss/ssl/sslso...
net/third_party/nss/ssl/sslsock.c:1006: ssl_defaults.enableSignedCertTimestamps
= on;
On 2013/11/08 19:51:31, wtc wrote:
> 
> BUG: this should be
>     on = ssl_defaults.enableSignedCertTimestamps;

Yikes, I appear to have completely botched this file when migrating the patch to
chromium. Fixed, thanks!

https://codereview.chromium.org/64553002/diff/1/net/third_party/nss/ssl/sslso...
net/third_party/nss/ssl/sslsock.c:1173: break;
On 2013/11/08 19:51:31, wtc wrote:
> 
> Please add a case for SSL_ENABLE_SIGNED_CERT_TIMESTAMPS here.

Done.

https://codereview.chromium.org/64553002/diff/1/net/third_party/nss/ssl/sslso...
net/third_party/nss/ssl/sslsock.c:2022: return NULL;
On 2013/11/08 19:51:31, wtc wrote:
> 
> Please add:
>     PORT_SetError(SSL_ERROR_FEATURE_NOT_SUPPORTED_FOR_SSL2);
> 
> Strictly speaking, this feature (which requires a TLS extension) won't work
for
> SSL 3.0 either, right?

Strictly speaking yes, but I think it's nicer to allow the caller to just "see
if anything was sent" rather than force a check for the negotiated version.

There's still the SSLv2 version case left (because the data comes from u.ssl3)
but unless the application has SSLv2 enabled, they won't need to worry about
distinguishing this case.

Does this sound reasonable?

https://codereview.chromium.org/64553002/diff/1/net/third_party/nss/ssl/sslt.h
File net/third_party/nss/ssl/sslt.h (right):

https://codereview.chromium.org/64553002/diff/1/net/third_party/nss/ssl/sslt....
net/third_party/nss/ssl/sslt.h:209: ssl_signed_certificate_timestamp_xtn = 18  
/* RFC 6962 */
On 2013/11/08 19:51:31, wtc wrote:
> 
> Please list this extension after ssl_app_layer_protocol_xtn (= 16).
> 

Done.

> Nit: it is tempting to shorten this enum name a bit. Perhaps we can abbreviate
> "certificate" to "cert".

If you don't feel too strongly about it, I would keep the official enum name.
Makes it easier to match to RFC.

wtc

Patch set 4 LGTM! Thanks. https://codereview.chromium.org/64553002/diff/1/net/third_party/nss/ssl/sslnonce.c File net/third_party/nss/ssl/sslnonce.c (right): https://codereview.chromium.org/64553002/diff/1/net/third_party/nss/ssl/sslnonce.c#newcode146 net/third_party/nss/ssl/sslnonce.c:146: if (sid->version >= SSL_LIBRARY_VERSION_3_0) ...

7 years, 1 month ago (2013-11-19 23:52:28 UTC) #4

Patch set 4 LGTM! Thanks.

https://codereview.chromium.org/64553002/diff/1/net/third_party/nss/ssl/sslno...
File net/third_party/nss/ssl/sslnonce.c (right):

https://codereview.chromium.org/64553002/diff/1/net/third_party/nss/ssl/sslno...
net/third_party/nss/ssl/sslnonce.c:146: if (sid->version >=
SSL_LIBRARY_VERSION_3_0) {

On 2013/11/18 17:47:18, ekasper wrote:
>
> u is a union and u.ssl3 is only valid for a 3.0 session, so I think we need
this
> check. (In practice this is tail end of the union so should be zeroed for
ssl2,
> but still.)

I see. This makes sense, thanks.

I suggest that we combine this with the if statement on lines 122-125.

https://codereview.chromium.org/64553002/diff/1/net/third_party/nss/ssl/sslso...
File net/third_party/nss/ssl/sslsock.c (right):

https://codereview.chromium.org/64553002/diff/1/net/third_party/nss/ssl/sslso...
net/third_party/nss/ssl/sslsock.c:2022: return NULL;

On 2013/11/18 17:47:18, ekasper wrote:
>
> Strictly speaking yes, but I think it's nicer to allow the caller to just "see
> if anything was sent" rather than force a check for the negotiated version.
> 
> There's still the SSLv2 version case left (because the data comes from u.ssl3)
> but unless the application has SSLv2 enabled, they won't need to worry about
> distinguishing this case.
> 
> Does this sound reasonable?

Yes, it does. Thanks.

To make it clear it's the u.ssl3 union member access that we are checking, we
can use ss->sec.ci.sid->version instead of ss->version in the check above.

https://codereview.chromium.org/64553002/diff/1/net/third_party/nss/ssl/sslt.h
File net/third_party/nss/ssl/sslt.h (right):

https://codereview.chromium.org/64553002/diff/1/net/third_party/nss/ssl/sslt....
net/third_party/nss/ssl/sslt.h:209: ssl_signed_certificate_timestamp_xtn = 18  
/* RFC 6962 */

On 2013/11/18 17:47:18, ekasper wrote:
>
> If you don't feel too strongly about it, I would keep the official enum name.
> Makes it easier to match to RFC.

Yes, this is fine. I suggested ssl_signed_cert_timestamp_xtn partly because it
messed up the alignment of the equal signs, and partly because I saw you use
"ssl_signed_cert_timestamps" in the test script ssl.sh.

https://codereview.chromium.org/64553002/diff/270001/net/third_party/nss/ssl/...
File net/third_party/nss/ssl/ssl.h (right):

https://codereview.chromium.org/64553002/diff/270001/net/third_party/nss/ssl/...
net/third_party/nss/ssl/ssl.h:479: * (e.g. the authenticate certificate hook, or
the handshake callback) returns.

Thank you for improving this comment.

I now see that this comment comes from the comment for
SSL_PeerStapledOCSPResponses (lines 458-459). In the current implementation, I
believe the returned pointer is actually valid until the next handshake
(renegotiation) on the SSL connection. I wonder if we documented a shorter
validity period so that we may change the implementation if necessary.

https://codereview.chromium.org/64553002/diff/270001/net/third_party/nss/ssl/...
net/third_party/nss/ssl/ssl.h:482: * will be empty. If there was an error, or if
SSLv2 was negotiated, then the

Nit: It's not necessary to mention "or if SSLv2 was negotiated" because that is
one of the possible errors. Not mentioning SSLv2 here means we won't need to
update this comment when we remove SSLv2 code from NSS.

ekasper

Thanks so much for the careful review! Incredibly helpful on first contact with a new ...

7 years, 1 month ago (2013-11-20 16:06:27 UTC) #5

Thanks so much for the careful review! Incredibly helpful on first contact with
a new codebase.

I've addressed the remaining comments and rebased the patch on top of new
patches from last week. Is this good to go?

https://codereview.chromium.org/64553002/diff/1/net/third_party/nss/ssl/sslno...
File net/third_party/nss/ssl/sslnonce.c (right):

https://codereview.chromium.org/64553002/diff/1/net/third_party/nss/ssl/sslno...
net/third_party/nss/ssl/sslnonce.c:146: if (sid->version >=
SSL_LIBRARY_VERSION_3_0) {
On 2013/11/19 23:52:28, wtc wrote:
> 
> On 2013/11/18 17:47:18, ekasper wrote:
> >
> > u is a union and u.ssl3 is only valid for a 3.0 session, so I think we need
> this
> > check. (In practice this is tail end of the union so should be zeroed for
> ssl2,
> > but still.)
> 
> I see. This makes sense, thanks.
> 
> I suggest that we combine this with the if statement on lines 122-125.

Done.

https://codereview.chromium.org/64553002/diff/1/net/third_party/nss/ssl/sslso...
File net/third_party/nss/ssl/sslsock.c (right):

https://codereview.chromium.org/64553002/diff/1/net/third_party/nss/ssl/sslso...
net/third_party/nss/ssl/sslsock.c:2022: return NULL;
On 2013/11/19 23:52:28, wtc wrote:
> 
> On 2013/11/18 17:47:18, ekasper wrote:
> >
> > Strictly speaking yes, but I think it's nicer to allow the caller to just
"see
> > if anything was sent" rather than force a check for the negotiated version.
> > 
> > There's still the SSLv2 version case left (because the data comes from
u.ssl3)
> > but unless the application has SSLv2 enabled, they won't need to worry about
> > distinguishing this case.
> > 
> > Does this sound reasonable?
> 
> Yes, it does. Thanks.
> 
> To make it clear it's the u.ssl3 union member access that we are checking, we
> can use ss->sec.ci.sid->version instead of ss->version in the check above.

Done.

https://codereview.chromium.org/64553002/diff/270001/net/third_party/nss/ssl/...
File net/third_party/nss/ssl/ssl.h (right):

https://codereview.chromium.org/64553002/diff/270001/net/third_party/nss/ssl/...
net/third_party/nss/ssl/ssl.h:479: * (e.g. the authenticate certificate hook, or
the handshake callback) returns.
On 2013/11/19 23:52:29, wtc wrote:
> 
> Thank you for improving this comment.
> 
> I now see that this comment comes from the comment for
> SSL_PeerStapledOCSPResponses (lines 458-459). In the current implementation, I
> believe the returned pointer is actually valid until the next handshake
> (renegotiation) on the SSL connection. I wonder if we documented a shorter
> validity period so that we may change the implementation if necessary.

Yes, I'm pretty sure this is stricter than currently necessary but I wasn't sure
what exactly could trigger a reset (renegotiation, something else?) and, anyway,
I figured I'd keep it aligned with the stuff above.

https://codereview.chromium.org/64553002/diff/270001/net/third_party/nss/ssl/...
net/third_party/nss/ssl/ssl.h:482: * will be empty. If there was an error, or if
SSLv2 was negotiated, then the
On 2013/11/19 23:52:29, wtc wrote:
> 
> Nit: It's not necessary to mention "or if SSLv2 was negotiated" because that
is
> one of the possible errors. Not mentioning SSLv2 here means we won't need to
> update this comment when we remove SSLv2 code from NSS.

Done, and I've also made it a handshake error for the server to echo an empty
extension, so an empty response now always means "no SCT response from server".

wtc

Patch set 8 LGTM. I will submit this CL to the commit queue. https://codereview.chromium.org/64553002/diff/510001/net/third_party/nss/ssl/ssl.h File ...

7 years, 1 month ago (2013-11-20 23:05:54 UTC) #6

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ekasper@google.com/64553002/510001

7 years, 1 month ago (2013-11-20 23:42:38 UTC) #7

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ekasper@google.com/64553002/510001

7 years, 1 month ago (2013-11-21 01:45:13 UTC) #8

Message was sent while issue was closed.

Change committed as 236376

Expand Messages | Collapse Messages

Issue 64553002: Certificate Transparency TLS extension patch for NSS (Closed)

Description

Patch Set 1 #

Patch Set 2 : comments #

Patch Set 3 : update ssl.h #

Patch Set 4 : address one more comment #

Patch Set 5 : rebase #

Patch Set 6 : revert accidental changes #

Patch Set 7 : revert accidental change #

Patch Set 8 : more review comments #

Messages