block reading snapshots outside of the data dir

lib/craft/snapshot_server.ex

@@ -94,18 +94,16 @@ defmodule Craft.SnapshotServer do
     receive do
       {:tcp, _port, filename} ->
         filename = String.trim_trailing(filename, "\n")
-        path = Path.join(data_dir, filename)
 
-        if File.exists?(path) do
-          {:ok, _bytes_sent} = :file.sendfile(path, client)
-        else
-          Logger.warning(~s|client requested non-existent file "#{filename}" from data_dir "#{data_dir}"|)
-
-          :ok = :gen_tcp.close(client)
+        case requested_file_path(data_dir, filename) do
+          {:ok, path} ->
+            {:ok, _bytes_sent} = :file.sendfile(path, client)
+            loop_receive(state)
+          error ->
+            :gen_tcp.close(client)
+            error
         end
 
-        loop_receive(state)
-
       {:tcp_closed, _} ->
         :ok
 
@@ -116,4 +114,24 @@ defmodule Craft.SnapshotServer do
       Logger.warning("client connection timed out waiting for request")
     end
   end
+
+  defp requested_file_path(data_dir, filename) do
+    case Path.safe_relative(filename, data_dir) do
+      {:ok, safe_filename} ->
+        path = Path.join(data_dir, safe_filename)
+
+        if File.exists?(path) do
+          {:ok, path}
+        else
+          Logger.warning(~s|client requested non-existent file "#{filename}" from data_dir "#{data_dir}"|)
+
+          {:error, :not_found}
+        end
+
+      :error ->
+        Logger.warning(~s|client requested unsafe path "#{filename}", refusing to serve it|)
+
+        {:error, :unsafe}
+    end
+  end
 end

test/craft/snapshot_server_test.exs

@@ -0,0 +1,67 @@
+defmodule Craft.SnapshotServerTest do
+  use ExUnit.Case, async: false
+
+  @moduletag :capture_log
+
+  alias Craft.SnapshotServer
+
+  @tag :tmp_dir
+  test "a client cannot read a file outside data_dir via path traversal", %{tmp_dir: tmp_dir} do
+    # a data_dir with nothing secret in it
+    data_dir = Path.join(tmp_dir, "data")
+    File.mkdir_p!(data_dir)
+
+    secret_path = Path.join(tmp_dir, "secret.txt")
+    secret_contents = "top-secret-#{:erlang.unique_integer([:positive])}"
+    File.write!(secret_path, secret_contents)
+
+    port = start_server(data_dir)
+
+    traversal =
+      (tmp_dir
+      |> Path.split()
+      |> Enum.reduce(secret_path, fn _, acc -> "../" <> acc end))
+
+    {:ok, sock} = :gen_tcp.connect(~c"127.0.0.1", port, [:binary, active: false, packet: :raw])
+    :ok = :gen_tcp.send(sock, traversal)
+
+    assert "" == recv_all(sock, ""), "received data from a file outside the data directory"
+  end
+
+  @tag :tmp_dir
+  test "a client can read a legitimate nested file inside data_dir", %{tmp_dir: tmp_dir} do
+    data_dir = Path.join(tmp_dir, "data")
+
+    relative_name = "snapshots/42/log/1.sst"
+    contents = "snapshot-bytes-#{:erlang.unique_integer([:positive])}"
+    file_path = Path.join(data_dir, relative_name)
+    File.mkdir_p!(Path.dirname(file_path))
+    File.write!(file_path, contents)
+
+    port = start_server(data_dir)
+
+    {:ok, sock} = :gen_tcp.connect(~c"127.0.0.1", port, [:binary, active: false, packet: :raw])
+    :ok = :gen_tcp.send(sock, relative_name)
+
+    assert contents == recv_all(sock, "")
+  end
+
+  defp start_server(data_dir) do
+    server =
+      start_supervised!(%{
+        id: SnapshotServer,
+        start: {GenServer, :start_link, [SnapshotServer, %{data_dir: data_dir, port: 0}]}
+      })
+
+    %{port: port} = GenServer.call(server, :config)
+    port
+  end
+
+  defp recv_all(sock, acc) do
+    case :gen_tcp.recv(sock, 0, 2_000) do
+      {:ok, chunk} -> recv_all(sock, acc <> chunk)
+      {:error, :closed} -> acc
+      {:error, :timeout} -> acc
+    end
+  end
+end