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-23.3.4.19/lib/tftp/src/tftp_file.erl =================================================================== --- otp-OTP-23.3.4.19.orig/lib/tftp/src/tftp_file.erl +++ otp-OTP-23.3.4.19/lib/tftp/src/tftp_file.erl @@ -1,7 +1,7 @@ %% %% %CopyrightBegin% %% -%% Copyright Ericsson AB 2005-2018. All Rights Reserved. +%% Copyright Ericsson AB 2005-2016. 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. @@ -43,10 +43,6 @@ -include_lib("kernel/include/file.hrl"). --record(initial, - {filename, - is_native_ascii}). - -record(state, {access, filename, @@ -95,8 +91,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, @@ -105,9 +101,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. %% --------------------------------------------------------- @@ -153,12 +149,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. @@ -294,45 +290,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" -> @@ -360,15 +373,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-23.3.4.19/lib/tftp/test/tftp_SUITE.erl =================================================================== --- otp-OTP-23.3.4.19.orig/lib/tftp/test/tftp_SUITE.erl +++ otp-OTP-23.3.4.19/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, @@ -157,7 +153,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", @@ -184,6 +180,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 %%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%% @@ -195,7 +258,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", @@ -329,7 +392,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)), @@ -449,6 +512,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)), @@ -724,11 +790,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. @@ -890,7 +961,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, @@ -964,7 +1035,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", @@ -999,3 +1070,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-23.3.4.19/lib/tftp/test/tftp_test_lib.hrl =================================================================== --- otp-OTP-23.3.4.19.orig/lib/tftp/test/tftp_test_lib.hrl +++ otp-OTP-23.3.4.19/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() -> Index: otp-OTP-23.3.4.19/lib/tftp/doc/src/getting_started.xml =================================================================== --- otp-OTP-23.3.4.19.orig/lib/tftp/doc/src/getting_started.xml +++ otp-OTP-23.3.4.19/lib/tftp/doc/src/getting_started.xml @@ -5,7 +5,7 @@
1997 - 2018 + 2026 Ericsson AB. All Rights Reserved. @@ -62,12 +62,13 @@

Step 1. Create a sample file to be used for the transfer:

- $ echo "Erlang/OTP 21" > file.txt + $ echo "Erlang/OTP 21" > /tmp/file.txt

Step 2. Start the TFTP server:

- 1> {ok, Pid} = tftp:start([{port, 19999}]). + 1> Callback = {callback,{"",tftp_file,[{root_dir,"/tmp"}]}}. + 2> {ok, Pid} = tftp:start([{port, 19999}, Callback]). }]]> Index: otp-OTP-23.3.4.19/lib/tftp/doc/src/introduction.xml =================================================================== --- otp-OTP-23.3.4.19.orig/lib/tftp/doc/src/introduction.xml +++ otp-OTP-23.3.4.19/lib/tftp/doc/src/introduction.xml @@ -4,7 +4,7 @@
- 19972018 + 19972026 Ericsson AB. All Rights Reserved. @@ -45,10 +45,21 @@ authentication.

The tftp application implements the following IETF standards:

- RFC 1350, The TFTP Protocol (revision 2) - RFC 2347, TFTP Option Extension - RFC 2348, TFTP Blocksize Option - RFC 2349, TFTP Timeout Interval and Transfer Size Options + + + RFC 1350, The TFTP Protocol (revision 2) + + + + RFC 2347, TFTP Option Extension + + + + RFC 2348, TFTP Blocksize Option + + + RFC 2349, TFTP Timeout Interval and Transfer Size Options +

The only feature that not is implemented is the netascii transfer mode.

@@ -59,4 +70,36 @@ programming language, concepts of OTP, and has a basic understanding of the TFTP protocol.

+ +
+ Security Considerations +

+ As stated in + (RFC 1350) + be aware that "Since TFTP includes no login or access control mechanisms, + care must be taken in the rights granted to a TFTP server process so as + not to violate the security of the server hosts file system. + TFTP is often installed with controls such that only files that have + public read access are available via TFTP and writing files via TFTP + is disallowed." +

+

+ This essentially means that any machine on the network + that can reach the TFTP server is able to read and write, + without authentication, any file on the machine that runs + the TFTP server, that the user (or group) that runs the TFTP server + (in this case the Erlang VM) is allowed to read or write. + The machine configuration has to be prepared for that. +

+ +

+ The default behavior mentioned above is in general very risky, + and as a remedy, this TFTP application's default callback + tftp_file implements an initial state option + {root_dir,Dir} that restricts the callback's file accesses + to Dir and subdirectories. It is recommended + to use that option when starting start this TFTP server. +

+ +
Index: otp-OTP-23.3.4.19/lib/tftp/doc/src/tftp.xml =================================================================== --- otp-OTP-23.3.4.19.orig/lib/tftp/doc/src/tftp.xml +++ otp-OTP-23.3.4.19/lib/tftp/doc/src/tftp.xml @@ -4,7 +4,7 @@
- 20062018 + 20062026 Ericsson AB. All Rights Reserved. @@ -132,11 +132,11 @@ mostly useful for the server as it can restrict the use of certain TFTP options or read/write access.

- {callback, {RegExp, Module, State}} + {callback, {RegExp, Module, InitialState}}

RegExp = string()

Module = atom()

-State = term()

+InitialState = term()

Registration of a callback module. When a file is to be transferred, its local filename is matched to the regular expressions of the registered callbacks. The first matching @@ -144,8 +144,34 @@ read_file/3 and write_file/3.

-

The callback module must implement the tftp behavior, see - CALLBACK FUNCTIONS.

+

The callback module must implement the tftp behavior, see + CALLBACK FUNCTIONS.

+

+ At the end of the list of callbacks there are always + the default callbacks tftp_file and tftp_binary + with the RegExp "" and InitialState + []. +

+

+ The InitialState should be an option list, and the + empty list should be accepted by any callback module. + The tftp_file callback module accepts + an InitialState = [{root_dir, Dir}] + that restrict local file operations to files in Dir + and subdirectories. All file names received in protocol + requests, relative or absolute, are regarded as + relative to this directory. +

+ +

+ The default callback module configuration allows + access to any file on any local filesystem that is + readable or writable by the user running the Erlang VM. + This can be a security vulnerability. It is therefore + recommended to explicitly configure the tftp_file + callback module to use the root_dir option. +

+ {logger, Module} @@ -297,6 +323,25 @@ port. When it receives a request for read or write, it spawns a temporary server process handling the actual transfer of the (virtual) file.

+

+ The request filename is matched against the regexps + of the registered callback modules, and the first match + selects the callback to handle the request. +

+

+ If there are no registered callback modules, + tftp_file is used, with the initial state []. +

+ +

+ The default callback module configuration allows + access to any file on any local filesystem that is + readable or writable by the user running the Erlang VM. + This can be a security vulnerability. See the + {callback,_} + option at the start of this module reference for a remedy. +

+