Issue 303000043: Usable ALPN

Can't Edit
Can't Publish+Mail
Start Review

Created:
7 years, 10 months ago by franziskus

Modified:
7 years, 10 months ago

Reviewers:
mt, ekr-rietveld

Visibility:
Public.

More Reviews

Patch Set 2 : Fixing NPN and tests #

Patch Set 3 : Fixing NPN and tests 2 (last upload failed) #

Total comments: 1

Created: 7 years, 10 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+222 lines, -32 lines)			Patch
M	external_tests/ssl_gtest/ssl_loopback_unittest.cc	View	1	1 chunk	+68 lines, -0 lines	0 comments	Download
M	external_tests/ssl_gtest/tls_agent.h	View	1	1 chunk	+1 line, -0 lines	0 comments	Download
M	external_tests/ssl_gtest/tls_agent.cc	View	1	1 chunk	+8 lines, -0 lines	0 comments	Download
M	lib/ssl/ssl3con.c	View	1	3 chunks	+35 lines, -0 lines	0 comments	Download
M	lib/ssl/ssl3ext.c	View	1	9 chunks	+90 lines, -16 lines	1 comment	Download
M	lib/ssl/sslerr.h	View	1	1 chunk	+1 line, -0 lines	0 comments	Download
M	lib/ssl/sslsock.c	View	1	1 chunk	+13 lines, -13 lines	0 comments	Download
M	lib/ssl/tls13con.c	View	1	1 chunk	+6 lines, -3 lines	0 comments	Download

Messages

Total messages: 4

Expand All Messages | Collapse All Messages

ekr-rietveld

This needs another pass/ https://codereview.appspot.com/303000043/diff/1/external_tests/ssl_gtest/tls_agent.h File external_tests/ssl_gtest/tls_agent.h (right): https://codereview.appspot.com/303000043/diff/1/external_tests/ssl_gtest/tls_agent.h#newcode208 external_tests/ssl_gtest/tls_agent.h:208: const uint8_t alpn_dummy_val_[4] = { ...

7 years, 10 months ago (2016-07-11 21:16:47 UTC) #1

More bugs to identify. https://codereview.appspot.com/303000043/diff/1/external_tests/ssl_gtest/ssl_loopback_unittest.cc File external_tests/ssl_gtest/ssl_loopback_unittest.cc (right): https://codereview.appspot.com/303000043/diff/1/external_tests/ssl_gtest/ssl_loopback_unittest.cc#newcode494 external_tests/ssl_gtest/ssl_loopback_unittest.cc:494: EnableAlpnWithCallback(); Don't create a custom ...

7 years, 10 months ago (2016-07-12 00:01:29 UTC) #2

franziskus

ok, so there were more things broken. I added a NPN server support to write ...

7 years, 10 months ago (2016-07-14 06:43:03 UTC) #3

ok, so there were more things broken.
I added a NPN server support to write tests for NPN. This should work now as
well even if there's no overlap in the advertised protocols.
I'm not entirely sure about the semantics of NPN here, i.e. if the server is
supposed to simply accept whatever protocol the client sends it in next_proto.
But that's the only thing that made sense to me (and I don't expect anyone to
actually use this...).

https://codereview.appspot.com/303000043/diff/1/external_tests/ssl_gtest/ssl_...
File external_tests/ssl_gtest/ssl_loopback_unittest.cc (right):

https://codereview.appspot.com/303000043/diff/1/external_tests/ssl_gtest/ssl_...
external_tests/ssl_gtest/ssl_loopback_unittest.cc:494: EnableAlpnWithCallback();
On 2016/07/12 00:01:28, mt wrote:
> Don't create a custom method for the use with one function.  Just
EnableAlpn(),
> then inline the callback setup.

Acknowledged.

https://codereview.appspot.com/303000043/diff/1/external_tests/ssl_gtest/tls_...
File external_tests/ssl_gtest/tls_agent.h (right):

https://codereview.appspot.com/303000043/diff/1/external_tests/ssl_gtest/tls_...
external_tests/ssl_gtest/tls_agent.h:208: const uint8_t alpn_dummy_val_[4] = {
0x01, 0x62, 0x01, 0x61 };
On 2016/07/11 21:16:47, ekr-webrtc wrote:
>  make this static and then you can refer to it by TlsAgent::alpn_dummy_val.
> 
> Also, why did you move it?

Acknowledged.

https://codereview.appspot.com/303000043/diff/1/external_tests/ssl_gtest/tls_...
File external_tests/ssl_gtest/tls_connect.cc (right):

https://codereview.appspot.com/303000043/diff/1/external_tests/ssl_gtest/tls_...
external_tests/ssl_gtest/tls_connect.cc:378: unsigned int protoMaxLen)
On 2016/07/12 00:01:28, mt wrote:
> Move this to the one place that you actually use custom value.

Done.

https://codereview.appspot.com/303000043/diff/1/external_tests/ssl_gtest/tls_...
external_tests/ssl_gtest/tls_connect.cc:383: *protoOutLen =
sizeof(agent->alpn_dummy_val_[2]);
On 2016/07/11 21:16:47, ekr-webrtc wrote:
> IMPORTANT: The semantic here is that you memcpy into the buf and then set the
> len to the length. Your assignment here assumes it's only one char but then it
> reads the length from dummy. A lot to go wrong here.

Done. Simply setting dummy value manually now.

https://codereview.appspot.com/303000043/diff/1/external_tests/ssl_gtest/tls_...
external_tests/ssl_gtest/tls_connect.cc:397:
server_->EnableAlpnWithCallback(client_->alpn_dummy_val_,
On 2016/07/11 21:16:47, ekr-webrtc wrote:
> You could overload these methods.
> 

Acknowledged.

https://codereview.appspot.com/303000043/diff/1/external_tests/ssl_gtest/tls_...
external_tests/ssl_gtest/tls_connect.cc:399: NextProtoCallback);
On 2016/07/12 00:01:28, mt wrote:
> You don't need to enable the custom callback on the client.  The server is the
> only one that runs it.

