From 64457458280adab8b8a10c29d722f42365ebe91d Mon Sep 17 00:00:00 2001 From: Peter Andersson Date: Thu, 1 Sep 2016 15:03:08 +0200 Subject: Handle directory traversal attack --- lib/stdlib/src/zip.erl | 82 ++++++++++++++++++++++++++++++++++++-------------- 1 file changed, 60 insertions(+), 22 deletions(-) (limited to 'lib') diff --git a/lib/stdlib/src/zip.erl b/lib/stdlib/src/zip.erl index f8ba6f18e9..340cc21390 100644 --- a/lib/stdlib/src/zip.erl +++ b/lib/stdlib/src/zip.erl @@ -279,7 +279,8 @@ do_openzip_get(F, #openzip{files = Files, in = In0, input = Input, case file_name_search(F, Files) of {#zip_file{offset = Offset},_}=ZFile -> In1 = Input({seek, bof, Offset}, In0), - case get_z_file(In1, Z, Input, Output, [], fun silent/1, CWD, ZFile) of + case get_z_file(In1, Z, Input, Output, [], fun silent/1, + CWD, ZFile, fun all/1) of {file, R, _In2} -> {ok, R}; _ -> throw(file_not_found) end; @@ -1403,9 +1404,10 @@ get_z_files([{#zip_file{offset = Offset},_} = ZFile | Rest], Z, In0, true -> In1 = Input({seek, bof, Offset}, In0), {In2, Acc1} = - case get_z_file(In1, Z, Input, Output, OpO, FB, CWD, ZFile) of + case get_z_file(In1, Z, Input, Output, OpO, FB, + CWD, ZFile, Filter) of {file, GZD, Inx} -> {Inx, [GZD | Acc0]}; - {dir, Inx} -> {Inx, Acc0} + {_, Inx} -> {Inx, Acc0} end, get_z_files(Rest, Z, In2, Opts, Acc1); _ -> @@ -1413,7 +1415,8 @@ get_z_files([{#zip_file{offset = Offset},_} = ZFile | Rest], Z, In0, end. %% get a file from the archive, reading chunks -get_z_file(In0, Z, Input, Output, OpO, FB, CWD, {ZipFile,Extra}) -> +get_z_file(In0, Z, Input, Output, OpO, FB, + CWD, {ZipFile,Extra}, Filter) -> case Input({read, ?LOCAL_FILE_HEADER_SZ}, In0) of {eof, In1} -> {eof, In1}; @@ -1433,29 +1436,64 @@ get_z_file(In0, Z, Input, Output, OpO, FB, CWD, {ZipFile,Extra}) -> end, {BFileN, In3} = Input({read, FileNameLen + ExtraLen}, In1), {FileName, _} = get_file_name_extra(FileNameLen, ExtraLen, BFileN), - FileName1 = add_cwd(CWD, FileName), - case lists:last(FileName) of - $/ -> - %% perhaps this should always be done? - Output({ensure_dir,FileName1},[]), - {dir, In3}; - _ -> - %% FileInfo = local_file_header_to_file_info(LH) - %%{Out, In4, CRC, UncompSize} = - {Out, In4, CRC, _UncompSize} = - get_z_data(CompMethod, In3, FileName1, - CompSize, Input, Output, OpO, Z), - In5 = skip_z_data_descriptor(GPFlag, Input, In4), - %% TODO This should be fixed some day: - %% In5 = Input({set_file_info, FileName, FileInfo#file_info{size=UncompSize}}, In4), - FB(FileName), - CRC =:= CRC32 orelse throw({bad_crc, FileName}), - {file, Out, In5} + ReadAndWrite = + case check_valid_location(CWD, FileName) of + {true,FileName1} -> + true; + {false,FileName1} -> + Filter({ZipFile#zip_file{name = FileName1},Extra}) + end, + case ReadAndWrite of + true -> + case lists:last(FileName) of + $/ -> + %% perhaps this should always be done? + Output({ensure_dir,FileName1},[]), + {dir, In3}; + _ -> + %% FileInfo = local_file_header_to_file_info(LH) + %%{Out, In4, CRC, UncompSize} = + {Out, In4, CRC, _UncompSize} = + get_z_data(CompMethod, In3, FileName1, + CompSize, Input, Output, OpO, Z), + In5 = skip_z_data_descriptor(GPFlag, Input, In4), + %% TODO This should be fixed some day: + %% In5 = Input({set_file_info, FileName, + %% FileInfo#file_info{size=UncompSize}}, In4), + FB(FileName), + CRC =:= CRC32 orelse throw({bad_crc, FileName}), + {file, Out, In5} + end; + false -> + {ignore, In3} end; _ -> throw(bad_local_file_header) end. +%% make sure FileName doesn't have relative path that points over CWD +check_valid_location(CWD, FileName) -> + %% check for directory traversal exploit + 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,add_cwd(CWD, FileOrDir)}; + _ -> + {true,add_cwd(CWD, FileName)} + end. + +check_dir_level([FileOrDir], Level) -> + {FileOrDir,Level}; +check_dir_level(["." | Parts], Level) -> + check_dir_level(Parts, Level); +check_dir_level([".." | Parts], Level) -> + check_dir_level(Parts, Level-1); +check_dir_level([_Dir | Parts], Level) -> + check_dir_level(Parts, Level+1). get_file_name_extra(FileNameLen, ExtraLen, B) -> case B of -- cgit v1.2.3 From c5833df02b376d82a1abcd1da95f35cbdb60261e Mon Sep 17 00:00:00 2001 From: Peter Andersson Date: Mon, 5 Sep 2016 15:37:55 +0200 Subject: Add test case in zip_SUITE --- lib/stdlib/test/zip_SUITE.erl | 50 ++++++++++++++++++++++++++++- lib/stdlib/test/zip_SUITE_data/exploit.zip | Bin 0 -> 797 bytes 2 files changed, 49 insertions(+), 1 deletion(-) create mode 100644 lib/stdlib/test/zip_SUITE_data/exploit.zip (limited to 'lib') diff --git a/lib/stdlib/test/zip_SUITE.erl b/lib/stdlib/test/zip_SUITE.erl index 2add5a39a2..7d90795c9e 100644 --- a/lib/stdlib/test/zip_SUITE.erl +++ b/lib/stdlib/test/zip_SUITE.erl @@ -25,6 +25,7 @@ zip_to_binary/1, unzip_options/1, zip_options/1, list_dir_options/1, aliases/1, openzip_api/1, zip_api/1, open_leak/1, unzip_jar/1, + unzip_traversal_exploit/1, compress_control/1, foldl/1]). @@ -38,7 +39,8 @@ all() -> [borderline, atomic, bad_zip, unzip_from_binary, unzip_to_binary, zip_to_binary, unzip_options, zip_options, list_dir_options, aliases, openzip_api, - zip_api, open_leak, unzip_jar, compress_control, foldl]. + zip_api, open_leak, unzip_jar, compress_control, foldl, + unzip_traversal_exploit]. groups() -> []. @@ -377,6 +379,52 @@ unzip_options(Config) when is_list(Config) -> 0 = delete_files([Subdir]), ok. +%% Test that unzip handles directory traversal exploit (OTP-13633) +unzip_traversal_exploit(Config) -> + DataDir = proplists:get_value(data_dir, Config), + PrivDir = proplists:get_value(priv_dir, Config), + ZipName = filename:join(DataDir, "exploit.zip"), + + %% $ zipinfo -1 test/zip_SUITE_data/exploit.zip + %% clash.txt + %% ../clash.txt + %% ../above.txt + %% subdir/../in_root_dir.txt + + %% create a temp directory + SubDir = filename:join(PrivDir, "exploit_test"), + ok = file:make_dir(SubDir), + + ClashFile = filename:join(SubDir,"clash.txt"), + AboveFile = filename:join(SubDir,"above.txt"), + RelativePathFile = filename:join(SubDir,"subdir/../in_root_dir.txt"), + + %% unzip in SubDir + {ok, [ClashFile, ClashFile, AboveFile, RelativePathFile]} = + zip:unzip(ZipName, [{cwd,SubDir}]), + + {ok,<<"This file will overwrite other file.\n">>} = + file:read_file(ClashFile), + {ok,_} = file:read_file(AboveFile), + {ok,_} = file:read_file(RelativePathFile), + + %% clean up + delete_files([SubDir]), + + %% create the temp directory again + ok = file:make_dir(SubDir), + + %% unzip in SubDir + {ok, [ClashFile, AboveFile, RelativePathFile]} = + zip:unzip(ZipName, [{cwd,SubDir},keep_old_files]), + + {ok,<<"This is the original file.\n">>} = + file:read_file(ClashFile), + + %% clean up + delete_files([SubDir]), + ok. + %% Test unzip a jar file (OTP-7382). unzip_jar(Config) when is_list(Config) -> DataDir = proplists:get_value(data_dir, Config), diff --git a/lib/stdlib/test/zip_SUITE_data/exploit.zip b/lib/stdlib/test/zip_SUITE_data/exploit.zip new file mode 100644 index 0000000000..afb8dbd192 Binary files /dev/null and b/lib/stdlib/test/zip_SUITE_data/exploit.zip differ -- cgit v1.2.3