Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix zip:extract/2 with keep_old_files to respect cwd #9097

Merged
merged 1 commit into from
Nov 25, 2024
Merged

Fix zip:extract/2 with keep_old_files to respect cwd #9097

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 28 additions & 20 deletions lib/stdlib/src/zip.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -886,15 +886,15 @@ get_unzip_opt([memory | Rest], Opts) ->
get_unzip_opt([{cwd, CWD} | Rest], Opts) ->
get_unzip_opt(Rest, Opts#unzip_opts{cwd = CWD});
get_unzip_opt([{file_filter, F} | Rest], Opts) ->
Filter1 = fun({ZipFile,_Extra}) -> F(ZipFile) end,
Filter1 = fun({ZipFile,_Extra, _CWD}) -> F(ZipFile) end,
Filter2 = fun_and_1(Filter1, Opts#unzip_opts.file_filter),
get_unzip_opt(Rest, Opts#unzip_opts{file_filter = Filter2});
get_unzip_opt([{file_list, L} | Rest], Opts) ->
FileInList = fun(F) -> file_in_list(F, L) end,
FileInList = fun({ZipFile, _Extra, _CWD}) -> file_in_list(ZipFile, L) end,
Filter = fun_and_1(FileInList, Opts#unzip_opts.file_filter),
get_unzip_opt(Rest, Opts#unzip_opts{file_filter = Filter});
get_unzip_opt([keep_old_files | Rest], Opts) ->
Keep = fun keep_old_file/1,
Keep = fun({ZipFile, _Extra, CWD}) -> keep_old_file(ZipFile, CWD) end,
Filter = fun_and_1(Keep, Opts#unzip_opts.file_filter),
get_unzip_opt(Rest, Opts#unzip_opts{file_filter = Filter});
get_unzip_opt([skip_directories | Rest], Opts) ->
Expand Down Expand Up @@ -997,14 +997,15 @@ verbose_zip(FN) ->
%% file filter funs
all(_) -> true.

file_in_list({#zip_file{name = FileName},_}, List) ->
file_in_list(#zip_file{name = FileName}, List) ->
lists:member(FileName, List);
file_in_list(_, _) ->
false.

keep_old_file({#zip_file{name = FileName},_}) ->
not (filelib:is_file(FileName) orelse filelib:is_dir(FileName));
keep_old_file(_) ->
keep_old_file(#zip_file{name = FileName}, CWD) ->
FileName1 = add_cwd(CWD, FileName),
not (filelib:is_file(FileName1) orelse filelib:is_dir(FileName1));
Comment on lines +1005 to +1007
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main fix is here, we add CWD before making file system checks.

keep_old_file(_, _) ->
false.

%% fun combiner
Expand Down Expand Up @@ -1246,7 +1247,12 @@ get_filename(Name, directory) ->

add_cwd(_CWD, {_Name, _} = F) -> F;
add_cwd("", F) -> F;
add_cwd(CWD, F) -> filename:join(CWD, F).
add_cwd(CWD, F) ->
TrailingSlash = case lists:last(F) of
$/ -> "/";
_ -> ""
end,
string:trim(filename:join(CWD, F), trailing, "/") ++ TrailingSlash.

%% already compressed data should be stored as is in archive,
%% a simple name-match is used to check for this
Expand Down Expand Up @@ -2214,16 +2220,16 @@ get_z_files([], _Z, _In, _Opts, Acc) ->
lists:reverse(Acc);
get_z_files([#zip_comment{comment = _} | Rest], Z, In, Opts, Acc) ->
get_z_files(Rest, Z, In, Opts, Acc);
get_z_files([{#zip_file{offset = Offset},_} = ZFile | Rest], Z, In0,
get_z_files([{#zip_file{offset = Offset} = ZipFile, ZipExtra} | Rest], Z, In0,
#unzip_opts{input = Input, output = Output, open_opts = OpO,
file_filter = Filter, feedback = FB,
cwd = CWD, skip_dirs = SkipDirs, extra = ExtraOpts} = Opts, Acc0) ->
case Filter(ZFile) of
case Filter({ZipFile, ZipExtra, CWD}) of
true ->
In1 = Input({seek, bof, Offset}, In0),
{In2, Acc1} =
case get_z_file(In1, Z, Input, Output, OpO, FB,
CWD, ZFile, Filter, SkipDirs, ExtraOpts) of
CWD, {ZipFile, ZipExtra}, Filter, SkipDirs, ExtraOpts) of
{Type, GZD, Inx} when Type =:= file; Type =:= dir ->
{Inx, [GZD | Acc0]};
{_, Inx} -> {Inx, Acc0}
Expand Down Expand Up @@ -2265,21 +2271,23 @@ get_z_file(In0, Z, Input, Output, OpO, FB,
{true,FileName1} ->
true;
{false,FileName1} ->
Filter({ZipFile#zip_file{name = FileName1},ZipExtra})
Filter({ZipFile#zip_file{name = FileName1},ZipExtra, CWD})
end,

FileNameWithCwd = add_cwd(CWD, FileName1),
Copy link
Contributor Author

@jonatanklosko jonatanklosko Nov 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously check_valid_location would add CWD to the filename, but I don't think the extra Filter call above should include CWD in the zip_file name, since CWD has nothing to do with the zip_file information itself (and the first Filter call include it either).

So I changed check_valid_location to not add CWD, and we do it here separately.


IsDir = lists:last(FileName) =:= $/,

case ReadAndWrite andalso not (IsDir andalso SkipDirs) of
true ->
{Type, Out, In} =
case lists:last(FileName) of
$/ ->
Out1 = Output({ensure_path,FileName1},[]),
Out1 = Output({ensure_path,FileNameWithCwd},[]),
{dir, Out1, In3};
_ ->
{Out1, In4, CRC, _UncompSize} =
get_z_data(CompMethod, In3, FileName1,
get_z_data(CompMethod, In3, FileNameWithCwd,
CompSize, Input, Output, OpO, Z),
In5 = skip_z_data_descriptor(GPFlag, Input, In4),

Expand All @@ -2289,10 +2297,10 @@ get_z_file(In0, Z, Input, Output, OpO, FB,
end,

FileInfo = local_file_header_to_file_info(
Output({file_info, FileName1}, Out),
Output({file_info, FileNameWithCwd}, Out),
LHExtra, ZipFile),

Out2 = Output({set_file_info, FileName1, FileInfo, [{time, local}]}, Out),
Out2 = Output({set_file_info, FileNameWithCwd, FileInfo, [{time, local}]}, Out),
{Type, Out2, In};
false ->
{ignore, In3}
Expand All @@ -2317,19 +2325,19 @@ check_valid_location(CWD, FileName) ->
_ -> ""
end,
%% check for directory traversal exploit
{IsValid, Cwd, Name} =
{IsValid, Name} =
case check_dir_level(filename:split(FileName), 0) of
{FileOrDir,Level} when Level < 0 ->
CWD1 = if CWD == "" -> "./";
true -> CWD
end,
error_logger:format("Illegal path: ~ts, extracting in ~ts~n",
[add_cwd(CWD,FileName),CWD1]),
{false, CWD, FileOrDir};
{false, FileOrDir};
_ ->
{true, CWD, FileName}
{true, FileName}
end,
{IsValid, string:trim(add_cwd(Cwd, Name), trailing, "/") ++ TrailingSlash}.
{IsValid, string:trim(Name, trailing, "/") ++ TrailingSlash}.

check_dir_level([FileOrDir], Level) ->
{FileOrDir,Level};
Expand Down
30 changes: 29 additions & 1 deletion lib/stdlib/test/zip_SUITE.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -436,7 +436,7 @@ spawned_zip_dead(ZipSrv) ->
false
end.

%% Test options for unzip, only cwd and file_list currently.
%% Test options for unzip, only cwd, file_list and keep_old_files currently.
unzip_options(Config) when is_list(Config) ->
DataDir = get_value(data_dir, Config),
PrivDir = get_value(priv_dir, Config),
Expand Down Expand Up @@ -478,6 +478,34 @@ unzip_options(Config) when is_list(Config) ->

%% Clean up and verify no more files.
0 = delete_files([Subdir]),

OriginalFile1 = filename:join(Subdir, "abc.txt"),
OriginalFile2 = filename:join(Subdir, "quotes/rain.txt"),

ok = file:make_dir(filename:dirname(OriginalFile1)),
ok = file:write_file(OriginalFile1, ["Original 1"]),
ok = file:make_dir(filename:dirname(OriginalFile2)),
ok = file:write_file(OriginalFile2, ["Original 2"]),

FList3 = ["wikipedia.txt","emptyFile"],

%% Unzip a zip file in Subdir
{ok, RetList3} = zip:unzip(Long, [{cwd, Subdir},skip_directories,keep_old_files]),
{ok, []} = zip:unzip(Long, [{cwd, Subdir},skip_directories,keep_old_files]),

%% Verify.
true = (length(RetList3) =:= 2),
{ok,<<"Original 1">>} = file:read_file(OriginalFile1),
{ok,<<"Original 2">>} = file:read_file(OriginalFile2),
lists:foreach(fun(F)-> {ok,B} = file:read_file(filename:join(DataDir, F)),
{ok,B} = file:read_file(filename:join(Subdir, F)) end,
FList3),
lists:foreach(fun(F)-> 1 = delete_files([F]) end,
RetList3),

%% Clean up and verify no more files.
2 = delete_files([OriginalFile1, OriginalFile2]),
0 = delete_files([Subdir]),
ok.

%% Test that unzip handles directory traversal exploit (OTP-13633)
Expand Down
Loading