It turns out that the commented code needed to be removed, though I had to ...
8 years, 2 months ago
(2016-01-19 04:04:01 UTC)
#5
It turns out that the commented code needed to be removed, though I had to check
carefully.
ssl3_CheckFalseStart is invoked at most once in any case.
The first opportunity is when the client sends its second flight. Here, it is
only called if the server certificate has been successfully checked.
The second opportunity is when the server certificate check completes. A call
to SSL_AuthCertificateComplete() is only permitted if the AuthCertificateHook
returns SECWouldBlock.
The two call sites are strictly mutually exclusive which is enforced based on
the value of ss->ssl3.hs.authCertificatePending. That flag also prevents
multiple calls to SSL_AuthCertificateComplete() from succeeding.
Adding the extra check (the one I had commented out: ss->ssl3.hs.ws !=
wait_finished) is safe-ish, but it prevents false start from being enabled
between receiving CCS and Finished, which is unnecessary.
I've removed the most convoluted test case now.
A few notes. https://codereview.appspot.com/286790043/diff/60001/external_tests/ssl_gtest/tls_agent.cc File external_tests/ssl_gtest/tls_agent.cc (right): https://codereview.appspot.com/286790043/diff/60001/external_tests/ssl_gtest/tls_agent.cc#newcode509 external_tests/ssl_gtest/tls_agent.cc:509: if (err != PR_WOULD_BLOCK_ERROR) { The ...
8 years, 2 months ago
(2016-01-20 09:00:12 UTC)
#8
Thanks Wan-Teh. Would you mind checking that the changes to the makefile (gtests.mk) are OK? ...
8 years, 2 months ago
(2016-01-20 22:38:15 UTC)
#12
Thanks Wan-Teh.
Would you mind checking that the changes to the makefile (gtests.mk) are OK?
The targets in platrules.mk were overriding the default target and they weren't
providing any value.
Ready for one last look ekr. https://codereview.appspot.com/286790043/diff/100001/external_tests/common/gtest.mk File external_tests/common/gtest.mk (left): https://codereview.appspot.com/286790043/diff/100001/external_tests/common/gtest.mk#oldcode8 external_tests/common/gtest.mk:8: include ../../cmd/platrules.mk On ...
8 years, 2 months ago
(2016-01-20 23:00:33 UTC)
#14
Ready for one last look ekr.
https://codereview.appspot.com/286790043/diff/100001/external_tests/common/gt...
File external_tests/common/gtest.mk (left):
https://codereview.appspot.com/286790043/diff/100001/external_tests/common/gt...
external_tests/common/gtest.mk:8: include ../../cmd/platrules.mk
On 2016/01/20 22:52:17, wtc1 wrote:
>
> On 2016/01/20 22:38:15, mt wrote:
> >
> > Would you mind checking that the changes to the makefile (gtests.mk) are OK?
> > The targets in platrules.mk were overriding the default target and they
> weren't
> > providing any value.
>
> Hmm... cmd/platrules.mk defines the |show| and |showplatform|
> targets, but I don't think those two targets are used.
>
> If you really want to include cmd/platrules.mk, you need
> to include it after including $(CORE_DEPTH)/coreconf/rules.mk.
>
> Shall we just remove cmd/platrules.mk?
I don't think that it is useful, but I also don't know who might be using it.
I'll open a bug on it.
https://codereview.appspot.com/286790043/diff/100001/lib/ssl/ssl3con.c
File lib/ssl/ssl3con.c (right):
https://codereview.appspot.com/286790043/diff/100001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:7458: case wait_change_cipher:
On 2016/01/20 22:52:17, wtc1 wrote:
>
> Nit: I suggest we list them in the order they are received:
>
> case wait_new_session_ticket:
> case wait_change_cipher:
> case wait_finished:
Done.
https://codereview.appspot.com/286790043/diff/100001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:10820: * haven't received any of the server's second round
yet.
On 2016/01/20 22:52:17, wtc1 wrote:
>
> I think "any of" should be changed to "all of".
Done.
I didn't find anything major above Wan-teh's comments https://codereview.appspot.com/286790043/diff/60001/external_tests/ssl_gtest/tls_agent.cc File external_tests/ssl_gtest/tls_agent.cc (right): https://codereview.appspot.com/286790043/diff/60001/external_tests/ssl_gtest/tls_agent.cc#newcode509 external_tests/ssl_gtest/tls_agent.cc:509: if ...
8 years, 2 months ago
(2016-01-21 00:48:06 UTC)
#15
I didn't find anything major above Wan-teh's comments
https://codereview.appspot.com/286790043/diff/60001/external_tests/ssl_gtest/...
File external_tests/ssl_gtest/tls_agent.cc (right):
https://codereview.appspot.com/286790043/diff/60001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/tls_agent.cc:509: if (err != PR_WOULD_BLOCK_ERROR) {
On 2016/01/20 09:00:12, mt wrote:
> The value of the error code isn't cleared ever as far as I can tell. So this
> was skipping a completely valid read.
Nice catch.
https://codereview.appspot.com/286790043/diff/60001/external_tests/ssl_gtest/...
File external_tests/ssl_gtest/tls_filter.h (right):
https://codereview.appspot.com/286790043/diff/60001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/tls_filter.h:36: };
On 2016/01/20 08:56:19, mt wrote:
> On 2016/01/20 01:43:48, ekr-webrtc wrote:
> > Composition, not inheritance! :)
>
> Yeah, I'm going for "mixin" here to save on code. Composition actually
> increases the amount of typing over straight up duplication.
>
> > In any case, add vertical whitespace.
>
> Done.
mixin sounds like another word for "inheritance"
https://codereview.appspot.com/286790043/diff/60001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/tls_filter.h:44: private:
On 2016/01/20 08:56:19, mt wrote:
> On 2016/01/20 01:43:48, ekr-webrtc wrote:
> > whitespace.
> >
> > Arguably you should have DtlsRecordHeader and TlsRecordHeader?
>
> Maybe. That makes the structure more complex though.
OK.
https://codereview.appspot.com/286790043/diff/60001/lib/ssl/ssl3con.c
File lib/ssl/ssl3con.c (right):
https://codereview.appspot.com/286790043/diff/60001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:7457: case wait_finished:
On 2016/01/20 22:25:32, wtc1 wrote:
> On 2016/01/20 08:56:19, mt wrote:
> >
> > This is a pretty normal structure. I reserve fallthrough comments for when
> the
> > first case (wait_new_session_ticket) has code of its own.
>
> I agree with Martin.
>
OK
https://codereview.appspot.com/286790043/diff/80001/lib/ssl/ssl3con.c
File lib/ssl/ssl3con.c (right):
https://codereview.appspot.com/286790043/diff/80001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:7461: result = !ssl3_ExtensionNegotiated(ss,
ssl_session_ticket_xtn);
On 2016/01/20 22:19:28, wtc1 wrote:
>
> IMPORTANT: it seems that if you allow the wait_finished state, then
> you should also allow the wait_change_cipher state *unconditionally*.
I think Wan-Teh is right here.
The order of messages is:
new_session_ticket [optional]
ccs
finished
So at a higher level, if the test is "we sent our first round and the second
round is not finished" then any of these should be true.
https://codereview.appspot.com/286790043/diff/80001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:10822: * haven't received any of the server's second round
yet.
On 2016/01/20 22:25:33, wtc1 wrote:
>
> IMPORTANT: I think "any of the server's second round" also needs to be
> updated, right? I guess it should be "all of the server's second round".
The previous wording might apply in either case, since you could read it as:
!received (any of second round)
or
for any of second round: !received
but I agree WTC's wording is better.
Issue 286790043: False start tweaks
(Closed)
Created 8 years, 2 months ago by mt
Modified 7 years, 7 months ago
Reviewers: ekr-rietveld, wtc1
Base URL:
Comments: 58