Merge branch 'peppe/stdlib/zip-security-flaw/OTP-13633' into maint

OTP-13633

3 files changed, 109 insertions, 23 deletions

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
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
--- /dev/null
+++ b/lib/stdlib/test/zip_SUITE_data/exploit.zip