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

[Ingela Andin]

Hi!

2014-04-16 15:56 GMT+02:00 Danil Zagoskin <z@REDACTED>:

> Thank you for the patch. I'll check it.
>


New patch, better tested ;),  on branch
ia/ssl/user-suites-match-negotiated-version at
https://github.com/IngelaAndin/otp


>
> (Style question: is lists:filtermap better than list comprehension?)
>


I think just a matter of taste!


> > #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.
>
>
Yes  you have a good point.  I do not have a good short answer. Testing
error cases especially when the protocol sometimes
tries to hide the real error reason due to security issues and shortcutting
the API and creating raw protocol command is not trivial.

Regards Ingela Erlang/OTP team - Ericsson AB





>
> 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/5c82410b/attachment.htm>