ssl: Process "supported_versions" before decoding

Change-Id: I465760b7001692367c68839219745e40abafdfa8

5 files changed, 67 insertions, 14 deletions

diff --git a/lib/ssl/src/dtls_handshake.erl b/lib/ssl/src/dtls_handshake.erl
index 36c4b540b6..eb0f742e70 100644
--- a/lib/ssl/src/dtls_handshake.erl
+++ b/lib/ssl/src/dtls_handshake.erl
@@ -340,8 +340,9 @@ decode_handshake(Version, ?CLIENT_HELLO, <<?UINT24(_), ?UINT16(_),
 					    ?BYTE(Cm_length), Comp_methods:Cm_length/binary,
 					    Extensions/binary>>) ->
     TLSVersion = dtls_v1:corresponding_tls_version(Version),
+    LegacyVersion = dtls_v1:corresponding_tls_version({Major, Minor}),
     Exts = ssl_handshake:decode_vector(Extensions),
-    DecodedExtensions = ssl_handshake:decode_hello_extensions(Exts, TLSVersion, client),
+    DecodedExtensions = ssl_handshake:decode_hello_extensions(Exts, TLSVersion, LegacyVersion, client),
 
     #client_hello{
        client_version = {Major,Minor},
diff --git a/lib/ssl/src/ssl_handshake.erl b/lib/ssl/src/ssl_handshake.erl
index 9ed2654668..1b555f0277 100644
--- a/lib/ssl/src/ssl_handshake.erl
+++ b/lib/ssl/src/ssl_handshake.erl
@@ -61,7 +61,7 @@
 -export([encode_handshake/2, encode_hello_extensions/1, encode_extensions/1, encode_extensions/2,
 	 encode_client_protocol_negotiation/2, encode_protocols_advertised_on_server/1]).
 %% Decode
