<div dir="ltr">Hi Danil!<br><div><div class="gmail_extra"><br><br><div class="gmail_quote">2014-04-17 8:52 GMT+02:00 Danil Zagoskin <span dir="ltr"><<a href="mailto:z@gosk.in" target="_blank">z@gosk.in</a>></span>:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr">Hi, Ingela!<div><br></div><div>The patch works like a charm, thank you! It completely fixes the initial problem with buggy client.</div>
<div><br></div></div></blockquote><div><br></div><div>One bug down!<br></div><div><br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr">
<div></div><div><br></div><div>But there are some sad news, too:<br>
<div><div><br></div></div></div></div></blockquote><div><br></div><div>Well reproducible is always good, means we can haunt it down and fix it :) <br></div><div><br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div dir="ltr"><div><div><div></div><div>While playing with my dummy server (<a href="https://github.com/stolen/ssldump/blob/master/src/ssldump.erl#L70" target="_blank">https://github.com/stolen/ssldump/blob/master/src/ssldump.erl#L70</a>) I accidentally reproduced badarg in erlang:size once more:</div>
<div><br></div><div><div>=ERROR REPORT==== 17-Apr-2014::07:34:58 ===</div><div>** State machine <0.58.0> terminating</div><div>** Last message in was {tcp,#Port<0.1498>,</div><div> [22,3,1,0,173,1,0,0,169,3,3,83,79,75,226,78,81,8,</div>
<div> 77,79,97,154,127,165,92,183,20,239,70,183,158,</div><div> 193,222,139,77,116,13,128,12,66,120,63,110,0,0,</div><div> 94,0,255,192,36,192,35,192,10,192,9,192,7,192,8,</div>
<div> 192,40,192,39,192,20,192,19,192,17,192,18,192,38,</div><div> 192,37,192,42,192,41,192,5,192,4,192,2,192,3,192,</div><div> 15,192,14,192,12,192,13,0,61,0,60,0,47,0,5,0,4,0,</div>
<div> 53,0,10,0,103,0,107,0,51,0,57,0,22,0,175,0,174,0,</div><div> 141,0,140,0,138,0,139,0,177,0,176,0,44,0,59,1,0,</div><div> 0,34,0,10,0,8,0,6,0,23,0,24,0,25,0,11,0,2,1,0,0,</div>
<div> 13,0,12,0,10,5,1,4,1,2,1,4,3,2,3]}</div></div><div>(meaningless state omitted)</div><div><div>** Reason for termination =</div><div>** {badarg,[{erlang,size,</div><div> [[22,3,1,0,173,1,0,0,169,3,3,83,79,75,226,78,81,8,77,79,</div>
<div> 97,154,127,165,92,183,20,239,70,183,158,193,222,139,77,</div><div> 116,13,128,12,66,120,63,110,0,0,94,0,255,192,36,192,35,</div><div> 192,10,192,9,192,7,192,8,192,40,192,39,192,20,192,19,</div>
<div> 192,17,192,18,192,38,192,37,192,42,192,41,192,5,192,4,</div><div> 192,2,192,3,192,15,192,14,192,12,192,13,0,61,0,60,0,47,</div><div> 0,5,0,4,0,53,0,10,0,103,0,107,0,51,0,57,0,22,0,175,0,</div>
<div> 174,0,141,0,140,0,138,0,139,0,177,0,176,0,44,0,59,1,0,0,</div><div> 34,0,10,0,8,0,6,0,23,0,24,0,25,0,11,0,2,1,0,0,13,0,12,0,</div><div> 10,5,1,4,1,2,1,4,3,2,3]],</div>
<div> []},</div><div> {tls_record,get_tls_records_aux,2,</div><div> [{file,"tls_record.erl"},{line,122}]},</div><div> {tls_connection,next_tls_record,2,</div>
<div> [{file,"tls_connection.erl"},{line,484}]},</div><div> {tls_connection,handle_info,3,</div><div> [{file,"tls_connection.erl"},{line,307}]},</div>
<div class="">
<div> {gen_fsm,handle_msg,7,[{file,"gen_fsm.erl"},{line,503}]},</div><div> {proc_lib,init_p_do_apply,3,[{file,"proc_lib.erl"},{line,239}]}]}</div></div></div><div><br></div><div>
The client used was simple curl under mac os, and this crash did not reproduce.</div>
<div>As you see, the packet received as list instead of binary is client_hello which is the first message ssl process gets.</div><div>Maybe there is some race condition when changing mode on just transport_accepted socket.</div>
<div><br></div><div><br></div><div><br></div><div>Also, the same server when working with the same curl may stall at this line:<br> {ok, _} = ssl:recv(Socket, 0, 10000),<br>process_info showed that the worker has {ssl,#sslsocket{}, Data} in its mailbox.<br>
<br><div>I've tuned the server to reproduce this stall easier: <a href="https://github.com/stolen/ssldump/blob/b3eebc2fa85ca0bd8ed8470c400b972aa737bffc/src/ssldump.erl" target="_blank">https://github.com/stolen/ssldump/blob/b3eebc2fa85ca0bd8ed8470c400b972aa737bffc/src/ssldump.erl</a> (master may change)</div>
</div><div><br></div><div>The client is manually invoked curl which is manually interrupted after sufficiently large time of inactivity. The console log looks like this:</div><div><br></div><div><div>stolen@pipe:~/ssldump$ ./dummyserver.sh 9999 ../cert/pipe.trololo</div>
<div>Erlang/OTP 17 [erts-6.0] [source-07b8f44] [smp:2:2] [async-threads:10] [hipe] [kernel-poll:false]</div><div><br></div><div>Eshell V6.0 (abort with ^G)</div><div>1> Accepting SSL in <0.46.0></div><div>Socket state: connection, options: {socket_options,list,0,0,0,true}</div>
<div><br></div><div>=ERROR REPORT==== 17-Apr-2014::09:08:01 ===</div><div>Error in process <0.46.0> with exit value: {{badmatch,{error,timeout}},[{ssldump,ssl_acceptor,1,[{file,"src/ssldump.erl"},{line,98}]}]}</div>
<div><br></div><div>Accepting SSL in <0.47.0></div><div>Socket state: connection, options: {socket_options,binary,0,0,0,false}</div><div>Accepting SSL in <0.48.0></div><div>Socket state: connection, options: {socket_options,binary,0,0,0,false}</div>
<div>Accepting SSL in <0.51.0></div><div>Socket state: connection, options: {socket_options,list,0,0,0,true}</div><div><br></div><div>=ERROR REPORT==== 17-Apr-2014::09:09:17 ===</div><div>Error in process <0.51.0> with exit value: {{badmatch,{error,closed}},[{ssldump,ssl_acceptor,1,[{file,"src/ssldump.erl"},{line,98}]}]}</div>
<div><br></div><div>Accepting SSL in <0.53.0></div><div>Socket state: connection, options: {socket_options,binary,0,0,0,false}</div><div>Accepting SSL in <0.55.0></div><div>Socket state: connection, options: {socket_options,binary,0,0,0,false}</div>
<div>Accepting SSL in <0.57.0></div><div>Socket state: connection, options: {socket_options,list,0,0,0,true}</div><div><br></div><div>=ERROR REPORT==== 17-Apr-2014::09:11:26 ===</div><div>Error in process <0.57.0> with exit value: {{badmatch,{error,closed}},[{ssldump,ssl_acceptor,1,[{file,"src/ssldump.erl"},{line,98}]}]}</div>
</div><div><br></div><div><br></div><div>My interpretation:</div><div> - Given pool size X, the first of each X requests fails.</div><div> - First of failing requests catches timeout as expected, other ones just stall.</div>
<div> - Failing requests have emulated options [{mode, list}, {active, true}] while working ones have the opposite [{mode, binary}, {active, false}].</div><div><br></div><div>After couple of further experiments I discovered that only every (X*N + 1)th acceptor inherits listening socket options, other ones always have [{mode, binary}, {active, false}].</div>
<div><br></div><div><br></div></div></div></div></blockquote><div><br></div><div>Humm... I am not sure if this is what happens in your case but the following should fix a potential inheritance problem<br><br>diff --git a/lib/ssl/src/ssl.erl b/lib/ssl/src/ssl.erl<br>
index 743753b..7768bbb 100644<br>--- a/lib/ssl/src/ssl.erl<br>+++ b/lib/ssl/src/ssl.erl<br>@@ -187,6 +187,7 @@ transport_accept(#sslsocket{pid = {ListenSocket,<br> {error, Reason}<br> end;<br> {error, Reason} -><br>
+ ok = ssl_socket:setopts(Transport, ListenSocket, SocketValues),<br> {error, Reason}<br> end.<br> <br><br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div dir="ltr"><div><div><div></div><div>And, apart from eventual option inheritance, shouldn't ssl:recv on active socket return {error, einval} like gen_tcp:recv does?</div></div></div></div><div class="gmail_extra">
<br><br></div></blockquote><div><br></div><div>I think it should.<br><br><br></div><div>Regards Ingela Erlang/OTP Team Ericssson AB<br></div><div><br><br><br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div class="gmail_extra">
<div class="gmail_quote">2014-04-16 19:36 GMT+04:00 Ingela Andin <span dir="ltr"><<a href="mailto:ingela.andin@gmail.com" target="_blank">ingela.andin@gmail.com</a>></span>:<div><div class="h5"><br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div dir="ltr">Hi!<br><div><div class="gmail_extra"><br><div class="gmail_quote">2014-04-16 15:56 GMT+02:00 Danil Zagoskin <span dir="ltr"><<a href="mailto:z@gosk.in" target="_blank">z@gosk.in</a>></span>:<div>
<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div dir="ltr"><div>Thank you for the patch. I'll check it.</div></div></blockquote><div> </div><br></div><div>New patch, better tested ;), on branch ia/ssl/user-suites-match-negotiated-version at <a href="https://github.com/IngelaAndin/otp" target="_blank">https://github.com/IngelaAndin/otp</a><br>
</div><div><div><br> <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div>(Style question: is <span style="font-family:arial,sans-serif;font-size:13px">lists:filtermap better than list comprehension?)</span></div>
</div></blockquote><div><br><br></div></div><div>I think just a matter of taste!<br></div><div><br></div></div><div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div dir="ltr"><div><div><br></div>> <span style="font-family:arial,sans-serif;font-size:13px">#ssl_options{] is an internal representation of the proplists input list and should not be a visible in the API, records are generally not a good API choice as they enforce compile time dependencies. </span><span style="font-family:arial,sans-serif;font-size:13px"> </span><div style="font-family:arial,sans-serif;font-size:13px">
<br></div></div><div style="font-family:arial,sans-serif;font-size:13px">Yes, I understand that. I wanted to start broken client to see how server would work with it.</div><div style="font-family:arial,sans-serif;font-size:13px">
The option to do that could be using sys:replace_state, but as far as I understand the code, client_hello is sent immediately after starting a process, so it seems there is no way to send crafted hello and continue handshake with couple of lines instead of ssl:connect.</div>
<div style="font-family:arial,sans-serif;font-size:13px"><br></div><div style="font-family:arial,sans-serif;font-size:13px">Given that possibility it could be much easier to reproduce server crashes when working with buggy clients.</div>
<div><font face="arial, sans-serif">For example, How do I write a test for badmatch in </font><span style="font-family:arial,sans-serif;font-size:13px;color:rgb(80,0,80)">tls_record:decode_cipher_text? The function takes </span><font color="#500050" face="arial, sans-serif">#connection_states{}, and creating that record relies on </font><span style="color:rgb(80,0,80);font-family:arial,sans-serif;font-size:13px">#ssl_options{} which cannot be created without opening a socket and calling gen_fsm:enter_loop.</span></div>
<div class="gmail_extra"><br></div></div></blockquote><div><br></div></div><div>Yes you have a good point. I do not have a good short answer. Testing error cases especially when the protocol sometimes<br></div><div>tries to hide the real error reason due to security issues and shortcutting the API and creating raw protocol command is not trivial. <br>
<br></div><div><div><div>Regards Ingela Erlang/OTP team - Ericsson AB<br></div><div><br><br><br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div dir="ltr">
<div class="gmail_extra"><br><div class="gmail_quote">2014-04-16 17:07 GMT+04:00 Ingela Andin <span dir="ltr"><<a href="mailto:ingela.andin@gmail.com" target="_blank">ingela.andin@gmail.com</a>></span>:<div><div>
<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr">Hi!<br><div><div class="gmail_extra"><br><div class="gmail_quote">2014-04-16 0:07 GMT+02:00 Danil Zagoskin <span dir="ltr"><<a href="mailto:z@gosk.in" target="_blank">z@gosk.in</a>></span>:<div>
<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div dir="ltr">Hi, Ingela.<div><br></div><div>I finally had some time for investigating this problem.</div><div><br></div><div>I didn't see badarg in erlang:size again, but it definitely appeared in 17.0 with simple accept method described as minimal example at <a href="http://www.erlang.org/doc/apps/ssl/using_ssl.html" target="_blank">http://www.erlang.org/doc/apps/ssl/using_ssl.html</a> (</div>
</div></blockquote><div><br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div>the difference was transport_accept and ssl_accept were run in separate process)</div>
</div></blockquote><div><br></div></div><div>That should be ok, you are suppose to be able to do that. <br></div><div><div><br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div dir="ltr"><div>Today I met some new crash:</div><div><div>** {{badmatch,{alert,2,20,{"ssl_cipher.erl",215}}},</div><div> [{tls_record,decode_cipher_text,2,[{file,"tls_record.erl"},{line,157}]},</div>
<div> {tls_connection,next_record,1,[{file,"tls_connection.erl"},{line,503}]},</div><div> {tls_connection,next_state,4,[{file,"tls_connection.erl"},{line,475}]},</div><div> {gen_fsm,handle_msg,7,[{file,"gen_fsm.erl"},{line,503}]},</div>
<div> {proc_lib,init_p_do_apply,3,[{file,"proc_lib.erl"},{line,239}]}]}</div></div><div><br></div><div>That's because ssl_record:decipher/3 may return #alert{}, but tls_record:decode_cipher_text/2 isn't expecting that.</div>
<div><br></div></div></blockquote><div><br></div></div><div>Hum, looks like a bug. We will look into that. <br></div><div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div dir="ltr"><div><br></div><div>As for {case_clause,{4}} in ssl_v3:mac_hash/3 I found that precondition for crash is easily reproducible: <a href="https://gist.github.com/stolen/10780653" target="_blank">https://gist.github.com/stolen/10780653</a></div>
<div>Ordinary servers negotiate with sha hash or close socket while ssl in 17.0 negotiates with sha256 hash.</div><div>I don't know the protocol good enough to perform further states, but I hope this helps.</div><div>
<br></div></div></blockquote><div><br></div></div><div>Okej, the following patch should take care of that problem. <br></div><div><br>diff --git a/lib/ssl/src/ssl.erl b/lib/ssl/src/ssl.erl<br>index 743753b..093d4af 100644<br>
--- a/lib/ssl/src/ssl.erl<br>
+++ b/lib/ssl/src/ssl.erl<br>@@ -953,8 +953,8 @@ handle_cipher_option(Value, Version) when is_list(Value) -><br> error:_-><br> throw({error, {options, {ciphers, Value}}})<br> end.<br>-binary_cipher_suites(Version, []) -> %% Defaults to all supported suits<br>
- ssl_cipher:suites(Version);<br>+binary_cipher_suites(Version, []) -> <br>+ [];<br> binary_cipher_suites(Version, [{_,_,_,_}| _] = Ciphers0) -> %% Backwards compatibility<br> Ciphers = [{KeyExchange, Cipher, Hash} || {KeyExchange, Cipher, Hash, _} <- Ciphers0],<br>
binary_cipher_suites(Version, Ciphers);<br>diff --git a/lib/ssl/src/ssl_handshake.erl b/lib/ssl/src/ssl_handshake.erl<br>index 1108edc..93d4efd 100644<br>--- a/lib/ssl/src/ssl_handshake.erl<br>+++ b/lib/ssl/src/ssl_handshake.erl<br>
@@ -1017,11 +1017,14 @@ decode_suites('3_bytes', Dec) -><br> %%-------------Cipeher suite handling --------------------------------<br> <br> available_suites(UserSuites, Version) -><br>+ VersionSupportedSuites = ssl_cipher:suites(Version),<br>
case UserSuites of<br> [] -><br>- ssl_cipher:suites(Version);<br>+ VersionSupportedSuites;<br> _ -><br>- UserSuites<br>+ lists:filtermap(fun(Suite) -><br>+ lists:member(Suite, VersionSupportedSuites)<br>
+ end, UserSuites) <br> end.<br> <br> available_suites(ServerCert, UserSuites, Version, Curve) -><br><br><br> </div><div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div dir="ltr"><div>Unfortunately, I don't understand how this code should be tested — most of functions require #state{} or #connection_states{} which require #ssl_options{} and other stuff, and at I have found no exported function creating #ssl_options{} without side-effects.</div>
</div></blockquote><div><br></div></div><div>#ssl_options{] is an internal representation of the proplists input list and should not be a visible in the API, records are generally not a good API choice as they enforce compile time dependencies. <br>
</div><div><br><br></div><div>Regards Ingela - Erlang/OTP team - Ericsson AB <br></div><div><div><br><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div class="gmail_extra"><br><br><div class="gmail_quote">2014-04-15 16:52 GMT+04:00 Ingela Andin <span dir="ltr"><<a href="mailto:ingela.andin@gmail.com" target="_blank">ingela.andin@gmail.com</a>></span>:<div><div>
<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr">Hello again!<br><div><div><div class="gmail_extra"> <div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div dir="ltr">
sometimes badarg in erlang:size([22,3,1,0,158,1,0,0,154,3,1,83,74|...]) at tls_record.erl:122.<br></div></blockquote></div><br></div></div><div class="gmail_extra">Same symptom was recently reported on erlang-bugs and it turned out to be due to upgrading a gen_tcp socket in active mode. When upgrading a gen_tcp socket to an ssl socket it must be put in passive mode ({active, false}) before the client<br>
</div><div class="gmail_extra">is allowed to start the handshake. Normaly if the upgrade is negotiated this is not a problem for the server to set the option<br></div><div class="gmail_extra">before signaling to the client to go ahead with the handshake. If the upgrade is only performed on the server side instead of calling the ssl API ( some people may do that due to the previously lack of possibility to specify ssl options when calling
ssl:ssl_accept with an "sslsocket", this is no longer the case in 17.0) the listen socket needs to be put in passive mode (listen options are inherited by the acceptsocket) to make sure that the it will work, otherwhise it will work sometimes and sometime result in the error above.
</div><div><div class="gmail_extra"><br></div><div class="gmail_extra">Regards Ingela Erlang/OTP team - Ericsson AB<br></div></div></div></div>
</blockquote></div></div></div><span><font color="#888888"><br><br clear="all"><div><br></div>-- <br><div dir="ltr"><div><font face="'courier new', monospace">Danil Zagoskin | <a href="mailto:z@gosk.in" target="_blank">z@gosk.in</a></font></div>
</div>
</font></span></div>
</blockquote></div></div><br></div></div></div>
</blockquote></div></div></div><span><font color="#888888"><br><br clear="all"><div><br></div>-- <br><div dir="ltr"><div><font face="'courier new', monospace">Danil Zagoskin | <a href="mailto:z@gosk.in" target="_blank">z@gosk.in</a></font></div>
</div></font></span></div></div></blockquote></div></div></div><br></div></div></div>
</blockquote></div></div></div><span class=""><font color="#888888"><br><br clear="all"><div><br></div>-- <br><div dir="ltr"><div><font face="'courier new', monospace">Danil Zagoskin | <a href="mailto:z@gosk.in" target="_blank">z@gosk.in</a></font></div>
</div>
</font></span></div>
</blockquote></div><br></div></div></div>