Acknowledged.

https://codereview.appspot.com/303000043/diff/1/external_tests/ssl_gtest/tls_...
external_tests/ssl_gtest/tls_connect.cc:424: sizeof(client_->alpn_dummy_val_));
On 2016/07/12 00:01:29, mt wrote:
> This change seems unnecessary, as ekr noted.

Acknowledged.

https://codereview.appspot.com/303000043/diff/1/lib/ssl/ssl3ext.c
File lib/ssl/ssl3ext.c (right):

https://codereview.appspot.com/303000043/diff/1/lib/ssl/ssl3ext.c#newcode726
lib/ssl/ssl3ext.c:726: unsigned int taglen = (unsigned int)data[offset];
On 2016/07/12 00:01:29, mt wrote:
> Maybe add a PORT_Assert(offset + 1 + taglen <= length);

Done.

https://codereview.appspot.com/303000043/diff/1/lib/ssl/ssl3ext.c#newcode734
lib/ssl/ssl3ext.c:734: }
On 2016/07/11 21:16:47, ekr-webrtc wrote:
> Is this just my code?

yeah, I messed that up. See comments below.

https://codereview.appspot.com/303000043/diff/1/lib/ssl/ssl3ext.c#newcode736
lib/ssl/ssl3ext.c:736: 
On 2016/07/11 21:16:47, ekr-webrtc wrote:
> extra ws

Done.

https://codereview.appspot.com/303000043/diff/1/lib/ssl/ssl3ext.c#newcode781
lib/ssl/ssl3ext.c:781: }
On 2016/07/11 21:16:47, ekr-webrtc wrote:
> IMPORTANT: This code actually doesn't work because it depends on
> |ssl->ssl3.hs.nextProtoNego| being set which you don't do when you set the
> callback. Your test *both* calls SSL_SetNextProtoNego() and
> SSL_SetNextProtoCallback() which hides this error. To fix this you need:
> 
> 1. Remove the EnableAlpn() call in EnableAlpnWithCallback() replacing it with
> EnsureTlsSetup().
> 2. Have the client call EnableAlpn()
> 
> This will reveal the bug.
> 
> Then change ssl3_AlpnTagAllowed() to return PR_TRUE if the callback is set
> regardless of the value of |ss->ssl3.hs.nextProtoNego| (asserting that it's
> empty).

I'd have expected we require to call SSL_SetNextProtoNego in any case. But we
probably shouldn't.

I changed ssl3_AlpnTagAllowed to do something useful in this case, i.e. check
the tag chosen by the callback against the client provided tags.

https://codereview.appspot.com/303000043/diff/1/lib/ssl/ssl3ext.c#newcode781
lib/ssl/ssl3ext.c:781: }
On 2016/07/12 00:01:29, mt wrote:
> On 2016/07/11 21:16:47, ekr-webrtc wrote:
> > IMPORTANT: This code actually doesn't work because it depends on
> > |ssl->ssl3.hs.nextProtoNego| being set which you don't do when you set the
> > callback. Your test *both* calls SSL_SetNextProtoNego() and
> > SSL_SetNextProtoCallback() which hides this error. To fix this you need:
> > 
> > 1. Remove the EnableAlpn() call in EnableAlpnWithCallback() replacing it
with
> > EnsureTlsSetup().
> > 2. Have the client call EnableAlpn()
> > 
> > This will reveal the bug.
> > 
> > Then change ssl3_AlpnTagAllowed() to return PR_TRUE if the callback is set
> > regardless of the value of |ss->ssl3.hs.nextProtoNego| (asserting that it's
> > empty).
> 
> I think that we probably want two tests in this case: one that calls both and
> one that just sets the callback. 
> 
> Note that the test you want to add needs to check if it is the peer that is
> making the call, that means that you need to have a nasty check on the ALPN
> type.  For that, I would add the check to this function rather than
> ssl3_AlpnTagAllowed():
> 
> if ((ss->sec.isServer ^ (ex_type == ssl_app_layer_protocol_xtn)) &&
>     !ss->nextProtoCallback && !ssl3_AlpnTagAllowed(ss, &result)) {
> }

We can't check !ss->nextProtoCallback. That's also set in case we use the NSS
internal callback (there's even an assert further up for ss->nextProtoCallback).

https://codereview.appspot.com/303000043/diff/1/lib/ssl/sslsock.c
File lib/ssl/sslsock.c (right):

https://codereview.appspot.com/303000043/diff/1/lib/ssl/sslsock.c#newcode1928
lib/ssl/sslsock.c:1928: /*  ss->ssl3.nextProtoState is set in when the callback
returns. */
On 2016/07/12 00:01:29, mt wrote:
> This is going to break NPN.  NPN assumes that the client will proceed with its
> preferred protocol even if the server doesn't support it.
> 
> Now, I'm OK with us having NPN ONLY work with our callback.  After all, that
> seems to have been always the case.  However, you need to fix it somehow.

right, I added NPN test cases (after implementing the server side). This should
work now. We always set result if it's a client, i.e. NPN.

franziskus

7 years, 10 months ago (2016-07-14 07:48:04 UTC) #4

https://codereview.appspot.com/303000043/diff/40001/lib/ssl/ssl3ext.c
File lib/ssl/ssl3ext.c (right):

https://codereview.appspot.com/303000043/diff/40001/lib/ssl/ssl3ext.c#newcode...
lib/ssl/ssl3ext.c:1085: printf("\n");
this is obviously debug output and will be removed...

Expand All Messages | Collapse All Messages