From 506a1c98e716754455387be9ace3ad7aec9c47a3 Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Mon, 29 May 2023 13:55:48 -0400 Subject: [PATCH 1/7] ConnCase: Make sure the host we use in tests is the actual Endpoint host --- test/support/conn_case.ex | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/test/support/conn_case.ex b/test/support/conn_case.ex index f010fec33..d6729ab87 100644 --- a/test/support/conn_case.ex +++ b/test/support/conn_case.ex @@ -120,6 +120,10 @@ defmodule Pleroma.Web.ConnCase do Mox.verify_on_exit!() - {:ok, conn: Phoenix.ConnTest.build_conn()} + {:ok, + conn: + Phoenix.ConnTest.build_conn() + |> Map.put(:host, Pleroma.Web.Endpoint.host()) + |> Plug.Test.init_test_session(%{})} end end From 843fcca5b4d022e4c088d4a60839b4a286500148 Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Mon, 29 May 2023 13:59:51 -0400 Subject: [PATCH 2/7] Validate Host header matches expected value before allowing access to MediaProxy --- .../web/media_proxy/media_proxy_controller.ex | 12 ++++++++++++ .../media_proxy/media_proxy_controller_test.exs | 17 +++++++++++++++++ 2 files changed, 29 insertions(+) diff --git a/lib/pleroma/web/media_proxy/media_proxy_controller.ex b/lib/pleroma/web/media_proxy/media_proxy_controller.ex index bda5b36ed..767496b68 100644 --- a/lib/pleroma/web/media_proxy/media_proxy_controller.ex +++ b/lib/pleroma/web/media_proxy/media_proxy_controller.ex @@ -12,6 +12,7 @@ defmodule Pleroma.Web.MediaProxy.MediaProxyController do alias Pleroma.Web.MediaProxy alias Plug.Conn + plug(:validate_host) plug(:sandbox) def remote(conn, %{"sig" => sig64, "url" => url64}) do @@ -205,6 +206,17 @@ defmodule Pleroma.Web.MediaProxy.MediaProxyController do Config.get([:media_proxy, :proxy_opts], []) end + defp validate_host(conn, _params) do + proxy_host = MediaProxy.base_url() |> URI.parse() |> Map.get(:host) + + if match?(^proxy_host, conn.host) do + conn + else + send_resp(conn, 400, Conn.Status.reason_phrase(400)) + |> halt() + end + end + defp sandbox(conn, _params) do conn |> merge_resp_headers([{"content-security-policy", "sandbox;"}]) diff --git a/test/pleroma/web/media_proxy/media_proxy_controller_test.exs b/test/pleroma/web/media_proxy/media_proxy_controller_test.exs index 9ce092fd8..3971330d4 100644 --- a/test/pleroma/web/media_proxy/media_proxy_controller_test.exs +++ b/test/pleroma/web/media_proxy/media_proxy_controller_test.exs @@ -54,6 +54,23 @@ defmodule Pleroma.Web.MediaProxy.MediaProxyControllerTest do } = get(conn, "/proxy/hhgfh/eeee/fff") end + test "it returns a 400 for invalid host", %{conn: conn} do + clear_config([:media_proxy, :base_url], "http://mp.localhost/") + + url = + MediaProxy.encode_url("https://pleroma.social/logo.jpeg") + |> URI.parse() + |> Map.put(:host, "wronghost") + |> URI.to_string() + + with_mock Pleroma.ReverseProxy, + call: fn _conn, _url, _opts -> %Conn{status: :success} end do + %{status: status} = get(conn, url) + + assert status == 400 + end + end + test "redirects to valid url when filename is invalidated", %{conn: conn, url: url} do invalid_url = String.replace(url, "test.png", "test-file.png") response = get(conn, invalid_url) From a60dd0d92dae2471025827bc57d3cf3194003110 Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Mon, 29 May 2023 14:16:03 -0400 Subject: [PATCH 3/7] Validate Host header matches expected value before allowing access to Uploads --- lib/pleroma/web/plugs/uploaded_media.ex | 11 ++++++++++- .../pleroma/web/plugs/uploaded_media_plug_test.exs | 14 ++++++++++++++ 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/lib/pleroma/web/plugs/uploaded_media.ex b/lib/pleroma/web/plugs/uploaded_media.ex index 8b3bc9acb..49db9808f 100644 --- a/lib/pleroma/web/plugs/uploaded_media.ex +++ b/lib/pleroma/web/plugs/uploaded_media.ex @@ -46,12 +46,21 @@ defmodule Pleroma.Web.Plugs.UploadedMedia do config = Pleroma.Config.get(Pleroma.Upload) - with uploader <- Keyword.fetch!(config, :uploader), + media_host = Pleroma.Upload.base_url() |> URI.parse() |> Map.get(:host) + + with {:valid_host, true} <- {:valid_host, match?(^media_host, conn.host)}, + uploader <- Keyword.fetch!(config, :uploader), proxy_remote = Keyword.get(config, :proxy_remote, false), {:ok, get_method} <- uploader.get_file(file), false <- media_is_banned(conn, get_method) do get_media(conn, get_method, proxy_remote, opts) else + {:valid_host, false} -> + conn + + send_resp(conn, 400, Plug.Conn.Status.reason_phrase(400)) + |> halt() + _ -> conn |> send_resp(:internal_server_error, dgettext("errors", "Failed")) diff --git a/test/pleroma/web/plugs/uploaded_media_plug_test.exs b/test/pleroma/web/plugs/uploaded_media_plug_test.exs index 8323ff6ab..262c8c288 100644 --- a/test/pleroma/web/plugs/uploaded_media_plug_test.exs +++ b/test/pleroma/web/plugs/uploaded_media_plug_test.exs @@ -40,4 +40,18 @@ defmodule Pleroma.Web.Plugs.UploadedMediaPlugTest do &(&1 == {"content-disposition", ~s[inline; filename="\\"cofe\\".gif"]}) ) end + + test "denies access to media if wrong Host", %{ + attachment_url: attachment_url + } do + conn = get(build_conn(), attachment_url) + + assert conn.status == 200 + + clear_config([Pleroma.Upload, :base_url], "http://media.localhost/") + + conn = get(build_conn(), attachment_url) + + assert conn.status == 400 + end end From 84974efe4c6e26f20bc14fece6cc74efe8777547 Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Mon, 29 May 2023 14:17:27 -0400 Subject: [PATCH 4/7] Host header validation is now required for MediaProxy and Uploads --- changelog.d/3896.add | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/3896.add diff --git a/changelog.d/3896.add b/changelog.d/3896.add new file mode 100644 index 000000000..3124e07dd --- /dev/null +++ b/changelog.d/3896.add @@ -0,0 +1 @@ +Validate Host header for MediaProxy and Uploads From 43bb2f39db0bd7028512cba402f6b5aa1610c1db Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Mon, 29 May 2023 15:05:37 -0400 Subject: [PATCH 5/7] Remove unwanted parameter --- test/support/conn_case.ex | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/test/support/conn_case.ex b/test/support/conn_case.ex index d6729ab87..c1cb0295b 100644 --- a/test/support/conn_case.ex +++ b/test/support/conn_case.ex @@ -123,7 +123,6 @@ defmodule Pleroma.Web.ConnCase do {:ok, conn: Phoenix.ConnTest.build_conn() - |> Map.put(:host, Pleroma.Web.Endpoint.host()) - |> Plug.Test.init_test_session(%{})} + |> Map.put(:host, Pleroma.Web.Endpoint.host())} end end From da7394f33b4402325b220dcd2957945464517f8b Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Mon, 29 May 2023 15:09:31 -0400 Subject: [PATCH 6/7] Fix unused assignment --- lib/pleroma/web/plugs/uploaded_media.ex | 2 -- 1 file changed, 2 deletions(-) diff --git a/lib/pleroma/web/plugs/uploaded_media.ex b/lib/pleroma/web/plugs/uploaded_media.ex index 49db9808f..f97c64cee 100644 --- a/lib/pleroma/web/plugs/uploaded_media.ex +++ b/lib/pleroma/web/plugs/uploaded_media.ex @@ -56,8 +56,6 @@ defmodule Pleroma.Web.Plugs.UploadedMedia do get_media(conn, get_method, proxy_remote, opts) else {:valid_host, false} -> - conn - send_resp(conn, 400, Plug.Conn.Status.reason_phrase(400)) |> halt() From b3c3bd99c390a4e5081d411011688e38285547b0 Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Tue, 30 May 2023 16:56:09 -0400 Subject: [PATCH 7/7] Switch from serving a 400 to a 302 --- changelog.d/3896.add | 2 +- .../web/media_proxy/media_proxy_controller.ex | 17 ++++++++++-- lib/pleroma/web/plugs/uploaded_media.ex | 17 ++++++++++-- .../media_proxy_controller_test.exs | 27 +++++++++++++++---- .../web/plugs/uploaded_media_plug_test.exs | 21 +++++++++++++-- 5 files changed, 72 insertions(+), 12 deletions(-) diff --git a/changelog.d/3896.add b/changelog.d/3896.add index 3124e07dd..36d8286ff 100644 --- a/changelog.d/3896.add +++ b/changelog.d/3896.add @@ -1 +1 @@ -Validate Host header for MediaProxy and Uploads +Validate Host header for MediaProxy and Uploads and return a 302 if the base_url has changed diff --git a/lib/pleroma/web/media_proxy/media_proxy_controller.ex b/lib/pleroma/web/media_proxy/media_proxy_controller.ex index 767496b68..20f3a3438 100644 --- a/lib/pleroma/web/media_proxy/media_proxy_controller.ex +++ b/lib/pleroma/web/media_proxy/media_proxy_controller.ex @@ -207,12 +207,25 @@ defmodule Pleroma.Web.MediaProxy.MediaProxyController do end defp validate_host(conn, _params) do - proxy_host = MediaProxy.base_url() |> URI.parse() |> Map.get(:host) + %{scheme: proxy_scheme, host: proxy_host, port: proxy_port} = + MediaProxy.base_url() |> URI.parse() if match?(^proxy_host, conn.host) do conn else - send_resp(conn, 400, Conn.Status.reason_phrase(400)) + redirect_url = + %URI{ + scheme: proxy_scheme, + host: proxy_host, + port: proxy_port, + path: conn.request_path, + query: conn.query_string + } + |> URI.to_string() + |> String.trim_trailing("?") + + conn + |> Phoenix.Controller.redirect(external: redirect_url) |> halt() end end diff --git a/lib/pleroma/web/plugs/uploaded_media.ex b/lib/pleroma/web/plugs/uploaded_media.ex index f97c64cee..9dd5eb239 100644 --- a/lib/pleroma/web/plugs/uploaded_media.ex +++ b/lib/pleroma/web/plugs/uploaded_media.ex @@ -46,7 +46,8 @@ defmodule Pleroma.Web.Plugs.UploadedMedia do config = Pleroma.Config.get(Pleroma.Upload) - media_host = Pleroma.Upload.base_url() |> URI.parse() |> Map.get(:host) + %{scheme: media_scheme, host: media_host, port: media_port} = + Pleroma.Upload.base_url() |> URI.parse() with {:valid_host, true} <- {:valid_host, match?(^media_host, conn.host)}, uploader <- Keyword.fetch!(config, :uploader), @@ -56,7 +57,19 @@ defmodule Pleroma.Web.Plugs.UploadedMedia do get_media(conn, get_method, proxy_remote, opts) else {:valid_host, false} -> - send_resp(conn, 400, Plug.Conn.Status.reason_phrase(400)) + redirect_url = + %URI{ + scheme: media_scheme, + host: media_host, + port: media_port, + path: conn.request_path, + query: conn.query_string + } + |> URI.to_string() + |> String.trim_trailing("?") + + conn + |> Phoenix.Controller.redirect(external: redirect_url) |> halt() _ -> diff --git a/test/pleroma/web/media_proxy/media_proxy_controller_test.exs b/test/pleroma/web/media_proxy/media_proxy_controller_test.exs index 3971330d4..019e389b7 100644 --- a/test/pleroma/web/media_proxy/media_proxy_controller_test.exs +++ b/test/pleroma/web/media_proxy/media_proxy_controller_test.exs @@ -54,20 +54,37 @@ defmodule Pleroma.Web.MediaProxy.MediaProxyControllerTest do } = get(conn, "/proxy/hhgfh/eeee/fff") end - test "it returns a 400 for invalid host", %{conn: conn} do - clear_config([:media_proxy, :base_url], "http://mp.localhost/") + test "it returns a 302 for invalid host", %{conn: conn} do + new_proxy_base = "http://mp.localhost/" - url = + %{scheme: new_proxy_scheme, host: new_proxy_host, port: new_proxy_port} = + URI.parse(new_proxy_base) + + clear_config([:media_proxy, :base_url], new_proxy_base) + + proxy_url = MediaProxy.encode_url("https://pleroma.social/logo.jpeg") |> URI.parse() |> Map.put(:host, "wronghost") |> URI.to_string() + expected_url = + URI.parse(proxy_url) + |> Map.put(:host, new_proxy_host) + |> Map.put(:port, new_proxy_port) + |> Map.put(:scheme, new_proxy_scheme) + |> URI.to_string() + with_mock Pleroma.ReverseProxy, call: fn _conn, _url, _opts -> %Conn{status: :success} end do - %{status: status} = get(conn, url) + %{resp_headers: resp_headers, status: status} = get(conn, proxy_url) - assert status == 400 + assert status == 302 + + assert Enum.any?( + resp_headers, + &(&1 == {"location", expected_url}) + ) end end diff --git a/test/pleroma/web/plugs/uploaded_media_plug_test.exs b/test/pleroma/web/plugs/uploaded_media_plug_test.exs index 262c8c288..fcc523cf4 100644 --- a/test/pleroma/web/plugs/uploaded_media_plug_test.exs +++ b/test/pleroma/web/plugs/uploaded_media_plug_test.exs @@ -48,10 +48,27 @@ defmodule Pleroma.Web.Plugs.UploadedMediaPlugTest do assert conn.status == 200 - clear_config([Pleroma.Upload, :base_url], "http://media.localhost/") + new_media_base = "http://media.localhost:8080" + + %{scheme: new_media_scheme, host: new_media_host, port: new_media_port} = + URI.parse(new_media_base) + + clear_config([Pleroma.Upload, :base_url], new_media_base) conn = get(build_conn(), attachment_url) - assert conn.status == 400 + expected_url = + URI.parse(attachment_url) + |> Map.put(:host, new_media_host) + |> Map.put(:port, new_media_port) + |> Map.put(:scheme, new_media_scheme) + |> URI.to_string() + + assert conn.status == 302 + + assert Enum.any?( + conn.resp_headers, + &(&1 == {"location", expected_url}) + ) end end