diff --git a/lib/craft/snapshot_server.ex b/lib/craft/snapshot_server.ex index 8209b86..c3e14d4 100644 --- a/lib/craft/snapshot_server.ex +++ b/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 diff --git a/test/craft/snapshot_server_test.exs b/test/craft/snapshot_server_test.exs new file mode 100644 index 0000000..27f1915 --- /dev/null +++ b/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