From 5b15656942ac27b1d706e3297f946e34810ba7e8 Mon Sep 17 00:00:00 2001 From: Raimo Niskanen Date: Tue, 10 Feb 2026 18:13:21 +0100 Subject: [PATCH 1/7] Validate initial options Ensure that relative path components does not allow a requested file name to go outside the configured root_dir. root_dir should be checked to be a directory and absolute. If root_dir is used, Filename should be checked to be relative under root_dir. --- lib/tftp/src/tftp_file.erl | 87 ++++++++++++++++++++++---------------- 1 file changed, 50 insertions(+), 37 deletions(-) Index: otp-OTP-27.1.3/lib/tftp/src/tftp_file.erl =================================================================== --- otp-OTP-27.1.3.orig/lib/tftp/src/tftp_file.erl +++ otp-OTP-27.1.3/lib/tftp/src/tftp_file.erl @@ -44,10 +44,6 @@ -include_lib("kernel/include/file.hrl"). --record(initial, - {filename, - is_native_ascii}). - -record(state, {access, filename, @@ -96,8 +92,8 @@ prepare(_Peer, Access, Filename, Mode, SuggestedOptions, Initial) when is_list(Initial) -> %% Client side - case catch handle_options(Access, Filename, Mode, SuggestedOptions, Initial) of - {ok, Filename2, IsNativeAscii, IsNetworkAscii, AcceptedOptions} -> + try handle_options(Access, Filename, Mode, SuggestedOptions, Initial) of + {Filename2, IsNativeAscii, IsNetworkAscii, AcceptedOptions} -> State = #state{access = Access, filename = Filename2, is_native_ascii = IsNativeAscii, @@ -106,9 +102,9 @@ prepare(_Peer, Access, Filename, Mode, S blksize = lookup_blksize(AcceptedOptions), count = 0, buffer = []}, - {ok, AcceptedOptions, State}; - {error, {Code, Text}} -> - {error, {Code, Text}} + {ok, AcceptedOptions, State} + catch throw : Error -> + {error, Error} end. %% --------------------------------------------------------- @@ -154,12 +150,12 @@ open(Peer, Access, Filename, Mode, Sugge end; open(_Peer, Access, Filename, Mode, NegotiatedOptions, State) when is_record(State, state) -> %% Both sides - case catch handle_options(Access, Filename, Mode, NegotiatedOptions, State) of - {ok, _Filename2, _IsNativeAscii, _IsNetworkAscii, Options} - when Options =:= NegotiatedOptions -> - do_open(State); - {error, {Code, Text}} -> - {error, {Code, Text}} + try handle_options(Access, Filename, Mode, NegotiatedOptions, State) of + {_Filename2, _IsNativeAscii, _IsNetworkAscii, Options} + when Options =:= NegotiatedOptions -> + do_open(State) + catch throw : Error -> + {error, Error} end; open(Peer, Access, Filename, Mode, NegotiatedOptions, State) -> %% Handle upgrade from old releases. Please, remove this clause in next release. @@ -295,45 +291,62 @@ abort(_Code, _Text, #state{fd = Fd, acce %%------------------------------------------------------------------- handle_options(Access, Filename, Mode, Options, Initial) -> - I = #initial{filename = Filename, is_native_ascii = is_native_ascii()}, - {Filename2, IsNativeAscii} = handle_initial(Initial, I), - IsNetworkAscii = handle_mode(Mode, IsNativeAscii), + {Filename2, IsNativeAscii} = handle_initial(Initial, Filename), + IsNetworkAscii = + case Mode of + "netascii" when IsNativeAscii =:= true -> + true; + "octet" -> + false; + _ -> + throw({badop, "Illegal mode " ++ Mode}) + end, Options2 = do_handle_options(Access, Filename2, Options), - {ok, Filename2, IsNativeAscii, IsNetworkAscii, Options2}. + {Filename2, IsNativeAscii, IsNetworkAscii, Options2}. -handle_mode(Mode, IsNativeAscii) -> - case Mode of - "netascii" when IsNativeAscii =:= true -> true; - "octet" -> false; - _ -> throw({error, {badop, "Illegal mode " ++ Mode}}) +handle_initial( + #state{filename = Filename, is_native_ascii = IsNativeAscii}, _FName) -> + {Filename, IsNativeAscii}; +handle_initial(Initial, Filename) when is_list(Initial) -> + Opts = get_initial_opts(Initial, #{}), + {case Opts of + #{ root_dir := RootDir } -> + safe_filename(Filename, RootDir); + #{} -> + Filename + end, + maps:get(is_native_ascii, Opts, is_native_ascii())}. + +get_initial_opts([], Opts) -> Opts; +get_initial_opts([Opt | Initial], Opts) -> + case Opt of + {root_dir, RootDir} -> + is_map_key(root_dir, Opts) andalso + throw({badop, "Internal error. root_dir already set"}), + get_initial_opts(Initial, Opts#{ root_dir => RootDir }); + {native_ascii, Bool} when is_boolean(Bool) -> + get_initial_opts(Initial, Opts#{ is_native_ascii => Bool }) end. -handle_initial([{root_dir, Dir} | Initial], I) -> - case catch filename_join(Dir, I#initial.filename) of - {'EXIT', _} -> - throw({error, {badop, "Internal error. root_dir is not a string"}}); - Filename2 -> - handle_initial(Initial, I#initial{filename = Filename2}) - end; -handle_initial([{native_ascii, Bool} | Initial], I) -> - case Bool of - true -> handle_initial(Initial, I#initial{is_native_ascii = true}); - false -> handle_initial(Initial, I#initial{is_native_ascii = false}) - end; -handle_initial([], I) when is_record(I, initial) -> - {I#initial.filename, I#initial.is_native_ascii}; -handle_initial(State, _) when is_record(State, state) -> - {State#state.filename, State#state.is_native_ascii}. - -filename_join(Dir, Filename) -> - case filename:pathtype(Filename) of - absolute -> - [_ | RelFilename] = filename:split(Filename), - filename:join([Dir, RelFilename]); - _ -> - filename:join([Dir, Filename]) +safe_filename(Filename, RootDir) -> + absolute =:= filename:pathtype(RootDir) orelse + throw({badop, "Internal error. root_dir is not absolute"}), + filelib:is_dir(RootDir) orelse + throw({badop, "Internal error. root_dir not a directory"}), + RelFilename = + case filename:pathtype(Filename) of + absolute -> + filename:join(tl(filename:split(Filename))); + _ -> Filename + end, + case filelib:safe_relative_path(RelFilename, RootDir) of + unsafe -> + throw({badop, "Internal error. Filename out of bounds"}); + SafeFilename -> + filename:join(RootDir, SafeFilename) end. + do_handle_options(Access, Filename, [{Key, Val} | T]) -> case Key of "tsize" -> @@ -361,15 +374,15 @@ do_handle_options(_Access, _Filename, [] handle_integer(Access, Filename, Key, Val, Options, Min, Max) -> - case catch list_to_integer(Val) of - {'EXIT', _} -> - do_handle_options(Access, Filename, Options); + try list_to_integer(Val) of Int when Int >= Min, Int =< Max -> [{Key, Val} | do_handle_options(Access, Filename, Options)]; Int when Int >= Min, Max =:= infinity -> [{Key, Val} | do_handle_options(Access, Filename, Options)]; _Int -> - throw({error, {badopt, "Illegal " ++ Key ++ " value " ++ Val}}) + throw({badopt, "Illegal " ++ Key ++ " value " ++ Val}) + catch error : _ -> + do_handle_options(Access, Filename, Options) end. lookup_blksize(Options) -> Index: otp-OTP-27.1.3/lib/tftp/test/tftp_SUITE.erl =================================================================== --- otp-OTP-27.1.3.orig/lib/tftp/test/tftp_SUITE.erl +++ otp-OTP-27.1.3/lib/tftp/test/tftp_SUITE.erl @@ -20,7 +20,7 @@ -module(tftp_SUITE). --compile(export_all). +-compile([export_all, nowarn_export_all]). %%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%% %% Includes and defines @@ -29,18 +29,13 @@ -include_lib("common_test/include/ct.hrl"). -include("tftp_test_lib.hrl"). --define(START_DAEMON(Port, Options), +-define(START_DAEMON(Options), begin - {ok, Pid} = ?VERIFY({ok, _Pid}, tftp:start([{port, Port} | Options])), - if - Port == 0 -> - {ok, ActualOptions} = ?IGNORE(tftp:info(Pid)), - {value, {port, ActualPort}} = - lists:keysearch(port, 1, ActualOptions), - {ActualPort, Pid}; - true -> - {Port, Pid} - end + {ok, Pid} = ?VERIFY({ok, _Pid}, tftp:start([{port, 0} | Options])), + {ok, ActualOptions} = ?IGNORE(tftp:info(Pid)), + {value, {port, ActualPort}} = + lists:keysearch(port, 1, ActualOptions), + {ActualPort, Pid} end). %%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%% @@ -78,6 +73,7 @@ suite() -> [{ct_hooks,[ts_install_cth]}] all() -> [ simple, + root_dir, extra, reuse_connection, resend_client, @@ -126,7 +122,7 @@ simple(suite) -> simple(Config) when is_list(Config) -> ?VERIFY(ok, application:start(tftp)), - {Port, DaemonPid} = ?IGNORE(?START_DAEMON(0, [{debug, brief}])), + {Port, DaemonPid} = ?IGNORE(?START_DAEMON([{debug, brief}])), %% Read fail RemoteFilename = "tftp_temporary_remote_test_file.txt", @@ -153,6 +149,73 @@ simple(Config) when is_list(Config) -> ok. %%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%% +%% root_dir +%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%% + +root_dir(doc) -> + ["Start the daemon and check the root_dir option."]; +root_dir(suite) -> + []; +root_dir(Config) when is_list(Config) -> + ?VERIFY(ok, application:start(tftp)), + PrivDir = get_conf(priv_dir, Config), + Root = hd(filename:split(PrivDir)), + Up = "..", + Remote = "remote.txt", + Local = "tftp_temporary_local_test_file.txt", + SideDir = fn_jn(PrivDir,tftp_side), + RootDir = fn_jn(PrivDir,tftp_root), + ?IGNORE(file:del_dir_r(RootDir)), + ?IGNORE(file:del_dir_r(SideDir)), + ok = filelib:ensure_path(fn_jn(RootDir,sub)), + ok = filelib:ensure_path(SideDir), + Blob = binary:copy(<<$1>>, 2000), + Size = byte_size(Blob), + ok = file:write_file(fn_jn(SideDir,Remote), Blob), + {Port, DaemonPid} = + ?IGNORE(?START_DAEMON([{debug, brief}, + {callback, + {"", tftp_file, [{root_dir, RootDir}]}}])), + try + %% Outside root_dir + ?VERIFY({error, {client_open, badop, _}}, + tftp:read_file( + fn_jn([Up,tftp_side,Remote]), binary, [{port, Port}])), + ?VERIFY({error, {client_open, badop, _}}, + tftp:write_file( + fn_jn([Up,tftp_side,Remote]), Blob, [{port, Port}])), + %% Nonexistent + ?VERIFY({error, {client_open, enoent, _}}, + tftp:read_file( + fn_jn(sub,Remote), binary, [{port, Port}])), + ?VERIFY({error, {client_open, enoent, _}}, + tftp:write_file( + fn_jn(nonexistent,Remote), Blob, [{port, Port}])), + %% Write and read + ?VERIFY({ok, Size}, + tftp:write_file( + fn_jn(sub,Remote), Blob, [{port, Port}])), + ?VERIFY({ok, Blob}, + tftp:read_file( + fn_jn([Root,sub,Remote]), binary, [{port, Port}])), + ?VERIFY({ok, Size}, + tftp:read_file( + fn_jn(sub,Remote), Local, [{port, Port}])), + ?VERIFY({ok, Blob}, file:read_file(Local)), + ?VERIFY(ok, file:delete(Local)), + ?VERIFY(ok, application:stop(tftp)) + after + %% Cleanup + unlink(DaemonPid), + exit(DaemonPid, kill), + ?IGNORE(file:del_dir_r(SideDir)), + ?IGNORE(file:del_dir_r(RootDir)), + ?IGNORE(application:stop(tftp)) + end, + ok. + + +%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%% %% Extra %%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%% @@ -164,7 +227,7 @@ extra(Config) when is_list(Config) -> ?VERIFY({'EXIT', {badarg,{fake_key, fake_flag}}}, tftp:start([{port, 0}, {fake_key, fake_flag}])), - {Port, DaemonPid} = ?IGNORE(?START_DAEMON(0, [{debug, brief}])), + {Port, DaemonPid} = ?IGNORE(?START_DAEMON([{debug, brief}])), RemoteFilename = "tftp_extra_temporary_remote_test_file.txt", LocalFilename = "tftp_extra_temporary_local_test_file.txt", @@ -298,7 +361,7 @@ resend_client(suite) -> []; resend_client(Config) when is_list(Config) -> Host = {127, 0, 0, 1}, - {Port, DaemonPid} = ?IGNORE(?START_DAEMON(0, [{debug, all}])), + {Port, DaemonPid} = ?IGNORE(?START_DAEMON([{debug, all}])), ?VERIFY(ok, resend_read_client(Host, Port, 10)), ?VERIFY(ok, resend_read_client(Host, Port, 512)), @@ -418,6 +481,9 @@ resend_read_client(Host, Port, BlkSize) Ack5Bin = <<0, 4, 0, 5>>, ?VERIFY(ok, gen_udp:send(Socket, Host, NewPort, Ack5Bin)), + %% Recv ACK #6 + ?VERIFY({udp, Socket, Host, NewPort, <<0,3,0,6>>}, recv(Timeout)), + %% Close socket ?VERIFY(ok, gen_udp:close(Socket)), @@ -693,11 +759,16 @@ resend_read_server(Host, BlkSize) -> Data6Bin = list_to_binary([0, 3, 0, 6 | Block6]), ?VERIFY(ok, gen_udp:send(ServerSocket, Host, ClientPort, Data6Bin)), + %% Recv ACK #6 + Ack6Bin = <<0, 4, 0, 6>>, + ?VERIFY({udp, ServerSocket, Host, ClientPort, Ack6Bin}, recv(Timeout)), + %% Close daemon and server sockets ?VERIFY(ok, gen_udp:close(ServerSocket)), ?VERIFY(ok, gen_udp:close(DaemonSocket)), - ?VERIFY({ClientPid, {tftp_client_reply, {ok, Blob}}}, recv(Timeout)), + ?VERIFY({ClientPid, {tftp_client_reply, {ok, Blob}}}, + recv(2 * (Timeout + timer:seconds(1)))), ?VERIFY(timeout, recv(Timeout)), ok. @@ -859,7 +930,7 @@ reuse_connection(suite) -> []; reuse_connection(Config) when is_list(Config) -> Host = {127, 0, 0, 1}, - {Port, DaemonPid} = ?IGNORE(?START_DAEMON(0, [{debug, all}])), + {Port, DaemonPid} = ?IGNORE(?START_DAEMON([{debug, all}])), RemoteFilename = "reuse_connection.tmp", BlkSize = 512, @@ -933,7 +1004,7 @@ large_file(suite) -> large_file(Config) when is_list(Config) -> ?VERIFY(ok, application:start(tftp)), - {Port, DaemonPid} = ?IGNORE(?START_DAEMON(0, [{debug, brief}])), + {Port, DaemonPid} = ?IGNORE(?START_DAEMON([{debug, brief}])), %% Read fail RemoteFilename = "tftp_temporary_large_file_remote_test_file.txt", @@ -968,3 +1039,15 @@ recv(Timeout) -> after Timeout -> timeout end. + +get_conf(Key, Config) -> + Default = make_ref(), + case proplists:get_value(Key, Config, Default) of + Default -> + erlang:error({no_key, Key}); + Value -> + Value + end. + +fn_jn(A, B) -> filename:join(A, B). +fn_jn(P) -> filename:join(P). Index: otp-OTP-27.1.3/lib/tftp/test/tftp_test_lib.hrl =================================================================== --- otp-OTP-27.1.3.orig/lib/tftp/test/tftp_test_lib.hrl +++ otp-OTP-27.1.3/lib/tftp/test/tftp_test_lib.hrl @@ -1,7 +1,7 @@ %% %% %CopyrightBegin% %% -%% Copyright Ericsson AB 2007-2018. All Rights Reserved. +%% Copyright Ericsson AB 2007-2026. All Rights Reserved. %% %% Licensed under the Apache License, Version 2.0 (the "License"); %% you may not use this file except in compliance with the License. @@ -24,7 +24,8 @@ tftp_test_lib:log(Format, Args, ?MODULE, ?LINE)). -define(ERROR(Reason), - tftp_test_lib:error(Reason, ?MODULE, ?LINE)). + erlang:error({?MODULE,?LINE,?FUNCTION_NAME,(Reason)})). + %% tftp_test_lib:error(Reason, ?MODULE, ?LINE)). -define(VERIFY(Expected, Expr), fun() ->