User-Agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2) Gecko/20100122 Fedora/3.6.1-1.fc13 Firefox/3.6 Build Identifier: Google has implemented next protocol negotiation support in their fork of NSS. It would be nice to have this merged upstream. I have attached their patch for review. This is an implementation of http://www.imperialviolet.org/binary/draft-agl-tls-nextprotoneg-00.html Reproducible: Always

future ref https://technotes.googlecode.com/git/nextprotoneg.html

(Required for SPDY.)

This is adam langley's NPN patch for NSS. This patch does not integrate it into necko in any way. Josh recently asked that spdy reviews at least start with honza so I'm setting that as r? - but as nss is something that we normally coordinate externally with the I understand workflow might be different - feel free to reassign amongst yourselves.

Comment on attachment 563752 [details] [diff] [review] patch 0 Adam and Wan-Teh, what is going on with NPN vs NP? Are there any servers that support NP at all? Are we OK with potentially having both implemented in NSS, if the IETF decides to standardize NP instead of NPN? Patrick, changes to NSPR or NSS must always be reviewed by NSS peers and then put into an NSPR and/or NSS release. Then that release gets imported into mozilla-central by a PSM peer using the process described at [1]. We have to do things this way, otherwise we will silently break Firefox when it is build with --use-system-nss. That option is used by many (every?) Linux distro that repackages Firefox, and it is probably an important startup performance optimization in those cases. If this is the same patch as Adam wrote for Chromium, then Wan-Teh probably already reviewed it once. If Wan-Teh is too busy to review it, then I will do so. [1] https://developer.mozilla.org/en/Updating_NSPR_or_NSS_in_mozilla-central

Thanks for shepherding this, Brian. This is adam's patch.

Brian: I don't know what you meant by "NP". The only "NP" I've heard of is the "P = NP" problem. Could you clarify? Thanks.