--export([decode_handshake/3, decode_vector/1, decode_hello_extensions/3, decode_extensions/3,
+-export([decode_handshake/3, decode_vector/1, decode_hello_extensions/4, decode_extensions/3,
 	 decode_server_key/3, decode_client_key/3,
 	 decode_suites/2
 	]).
@@ -746,7 +746,7 @@ decode_handshake(Version, ?SERVER_HELLO, <<?BYTE(Major), ?BYTE(Minor), Random:32
 		       Cipher_suite:2/binary, ?BYTE(Comp_method),
 		       ?UINT16(ExtLen), Extensions:ExtLen/binary>>) ->
 
-    HelloExtensions = decode_hello_extensions(Extensions, Version, server_hello),
+    HelloExtensions = decode_hello_extensions(Extensions, Version, {Major, Minor}, server_hello),
 
     #server_hello{
        server_version = {Major,Minor},
@@ -803,11 +803,12 @@ decode_vector(<<?UINT16(Len), Vector:Len/binary>>) ->
     Vector.
 
 %%--------------------------------------------------------------------
--spec decode_hello_extensions(binary(), ssl_record:ssl_version(), atom()) -> map().
+-spec decode_hello_extensions(binary(), ssl_record:ssl_version(),
+                              ssl_record:ssl_version(), atom()) -> map().
 %%
 %% Description: Decodes TLS hello extensions
 %%--------------------------------------------------------------------
-decode_hello_extensions(Extensions, Version, MessageType0) ->
+decode_hello_extensions(Extensions, LocalVersion, LegacyVersion, MessageType0) ->
     %% Convert legacy atoms
     MessageType =
         case MessageType0 of
@@ -815,6 +816,13 @@ decode_hello_extensions(Extensions, Version, MessageType0) ->
             server -> server_hello;
             T -> T
         end,
+    %% RFC 8446 - 4.2.1
+    %% Servers MUST be prepared to receive ClientHellos that include this extension but
+    %% do not include 0x0304 in the list of versions.
+    %% Clients MUST check for this extension prior to processing the rest of the
+    %% ServerHello (although they will have to parse the ServerHello in order to read
+    %% the extension).
+    Version = process_supported_versions_extension(Extensions, LocalVersion, LegacyVersion),
     decode_extensions(Extensions, Version, MessageType, empty_extensions(Version, MessageType)).
 
 %%--------------------------------------------------------------------
@@ -2195,6 +2203,47 @@ dec_server_key_signature(Params, <<?UINT16(Len), Signature:Len/binary>>, _) ->
 dec_server_key_signature(_, _, _) ->
     throw(?ALERT_REC(?FATAL, ?HANDSHAKE_FAILURE, failed_to_decrypt_server_key_sign)).
 
+%% Processes a ClientHello/ServerHello message and returns the version to be used
+%% in the decoding functions. The following rules apply:
+%% - IF supported_versions extension is absent:
+%%   RETURN the lowest of (LocalVersion and LegacyVersion)
+%% - IF supported_versions estension is present:
+%%   RETURN the lowest of (LocalVersion and first element of supported versions)
+process_supported_versions_extension(<<>>, LocalVersion, LegacyVersion)
+  when LegacyVersion =< LocalVersion ->
+    LegacyVersion;
+process_supported_versions_extension(<<>>, LocalVersion, _LegacyVersion) ->
+    LocalVersion;
+process_supported_versions_extension(<<?UINT16(?SUPPORTED_VERSIONS_EXT), ?UINT16(Len),
+                                       ExtData:Len/binary, _Rest/binary>>,
+                                     LocalVersion, _LegacyVersion) when Len > 2 ->
+    <<?UINT16(_),Versions0/binary>> = ExtData,
+    [Highest|_] = decode_versions(Versions0),
+    if Highest =< LocalVersion ->
+            Highest;
+       true ->
+            LocalVersion
+    end;
+process_supported_versions_extension(<<?UINT16(?SUPPORTED_VERSIONS_EXT), ?UINT16(Len),
+                                       ?BYTE(Major),?BYTE(Minor), _Rest/binary>>,
+                                     LocalVersion, _LegacyVersion) when Len =:= 2 ->
+    SelectedVersion = {Major, Minor},
+    if SelectedVersion =< LocalVersion ->
+            SelectedVersion;
+       true ->
+            LocalVersion
+    end;
+process_supported_versions_extension(<<?UINT16(_), ?UINT16(Len),
+                                       _ExtData:Len/binary, Rest/binary>>,
+                                     LocalVersion, LegacyVersion) ->
+    process_supported_versions_extension(Rest, LocalVersion, LegacyVersion);
+%% Tolerate protocol encoding errors and skip parsing the rest of the extension.
+process_supported_versions_extension(_, LocalVersion, LegacyVersion)
+  when LegacyVersion =< LocalVersion ->
+    LegacyVersion;
+process_supported_versions_extension(_, LocalVersion, _) ->
+    LocalVersion.
+
 decode_extensions(<<>>, _Version, _MessageType, Acc) ->
     Acc;
 decode_extensions(<<?UINT16(?ALPN_EXT), ?UINT16(ExtLen), ?UINT16(Len),
diff --git a/lib/ssl/src/tls_handshake.erl b/lib/ssl/src/tls_handshake.erl
index 644763651f..3766bc1302 100644
--- a/lib/ssl/src/tls_handshake.erl
+++ b/lib/ssl/src/tls_handshake.erl
@@ -401,14 +401,15 @@ get_tls_handshake_aux(_Version, Data, _, Acc) ->
 
 decode_handshake({3, N}, ?HELLO_REQUEST, <<>>) when N < 4 ->
     #hello_request{};
-decode_handshake(Version, ?CLIENT_HELLO, 
+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>>) ->
     Exts = ssl_handshake:decode_vector(Extensions),
-    DecodedExtensions = ssl_handshake:decode_hello_extensions(Exts, Version, client_hello),
+    DecodedExtensions = ssl_handshake:decode_hello_extensions(Exts, Version, {Major, Minor},
+                                                              client_hello),
     #client_hello{
        client_version = {Major,Minor},
        random = Random,
diff --git a/lib/ssl/test/property_test/ssl_eqc_handshake.erl b/lib/ssl/test/property_test/ssl_eqc_handshake.erl
index 6ffb6d311f..20a581328e 100644
--- a/lib/ssl/test/property_test/ssl_eqc_handshake.erl
+++ b/lib/ssl/test/property_test/ssl_eqc_handshake.erl
@@ -326,7 +326,7 @@ extensions(?'TLS_v1.3' = Version, client_hello) ->
            %% oneof([psk_key_exchange_modes(), undefined]),
            %% oneof([early_data(), undefined]),
            %% oneof([cookie(), undefined]),
-           oneof([client_hello_versions(Version), undefined]),
+           oneof([client_hello_versions(Version)]),
            %% oneof([cert_authorities(), undefined]),
            %% oneof([post_handshake_auth(), undefined]),
            oneof([signature_algs_cert(), undefined])
@@ -403,7 +403,7 @@ extensions(?'TLS_v1.3' = Version, server_hello) ->
          {
           oneof([key_share(server_hello), undefined]),
           %% oneof([pre_shared_keys(),  undefined]),
-          oneof([server_hello_selected_version(), undefined])
+          oneof([server_hello_selected_version()])
          },
          maps:filter(fun(_, undefined) ->
                              false;
@@ -514,7 +514,9 @@ sig_scheme_list() ->
 client_hello_versions(?'TLS_v1.3') ->
     ?LET(SupportedVersions,
          oneof([[{3,4}],
-                [{3,3},{3,4}],
+                %% This list breaks the property but can be used for negative tests
+                %% [{3,3},{3,4}],
+                [{3,4},{3,3}],
                 [{3,4},{3,3},{3,2},{3,1},{3,0}]
                ]),
         #client_hello_versions{versions = SupportedVersions});
diff --git a/lib/ssl/test/ssl_handshake_SUITE.erl b/lib/ssl/test/ssl_handshake_SUITE.erl
index 5392729af2..da8d8a7be8 100644
--- a/lib/ssl/test/ssl_handshake_SUITE.erl
+++ b/lib/ssl/test/ssl_handshake_SUITE.erl
@@ -126,13 +126,13 @@ decode_supported_elliptic_curves_hello_extension_correctly(_Config) ->
     Len = ListLen + 2,
     Extension = <<?UINT16(?ELLIPTIC_CURVES_EXT), ?UINT16(Len), ?UINT16(ListLen), EllipticCurveList/binary>>,
     % after decoding we should see only valid curves
-    Extensions = ssl_handshake:decode_hello_extensions(Extension, {3,2}, client),
+    Extensions = ssl_handshake:decode_hello_extensions(Extension, {3,2}, {3,2}, client),
     #{elliptic_curves := #elliptic_curves{elliptic_curve_list = [?sect233k1, ?sect193r2]}} = Extensions. 
 
 decode_unknown_hello_extension_correctly(_Config) ->
     FourByteUnknown = <<16#CA,16#FE, ?UINT16(4), 3, 0, 1, 2>>,
     Renegotiation = <<?UINT16(?RENEGOTIATION_EXT), ?UINT16(1), 0>>,
-    Extensions = ssl_handshake:decode_hello_extensions(<<FourByteUnknown/binary, Renegotiation/binary>>, {3,2}, client),
+    Extensions = ssl_handshake:decode_hello_extensions(<<FourByteUnknown/binary, Renegotiation/binary>>, {3,2}, {3,2}, client),
     #{renegotiation_info := #renegotiation_info{renegotiated_connection = <<0>>}} = Extensions.
 
 
@@ -147,12 +147,12 @@ encode_single_hello_sni_extension_correctly(_Config) ->
 decode_single_hello_sni_extension_correctly(_Config) ->
     SNI = <<16#00, 16#00, 16#00, 16#0d, 16#00, 16#0b, 16#00, 16#00, 16#08,
 	    $t,    $e,    $s,    $t,    $.,    $c,    $o,    $m>>,
-    Decoded = ssl_handshake:decode_hello_extensions(SNI, {3,3}, client),
+    Decoded = ssl_handshake:decode_hello_extensions(SNI, {3,3}, {3,3}, client),
     #{sni := #sni{hostname = "test.com"}} = Decoded.
 
 decode_empty_server_sni_correctly(_Config) ->
     SNI = <<?UINT16(?SNI_EXT),?UINT16(0)>>,
-    Decoded = ssl_handshake:decode_hello_extensions(SNI, {3,3}, server),
+    Decoded = ssl_handshake:decode_hello_extensions(SNI, {3,3}, {3,3}, server),
     #{sni := #sni{hostname = ""}} = Decoded.