From 6d88cd7a205d6152582ea1ffec237ad84eda3682 Mon Sep 17 00:00:00 2001 From: Ivan Dyachkov Date: Mon, 12 Jun 2023 18:56:48 +0200 Subject: [PATCH 1/3] fix(tls): fix incompatible tls options and issue with tls version gap --- apps/emqx/src/emqx_tls_lib.erl | 97 ++++++++++++++++++++++++++++------ 1 file changed, 80 insertions(+), 17 deletions(-) diff --git a/apps/emqx/src/emqx_tls_lib.erl b/apps/emqx/src/emqx_tls_lib.erl index b0115ddc5..d40ae5033 100644 --- a/apps/emqx/src/emqx_tls_lib.erl +++ b/apps/emqx/src/emqx_tls_lib.erl @@ -478,7 +478,7 @@ to_server_opts(Type, Opts) -> Versions = integral_versions(Type, maps:get(versions, Opts, undefined)), Ciphers = integral_ciphers(Versions, maps:get(ciphers, Opts, undefined)), Path = fun(Key) -> resolve_cert_path_for_read_strict(maps:get(Key, Opts, undefined)) end, - filter( + ensure_valid_options( maps:to_list(Opts#{ keyfile => Path(keyfile), certfile => Path(certfile), @@ -511,7 +511,7 @@ to_client_opts(Type, Opts) -> SNI = ensure_sni(Get(server_name_indication)), Versions = integral_versions(Type, Get(versions)), Ciphers = integral_ciphers(Versions, Get(ciphers)), - filter( + ensure_valid_options( [ {keyfile, KeyFile}, {certfile, CertFile}, @@ -556,33 +556,96 @@ resolve_cert_path_for_read_strict(Path) -> resolve_cert_path_for_read(Path) -> emqx_schema:naive_env_interpolation(Path). -filter([], _) -> - []; -filter([{_, undefined} | T], Versions) -> - filter(T, Versions); -filter([{_, ""} | T], Versions) -> - filter(T, Versions); -filter([{K, V} | T], Versions) -> +ensure_valid_options(Options, Versions0) -> + Versions = validate_version_gap(Versions0), + ensure_valid_options(Options, Versions, []). + +%% See also lib/ssl/src/ssl.erl#L2617. +%% Do not allow configuration of TLS 1.3 with a gap where TLS 1.2 is not supported +%% as that configuration can trigger the built in version downgrade protection +%% mechanism and the handshake can fail with an Illegal Parameter alert. +validate_version_gap(Versions) -> + case lists:member('tlsv1.3', Versions) of + true when length(Versions) >= 2 -> + case lists:member('tlsv1.2', Versions) of + true -> + Versions; + false -> + NewVersions = ['tlsv1.3'], + ?SLOG(warning, #{ + msg => "tlsv13_version_gap", + versions => Versions, + new_versions => NewVersions + }), + NewVersions + end; + _ -> + Versions + end. + +ensure_valid_options([], _, Acc) -> + lists:reverse(Acc); +ensure_valid_options([{_, undefined} | T], Versions, Acc) -> + ensure_valid_options(T, Versions, Acc); +ensure_valid_options([{_, ""} | T], Versions, Acc) -> + ensure_valid_options(T, Versions, Acc); +ensure_valid_options([{K, V} | T], Versions, Acc) -> case tls_option_compatible_versions(K) of all -> - [{K, V} | filter(T, Versions)]; + ensure_valid_options(T, Versions, [{K, V} | Acc]); CompatibleVersions -> - case CompatibleVersions -- (CompatibleVersions -- Versions) of - [] -> - filter(T, Versions); - _ -> - [{K, V} | filter(T, Versions)] + Enabled = sets:from_list(Versions), + Compatible = sets:from_list(CompatibleVersions), + case sets:size(sets:intersection(Enabled, Compatible)) > 0 of + true -> + ensure_valid_options(T, Versions, [{K, V} | Acc]); + false -> + ?SLOG(warning, #{ + msg => "drop_incompatible_tls_option", option => K, versions => Versions + }), + ensure_valid_options(T, Versions, Acc) end end. +%% see lib/ssl/src/ssl.erl, assert_option_dependency/4 +tls_option_compatible_versions(beast_mitigation) -> + [dtlsv1, 'tlsv1']; +tls_option_compatible_versions(padding_check) -> + [dtlsv1, 'tlsv1']; +tls_option_compatible_versions(client_renegotiation) -> + [dtlsv1, 'dtlsv1.2', 'tlsv1', 'tlsv1.1', 'tlsv1.2']; +tls_option_compatible_versions(reuse_session) -> + [dtlsv1, 'dtlsv1.2', 'tlsv1', 'tlsv1.1', 'tlsv1.2']; tls_option_compatible_versions(reuse_sessions) -> [dtlsv1, 'dtlsv1.2', 'tlsv1', 'tlsv1.1', 'tlsv1.2']; tls_option_compatible_versions(secure_renegotiate) -> [dtlsv1, 'dtlsv1.2', 'tlsv1', 'tlsv1.1', 'tlsv1.2']; +tls_option_compatible_versions(next_protocol_advertised) -> + [dtlsv1, 'dtlsv1.2', 'tlsv1', 'tlsv1.1', 'tlsv1.2']; +tls_option_compatible_versions(client_preferred_next_protocols) -> + [dtlsv1, 'dtlsv1.2', 'tlsv1', 'tlsv1.1', 'tlsv1.2']; +tls_option_compatible_versions(psk_identity) -> + [dtlsv1, 'dtlsv1.2', 'tlsv1', 'tlsv1.1', 'tlsv1.2']; +tls_option_compatible_versions(srp_identity) -> + [dtlsv1, 'dtlsv1.2', 'tlsv1', 'tlsv1.1', 'tlsv1.2']; tls_option_compatible_versions(user_lookup_fun) -> [dtlsv1, 'dtlsv1.2', 'tlsv1', 'tlsv1.1', 'tlsv1.2']; -tls_option_compatible_versions(client_renegotiation) -> - [dtlsv1, 'dtlsv1.2', 'tlsv1', 'tlsv1.1', 'tlsv1.2']; +tls_option_compatible_versions(early_data) -> + ['tlsv1.3']; +tls_option_compatible_versions(certificate_authorities) -> + ['tlsv1.3']; +tls_option_compatible_versions(cookie) -> + ['tlsv1.3']; +tls_option_compatible_versions(key_update_at) -> + ['tlsv1.3']; +tls_option_compatible_versions(anti_replay) -> + ['tlsv1.3']; +tls_option_compatible_versions(session_tickets) -> + ['tlsv1.3']; +tls_option_compatible_versions(supported_groups) -> + ['tlsv1.3']; +tls_option_compatible_versions(use_ticket) -> + ['tlsv1.3']; tls_option_compatible_versions(_) -> all. From 44259420303a94d86c3c234dbb69dacc579a9986 Mon Sep 17 00:00:00 2001 From: Serge Tupchii Date: Wed, 14 Jun 2023 14:57:55 +0300 Subject: [PATCH 2/3] fix(emqx_schema): move tls version gap validation to emqx_schema --- apps/emqx/src/emqx_schema.erl | 22 +++++++++++++++++++++- apps/emqx/src/emqx_tls_lib.erl | 28 ++-------------------------- apps/emqx/test/emqx_schema_tests.erl | 12 ++++++++++++ changes/ce/fix-11028.en.md | 7 +++++++ 4 files changed, 42 insertions(+), 27 deletions(-) create mode 100644 changes/ce/fix-11028.en.md diff --git a/apps/emqx/src/emqx_schema.erl b/apps/emqx/src/emqx_schema.erl index 40910614a..f97a1500b 100644 --- a/apps/emqx/src/emqx_schema.erl +++ b/apps/emqx/src/emqx_schema.erl @@ -2679,10 +2679,30 @@ validate_ciphers(Ciphers) -> validate_tls_versions(Collection, Versions) -> AvailableVersions = available_tls_vsns(Collection), case lists:filter(fun(V) -> not lists:member(V, AvailableVersions) end, Versions) of - [] -> ok; + [] -> validate_tls_version_gap(Versions); Vs -> {error, {unsupported_tls_versions, Vs}} end. +%% See also `validate_version_gap/1` in OTP ssl.erl, +%% e.g: https://github.com/emqx/otp/blob/emqx-OTP-25.1.2/lib/ssl/src/ssl.erl#L2566. +%% Do not allow configuration of TLS 1.3 with a gap where TLS 1.2 is not supported +%% as that configuration can trigger the built in version downgrade protection +%% mechanism and the handshake can fail with an Illegal Parameter alert. +validate_tls_version_gap(Versions) -> + case lists:member('tlsv1.3', Versions) of + true when length(Versions) >= 2 -> + case lists:member('tlsv1.2', Versions) of + true -> + ok; + false -> + {error, + "Using multiple versions that include tlsv1.3 but " + "exclude tlsv1.2 is not allowed"} + end; + _ -> + ok + end. + validations() -> [ {check_process_watermark, fun check_process_watermark/1}, diff --git a/apps/emqx/src/emqx_tls_lib.erl b/apps/emqx/src/emqx_tls_lib.erl index d40ae5033..b5b653f56 100644 --- a/apps/emqx/src/emqx_tls_lib.erl +++ b/apps/emqx/src/emqx_tls_lib.erl @@ -556,33 +556,9 @@ resolve_cert_path_for_read_strict(Path) -> resolve_cert_path_for_read(Path) -> emqx_schema:naive_env_interpolation(Path). -ensure_valid_options(Options, Versions0) -> - Versions = validate_version_gap(Versions0), +ensure_valid_options(Options, Versions) -> ensure_valid_options(Options, Versions, []). -%% See also lib/ssl/src/ssl.erl#L2617. -%% Do not allow configuration of TLS 1.3 with a gap where TLS 1.2 is not supported -%% as that configuration can trigger the built in version downgrade protection -%% mechanism and the handshake can fail with an Illegal Parameter alert. -validate_version_gap(Versions) -> - case lists:member('tlsv1.3', Versions) of - true when length(Versions) >= 2 -> - case lists:member('tlsv1.2', Versions) of - true -> - Versions; - false -> - NewVersions = ['tlsv1.3'], - ?SLOG(warning, #{ - msg => "tlsv13_version_gap", - versions => Versions, - new_versions => NewVersions - }), - NewVersions - end; - _ -> - Versions - end. - ensure_valid_options([], _, Acc) -> lists:reverse(Acc); ensure_valid_options([{_, undefined} | T], Versions, Acc) -> @@ -607,7 +583,7 @@ ensure_valid_options([{K, V} | T], Versions, Acc) -> end end. -%% see lib/ssl/src/ssl.erl, assert_option_dependency/4 +%% see otp/lib/ssl/src/ssl.erl, `assert_option_dependency/4` tls_option_compatible_versions(beast_mitigation) -> [dtlsv1, 'tlsv1']; tls_option_compatible_versions(padding_check) -> diff --git a/apps/emqx/test/emqx_schema_tests.erl b/apps/emqx/test/emqx_schema_tests.erl index b5fd156ad..a6e72cd27 100644 --- a/apps/emqx/test/emqx_schema_tests.erl +++ b/apps/emqx/test/emqx_schema_tests.erl @@ -94,6 +94,18 @@ ssl_opts_tls_psk_test() -> Checked = validate(Sc, #{<<"versions">> => [<<"tlsv1.2">>]}), ?assertMatch(#{versions := ['tlsv1.2']}, Checked). +ssl_opts_version_gap_test_() -> + Sc = emqx_schema:server_ssl_opts_schema(#{}, false), + RanchSc = emqx_schema:server_ssl_opts_schema(#{}, true), + Reason = "Using multiple versions that include tlsv1.3 but exclude tlsv1.2 is not allowed", + [ + ?_assertThrow( + {_, [#{kind := validation_error, reason := Reason}]}, + validate(S, #{<<"versions">> => [<<"tlsv1.1">>, <<"tlsv1.3">>]}) + ) + || S <- [Sc, RanchSc] + ]. + bad_cipher_test() -> Sc = emqx_schema:server_ssl_opts_schema(#{}, false), Reason = {bad_ciphers, ["foo"]}, diff --git a/changes/ce/fix-11028.en.md b/changes/ce/fix-11028.en.md new file mode 100644 index 000000000..c47351520 --- /dev/null +++ b/changes/ce/fix-11028.en.md @@ -0,0 +1,7 @@ +Disallow using multiple TLS versions in the listener config that include tlsv1.3 but exclude tlsv1.2. + +Using TLS configuration with such version gap caused connection errors. +Additionally, drop and log TLS options that are incompatible with the selected TLS version(s). + +Note: any old listener configuration with the version gap described above will fail to load +after applying this fix and must be manually fixed. From 7882755114534089eaae828a6ca98034224d27e5 Mon Sep 17 00:00:00 2001 From: Serge Tupchii Date: Wed, 14 Jun 2023 16:25:26 +0300 Subject: [PATCH 3/3] chore: bump cowboy to 2.9.2 cowboy 2.9.2 uses ranch 1.8.1-emqx that fixes incompatible TLS options --- apps/emqx/rebar.config | 2 +- mix.exs | 4 ++-- rebar.config | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/apps/emqx/rebar.config b/apps/emqx/rebar.config index 070922d22..8f31644fb 100644 --- a/apps/emqx/rebar.config +++ b/apps/emqx/rebar.config @@ -25,7 +25,7 @@ {emqx_utils, {path, "../emqx_utils"}}, {lc, {git, "https://github.com/emqx/lc.git", {tag, "0.3.2"}}}, {gproc, {git, "https://github.com/emqx/gproc", {tag, "0.9.0.1"}}}, - {cowboy, {git, "https://github.com/emqx/cowboy", {tag, "2.9.0"}}}, + {cowboy, {git, "https://github.com/emqx/cowboy", {tag, "2.9.2"}}}, {esockd, {git, "https://github.com/emqx/esockd", {tag, "5.9.6"}}}, {ekka, {git, "https://github.com/emqx/ekka", {tag, "0.15.2"}}}, {gen_rpc, {git, "https://github.com/emqx/gen_rpc", {tag, "2.8.1"}}}, diff --git a/mix.exs b/mix.exs index 028a5dfff..c49394786 100644 --- a/mix.exs +++ b/mix.exs @@ -52,7 +52,7 @@ defmodule EMQXUmbrella.MixProject do {:ehttpc, github: "emqx/ehttpc", tag: "0.4.10", override: true}, {:gproc, github: "emqx/gproc", tag: "0.9.0.1", override: true}, {:jiffy, github: "emqx/jiffy", tag: "1.0.5", override: true}, - {:cowboy, github: "emqx/cowboy", tag: "2.9.0", override: true}, + {:cowboy, github: "emqx/cowboy", tag: "2.9.2", override: true}, {:esockd, github: "emqx/esockd", tag: "5.9.6", override: true}, {:rocksdb, github: "emqx/erlang-rocksdb", tag: "1.7.2-emqx-11", override: true}, {:ekka, github: "emqx/ekka", tag: "0.15.2", override: true}, @@ -92,7 +92,7 @@ defmodule EMQXUmbrella.MixProject do github: "ninenines/cowlib", ref: "c6553f8308a2ca5dcd69d845f0a7d098c40c3363", override: true}, # in conflict by cowboy_swagger and cowboy {:ranch, - github: "ninenines/ranch", ref: "a692f44567034dacf5efcaa24a24183788594eb7", override: true}, + github: "emqx/ranch", ref: "de8ba2a00817c0a6eb1b8f20d6fb3e44e2c9a5aa", override: true}, # in conflict by grpc and eetcd {:gpb, "4.19.7", override: true, runtime: false}, {:hackney, github: "emqx/hackney", tag: "1.18.1-1", override: true} diff --git a/rebar.config b/rebar.config index 57661f75f..4b6cf937a 100644 --- a/rebar.config +++ b/rebar.config @@ -59,7 +59,7 @@ , {ehttpc, {git, "https://github.com/emqx/ehttpc", {tag, "0.4.10"}}} , {gproc, {git, "https://github.com/emqx/gproc", {tag, "0.9.0.1"}}} , {jiffy, {git, "https://github.com/emqx/jiffy", {tag, "1.0.5"}}} - , {cowboy, {git, "https://github.com/emqx/cowboy", {tag, "2.9.0"}}} + , {cowboy, {git, "https://github.com/emqx/cowboy", {tag, "2.9.2"}}} , {esockd, {git, "https://github.com/emqx/esockd", {tag, "5.9.6"}}} , {rocksdb, {git, "https://github.com/emqx/erlang-rocksdb", {tag, "1.7.2-emqx-11"}}} , {ekka, {git, "https://github.com/emqx/ekka", {tag, "0.15.2"}}}