Uploaded image for project: 'Erlang/OTP'
  1. Erlang/OTP
  2. ERL-347

Incorrectly aborted TLS handshake - ssl_v2_client_hello_no_supported

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 19.2
    • Fix Version/s: 19.3
    • Component/s: ssl
    • Labels:
      None

      Description

      =ERROR REPORT==== 31-Jan-2017::11:21:29 ===
      SSL: hello: ../src/tls_handshake.erl:204:Fatal error: handshake failure - handshake_decode_error

      Having setup a simple SSL/TLS server not supporting SSLv2, sending a CLIENT_HELLO (for TLSv1.2) where the lower 16 bits of the GMT time is 0 and where the upper 16 bits of the GMT time + the first two bytes of the ClientRandom data happens to match the length of the rest of the message the handshake will be aborted with 'ssl_v2_client_hello_no_supported'.

      A concrete HELLO_CLIENT package that will be rejected is:
      {{<<1,0,0,71,3,3,0,0,0,0,0,63,210,235,149,6,244,140,108,13,177,74,
      16,218,33,108,219,41,73,228,3,82,132,123,73,144,118,100,0,0,
      32,192,4,0,10,192,45,192,38,0,47,192,18,0,163,0,22,0,165,192,
      29,192,18,192,30,0,103,0,57,192,48,0,47,1,0>>
      }}

        Activity

        Hide
        ingela Ingela Anderton Andin added a comment -

        That is unfortunate, the following patch should fix the problem.

        diff --git a/lib/ssl/src/tls_handshake.erl b/lib/ssl/src/tls_handshake.erl
        index 2800ee6..7584966 100644
        --- a/lib/ssl/src/tls_handshake.erl
        +++ b/lib/ssl/src/tls_handshake.erl
        @@ -207,6 +207,23 @@ get_tls_handshake_aux(_Version, Data, _, Acc) ->
         decode_handshake(_, ?HELLO_REQUEST, <<>>, _) ->
             #hello_request{};
         
        +decode_handshake(_Version, ?CLIENT_HELLO, <<?BYTE(Major), ?BYTE(Minor), Random:32/binary,
        +					    ?BYTE(SID_length), Session_ID:SID_length/binary,
        +					    ?UINT16(Cs_length), CipherSuites:Cs_length/binary,
        +					    ?BYTE(Cm_length), Comp_methods:Cm_length/binary,
        +					    Extensions/binary>>, _) ->
        +    
        +    DecodedExtensions = ssl_handshake:decode_hello_extensions({client, Extensions}),
        +
        +    #client_hello{
        +       client_version = {Major,Minor},
        +       random = Random,
        +       session_id = Session_ID,
        +       cipher_suites = ssl_handshake:decode_suites('2_bytes', CipherSuites),
        +       compression_methods = Comp_methods,
        +       extensions = DecodedExtensions
        +      };
        +
         %% Client hello v2.
         %% The server must be able to receive such messages, from clients that
         %% are willing to use ssl v3 or higher, but have ssl v2 compatibility.
        Stage this hunk [y,n,q,a,d,/,j,J,g,e,?]? y
        @@ -228,22 +245,6 @@ decode_handshake(_Version, ?CLIENT_HELLO, <<?BYTE(_), ?BYTE(_),
         					    _CipherSuites:CSLength/binary,
         					    _ChallengeData:CDLength/binary>>, false) ->
             throw(?ALERT_REC(?FATAL, ?PROTOCOL_VERSION, ssl_v2_client_hello_no_supported));
        -decode_handshake(_Version, ?CLIENT_HELLO, <<?BYTE(Major), ?BYTE(Minor), Random:32/binary,
        -					    ?BYTE(SID_length), Session_ID:SID_length/binary,
        -					    ?UINT16(Cs_length), CipherSuites:Cs_length/binary,
        -					    ?BYTE(Cm_length), Comp_methods:Cm_length/binary,
        -					    Extensions/binary>>, _) ->
        -    
        -    DecodedExtensions = ssl_handshake:decode_hello_extensions({client, Extensions}),
        -
        -    #client_hello{
        -       client_version = {Major,Minor},
        -       random = Random,
        -       session_id = Session_ID,
        -       cipher_suites = ssl_handshake:decode_suites('2_bytes', CipherSuites),
        -       compression_methods = Comp_methods,
        -       extensions = DecodedExtensions
        -      };
         
         decode_handshake(Version, Tag, Msg, _) ->
             ssl_handshake:decode_handshake(Version, Tag, Msg).
        
        

        Show
        ingela Ingela Anderton Andin added a comment - That is unfortunate, the following patch should fix the problem. diff --git a/lib/ssl/src/tls_handshake.erl b/lib/ssl/src/tls_handshake.erl index 2800ee6.. 7584966 100644 --- a/lib/ssl/src/tls_handshake.erl +++ b/lib/ssl/src/tls_handshake.erl @@ - 207 , 6 + 207 , 23 @@ get_tls_handshake_aux(_Version, Data, _, Acc) -> decode_handshake(_, ?HELLO_REQUEST, <<>>, _) -> #hello_request{}; +decode_handshake(_Version, ?CLIENT_HELLO, <<?BYTE(Major), ?BYTE(Minor), Random: 32 /binary, + ?BYTE(SID_length), Session_ID:SID_length/binary, + ?UINT16(Cs_length), CipherSuites:Cs_length/binary, + ?BYTE(Cm_length), Comp_methods:Cm_length/binary, + Extensions/binary>>, _) -> + + DecodedExtensions = ssl_handshake:decode_hello_extensions({client, Extensions}), + + #client_hello{ + client_version = {Major,Minor}, + random = Random, + session_id = Session_ID, + cipher_suites = ssl_handshake:decode_suites( '2_bytes' , CipherSuites), + compression_methods = Comp_methods, + extensions = DecodedExtensions + }; + %% Client hello v2. %% The server must be able to receive such messages, from clients that %% are willing to use ssl v3 or higher, but have ssl v2 compatibility. Stage this hunk [y,n,q,a,d,/,j,J,g,e,?]? y @@ - 228 , 22 + 245 , 6 @@ decode_handshake(_Version, ?CLIENT_HELLO, <<?BYTE(_), ?BYTE(_), _CipherSuites:CSLength/binary, _ChallengeData:CDLength/binary>>, false ) -> throw (?ALERT_REC(?FATAL, ?PROTOCOL_VERSION, ssl_v2_client_hello_no_supported)); -decode_handshake(_Version, ?CLIENT_HELLO, <<?BYTE(Major), ?BYTE(Minor), Random: 32 /binary, - ?BYTE(SID_length), Session_ID:SID_length/binary, - ?UINT16(Cs_length), CipherSuites:Cs_length/binary, - ?BYTE(Cm_length), Comp_methods:Cm_length/binary, - Extensions/binary>>, _) -> - - DecodedExtensions = ssl_handshake:decode_hello_extensions({client, Extensions}), - - #client_hello{ - client_version = {Major,Minor}, - random = Random, - session_id = Session_ID, - cipher_suites = ssl_handshake:decode_suites( '2_bytes' , CipherSuites), - compression_methods = Comp_methods, - extensions = DecodedExtensions - }; decode_handshake(Version, Tag, Msg, _) -> ssl_handshake:decode_handshake(Version, Tag, Msg).
        Hide
        hanssv hanssv added a comment -

        I don't have full insight in all the steps involved in the parsing here, but doesn't the suggested patch have the risk of parsing 'Client hello v2' messages already in the first clause that is intended for SSLv3/TLS?

        Show
        hanssv hanssv added a comment - I don't have full insight in all the steps involved in the parsing here, but doesn't the suggested patch have the risk of parsing 'Client hello v2' messages already in the first clause that is intended for SSLv3/TLS?
        Hide
        ingela Ingela Anderton Andin added a comment - - edited

        I think that the risk that an SSLv2 Hello message accidentally matches the format of a SSL/TLS hello massage is
        much smaller than the other way around. Also support for SSLv2 hello messages are being phased out and are much less important than SSL/TLS hello messages. In the unlikely event an SSLv2 message should match SSL/TLS hello message it would probably fail trying to use incorrect data. And choosing between somtimes failing to support SSLv2 enabled clients (which should be rare nowadays ) and sporadic TLS handshake failuers...

        Show
        ingela Ingela Anderton Andin added a comment - - edited I think that the risk that an SSLv2 Hello message accidentally matches the format of a SSL/TLS hello massage is much smaller than the other way around. Also support for SSLv2 hello messages are being phased out and are much less important than SSL/TLS hello messages. In the unlikely event an SSLv2 message should match SSL/TLS hello message it would probably fail trying to use incorrect data. And choosing between somtimes failing to support SSLv2 enabled clients (which should be rare nowadays ) and sporadic TLS handshake failuers...
        Hide
        hanssv hanssv added a comment -

        Sounds reasonable

        Show
        hanssv hanssv added a comment - Sounds reasonable
        Hide
        ingela Ingela Anderton Andin added a comment -

        Well some testing says it apparently happens more often than you might think so I have a slightly more advanced patch in testing now.

        Show
        ingela Ingela Anderton Andin added a comment - Well some testing says it apparently happens more often than you might think so I have a slightly more advanced patch in testing now.
        Hide
        ingela Ingela Anderton Andin added a comment -

        Branch merged to maint and pushed to github.

        Show
        ingela Ingela Anderton Andin added a comment - Branch merged to maint and pushed to github.

          People

          • Assignee:
            ingela Ingela Anderton Andin
            Reporter:
            hanssv hanssv
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development