diff --git a/NEWS.md b/NEWS.md index fbff3006..057023b2 100644 --- a/NEWS.md +++ b/NEWS.md @@ -36,6 +36,10 @@ Unreleased proxy. The connection to the proxy only set the server name, so on OTP 27 and later it failed unless `proxy_ssl_options` set `cacerts` or `verify_none`. An explicit `{verify, verify_none}` is still honoured (#988). +- A `proxy_auth_fun` that returns a value containing CR, LF or NUL is now + refused with `{error, {proxy_auth_error, invalid_header}}`. The value was + written as is into the CONNECT request, so those bytes added header lines + of their own (#989). ### Changed diff --git a/src/hackney_http_connect.erl b/src/hackney_http_connect.erl index 0504554c..8674b0a4 100644 --- a/src/hackney_http_connect.erl +++ b/src/hackney_http_connect.erl @@ -218,17 +218,17 @@ do_handshake(Socket, ProxyTransport, Host, Port, Options) -> %% and the Host header. A percent-decoded host can carry raw CR/LF/NUL, which %% would split the request sent to the proxy. Reject it before building any %% payload, mirroring the WebSocket/WebTransport authority guards. - case host_has_ctl_bytes(Host) of + case has_ctl_bytes(Host) of true -> {error, invalid_connect_host}; false -> do_handshake_1(Socket, ProxyTransport, Host, Port, Options) end. -host_has_ctl_bytes(Host) when is_list(Host) -> - lists:any(fun(C) -> C =:= $\r orelse C =:= $\n orelse C =:= 0 end, Host); -host_has_ctl_bytes(Host) when is_binary(Host) -> - binary:match(Host, [<<"\r">>, <<"\n">>, <<0>>]) =/= nomatch. +has_ctl_bytes(Value) when is_list(Value) -> + lists:any(fun(C) -> C =:= $\r orelse C =:= $\n orelse C =:= 0 end, Value); +has_ctl_bytes(Value) when is_binary(Value) -> + binary:match(Value, [<<"\r">>, <<"\n">>, <<0>>]) =/= nomatch. do_handshake_1(Socket, ProxyTransport, Host, Port, Options) -> ProxyUser = proplists:get_value(connect_user, Options), @@ -304,18 +304,31 @@ handle_proxy_auth(Socket, ProxyTransport, Host, Port, Headers, %% Call the user's auth function case ProxyAuthFun(StatusCode, RespHeaders) of {ok, AuthHeader} when is_binary(AuthHeader) -> - %% Remove any existing Proxy-Authorization and add new one - Headers1 = lists:filter(fun(H) -> not is_proxy_auth_header(H) end, Headers), - Headers2 = Headers1 ++ [<<"Proxy-Authorization: ", AuthHeader/binary>>], - %% Retry with new auth header - do_handshake_with_auth(Socket, ProxyTransport, Host, Port, Headers2, - RecvTimeout, OnConnectResponse, ProxyAuthFun, Retries + 1); + %% The value is written as one header line: CR/LF/NUL in it would add + %% lines of its own to the CONNECT request. + case has_ctl_bytes(AuthHeader) of + true -> + {error, {proxy_auth_error, invalid_header}}; + false -> + retry_with_auth(Socket, ProxyTransport, Host, Port, Headers, + RecvTimeout, OnConnectResponse, ProxyAuthFun, + AuthHeader, Retries) + end; {error, Reason} -> {error, {proxy_auth_error, Reason}}; _ -> {error, proxy_auth_failed} end. +retry_with_auth(Socket, ProxyTransport, Host, Port, Headers, RecvTimeout, + OnConnectResponse, ProxyAuthFun, AuthHeader, Retries) -> + %% Remove any existing Proxy-Authorization and add new one + Headers1 = lists:filter(fun(H) -> not is_proxy_auth_header(H) end, Headers), + Headers2 = Headers1 ++ [<<"Proxy-Authorization: ", AuthHeader/binary>>], + %% Retry with new auth header + do_handshake_with_auth(Socket, ProxyTransport, Host, Port, Headers2, + RecvTimeout, OnConnectResponse, ProxyAuthFun, Retries + 1). + %% Check if a header is a Proxy-Authorization header is_proxy_auth_header(Header) when is_binary(Header) -> case binary:match(hackney_bstr:to_lower(Header), <<"proxy-authorization">>) of diff --git a/test/hackney_proxy_tests.erl b/test/hackney_proxy_tests.erl index 769028f2..397baffb 100644 --- a/test/hackney_proxy_tests.erl +++ b/test/hackney_proxy_tests.erl @@ -971,6 +971,21 @@ proxy_auth_fun_test_() -> error(auth_callback_not_called) end end}}, + {"proxy_auth_fun result with CR/LF is refused", {timeout, 10, fun() -> + %% The value is written as one header line; CR/LF in it would + %% add header lines of its own to the CONNECT request. + AuthFun = fun(407, _Headers) -> + {ok, <<"Basic abc\r\nX-Injected: 1">>} + end, + Options = [ + {proxy, {connect, "127.0.0.1", ProxyPort}}, + {recv_timeout, 5000}, + {connect_options, [{proxy_auth_fun, AuthFun}]} + ], + Url = <<"http://example.com:80/test">>, + ?assertEqual({error, {proxy_auth_error, invalid_header}}, + hackney:request(get, Url, [], <<>>, Options)) + end}}, {"proxy_auth_required error when no callback", {timeout, 10, fun() -> Options = [ {proxy, {connect, "127.0.0.1", ProxyPort}},