Merge pull request #2268 from IngelaAndin/ingela/ssl/DTLS-version-check/ERL-920

ssl: Change check of DTLS record version

OTP-15807 Merged as it is an confirmed improvement, however more work needs to be done
on DTLS test framework.

2 files changed, 41 insertions, 55 deletions

diff --git a/lib/ssl/src/dtls_connection.erl b/lib/ssl/src/dtls_connection.erl
index 6928d7a93d..b220691e79 100644
--- a/lib/ssl/src/dtls_connection.erl
+++ b/lib/ssl/src/dtls_connection.erl
@@ -836,9 +836,12 @@ initial_flight_state(_) ->
 next_dtls_record(Data, StateName, #state{protocol_buffers = #protocol_buffers{
 						   dtls_record_buffer = Buf0,
 						   dtls_cipher_texts = CT0} = Buffers,
+                                         connection_env = #connection_env{negotiated_version = Version},
+                                         static_env = #static_env{data_tag = DataTag},
                                          ssl_options = SslOpts} = State0) ->
     case dtls_record:get_dtls_records(Data,
-                                      acceptable_record_versions(StateName, State0), 
+                                      {DataTag, StateName, Version, 
+                                       [dtls_record:protocol_version(Vsn) || Vsn <- ?ALL_AVAILABLE_DATAGRAM_VERSIONS]}, 
                                       Buf0, SslOpts) of
 	{Records, Buf1} ->
 	    CT1 = CT0 ++ Records,
