diff --git a/src/hackney.erl b/src/hackney.erl index 7c5e229b..1d1cd330 100644 --- a/src/hackney.erl +++ b/src/hackney.erl @@ -1477,7 +1477,14 @@ follow_redirect(ConnPid, Method, Body, WithBody, Options, CurrentURL, RespHeader end, NewBody = case NewMethod of <<"GET">> -> <<>>; - _ -> Body + _ -> + %% A 307/308 keeps the method and body. Do not forward the body to a + %% different host unless the caller set location_trusted, mirroring + %% the auth/cookie stripping in maybe_strip_auth_on_redirect/3. + case redirect_crosses_host(CurrentURL, NewURL, Options) of + true -> <<>>; + false -> Body + end end, %% Make new request to the redirect URL %% Remove old redirect_count and add incremented one @@ -1570,6 +1577,12 @@ find_last_slash(Bin, Pos) -> %% @private Strip sensitive auth options when redirecting to a different host. %% This prevents credential leakage per CVE-2018-1000007. %% Use location_trusted option to allow forwarding auth to different hosts (like curl's --location-trusted). +%% @private True when a redirect leaves the original host and the caller has not +%% opted into location_trusted, i.e. the request body must not be forwarded. +redirect_crosses_host(CurrentURL, NewURL, Options) -> + proplists:get_value(location_trusted, Options, false) =:= false + andalso CurrentURL#hackney_url.host =/= NewURL#hackney_url.host. + maybe_strip_auth_on_redirect(CurrentURL, NewURL, Options) -> LocationTrusted = proplists:get_value(location_trusted, Options, false), case LocationTrusted of diff --git a/src/hackney_h3.erl b/src/hackney_h3.erl index 75febc3e..51d11364 100644 --- a/src/hackney_h3.erl +++ b/src/hackney_h3.erl @@ -189,10 +189,16 @@ handle_redirect(Status, RespHeaders, _RespBody, Method, Url, Headers, Body, Opts NewUrl = resolve_redirect_url(Location, Url), %% Determine new method based on status code NewMethod = redirect_method(Status, Method), - %% Clear body for POST->GET redirects + %% Clear body for POST->GET redirects, and for a 307/308 that + %% crosses origin unless the caller set location_trusted (mirrors + %% the credential-header strip below). NewBody = case NewMethod of get when Method =/= get -> <<>>; - _ -> Body + _ -> + case redirect_keeps_body(Url, NewUrl, Opts) of + true -> Body; + false -> <<>> + end end, %% GHSA-h73q: do not forward credential headers to a different %% origin unless the caller opted into location_trusted. @@ -217,6 +223,12 @@ get_redirect_location(Headers) -> end end. +%% @private Keep the request body on a 307/308 only for a same-origin redirect, +%% unless the caller trusts the location. Mirrors the credential-header strip. +redirect_keeps_body(OldUrl, NewUrl, Opts) -> + maps:get(location_trusted, Opts, false) =:= true + orelse same_origin(OldUrl, NewUrl). + %% @private GHSA-h73q: strip Authorization / Cookie / Proxy-Authorization %% before following a redirect to a different origin (scheme, host or port). %% location_trusted lets a caller opt back into forwarding them. diff --git a/test/hackney_redirect_body_tests.erl b/test/hackney_redirect_body_tests.erl new file mode 100644 index 00000000..0956664c --- /dev/null +++ b/test/hackney_redirect_body_tests.erl @@ -0,0 +1,70 @@ +%%% Request body must not be forwarded to a cross-origin 307/308 redirect +%%% target unless the caller opts into location_trusted. +%%% +%%% localhost and 127.0.0.1 resolve to the same listener but are distinct host +%%% strings, which is exactly hackney's cross-host test, so one server is enough. +-module(hackney_redirect_body_tests). + +-include_lib("eunit/include/eunit.hrl"). + +-define(PORT, 9879). + +redirect_body_test_() -> + {setup, fun setup/0, fun cleanup/1, + [{"307 body dropped on cross-origin", fun cross_origin_drops_body/0}, + {"307 body kept same-origin", fun same_origin_keeps_body/0}, + {"307 body kept when location_trusted", fun trusted_keeps_body/0}]}. + +setup() -> + {ok, _} = application:ensure_all_started(hackney), + {ok, _} = application:ensure_all_started(cowboy), + Dispatch = cowboy_router:compile([{'_', [{"/[...]", test_http_resource, []}]}]), + {ok, _} = cowboy:start_clear(test_redirect_body_http, [{port, ?PORT}], + #{env => #{dispatch => Dispatch}}), + ok. + +cleanup(_) -> + cowboy:stop_listener(test_redirect_body_http), + ok. + +-define(SECRET, <<"SUPER-SECRET-VALUE-12345">>). + +cross_origin_drops_body() -> + Body = post_follow(<<"localhost">>, sink_url(<<"127.0.0.1">>), []), + ?assertEqual(nomatch, binary:match(Body, ?SECRET)). + +same_origin_keeps_body() -> + Body = post_follow(<<"localhost">>, sink_url(<<"localhost">>), []), + ?assertNotEqual(nomatch, binary:match(Body, ?SECRET)). + +trusted_keeps_body() -> + Body = post_follow(<<"localhost">>, sink_url(<<"127.0.0.1">>), [location_trusted]), + ?assertNotEqual(nomatch, binary:match(Body, ?SECRET)). + +sink_url(Host) -> + <<"http://", Host/binary, ":", (integer_to_binary(?PORT))/binary, "/post">>. + +%% POST to /redirect-to on FromHost, which 307s to SinkUrl (the /post echo). +post_follow(FromHost, SinkUrl, ExtraOpts) -> + Url = <<"http://", FromHost/binary, ":", (integer_to_binary(?PORT))/binary, + "/redirect-to?status_code=307&url=", (cow_uri(SinkUrl))/binary>>, + Opts = [{follow_redirect, true}, {max_redirect, 3}, with_body | ExtraOpts], + {ok, 200, _H, RespBody} = + hackney:request(post, Url, + [{<<"Content-Type">>, <<"text/plain">>}], ?SECRET, Opts), + RespBody. + +cow_uri(Bin) -> + list_to_binary(http_uri_encode(binary_to_list(Bin))). + +http_uri_encode(Str) -> + lists:flatten([encode_char(C) || C <- Str]). + +encode_char(C) when C >= $a, C =< $z -> C; +encode_char(C) when C >= $A, C =< $Z -> C; +encode_char(C) when C >= $0, C =< $9 -> C; +encode_char($.) -> $.; +encode_char($-) -> $-; +encode_char($/) -> "%2F"; +encode_char($:) -> "%3A"; +encode_char(C) -> io_lib:format("%~2.16.0B", [C]). diff --git a/test/test_http_resource.erl b/test/test_http_resource.erl index 8daa4c79..eae75b02 100644 --- a/test/test_http_resource.erl +++ b/test/test_http_resource.erl @@ -63,8 +63,9 @@ handle_request(<<"GET">>, <<"/status/", CodeBin/binary>>, Req, State) -> reply_json(Code, #{<<"status">> => Code}, Req, State) end; -%% GET /redirect-to?url=X&status_code=Y - redirect to URL -handle_request(<<"GET">>, <<"/redirect-to">>, Req, State) -> +%% /redirect-to?url=X&status_code=Y - redirect to URL (any method, so +%% 307/308 body-forwarding can be exercised for POST/PUT/PATCH too) +handle_request(_Method, <<"/redirect-to">>, Req, State) -> QS = cowboy_req:parse_qs(Req), Url = proplists:get_value(<<"url">>, QS, <<"/">>), StatusCode = case proplists:get_value(<<"status_code">>, QS) of