From 86cfbfb43c6f81027617ce59a4791e5d39a30a31 Mon Sep 17 00:00:00 2001 From: Kjell Winblad Date: Wed, 25 Jan 2023 14:07:56 +0100 Subject: [PATCH 1/3] fix: Authorization header leak in log entries for webhook There might be another possibility for leakage. If the resource mangager for the webhook resource crashes, OTP might log the spec for the resource manager which contains the Config and thus the Authorization header. This is probably an issue for other resources as well and should be fixed in another commit. The following issue has been created for that: https://emqx.atlassian.net/browse/EMQX-8794 Fixes: https://emqx.atlassian.net/browse/EMQX-8791 --- apps/emqx/src/emqx_misc.erl | 6 ++- .../src/emqx_connector_http.erl | 52 ++++++++++++++++--- 2 files changed, 50 insertions(+), 8 deletions(-) diff --git a/apps/emqx/src/emqx_misc.erl b/apps/emqx/src/emqx_misc.erl index c20227c07..fbeec8724 100644 --- a/apps/emqx/src/emqx_misc.erl +++ b/apps/emqx/src/emqx_misc.erl @@ -609,7 +609,11 @@ do_redact(K, V, Checker) -> -define(REDACT_VAL, "******"). redact_v(V) when is_binary(V) -> <>; -redact_v(_V) -> ?REDACT_VAL. +%% The HOCON schema system may generate sensitive values with this format +redact_v([{str, Bin}]) when is_binary(Bin) -> + [{str, <>}]; +redact_v(_V) -> + ?REDACT_VAL. is_redacted(K, V) -> do_is_redacted(K, V, fun is_sensitive_key/1). diff --git a/apps/emqx_connector/src/emqx_connector_http.erl b/apps/emqx_connector/src/emqx_connector_http.erl index 7f84c665a..40df52d45 100644 --- a/apps/emqx_connector/src/emqx_connector_http.erl +++ b/apps/emqx_connector/src/emqx_connector_http.erl @@ -209,7 +209,7 @@ on_start( ?SLOG(info, #{ msg => "starting_http_connector", connector => InstId, - config => emqx_misc:redact(Config) + config => redact(Config) }), {Transport, TransportOpts} = case Scheme of @@ -285,7 +285,11 @@ on_query( ?TRACE( "QUERY", "http_connector_received", - #{request => Request, connector => InstId, state => State} + #{ + request => redact(Request), + connector => InstId, + state => redact(State) + } ), NRequest = formalize_request(Method, BasePath, Request), case @@ -310,7 +314,7 @@ on_query( {error, Reason} = Result -> ?SLOG(error, #{ msg => "http_connector_do_request_failed", - request => NRequest, + request => redact(NRequest), reason => Reason, connector => InstId }), @@ -322,7 +326,7 @@ on_query( {ok, StatusCode, Headers} -> ?SLOG(error, #{ msg => "http connector do request, received error response", - request => NRequest, + request => redact(NRequest), connector => InstId, status_code => StatusCode }), @@ -330,7 +334,7 @@ on_query( {ok, StatusCode, Headers, Body} -> ?SLOG(error, #{ msg => "http connector do request, received error response", - request => NRequest, + request => redact(NRequest), connector => InstId, status_code => StatusCode }), @@ -366,7 +370,11 @@ on_query_async( ?TRACE( "QUERY_ASYNC", "http_connector_received", - #{request => Request, connector => InstId, state => State} + #{ + request => redact(Request), + connector => InstId, + state => redact(State) + } ), NRequest = formalize_request(Method, BasePath, Request), Worker = @@ -401,7 +409,7 @@ do_get_status(PoolName, Timeout) -> {error, Reason} = Error -> ?SLOG(error, #{ msg => "http_connector_get_status_failed", - reason => Reason, + reason => redact(Reason), worker => Worker }), Error @@ -554,3 +562,33 @@ reply_delegator(ReplyFunAndArgs, Result) -> _ -> emqx_resource:apply_reply_fun(ReplyFunAndArgs, Result) end. + +%% The HOCON schema system may generate sensitive keys with this format +is_sensitive_key([{str, StringKey}]) -> + is_sensitive_key(StringKey); +is_sensitive_key(Atom) when is_atom(Atom) -> + is_sensitive_key(erlang:atom_to_binary(Atom)); +is_sensitive_key(Bin) when is_binary(Bin), (size(Bin) =:= 19 orelse size(Bin) =:= 13) -> + try + %% This is wrapped in a try-catch since we don't know that Bin is a + %% valid string so string:lowercase/1 might throw an exception. + %% + %% We want to convert this to lowercase since the http header fields + %% are case insensitive, which means that a user of the Webhook bridge + %% can write this field name in many different ways. + LowercaseBin = iolist_to_binary(string:lowercase(Bin)), + case LowercaseBin of + <<"authorization">> -> true; + <<"proxy-authorization">> -> true; + _ -> false + end + catch + _:_ -> false + end; +is_sensitive_key(_) -> + false. + +%% Function that will do a deep traversal of Data and remove sensitive +%% information (i.e., passwords) +redact(Data) -> + emqx_misc:redact(Data, fun is_sensitive_key/1). From 0c4134c4231992a8915bfb2d5f054c8a1a09126f Mon Sep 17 00:00:00 2001 From: Kjell Winblad Date: Thu, 26 Jan 2023 12:30:48 +0100 Subject: [PATCH 2/3] test: add unit test case for redact function in http connector --- .../src/emqx_connector_http.erl | 30 +++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/apps/emqx_connector/src/emqx_connector_http.erl b/apps/emqx_connector/src/emqx_connector_http.erl index 40df52d45..c99067bdc 100644 --- a/apps/emqx_connector/src/emqx_connector_http.erl +++ b/apps/emqx_connector/src/emqx_connector_http.erl @@ -592,3 +592,33 @@ is_sensitive_key(_) -> %% information (i.e., passwords) redact(Data) -> emqx_misc:redact(Data, fun is_sensitive_key/1). + +-ifdef(TEST). +-include_lib("eunit/include/eunit.hrl"). + +redact_test_() -> + TestData1 = [ + {<<"content-type">>, <<"application/json">>}, + {<<"Authorization">>, <<"Basic YWxhZGRpbjpvcGVuc2VzYW1l">>} + ], + + TestData2 = #{ + headers => + [ + {[{str, <<"content-type">>}], [{str, <<"application/json">>}]}, + {[{str, <<"Authorization">>}], [{str, <<"Basic YWxhZGRpbjpvcGVuc2VzYW1l">>}]} + ] + }, + [ + ?_assert(is_sensitive_key(<<"Authorization">>)), + ?_assert(is_sensitive_key(<<"AuthoriZation">>)), + ?_assert(is_sensitive_key('AuthoriZation')), + ?_assert(is_sensitive_key(<<"PrOxy-authoRizaTion">>)), + ?_assert(is_sensitive_key('PrOxy-authoRizaTion')), + ?_assertNot(is_sensitive_key(<<"Something">>)), + ?_assertNot(is_sensitive_key(89)), + ?_assertNotEqual(TestData1, redact(TestData1)), + ?_assertNotEqual(TestData2, redact(TestData2)) + ]. + +-endif. From e7ef53558055963b28a5d55b88636ee38274437b Mon Sep 17 00:00:00 2001 From: Kjell Winblad Date: Tue, 31 Jan 2023 09:48:56 +0100 Subject: [PATCH 3/3] docs: add change log entry for webhook Authorization header leak --- changes/v5.0.16/fix-9839.en.md | 1 + changes/v5.0.16/fix-9839.zh.md | 1 + 2 files changed, 2 insertions(+) create mode 100644 changes/v5.0.16/fix-9839.en.md create mode 100644 changes/v5.0.16/fix-9839.zh.md diff --git a/changes/v5.0.16/fix-9839.en.md b/changes/v5.0.16/fix-9839.en.md new file mode 100644 index 000000000..9962b6338 --- /dev/null +++ b/changes/v5.0.16/fix-9839.en.md @@ -0,0 +1 @@ +Make sure that the content of an Authorization header that users have specified for a webhook bridge is not printed to log files. diff --git a/changes/v5.0.16/fix-9839.zh.md b/changes/v5.0.16/fix-9839.zh.md new file mode 100644 index 000000000..d9e1e0ad8 --- /dev/null +++ b/changes/v5.0.16/fix-9839.zh.md @@ -0,0 +1 @@ +确保用户为webhook-bridge指定的Authorization-HTTP-header的内容不会被打印到日志文件。