diff --git a/src/hackney_conn.erl b/src/hackney_conn.erl index 625fb41c..ba0b2015 100644 --- a/src/hackney_conn.erl +++ b/src/hackney_conn.erl @@ -312,7 +312,7 @@ request(Pid, Method, Path, Headers, Body, Timeout) -> -spec request(pid(), binary(), binary(), list(), binary() | iolist(), timeout(), list()) -> {ok, integer(), list()} | {ok, integer(), list(), binary()} | {error, term()}. request(Pid, Method, Path, Headers, Body, Timeout, ReqOpts) -> - case valid_request_target(Path) of + case valid_request_line(Method, Path) of ok -> safe_call(Pid, {request, Method, Path, Headers, Body, ReqOpts}, Timeout); Err -> Err end. @@ -353,13 +353,29 @@ valid_request_target(Path) when is_list(Path) -> valid_request_target(_) -> ok. +%% @private Validate both the method and the request target before they are +%% serialized into the request line. The method sits on the same line as the +%% path, so a caller-influenced method carrying CR/LF/NUL could inject headers +%% or split the request just as a bad path could. +valid_request_line(Method, Path) -> + case valid_method(Method) of + ok -> valid_request_target(Path); + Error -> Error + end. + +valid_method(Method) -> + case binary:match(hackney_bstr:to_binary(Method), [<<"\r">>, <<"\n">>, <<0>>]) of + nomatch -> ok; + _ -> {error, {invalid_method, Method}} + end. + %% @doc Send an HTTP/3 request and return headers immediately. %% Returns {ok, Status, Headers} and allows subsequent stream_body/1 calls. %% This is for pull-based body streaming over HTTP/3. -spec request_streaming(pid(), binary(), binary(), list(), binary() | iolist()) -> {ok, integer(), list()} | {error, term()}. request_streaming(Pid, Method, Path, Headers, Body) -> - case valid_request_target(Path) of + case valid_request_line(Method, Path) of ok -> safe_call(Pid, {request_streaming, Method, Path, Headers, Body}, infinity); Err -> Err end. @@ -375,7 +391,7 @@ send_request_headers(Pid, Method, Path, Headers) -> %% send_timeout is used (HTTP/2 flow-control deadline for the body chunks). -spec send_request_headers(pid(), binary(), binary(), list(), list()) -> ok | {error, term()}. send_request_headers(Pid, Method, Path, Headers, ReqOpts) -> - case valid_request_target(Path) of + case valid_request_line(Method, Path) of ok -> safe_call(Pid, {send_headers, Method, Path, Headers, ReqOpts}, infinity); Err -> Err end. @@ -402,7 +418,7 @@ start_response(Pid) -> -spec open_h2_stream(pid(), binary(), binary(), list(), pid(), map()) -> {ok, pid(), pos_integer()} | {error, term()}. open_h2_stream(Pid, Method, Path, Headers, HandlerPid, Opts) -> - case valid_request_target(Path) of + case valid_request_line(Method, Path) of ok -> safe_call(Pid, {open_h2_stream, Method, Path, Headers, HandlerPid, Opts}, infinity); Err -> Err end. @@ -446,7 +462,7 @@ request_async(Pid, Method, Path, Headers, Body, AsyncMode, StreamTo) -> -spec request_async(pid(), binary(), binary(), list(), binary() | iolist(), true | once, pid(), boolean()) -> {ok, pid()} | {error, term()}. request_async(Pid, Method, Path, Headers, Body, AsyncMode, StreamTo, FollowRedirect) -> - case valid_request_target(Path) of + case valid_request_line(Method, Path) of ok -> safe_call(Pid, {request_async, Method, Path, Headers, Body, AsyncMode, StreamTo, FollowRedirect}); Err -> Err end. @@ -454,7 +470,7 @@ request_async(Pid, Method, Path, Headers, Body, AsyncMode, StreamTo, FollowRedir -spec request_async(pid(), binary(), binary(), list(), binary() | iolist(), true | once, pid(), boolean(), list()) -> {ok, pid()} | {error, term()}. request_async(Pid, Method, Path, Headers, Body, AsyncMode, StreamTo, FollowRedirect, ReqOpts) -> - case valid_request_target(Path) of + case valid_request_line(Method, Path) of ok -> safe_call(Pid, {request_async, Method, Path, Headers, Body, AsyncMode, StreamTo, FollowRedirect, ReqOpts}); Err -> Err end. @@ -1284,10 +1300,13 @@ streaming_body(internal, {send_headers_only, Method, Path, Headers}, Data) -> undefined -> hackney_headers:store(<<"Transfer-Encoding">>, <<"chunked">>, HeadersObj); _ -> HeadersObj end, - HeadersList = hackney_headers:to_list(HeadersWithTE), RequestLine = build_request_line(Method, Path), - HeaderLines = [[Name, <<": ">>, Value, <<"\r\n">>] || {Name, Value} <- HeadersList], - HeadersData = [RequestLine, HeaderLines, <<"\r\n">>], + %% Serialize through to_iolist/1 so header values are CR/LF sanitized like + %% the buffered path; the raw to_list/1 concatenation used before let a + %% header value carry its own CRLF and inject extra lines. to_iolist/1 + %% already appends the terminating blank line. + HeaderBlock = hackney_headers:to_iolist(HeadersWithTE), + HeadersData = [RequestLine, HeaderBlock], %% Record whether the caller asked the server to close (the request line is %% built here, not via do_send_request/5), for the keepalive decision. RequestClose = hackney_keepalive:request_closes(HeadersWithTE), diff --git a/test/hackney_conn_request_injection_tests.erl b/test/hackney_conn_request_injection_tests.erl new file mode 100644 index 00000000..681a0885 --- /dev/null +++ b/test/hackney_conn_request_injection_tests.erl @@ -0,0 +1,50 @@ +%%% Request-line and streaming-header serialization must not let a caller pass +%%% CR/LF through into the wire (header injection / request splitting). +%%% +%%% - the method is validated at the entry points, like the request target +%%% - the streaming header serializer sanitizes values, like the buffered one +-module(hackney_conn_request_injection_tests). + +-include_lib("eunit/include/eunit.hrl"). + +-define(BADMETHOD, <<"GET\r\nX-Injected: yes">>). + +%% The method sits on the request line next to the path; a CR/LF in it is +%% rejected at every entry point that also validates the path. +method_rejected_test_() -> + [?_assertMatch({error, {invalid_method, _}}, + hackney_conn:request(self(), ?BADMETHOD, <<"/">>, [], <<>>, 1000, [])), + ?_assertMatch({error, {invalid_method, _}}, + hackney_conn:request_streaming(self(), ?BADMETHOD, <<"/">>, [], <<>>)), + ?_assertMatch({error, {invalid_method, _}}, + hackney_conn:send_request_headers(self(), ?BADMETHOD, <<"/">>, [])), + ?_assertMatch({error, {invalid_method, _}}, + hackney_conn:request_async(self(), ?BADMETHOD, <<"/">>, [], <<>>, once, + self(), false))]. + +%% The streaming header serializer must strip CR/LF from header values, matching +%% the buffered path. Drive a real HTTP/1.1 streaming request into a raw socket +%% and inspect the exact bytes on the wire. +streaming_header_value_sanitized_test_() -> + {timeout, 20, fun streaming_header_value_sanitized/0}. + +streaming_header_value_sanitized() -> + {ok, _} = application:ensure_all_started(hackney), + {ok, LSock} = gen_tcp:listen(0, [binary, {active, false}, + {reuseaddr, true}, {ip, {127, 0, 0, 1}}]), + {ok, Port} = inet:port(LSock), + {ok, Pid} = hackney_conn:start_link(#{host => "127.0.0.1", port => Port, + transport => hackney_tcp, + connect_timeout => 1000}), + ok = hackney_conn:connect(Pid, 1000), + {ok, SSock} = gen_tcp:accept(LSock, 1000), + ok = hackney_conn:send_request_headers( + Pid, <<"GET">>, <<"/">>, + [{<<"X-Custom">>, <<"benign\r\nX-Injected: yes">>}]), + {ok, Wire} = gen_tcp:recv(SSock, 0, 1000), + catch hackney_conn:stop(Pid), + gen_tcp:close(SSock), + gen_tcp:close(LSock), + %% No injected header line, and the CR/LF collapsed within the value. + ?assertEqual(nomatch, binary:match(Wire, <<"\r\nX-Injected: yes">>)), + ?assertNotEqual(nomatch, binary:match(Wire, <<"X-Custom: benignX-Injected: yes\r\n">>)).