Merge pull request #43 from mojotech/kb/security-improvements

Two Security Improvements

README.md

@@ -43,7 +43,7 @@ Briefly can also create a temporary directory:
 
 
 ```elixir
-{:ok, dir} = Briefly.create(directory: true)
+{:ok, dir} = Briefly.create(type: :directory)
 File.write!(Path.join(dir, "test.txt"), "Some Text")
 # When this process exits, the directory and file are removed
 ```
@@ -59,6 +59,7 @@ following Mix config:
 # config/config.exs
 config :briefly,
   directory: [{:system, "TMPDIR"}, {:system, "TMP"}, {:system, "TEMP"}, "/tmp"],
+  directory_mode: 0o755,
   default_prefix: "briefly",
   default_extname: ""
 ```

guides/usage.livemd

@@ -28,7 +28,7 @@ When this process exits, the file at `path` is removed.
 Briefly can also create a temporary directory:
 
 ```elixir
-{:ok, dir} = Briefly.create(directory: true)
+{:ok, dir} = Briefly.create(type: :directory)
 ```
 
 You can use [`File.stat/1`](https://hexdocs.pm/elixir/File.html#stat/1) to check the type:

lib/briefly.ex

@@ -16,13 +16,17 @@ defmodule Briefly do
   @type create_opts :: [
           {:prefix, binary},
           {:extname, binary},
+          {:type, :path | :directory | :device},
           {:directory, boolean}
         ]
 
   @doc """
   Requests a temporary file to be created with the given options.
   """
-  @spec create(create_opts) :: {:ok, binary} | {:error, Exception.t()}
+  @spec create(create_opts) ::
+          {:ok, binary}
+          | {:ok, binary, pid}
+          | {:error, Exception.t()}
   def create(opts \\ []) do
     opts
     |> Enum.into(%{})

lib/briefly/config.ex

@@ -3,6 +3,8 @@ defmodule Briefly.Config do
 
   def directory, do: get(:directory)
 
+  def directory_mode, do: get(:directory_mode)
+
   def default_prefix, do: get(:default_prefix)
 
   def default_extname, do: get(:default_extname)

lib/briefly/entry.ex

@@ -21,9 +21,14 @@ defmodule Briefly.Entry do
   def create(%{monitor_pid: pid} = options) do
     IO.warn("the :monitor_pid option is deprecated, please use Briefly.give_away/3 instead.")
 
-    with {:ok, path} <- create(Map.delete(options, :monitor_pid)) do
-      :ok = give_away(path, pid, self())
-      {:ok, path}
+    case create(Map.delete(options, :monitor_pid)) do
+      {:ok, path} ->
+        :ok = give_away(path, pid, self())
+        {:ok, path}
+
+      {:ok, path, io_pid} ->
+        :ok = give_away(path, pid, self())
+        {:ok, path, io_pid}
     end
   end
 
@@ -36,13 +41,34 @@ defmodule Briefly.Entry do
   @doc false
   def cleanup(pid) do
     case :ets.lookup(@dir_table, pid) do
-      [{pid, _tmp}] ->
+      [{^pid, tmp}] ->
+        path_entries = :ets.lookup(@path_table, pid)
+
+        secondaries =
+          Enum.reduce(
+            path_entries,
+            MapSet.new(),
+            fn path_entry = {_pid, path_value}, seen ->
+              delete_path(path_entry)
+              if path_value.original_owner != pid do
+                MapSet.put(seen, %{path_value | path: nil})
+              else
+                seen
+              end
+            end
+          )
+
+        File.rmdir(tmp)
+
+        Enum.each(secondaries, fn %{original_owner: owner_pid, root_dir: dir} ->
+          if !pid_registered?(owner_pid) do
+            File.rmdir(dir)
+          end
+        end)
+
+        :ets.delete(@path_table, pid)
         :ets.delete(@dir_table, pid)
-
-        entries = :ets.lookup(@path_table, pid)
-        Enum.each(entries, &delete_path/1)
-
-        for {_path, path} <- entries, do: path
+        for {_pid, path} <- path_entries, do: path
 
       [] ->
         []
@@ -52,25 +78,22 @@ defmodule Briefly.Entry do
   @doc false
   def give_away(path, to_pid, from_pid)
       when is_binary(path) and is_pid(to_pid) and is_pid(from_pid) do
-    with [{^from_pid, _tmp}] <- :ets.lookup(@dir_table, from_pid),
-         true <- path_owner?(from_pid, path) do
-      case :ets.lookup(@dir_table, to_pid) do
-        [{^to_pid, _tmp}] ->
-          :ets.insert(@path_table, {to_pid, path})
-          :ets.delete_object(@path_table, {from_pid, path})
-
-          :ok
-
-        [] ->
-          server = server()
+    with true <- pid_registered?(from_pid),
+         path_entry when not is_nil(path_entry) <- path_entry_if_owner(from_pid, path) do
+      if pid_registered?(to_pid) do
+        :ets.insert(@path_table, {to_pid, path_entry})
+        :ets.delete_object(@path_table, {from_pid, path_entry})
+        :ok
+      else
+        server = server()
 
-          {:ok, tmps} = GenServer.call(server, :roots)
-          {:ok, tmp} = generate_tmp_dir(tmps)
-          :ok = GenServer.call(server, {:give_away, to_pid, tmp, path})
+        {:ok, tmps} = GenServer.call(server, :roots)
+        {:ok, tmp} = generate_tmp_dir(tmps)
+        :ok = GenServer.call(server, {:give_away, to_pid, tmp, path_entry})
 
-          :ets.delete_object(@path_table, {from_pid, path})
+        :ets.delete_object(@path_table, {from_pid, path_entry})
 
-          :ok
+        :ok
       end
     else
       _ ->
@@ -99,12 +122,12 @@ defmodule Briefly.Entry do
     {:reply, {:ok, dirs}, dirs}
   end
 
-  def handle_call({:give_away, pid, tmp, path}, _from, dirs) do
+  def handle_call({:give_away, pid, tmp, path_entry}, _from, dirs) do
     # Since we are writing on behalf of another process, we need to make sure
     # the monitor and writing to the tables happen within the same operation.
     Process.monitor(pid)
     :ets.insert_new(@dir_table, {pid, tmp})
-    :ets.insert(@path_table, {pid, path})
+    :ets.insert(@path_table, {pid, path_entry})
 
     {:reply, :ok, dirs}
   end
@@ -121,6 +144,17 @@ defmodule Briefly.Entry do
   def terminate(_reason, _state) do
     folder = fn entry, :ok -> delete_path(entry) end
     :ets.foldl(folder, :ok, @path_table)
+
+    dir_folder = fn {_key, dir}, :ok ->
+      case File.rmdir(dir) do
+        :ok -> :ok
+        {:error, :eexist} -> :ok
+        {:error, :enoent} -> :ok
+        {:error, :enotdir} -> :ok
+      end
+    end
+
+    :ets.foldl(dir_folder, :ok, @dir_table)
   end
 
   ## Helpers
@@ -144,29 +178,43 @@ defmodule Briefly.Entry do
   end
 
   defp generate_tmp_dir(tmp_roots) do
-    {mega, _, _} = :os.timestamp()
-    subdir = "briefly-#{mega}"
+    subdir = path(%{prefix: "briefly-", extname: ""})
 
-    if tmp = Enum.find_value(tmp_roots, &write_tmp_dir(Path.join(&1, subdir))) do
+    if tmp = Enum.find_value(tmp_roots, &write_tmp_dir({&1, subdir})) do
       {:ok, tmp}
     else
       {:error, %Briefly.NoRootDirectoryError{tmp_dirs: tmp_roots}}
     end
   end
 
-  defp write_tmp_dir(path) do
-    case File.mkdir_p(path) do
-      :ok -> path
-      {:error, _} -> nil
+  defp write_tmp_dir({root, path}) do
+    fullpath = Path.join(root, path)
+
+    case File.mkdir_p(root) do
+      {:error, _} ->
+        nil
+
+      :ok ->
+        case File.mkdir(fullpath) do
+          {:error, _} ->
+            nil
+
+          :ok ->
+            case File.chmod(fullpath, Briefly.Config.directory_mode()) do
+              {:error, _} -> nil
+              :ok -> fullpath
+            end
+        end
     end
   end
 
-  defp open(%{directory: true} = options, tmp, attempts, _) when attempts < @max_attempts do
+  defp open(%{type: :directory} = options, tmp, attempts, _) when attempts < @max_attempts do
     path = path(options, tmp)
 
     case File.mkdir_p(path) do
       :ok ->
-        :ets.insert(@path_table, {self(), path})
+        value = %{path: path, root_dir: tmp, original_owner: self()}
+        :ets.insert(@path_table, {self(), value})
         {:ok, path}
 
       {:error, reason} when reason in [:eexist, :eacces] ->
@@ -178,12 +226,36 @@ defmodule Briefly.Entry do
     end
   end
 
+  defp open(%{type: :device} = options, tmp, attempts, _) when attempts < @max_attempts do
+    path = path(options, tmp)
+
+    case File.open(path, [:read, :write, :exclusive]) do
+      {:ok, device_pid} ->
+        value = %{path: path, root_dir: tmp, original_owner: self()}
+        :ets.insert(@path_table, {self(), value})
+        {:ok, path, device_pid}
+
+      {:error, reason} when reason in [:eexist, :eacces] ->
+        last_error = %Briefly.WriteError{code: reason, entry_type: :file, tmp_dir: tmp}
+        open(options, tmp, attempts + 1, last_error)
+
+      {:error, code} ->
+        {:error, %Briefly.WriteError{code: code, entry_type: :file, tmp_dir: tmp}}
+    end
+  end
+
+  defp open(%{directory: true} = options, tmp, attempts, last_error) do
+    new_opts = Map.put(options, :type, :directory)
+    open(new_opts, tmp, attempts, last_error)
+  end
+
   defp open(options, tmp, attempts, _) when attempts < @max_attempts do
     path = path(options, tmp)
 
     case :file.write_file(path, "", [:write, :raw, :exclusive, :binary]) do
       :ok ->
-        :ets.insert(@path_table, {self(), path})
+        value = %{path: path, root_dir: tmp, original_owner: self()}
+        :ets.insert(@path_table, {self(), value})
         {:ok, path}
 
       {:error, reason} when reason in [:eexist, :eacces] ->
@@ -199,20 +271,21 @@ defmodule Briefly.Entry do
     {:error, last_error}
   end
 
-  defp path(options, tmp) do
+  defp path(options) do
     time = :erlang.monotonic_time() |> to_string |> String.trim("-")
 
-    folder =
-      Enum.join(
-        [
-          prefix(options),
-          time,
-          random_padding()
-        ],
-        "-"
-      ) <> extname(options)
+    Enum.join(
+      [
+        prefix(options),
+        time,
+        random_padding()
+      ],
+      "-"
+    ) <> extname(options)
+  end
 
-    Path.join([tmp, folder])
+  defp path(options, tmp) do
+    Path.join([tmp, path(options)])
   end
 
   defp random_padding(length \\ 20) do
@@ -228,12 +301,19 @@ defmodule Briefly.Entry do
   defp extname(%{extname: value}), do: value
   defp extname(_), do: Briefly.Config.default_extname()
 
-  defp path_owner?(pid, path) do
+  defp pid_registered?(pid) do
+    :ets.member(@dir_table, pid)
+  end
+
+  defp path_entry_if_owner(pid, path) do
     owned_paths = :ets.lookup(@path_table, pid)
-    Enum.any?(owned_paths, fn {_pid, p} -> p == path end)
+
+    Enum.find_value(owned_paths, fn {_pid, %{path: p} = entry} ->
+      if p == path, do: entry
+    end)
   end
 
-  defp delete_path({_pid, path}) do
+  defp delete_path({_pid, %{path: path}}) do
     File.rm_rf(path)
     :ok
   end

mix.exs

@@ -56,6 +56,7 @@ defmodule Briefly.Mixfile do
   defp default_env do
     [
       directory: [{:system, "TMPDIR"}, {:system, "TMP"}, {:system, "TEMP"}, "/tmp"],
+      directory_mode: 0o755,
       default_prefix: "briefly",
       default_extname: ""
     ]