Refuse CR/LF/NUL in the proxy_auth_fun result

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
 

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

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}},