[erlang-questions] Errors in SSL handshake (weird client)

[Danil Zagoskin]

Thank you for the patch. I'll check it. (Style question: is lists:filtermap
better than list comprehension?)

> #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.

Yes, I understand that. I wanted to start broken client to see how server
would work with it.
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.

Given that possibility it could be much easier to reproduce server crashes
when working with buggy clients.
For example, How do I write a test for badmatch in
tls_record:decode_cipher_text?
The function takes #connection_states{}, and creating that record
relies on #ssl_options{}
which cannot be created without opening a socket and calling
gen_fsm:enter_loop.



2014-04-16 17:07 GMT+04:00 Ingela Andin <ingela.andin@REDACTED>:

> Hi!
>
> 2014-04-16 0:07 GMT+02:00 Danil Zagoskin <z@REDACTED>:
>
> Hi, Ingela.
>>
>> I finally had some time for investigating this problem.
>>
>> 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
>> http://www.erlang.org/doc/apps/ssl/using_ssl.html (
>>
>
>
>
>> the difference was transport_accept and ssl_accept were run in separate
>> process)
>>
>
> That  should be ok, you are suppose to be able to do that.
>
>
>
>> Today I met some new crash:
>> ** {{badmatch,{alert,2,20,{"ssl_cipher.erl",215}}},
>>
>> [{tls_record,decode_cipher_text,2,[{file,"tls_record.erl"},{line,157}]},
>>
>>  {tls_connection,next_record,1,[{file,"tls_connection.erl"},{line,503}]},
>>
>>  {tls_connection,next_state,4,[{file,"tls_connection.erl"},{line,475}]},
>>      {gen_fsm,handle_msg,7,[{file,"gen_fsm.erl"},{line,503}]},
>>      {proc_lib,init_p_do_apply,3,[{file,"proc_lib.erl"},{line,239}]}]}
>>
>> That's because ssl_record:decipher/3 may return #alert{}, but
>> tls_record:decode_cipher_text/2 isn't expecting that.
>>
>>
> Hum, looks like a bug. We will look into that.
>
>
>>
>> As for {case_clause,{4}} in ssl_v3:mac_hash/3 I found that precondition
>> for crash is easily reproducible: https://gist.github.com/stolen/10780653
>> Ordinary servers negotiate with sha hash or close socket while ssl in
>> 17.0 negotiates with sha256 hash.
>> I don't know the protocol good enough to perform further states, but I
>> hope this helps.
>>
>>
> Okej, the following patch should take care of that problem.
>
> diff --git a/lib/ssl/src/ssl.erl b/lib/ssl/src/ssl.erl
> index 743753b..093d4af 100644
> --- a/lib/ssl/src/ssl.erl
> +++ b/lib/ssl/src/ssl.erl
> @@ -953,8 +953,8 @@ handle_cipher_option(Value, Version)  when
> is_list(Value) ->
>      error:_->
>          throw({error, {options, {ciphers, Value}}})
>      end.
> -binary_cipher_suites(Version, []) -> %% Defaults to all supported suits
> -    ssl_cipher:suites(Version);
> +binary_cipher_suites(Version, []) ->
> +    [];
>  binary_cipher_suites(Version, [{_,_,_,_}| _] = Ciphers0) -> %% Backwards
> compatibility
>      Ciphers = [{KeyExchange, Cipher, Hash} || {KeyExchange, Cipher, Hash,
> _} <- Ciphers0],
>      binary_cipher_suites(Version, Ciphers);
> diff --git a/lib/ssl/src/ssl_handshake.erl b/lib/ssl/src/ssl_handshake.erl
> index 1108edc..93d4efd 100644
> --- a/lib/ssl/src/ssl_handshake.erl
> +++ b/lib/ssl/src/ssl_handshake.erl
> @@ -1017,11 +1017,14 @@ decode_suites('3_bytes', Dec) ->
>  %%-------------Cipeher suite handling --------------------------------
>
>  available_suites(UserSuites, Version) ->
> +    VersionSupportedSuites = ssl_cipher:suites(Version),
>      case UserSuites of
>      [] ->
> -        ssl_cipher:suites(Version);
> +        VersionSupportedSuites;
>      _ ->
> -        UserSuites
> +        lists:filtermap(fun(Suite) ->
> +                    lists:member(Suite, VersionSupportedSuites)
> +                end, UserSuites)
>      end.
>
>  available_suites(ServerCert, UserSuites, Version, Curve) ->
>
>
>
>
>> 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.
>>
>
> #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.
>
>
> Regards Ingela - Erlang/OTP team - Ericsson AB
>
>
>
>>
>> 2014-04-15 16:52 GMT+04:00 Ingela Andin <ingela.andin@REDACTED>:
>>
>> Hello again!
>>>
>>>
>>>>  sometimes badarg in
>>>> erlang:size([22,3,1,0,158,1,0,0,154,3,1,83,74|...]) at tls_record.erl:122.
>>>>
>>>
>>> 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
>>> is allowed to start the handshake.  Normaly if the upgrade is negotiated
>>> this is not a problem for the server to set the option
>>> 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.
>>>
>>> Regards Ingela Erlang/OTP team - Ericsson AB
>>>
>>
>>
>>
>> --
>> Danil Zagoskin | z@REDACTED
>>
>
>


-- 
Danil Zagoskin | z@REDACTED
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://erlang.org/pipermail/erlang-questions/attachments/20140416/c590b7af/attachment.htm>