From ddd79ff22d7e8ba1931b6aa40996637e3b73a5ac Mon Sep 17 00:00:00 2001 From: Oneric Date: Sun, 10 Mar 2024 07:15:26 +0100 Subject: [PATCH] Proactively harden emoji pack against path traversal No new path traversal attacks are known. But given the many entrypoints and code flow complexity inside pack.ex, it unfortunately seems possible a future refactor or addition might reintroduce one. Furthermore, some old packs might still contain traversing path entries which could trigger undesireable actions on rename or delete. To ensure this can never happen, assert safety during path construction. Path.safe_relative was introduced in Elixir 1.14, but fortunately, we already require at least 1.14 anyway. --- lib/pleroma/emoji/pack.ex | 55 ++++++++++++++++++++++++-------- test/pleroma/emoji/pack_test.exs | 6 ++-- 2 files changed, 46 insertions(+), 15 deletions(-) diff --git a/lib/pleroma/emoji/pack.ex b/lib/pleroma/emoji/pack.ex index f007cde65..142208854 100644 --- a/lib/pleroma/emoji/pack.ex +++ b/lib/pleroma/emoji/pack.ex @@ -26,10 +26,32 @@ defmodule Pleroma.Emoji.Pack do alias Pleroma.Emoji.Pack alias Pleroma.Utils + # Invalid/Malicious names are supposed to be filtered out before path joining, + # but there are many entrypoints to affected functions so as the code changes + # we might accidentally let an unsanitised name slip through. + # To make sure, use the below which crash the process otherwise. + + # ALWAYS use this when constructing paths from external name! + # (name meaning it must be only a single path component) + defp path_join_name_safe(dir, name) do + if to_string(name) != Path.basename(name) or name in ["..", ".", ""] do + raise "Invalid or malicious pack name: #{name}" + else + Path.join(dir, name) + end + end + + # ALWAYS use this to join external paths + # (which are allowed to have several components) + defp path_join_safe(dir, path) do + {:ok, safe_path} = Path.safe_relative(path) + Path.join(dir, safe_path) + end + @spec create(String.t()) :: {:ok, t()} | {:error, File.posix()} | {:error, :empty_values} def create(name) do with :ok <- validate_not_empty([name]), - dir <- Path.join(emoji_path(), name), + dir <- path_join_name_safe(emoji_path(), name), :ok <- File.mkdir(dir) do save_pack(%__MODULE__{ path: dir, @@ -68,7 +90,7 @@ def show(opts) do {:ok, [binary()]} | {:error, File.posix(), binary()} | {:error, :empty_values} def delete(name) do with :ok <- validate_not_empty([name]), - pack_path <- Path.join(emoji_path(), name) do + pack_path <- path_join_name_safe(emoji_path(), name) do File.rm_rf(pack_path) end end @@ -110,7 +132,7 @@ def add_file(%Pack{} = pack, _, _, %Plug.Upload{content_type: "application/zip"} Enum.map_reduce(emojies, pack, fn item, emoji_pack -> emoji_file = %Plug.Upload{ filename: item[:filename], - path: Path.join(tmp_dir, item[:path]) + path: path_join_safe(tmp_dir, item[:path]) } {:ok, updated_pack} = @@ -200,6 +222,7 @@ def import_from_filesystem do {:ok, results} <- File.ls(emoji_path) do names = results + # items come from File.ls, thus safe |> Enum.map(&Path.join(emoji_path, &1)) |> Enum.reject(fn path -> File.dir?(path) and File.exists?(Path.join(path, "pack.json")) @@ -298,8 +321,8 @@ def update_metadata(name, data) do @spec load_pack(String.t()) :: {:ok, t()} | {:error, :file.posix()} def load_pack(name) do - name = Path.basename(name) - pack_file = Path.join([emoji_path(), name, "pack.json"]) + pack_dir = path_join_name_safe(emoji_path(), name) + pack_file = Path.join(pack_dir, "pack.json") with {:ok, _} <- File.stat(pack_file), {:ok, pack_data} <- File.read(pack_file) do @@ -423,7 +446,13 @@ defp downloadable?(pack) do end defp create_archive_and_cache(pack, hash) do - files = [~c"pack.json" | Enum.map(pack.files, fn {_, file} -> to_charlist(file) end)] + files = [ + ~c"pack.json" + | Enum.map(pack.files, fn {_, file} -> + {:ok, file} = Path.safe_relative(file) + to_charlist(file) + end) + ] {:ok, {_, result}} = :zip.zip(~c"#{pack.name}.zip", files, [:memory, cwd: to_charlist(pack.path)]) @@ -485,7 +514,7 @@ defp validate_not_empty(list) do end defp save_file(%Plug.Upload{path: upload_path}, pack, filename) do - file_path = Path.join(pack.path, filename) + file_path = path_join_safe(pack.path, filename) create_subdirs(file_path) with {:ok, _} <- File.copy(upload_path, file_path) do @@ -494,7 +523,7 @@ defp save_file(%Plug.Upload{path: upload_path}, pack, filename) do end defp save_file(file_data, pack, filename) when is_binary(file_data) do - file_path = Path.join(pack.path, filename) + file_path = path_join_safe(pack.path, filename) create_subdirs(file_path) File.write(file_path, file_data, [:binary]) end @@ -510,8 +539,8 @@ defp delete_emoji(pack, shortcode) do end defp rename_file(pack, filename, new_filename) do - old_path = Path.join(pack.path, filename) - new_path = Path.join(pack.path, new_filename) + old_path = path_join_safe(pack.path, filename) + new_path = path_join_safe(pack.path, new_filename) create_subdirs(new_path) with :ok <- File.rename(old_path, new_path) do @@ -529,7 +558,7 @@ defp create_subdirs(file_path) do defp remove_file(pack, shortcode) do with {:ok, filename} <- get_filename(pack, shortcode), - emoji <- Path.join(pack.path, filename), + emoji <- path_join_safe(pack.path, filename), :ok <- File.rm(emoji) do remove_dir_if_empty(emoji, filename) end @@ -547,7 +576,7 @@ defp remove_dir_if_empty(emoji, filename) do defp get_filename(pack, shortcode) do with %{^shortcode => filename} when is_binary(filename) <- pack.files, - file_path <- Path.join(pack.path, filename), + file_path <- path_join_safe(pack.path, filename), {:ok, _} <- File.stat(file_path) do {:ok, filename} else @@ -585,7 +614,7 @@ defp validate_downloadable(pack) do end defp copy_as(remote_pack, local_name) do - path = Path.join(emoji_path(), local_name) + path = path_join_name_safe(emoji_path(), local_name) %__MODULE__{ name: local_name, diff --git a/test/pleroma/emoji/pack_test.exs b/test/pleroma/emoji/pack_test.exs index 4d769789d..f5d2e2eef 100644 --- a/test/pleroma/emoji/pack_test.exs +++ b/test/pleroma/emoji/pack_test.exs @@ -93,7 +93,9 @@ test "add emoji file", %{pack: pack} do assert updated_pack.files_count == 1 end - test "load_pack/1 ignores path traversal in a forged pack name", %{pack: pack} do - assert {:ok, ^pack} = Pack.load_pack("../../../../../dump_pack") + test "load_pack/1 panics on path traversal in a forged pack name" do + assert_raise(RuntimeError, "Invalid or malicious pack name: ../../../../../dump_pack", fn -> + Pack.load_pack("../../../../../dump_pack") + end) end end