Wan-Teh, (In reply to Wan-Teh Chang from comment #7) > I don't know what you meant by "NP". Wan-Teh, I hope you don't mind me stepping in here: in this case, NP refers to https://datatracker.ietf.org/doc/draft-agl-tls-nextproto/ (Next Protocol extension). As Paul Hoffman wrote in May: > After the discussion of the previous "next protocol" draft at the meeting > in Prague, we realized that the "negotiation" wording was tripping people up, > so we removed it and made it clear that, at the end, this is about the client > telling the server which protocol the client will be using after the TLS > session is setup. To make that clearer, we re-titled the draft and started > over at -00.

(In reply to Brian Smith (:bsmith) from comment #5) > Adam and Wan-Teh, what is going on with NPN vs NP? Are there any servers > that support NP at all? Are we OK with potentially having both implemented > in NSS, if the IETF decides to standardize NP instead of NPN? NP was an attempt by Paul Hoffman to make NPN more IETF friendly. I don't believe that it has gone anywhere at all. As far as I know, there are no implementations of it. If the IETF were to standardise an acceptable variant of NPN we would look to transition over to it. That seems extremely unlikely at this point.

Comment on attachment 563752 [details] [diff] [review] patch 0 Review of attachment 563752 [details] [diff] [review]: ----------------------------------------------------------------- After having looked at it again, I would like to rework this patch before it goes upstream. Let me do that in the Chromium tree and then it'll be in better shape. I expect to be able to do that today or tomorrow. ::: security/nss/cmd/tstclnt/tstclnt.c @@ -867,4 +867,5 @@ > > SECU_PrintError(progName, "error enabling compression"); > > return 1; > > } > > > > + rv = SSL_SetNextProtoNego(s, "\004flip\004http1.1", 10); This was useful in testing, but should be removed in this patch. A future patch to allow an arbitrary set of protocols to be specified on the command line (like the OpenSSL patch) would be fine. ::: security/nss/lib/ssl/ssl.def @@ -157,4 +157,5 @@ > > SSL_ConfigSecureServerWithCertChain; > > ;+ local: > > ;+*; > > ;+}; > > +;+NSS_CHROMIUM { This is likely to be incorrect for an upstream patch. The additional symbols should be in a real NSS section.

Hi Adam - thanks for the NPN patch. It has been great. (In reply to Adam Langley from comment #10) > ----------------------------------------------------------------- > > After having looked at it again, I would like to rework this patch before it > goes upstream. Let me do that in the Chromium tree and then it'll be in > better shape. I expect to be able to do that today or tomorrow. Any thoughts on the timeline for moving this forward? It's a pre-req to getting spdy in the tree which I'm hoping can happen soon.

(In reply to Patrick McManus from comment #11) > Any thoughts on the timeline for moving this forward? It's a pre-req to > getting spdy in the tree which I'm hoping can happen soon. The (hopefully nearly final) patch is at http://codereview.chromium.org/8156001/. It's still pending another pass by wtc but should be committed soon.

Updated Chromium patch is at: http://src.chromium.org/viewvc/chrome/trunk/src/net/third_party/nss/patches/nextproto.patch?revision=106232&content-type=text%2Fplain

Adam, thanks! Brian is going to get this checked in (and presumably get SSL_GetNextProto and SSL_SetNextProtoNego into ssl.def under the right label?)

Hey Brian, "Adam Langley's Chromium patch (revision 106232)" doesn't seem to have ssl_getnextproto in the ssl.def.. will I be able to link against that? (I had that problem with the update referenced from comment 14) I'll go try..

The ssl.def will need updating when landing in NSS proper. We tend to put stuff in a CHROMIUM section, when we update it at all.

(In reply to Patrick McManus from comment #17) > I'll go try.. I can confirm this version of the patch builds in the firefox tree and works with my hand spdy tests with the following trivial changes: 1] ssl.h line 230 change a // style comment to a /* */ one 2] ssl3ext.c lines 593/594 change a // style comment to a /* */ one 3] add SSL_SetNextProtoNego and SSL_GetNextProto to ssl.def thanks!

(In reply to Patrick McManus from comment #19) > > I'll go try.. > > I can confirm this version of the patch builds in the firefox tree and works > with my hand spdy tests with the following trivial changes: > > 1] ssl.h line 230 change a // style comment to a /* */ one > 2] ssl3ext.c lines 593/594 change a // style comment to a /* */ one > 3] add SSL_SetNextProtoNego and SSL_GetNextProto to ssl.def Thanks Patrick. I am doing these in Splinter right now, and I have an updated patch that addresses the minor issues.

I've updated Chromium's tree to reflect this in r107188. Sorry about the comment style stuff. It's very easy to mess up in our build environment :(

Comment on attachment 569466 [details] [diff] [review] Adam Langley's Chromium patch (revision 106232) Review of attachment 569466 [details] [diff] [review]: ----------------------------------------------------------------- Adam, here is my feedback regarding your patch. It is mostly nits but there do seem to be some bugs. If you do not have time to update the patch, then let me know and I will do so. If there is something about the feedback that you or Wan-Teh or other NSS developers disagree with, then let's get it sorted out ASAP. ::: mozilla/security/nss/lib/ssl/ssl.def @@ +156,5 @@ > +;+ global: > +SSL_SetNextProtoCallback; > +;+ local: > +;+*; > +;+}; All of the new public functions (not just SSL_SetNextProtoCallback) need to be listed here, and the section should be named NSS_3.13.1. ::: mozilla/security/nss/lib/ssl/ssl.h @@ +159,5 @@ > + * to be well formed per the NPN spec. |protoOut| is a buffer provided by the > + * caller, of length 255 (the maximum allowed by the protocol). > + * On successful return, the protocol to be announced to the server will be in > + * |protoOut| and its length in |*protoOutLen|. */ > +typedef SECStatus (PR_CALLBACK *SSLNextProtoCallback)( Normally in NSS, when we supply an output buffer to a function, we define it with three parameters: pointer to the buffer, the size of the buffer, and a pointer to the length of the data written to the buffer. I think we should follow this pattern here. We should clarify that this callback is called for clients, and not for servers. Also, we should note that it is a good idea for an application to implement this callback so that it can PR_Write() its initial application data so that it can be included in the same packet as the change_cipher_spec and finished messages. (I need to verify if/how this works.) The callback cannot return SECWouldBlock. @@ +183,5 @@ > + * The supported protocols are specified in |data| in wire-format (8-bit > + * length-prefixed). For example: "\010http/1.1\006spdy/2". */ > +SSL_IMPORT SECStatus SSL_SetNextProtoNego(PRFileDesc *fd, > + const unsigned char *data, > + unsigned int length); Need a blank line here. @@ +186,5 @@ > + const unsigned char *data, > + unsigned int length); > +/* SSL_GetNextProto can be used after a handshake on a socket where > + * SSL_SetNextProtoNego was called to retrieve the result of the Next Protocol > + * negotiation. "where SSL_SetNextProtoNego was called" is more restrictive than what the code does (it works when SSL_SetNextProtoCallback was used too). Also, we should clarify that this function can be safely used in the HandshakeCallback (which can happen before the handshake is complete if false start is used). @@ +191,5 @@ > + * > + * state is set to one of the SSL_NEXT_PROTO_* constants. The negotiated > + * protocol, if any, is written into buf, which must be at least buf_len bytes > + * long. If the negotiated protocol is longer than this, it is truncated. The > + * number of bytes copied is written into *length. */ We should document that buf_len should be 255 bytes (right?) to hold the longest possible name. Also, we should return SECFailure when the buffer isn't large enough. @@ +198,5 @@ > + unsigned char *buf, > + unsigned int *length, > + unsigned int buf_len); > + > +// TODO(wtc): it may be a good idea to define these as an enum type. +1 ::: mozilla/security/nss/lib/ssl/ssl3con.c @@ +5743,5 @@ > if (rv != SECSuccess) { > goto loser; /* err code was set. */ > } > + > + rv = ssl3_SendNextProto(ss); According to Adam's draft, we should only send the NPN extension during the initial handshake, not during renegotiations. But, it doesn't say whether we should send the NextProtocol message during renegotiations. I assume that we are not supposed to but this seems to send the message during all handshakes. (If my assumption is right, we need to change this and also make the server side fail the connection if it receives a NextProtocol message during a renegotiation, once the server side implements this extension.) It is a little confusing how some ssl3_Send* functions call ssl3_SendRecord and others just append handshake records to the pending handshake buffer. Perhaps it would be better to call this function ssl3_AppendHandshakeNextProto instead. Also, we should update the spec to recommend or require that the NextProtocol message be sent in the same packet--the same send()/write() call, at least--as the Finished message to avoid any possibility of BEAST-like attacks. @@ +8181,5 @@ > +ssl3_SendNextProto(sslSocket *ss) > +{ > + SECStatus rv; > + int padding_len; > + static const unsigned char padding[32] = {0}; Adam's draft does not specify anything about the padding. This is similar to the situation with SSL 3.0 not specifying the format of the padding for records in CBC mode, which led to some attacks. It seems like Chromium is sending all zeros, which seems fine to me (though, this is different than the padding scheme used elsewhere in TLS). I suggest we state that it MUST be all zeros for now. We need to make the server side check that all the padding is all zeros like we do for TLS record padding. @@ +8463,5 @@ > flags = ssl_SEND_FLAG_FORCE_INTO_BUFFER; > } > + > + if (!isServer) { > + rv = ssl3_SendNextProto(ss); The same things mentioned for the first call to ssl3_SendNextProto apply here as well. In particular, we shouldn't send the extension during a renegotiation. ::: mozilla/security/nss/lib/ssl/ssl3ext.c @@ +539,5 @@ > > +/* handle an incoming Next Protocol Negotiation extension. */ > +SECStatus > +ssl3_ServerHandleNextProtoNegoXtn(sslSocket * ss, PRUint16 ex_type, SECItem *data) > +{ We need to fail here if this handshake is a renegotiation. @@ +561,5 @@ > + if (data[offset] == 0) { > + PORT_SetError(SSL_ERROR_NEXT_PROTOCOL_DATA_INVALID); > + return SECFailure; > + } > + offset += (unsigned int)data[offset] + 1; Should libssl reject embedded nulls in the NPN data? It seems like this would reduce a likely-to-be-common application-level error and wouldn't be overly-restrictive. @@ +578,5 @@ > + SECItem *data) > +{ > + SECStatus rv; > + unsigned char result[255]; > + unsigned int result_len; We need to fail here if this handshake is a renegotiation. We need to fail if we did not send the extension in our client hello. @@ +584,5 @@ > + rv = ssl3_ValidateNextProtoNego(data->data, data->len); > + if (rv != SECSuccess) > + return rv; > + > + rv = ss->nextProtoCallback(ss->nextProtoArg, ss->fd, How can we be sure that ss->nextProtoCallback is always non-null here? Do we need to assert that we are holding the locks that we use to protect ss->nextProtoCallback and ss->nextProtoArg? @@ +590,5 @@ > + result, &result_len); > + if (rv != SECSuccess) > + return rv; > + // If the callback wrote more than allowed to |result| it has corrupted our > + // stack. C++ comment needs to be rewritten as a C comment. ::: mozilla/security/nss/lib/ssl/sslerr.h @@ +203,4 @@ > > SSL_ERROR_WEAK_SERVER_EPHEMERAL_DH_KEY = (SSL_ERROR_BASE + 115), > > +SSL_ERROR_NEXT_PROTOCOL_DATA_INVALID = (SSL_ERROR_BASE + 117), The corresponding entry in SSLerrs.h is missing. ::: mozilla/security/nss/lib/ssl/sslimpl.h @@ +314,5 @@ > > typedef struct sslOptionsStr { > + /* If SSL_SetNextProtoNego has been called, then this contains the > + * list of supported protocols. */ > + SECItem nextProtoNego; Why is this in ss->ssl3 instead of ss->ssl3.hs? @@ +834,5 @@ > + > + /* In a client: if the server supports Next Protocol Negotiation, then > + * this is the protocol that was negotiated. > + * > + * If the data pointer is non-NULL, then it is malloced data. */ s/is malloced data/must be freed with PORT_Free/ Though, I think this the default assumption for SECItems, so perhaps this comment about the allocation isn't necessary. @@ +1516,1 @@ > Why do we declare these functions in this header? AFAICT, these declarations can be moved to ssl3ext.c and the functions can be made static. @@ +1550,1 @@ > These also seem like they can be made static in ssl3ext.c ::: mozilla/security/nss/lib/ssl/sslsock.c @@ +441,5 @@ > } > + if (ss->opt.nextProtoNego.data) { > + PORT_Free(ss->opt.nextProtoNego.data); > + ss->opt.nextProtoNego.data = NULL; > + } Nit: Use SECITEM_FreeItem(&ss->opt.nextProtoNego, PR_FALSE); instead? @@ +1279,5 @@ > + > + if (!ss) { > + SSL_DBG(("%d: SSL[%d]: bad socket in SSL_SetNextProtoNego", SSL_GETPID(), > + fd)); > + return SECFailure; Need to PORT_SetError(SEC_ERROR_INVALID_ARGS) before returning. @@ +1282,5 @@ > + fd)); > + return SECFailure; > + } > + > + ssl_GetSSL3HandshakeLock(ss); in SSL_HandshakeCallback callback, we do: ssl_Get1stHandshakeLock(ss); ssl_GetSSL3HandshakeLock(ss); Do we need to do the same here, or is SSL_HandshakeCallback wrong? @@ +1285,5 @@ > + > + ssl_GetSSL3HandshakeLock(ss); > + ss->nextProtoCallback = callback; > + ss->nextProtoArg = arg; > + ssl_ReleaseSSL3HandshakeLock(ss); ssl_Release1stHandshakeLock(ss);? return SECSucces; is missing. @@ +1291,5 @@ > + > +/* NextProtoStandardCallback is set as an NPN callback for the case when the > + * user of the sockets wants the standard selection algorithm. */ > +static SECStatus > +NextProtoStandardCallback(void *arg, I suggest naming this DefaultNextProtoCallback or NextProtoDefaultCallback. @@ +1302,5 @@ > + unsigned int i, j; > + const unsigned char *result; > + > + sslSocket *ss = ssl_FindSocket(fd); > + PORT_Assert(ss); In libssl, we don't usually use an assertion for this test. We always do a runtime test and return SECFailure (sometimes in addition to the assertion). I think we should be consistent here (either adding the runtime check here, or agree that they aren't necessary going forward). @@ +1309,5 @@ > + /* The server supports the extension, but doesn't have any protocols > + * configured. In this case we request our favoured protocol. */ > + goto pick_first; > + } > + We should assert that we hold the SSL3HandshakeLock protecting ss->opt.nextProtoNego. @@ +1321,5 @@ > + ss->ssl3.nextProtoState = SSL_NEXT_PROTO_NEGOTIATED; > + result = &protos[i]; > + goto found; > + } > + j += (unsigned int)ss->opt.nextProtoNego.data[j] + 1; NIT: "1 + (unsigned int)ss->opt.nextProtoNego.data[j]" is clearer because the one additional byte being skipped comes first. @@ +1323,5 @@ > + goto found; > + } > + j += (unsigned int)ss->opt.nextProtoNego.data[j] + 1; > + } > + i += (unsigned int)protos[i] + 1; NOT: 1 + (unsigned int)protos[i] for the same reason. @@ +1331,5 @@ > + ss->ssl3.nextProtoState = SSL_NEXT_PROTO_NO_OVERLAP; > + result = ss->opt.nextProtoNego.data; > + > +found: > + memcpy(protoOut, result + 1, result[0]); If we add a protoMaxOut argument, then we need to check that result[0] <= protoMaxOut here, and fail otherwise. @@ +1346,5 @@ > + sslSocket *ss = ssl_FindSocket(fd); > + > + if (!ss) { > + SSL_DBG(("%d: SSL[%d]: bad socket in SSL_SetNextProtoNego", > + SSL_GETPID(), fd)); PORT_SetError(SEC_ERROR_INVALID_ARGS); @@ +1354,5 @@ > + if (ssl3_ValidateNextProtoNego(data, length) != SECSuccess) > + return SECFailure; > + > + ssl_GetSSL3HandshakeLock(ss); > + if (ss->opt.nextProtoNego.data) NIT: use braces for this if's body. @@ +1377,5 @@ > + sslSocket *ss = ssl_FindSocket(fd); > + > + if (!ss) { > + SSL_DBG(("%d: SSL[%d]: bad socket in SSL_GetNextProto", SSL_GETPID(), > + fd)); PORT_SetError(SEC_ERROR_INVALID_ARGS); @@ +1380,5 @@ > + SSL_DBG(("%d: SSL[%d]: bad socket in SSL_GetNextProto", SSL_GETPID(), > + fd)); > + return SECFailure; > + } > + We should validate (state && buf && length) @@ +1386,5 @@ > + > + if (ss->ssl3.nextProtoState != SSL_NEXT_PROTO_NO_SUPPORT && > + ss->ssl3.nextProto.data) { > + *length = ss->ssl3.nextProto.len; > + if (*length > buf_len) We should return SECFailure here, instead of truncating.

(In reply to Brian Smith (:bsmith) from comment #22) > @@ +1516,1 @@ > > > > Why do we declare these functions in this header? AFAICT, these declarations > can be moved to ssl3ext.c and the functions can be made static. > > @@ +1550,1 @@ > > > > These also seem like they can be made static in ssl3ext.c It isn't clear in the Bugzilla comment interface or bugmail that these comments are referring to the new declarations in sslimpl.h.

I am working on an updated version of the patch that addresses the above comments and also some other small problems (more cases where PORT_SetError calls are missing and places where the wrong function name is mentioned in a log comment). I will post an update to Adam's patch in the morning.

In addition to the items mentioned above, I found one instance where the result of PORT_Alloc was not checked and a few instances where PR_LOG messages were wrong.

Comment on attachment 569837 [details] [diff] [review] Adam's patch with Brian's suggested changes and more fixes passes my (unfortunately manual) spdy tests

Comment on attachment 569837 [details] [diff] [review] Adam's patch with Brian's suggested changes and more fixes Thanks for cleaning that up. As you may have noticed, I never remember PORT_SetError. However, I think the memchr check for NULs in the protocol strings is a mistake. The protocol strings are supposed to be 8-bit clean and we may end up using that fact.

Comment on attachment 569837 [details] [diff] [review] Adam's patch with Brian's suggested changes and more fixes Review of attachment 569837 [details] [diff] [review]: ----------------------------------------------------------------- Adam, since Wan-Teh already reviewed the patch for Chromium that Adam wrote, and I made the next set of changes, could you change your feedback+ to a review+ so Wan-Teh doesn't have to re-review this too? The NSS team already agreed a few weeks ago during the teleconference that you can r+ patches for the NSS trunk. I will revert that memchr() change. Also, please confirm that my change to avoid sending the Next Protocol handshake message during renegotiations is correct.

Comment on attachment 569837 [details] [diff] [review] Adam's patch with Brian's suggested changes and more fixes Yes, your changes to avoid the NPN handshake on renegotiation are good, thank you. I've merged them into Chrome's branch.

Carrying over r+ from Adam.

I am leaving this open for the server side changes and the automated tests. Diffs: http://bonsai.mozilla.org/cvsview2.cgi?command=DIRECTORY&subdir=mozilla/security/nss/lib/ssl&files=SSLerrs.h:ssl.def:ssl.h:ssl3con.c:ssl3ext.c:ssl3prot.h:sslerr.h:sslimpl.h:sslsock.c:sslt.h&root=/cvsroot Changes committed by bsmith%mozilla.com: Update of /cvsroot/mozilla/security/nss/lib/ssl In directory dm-cvs01:/tmp/cvs-serv8317/security/nss/lib/ssl Modified Files: SSLerrs.h ssl.def ssl.h ssl3con.c ssl3ext.c ssl3prot.h sslerr.h sslimpl.h sslsock.c sslt.h Log Message: Bug 547312: Implement client-side support for NPN; original patch by agl r=wtc; changes by bsmith r=agl

In the code as checked in, the NPN callback is only called if the server sent the NPN extension. However, it would be useful if the NPN callback were called also in the case that the server did not send the extension too, so that the application can detect as early as possible that the server doesn't support NPN. This way, we could schedule a PR_Write() of the initial application data of the default protocol (HTTP in the HTTP vs SPDY case) as early as possible in the very likely case where the server doesn't support NPN. Also, should an application be allowed to PR_Write from within the NPN callback?

(In reply to Brian Smith (:bsmith) from comment #34) > However, it would be useful if the NPN callback were > called also in the case that the server did not send the extension too > Also, should an application be allowed to PR_Write from within the NPN > callback? In Chromium's code neither of these would be useful. We perform a TLS handshake a, by the time that it's finished, we know which protocol was negotiated. However, if it's easier in the Mozilla stack to have the callback called all the time I'm happy to make that change, just let me know.

(In reply to Adam Langley from comment #35) > In Chromium's code neither of these would be useful. We perform a TLS > handshake a, by the time that it's finished, we know which protocol was > negotiated. However, if it's easier in the Mozilla stack to have the > callback called all the time I'm happy to make that change, just let me know. Normally, when we do a resumption handshake, we include the initial client application data in the same packet as the client change_cipher_spec and Finished messages. If we wait for the handshake to complete, and then write, then we would no longer be sending the initial client application data in the same packet. (Similar considerations apply to the full handshake when false start is enabled.) This is why I am speculating that such changes would be useful. I will look into it more.

Adam, I was wrong about some of those PORT_SetError() calls. ssl_FindSocket already calls PORT_SetError() with the correct (and different than I suggested) error. Also, I introduced an unused local variable that the compiler warns about in my revisions to your patch.

Comment on attachment 572675 [details] [diff] [review] [CHECKED IN] Minor fixes for checked-in patch Modified Files: ssl3ext.c (rev 1.18) sslsock.c (rev 1.77) Log Message: Bug 547312: Next protocol negotiation support (minor fixes), r=agl

(In reply to Brian Smith (:bsmith) from comment #36) > Normally, when we do a resumption handshake, we include the initial client > application data in the same packet as the client change_cipher_spec and > Finished messages. If we wait for the handshake to complete, and then write, > then we would no longer be sending the initial client application data in > the same packet. (Similar considerations apply to the full handshake when > false start is enabled.) This is why I am speculating that such changes > would be useful. I will look into it more. I have looked into it more. I think that fixing bug 681839 will be sufficient for Mozilla's needs.

is this FIXED now that 698552 is FIXED?

Patrick, this will be marked FIXED when the tests are reviewed and checked into NSS CVS. I am waiting for review/feedback from rrelyea and others on the testing framework I made that I'm planning to use to do these tests. However, mozilla-central does include the NPN implementation, as the implementation is in NSS 3.13.2 BETA 1, which got checked into mozilla-central today. So, you are be good to go.

NSS 3.13.2 has been released. Marked the bug fixed so that this bug shows up in the Bugzilla query for bugs fixed in NSS 3.13.2. Please open a new bug for the tests.