@@ -849,10 +852,6 @@ next_dtls_record(Data, StateName, #state{protocol_buffers = #protocol_buffers{
 	    Alert
     end.
 
-acceptable_record_versions(hello, _) ->
-    [dtls_record:protocol_version(Vsn) || Vsn <- ?ALL_AVAILABLE_DATAGRAM_VERSIONS];
-acceptable_record_versions(_, #state{connection_env = #connection_env{negotiated_version = Version}}) ->
-    [Version].
 
 dtls_handshake_events(Packets) ->
     lists:map(fun(Packet) ->
diff --git a/lib/ssl/src/dtls_record.erl b/lib/ssl/src/dtls_record.erl
index a4846f42c5..8b8db7b2de 100644
--- a/lib/ssl/src/dtls_record.erl
+++ b/lib/ssl/src/dtls_record.erl
@@ -162,26 +162,16 @@ current_connection_state_epoch(#{current_write := #{epoch := Epoch}},
     Epoch.
 
 %%--------------------------------------------------------------------
--spec get_dtls_records(binary(), [ssl_record:ssl_version()], binary(),
+-spec get_dtls_records(binary(), {atom(), atom(), ssl_record:ssl_version(), [ssl_record:ssl_version()]}, binary(),
                        #ssl_options{}) -> {[binary()], binary()} | #alert{}.
 %%
 %% Description: Given old buffer and new data from UDP/SCTP, packs up a records
 %% and returns it as a list of tls_compressed binaries also returns leftover
 %% data
 %%--------------------------------------------------------------------
-get_dtls_records(Data, Versions, Buffer, SslOpts) ->
+get_dtls_records(Data, Vinfo, Buffer, SslOpts) ->
     BinData = list_to_binary([Buffer, Data]),
-    case erlang:byte_size(BinData) of
-        N when N >= 3 ->
-            case assert_version(BinData, Versions) of
-                true ->
-                    get_dtls_records_aux(BinData, [], SslOpts);
-                false ->
-                    ?ALERT_REC(?FATAL, ?BAD_RECORD_MAC)
-            end;
-        _ ->
-            get_dtls_records_aux(BinData, [], SslOpts)
-    end.
+    get_dtls_records_aux(Vinfo, BinData, [], SslOpts).
 
 %%====================================================================
 %% Encoding DTLS records
@@ -405,52 +395,49 @@ initial_connection_state(ConnectionEnd, BeastMitigation) ->
       client_verify_data => undefined,
       server_verify_data => undefined
      }.
-assert_version(<<?BYTE(_), ?BYTE(MajVer), ?BYTE(MinVer), _/binary>>, Versions) ->
-    is_acceptable_version({MajVer, MinVer}, Versions).
 
-get_dtls_records_aux(<<?BYTE(?APPLICATION_DATA),?BYTE(MajVer),?BYTE(MinVer),
-		       ?UINT16(Epoch), ?UINT48(SequenceNumber),
-		       ?UINT16(Length), Data:Length/binary, Rest/binary>> = RawDTLSRecord,
-		     Acc, SslOpts) ->
-    ssl_logger:debug(SslOpts#ssl_options.log_level, inbound, 'record', [RawDTLSRecord]),
-    get_dtls_records_aux(Rest, [#ssl_tls{type = ?APPLICATION_DATA,
-					 version = {MajVer, MinVer},
-					 epoch = Epoch, sequence_number = SequenceNumber,
-					 fragment = Data} | Acc], SslOpts);
-get_dtls_records_aux(<<?BYTE(?HANDSHAKE),?BYTE(MajVer),?BYTE(MinVer),
-		       ?UINT16(Epoch), ?UINT48(SequenceNumber),
-		       ?UINT16(Length),
-		       Data:Length/binary, Rest/binary>> = RawDTLSRecord,
-                     Acc, SslOpts) when MajVer >= 128 ->
-    ssl_logger:debug(SslOpts#ssl_options.log_level, inbound, 'record', [RawDTLSRecord]),
-    get_dtls_records_aux(Rest, [#ssl_tls{type = ?HANDSHAKE,
-					 version = {MajVer, MinVer},
-					 epoch = Epoch, sequence_number = SequenceNumber,
-					 fragment = Data} | Acc], SslOpts);
-get_dtls_records_aux(<<?BYTE(?ALERT),?BYTE(MajVer),?BYTE(MinVer),
-		       ?UINT16(Epoch), ?UINT48(SequenceNumber),
-		       ?UINT16(Length), Data:Length/binary,
-		       Rest/binary>> = RawDTLSRecord, Acc, SslOpts) ->
+get_dtls_records_aux({DataTag, StateName, _, Versions} = Vinfo, <<?BYTE(Type),?BYTE(MajVer),?BYTE(MinVer),
+                                                         ?UINT16(Epoch), ?UINT48(SequenceNumber),
+                                                         ?UINT16(Length), Data:Length/binary, Rest/binary>> = RawDTLSRecord,
+		     Acc, SslOpts) when ((StateName == hello) orelse
+                                         ((StateName == certify) andalso (DataTag == udp)) orelse
+                                         ((StateName == abbreviated) andalso(DataTag == udp))) 
+                                        andalso
+                                        ((Type == ?HANDSHAKE) orelse
+                                         (Type == ?ALERT)) ->
     ssl_logger:debug(SslOpts#ssl_options.log_level, inbound, 'record', [RawDTLSRecord]),
-    get_dtls_records_aux(Rest, [#ssl_tls{type = ?ALERT,
-					 version = {MajVer, MinVer},
-					 epoch = Epoch, sequence_number = SequenceNumber,
-					 fragment = Data} | Acc], SslOpts);
-get_dtls_records_aux(<<?BYTE(?CHANGE_CIPHER_SPEC),?BYTE(MajVer),?BYTE(MinVer),
+    case is_acceptable_version({MajVer, MinVer}, Versions) of
+        true ->
+            get_dtls_records_aux(Vinfo, Rest, [#ssl_tls{type = Type,
+                                                 version = {MajVer, MinVer},
+                                                 epoch = Epoch, sequence_number = SequenceNumber,
+                                                 fragment = Data} | Acc], SslOpts);
+        false ->
+              ?ALERT_REC(?FATAL, ?BAD_RECORD_MAC)
+        end;
+get_dtls_records_aux({_, _, Version, _} = Vinfo, <<?BYTE(Type),?BYTE(MajVer),?BYTE(MinVer),
 		       ?UINT16(Epoch), ?UINT48(SequenceNumber),
 		       ?UINT16(Length), Data:Length/binary, Rest/binary>> = RawDTLSRecord,
-		     Acc, SslOpts) ->
+		     Acc, SslOpts) when (Type == ?APPLICATION_DATA) orelse
+                                        (Type == ?HANDSHAKE) orelse
+                                        (Type == ?ALERT) orelse
+                                        (Type == ?CHANGE_CIPHER_SPEC) ->
     ssl_logger:debug(SslOpts#ssl_options.log_level, inbound, 'record', [RawDTLSRecord]),
-    get_dtls_records_aux(Rest, [#ssl_tls{type = ?CHANGE_CIPHER_SPEC,
-					 version = {MajVer, MinVer},
-					 epoch = Epoch, sequence_number = SequenceNumber,
-					 fragment = Data} | Acc], SslOpts);
-get_dtls_records_aux(<<?BYTE(_), ?BYTE(_MajVer), ?BYTE(_MinVer),
+    case {MajVer, MinVer} of
+        Version ->
+            get_dtls_records_aux(Vinfo, Rest, [#ssl_tls{type = Type,
+                                                 version = {MajVer, MinVer},
+                                                 epoch = Epoch, sequence_number = SequenceNumber,
+                                                 fragment = Data} | Acc], SslOpts);
+        _ ->
+            ?ALERT_REC(?FATAL, ?BAD_RECORD_MAC)
+    end;
+get_dtls_records_aux(_, <<?BYTE(_), ?BYTE(_MajVer), ?BYTE(_MinVer),
 		       ?UINT16(Length), _/binary>>,
 		     _Acc, _) when Length > ?MAX_CIPHER_TEXT_LENGTH ->
     ?ALERT_REC(?FATAL, ?RECORD_OVERFLOW);
 
-get_dtls_records_aux(Data, Acc, _) ->
+get_dtls_records_aux(_, Data, Acc, _) ->
     case size(Data) =< ?MAX_CIPHER_TEXT_LENGTH + ?INITIAL_BYTES of
 	true ->
 	    {lists:reverse(Acc), Data};