Reject cross-domain redirects when fetching AP objects
Such redirects on AP queries seem most likely to be a spoofing attempt. If the object is legit, the id should match the final domain anyway and users can directly use the canonical URL. The lack of such a check (and use of the initially queried domain’s authority instead of the final domain) was enabling the current exploit to even affect instances which already migrated away from a same-domain upload/proxy setup in the past, but retained a redirect to not break old attachments. (In theory this redirect could, with some effort, have been limited to only old files, but common guides employed a catch-all redirect, which allows even future uploads to be reachable via an initial query to the main domain) Same-domain redirects are valid and also used by ourselves, e.g. for redirecting /notice/XXX to /objects/YYY.
This commit is contained in:
parent
baaeffdebc
commit
c4cf4d7f0b
2 changed files with 74 additions and 1 deletions
|
@ -26,6 +26,8 @@ defmodule Pleroma.Object.Fetcher do
|
||||||
function use the former and perform some additional tasks
|
function use the former and perform some additional tasks
|
||||||
"""
|
"""
|
||||||
|
|
||||||
|
@mix_env Mix.env()
|
||||||
|
|
||||||
defp touch_changeset(changeset) do
|
defp touch_changeset(changeset) do
|
||||||
updated_at =
|
updated_at =
|
||||||
NaiveDateTime.utc_now()
|
NaiveDateTime.utc_now()
|
||||||
|
@ -283,6 +285,22 @@ defmodule Pleroma.Object.Fetcher do
|
||||||
def fetch_and_contain_remote_object_from_id(_id),
|
def fetch_and_contain_remote_object_from_id(_id),
|
||||||
do: {:error, "id must be a string"}
|
do: {:error, "id must be a string"}
|
||||||
|
|
||||||
|
defp check_crossdomain_redirect(final_host, original_url)
|
||||||
|
|
||||||
|
# HOPEFULLY TEMPORARY
|
||||||
|
# Basically none of our Tesla mocks in tests set the (supposed to
|
||||||
|
# exist for Tesla proper) url parameter for their responses
|
||||||
|
# causing almost every fetch in test to fail otherwise
|
||||||
|
if @mix_env == :test do
|
||||||
|
defp check_crossdomain_redirect(nil, _) do
|
||||||
|
{:cross_domain_redirect, false}
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
|
defp check_crossdomain_redirect(final_host, original_url) do
|
||||||
|
{:cross_domain_redirect, final_host != URI.parse(original_url).host}
|
||||||
|
end
|
||||||
|
|
||||||
@doc "Do NOT use; only public for use in tests"
|
@doc "Do NOT use; only public for use in tests"
|
||||||
def get_object(id) do
|
def get_object(id) do
|
||||||
date = Pleroma.Signature.signed_date()
|
date = Pleroma.Signature.signed_date()
|
||||||
|
@ -292,8 +310,13 @@ defmodule Pleroma.Object.Fetcher do
|
||||||
|> maybe_date_fetch(date)
|
|> maybe_date_fetch(date)
|
||||||
|> sign_fetch(id, date)
|
|> sign_fetch(id, date)
|
||||||
|
|
||||||
with {:ok, %{body: body, status: code, headers: headers}} when code in 200..299 <-
|
with {:ok, %{body: body, status: code, headers: headers, url: final_url}}
|
||||||
|
when code in 200..299 <-
|
||||||
HTTP.get(id, headers),
|
HTTP.get(id, headers),
|
||||||
|
remote_host <-
|
||||||
|
URI.parse(final_url).host,
|
||||||
|
{:cross_domain_redirect, false} <-
|
||||||
|
check_crossdomain_redirect(remote_host, id),
|
||||||
{:has_content_type, {_, content_type}} <-
|
{:has_content_type, {_, content_type}} <-
|
||||||
{:has_content_type, List.keyfind(headers, "content-type", 0)},
|
{:has_content_type, List.keyfind(headers, "content-type", 0)},
|
||||||
{:parse_content_type, {:ok, "application", subtype, type_params}} <-
|
{:parse_content_type, {:ok, "application", subtype, type_params}} <-
|
||||||
|
|
|
@ -57,6 +57,33 @@ defmodule Pleroma.Object.FetcherTest do
|
||||||
body: spoofed_object_with_ids("https://patch.cx/objects/spoof_content_type")
|
body: spoofed_object_with_ids("https://patch.cx/objects/spoof_content_type")
|
||||||
}
|
}
|
||||||
|
|
||||||
|
# Spoof: cross-domain redirect with original domain id
|
||||||
|
%{method: :get, url: "https://patch.cx/objects/spoof_media_redirect1"} ->
|
||||||
|
%Tesla.Env{
|
||||||
|
status: 200,
|
||||||
|
url: "https://media.patch.cx/objects/spoof",
|
||||||
|
headers: [{"content-type", "application/activity+json"}],
|
||||||
|
body: spoofed_object_with_ids("https://patch.cx/objects/spoof_media_redirect1")
|
||||||
|
}
|
||||||
|
|
||||||
|
# Spoof: cross-domain redirect with final domain id
|
||||||
|
%{method: :get, url: "https://patch.cx/objects/spoof_media_redirect2"} ->
|
||||||
|
%Tesla.Env{
|
||||||
|
status: 200,
|
||||||
|
url: "https://media.patch.cx/objects/spoof_media_redirect2",
|
||||||
|
headers: [{"content-type", "application/activity+json"}],
|
||||||
|
body: spoofed_object_with_ids("https://media.patch.cx/objects/spoof_media_redirect2")
|
||||||
|
}
|
||||||
|
|
||||||
|
# No-Spoof: same domain redirect
|
||||||
|
%{method: :get, url: "https://patch.cx/objects/spoof_redirect"} ->
|
||||||
|
%Tesla.Env{
|
||||||
|
status: 200,
|
||||||
|
url: "https://patch.cx/objects/spoof",
|
||||||
|
headers: [{"content-type", "application/activity+json"}],
|
||||||
|
body: spoofed_object_with_ids("https://patch.cx/objects/spoof_redirect")
|
||||||
|
}
|
||||||
|
|
||||||
env ->
|
env ->
|
||||||
apply(HttpRequestMock, :request, [env])
|
apply(HttpRequestMock, :request, [env])
|
||||||
end)
|
end)
|
||||||
|
@ -167,6 +194,29 @@ defmodule Pleroma.Object.FetcherTest do
|
||||||
"https://patch.cx/objects/spoof_content_type.json"
|
"https://patch.cx/objects/spoof_content_type.json"
|
||||||
)
|
)
|
||||||
end
|
end
|
||||||
|
|
||||||
|
test "it does not fetch an object via cross-domain redirects (initial id)" do
|
||||||
|
assert {:error, {:cross_domain_redirect, true}} =
|
||||||
|
Fetcher.fetch_and_contain_remote_object_from_id(
|
||||||
|
"https://patch.cx/objects/spoof_media_redirect1"
|
||||||
|
)
|
||||||
|
end
|
||||||
|
|
||||||
|
test "it does not fetch an object via cross-domain redirects (final id)" do
|
||||||
|
assert {:error, {:cross_domain_redirect, true}} =
|
||||||
|
Fetcher.fetch_and_contain_remote_object_from_id(
|
||||||
|
"https://patch.cx/objects/spoof_media_redirect2"
|
||||||
|
)
|
||||||
|
end
|
||||||
|
|
||||||
|
test "it accepts same-domain redirects" do
|
||||||
|
assert {:ok, %{"id" => id} = _object} =
|
||||||
|
Fetcher.fetch_and_contain_remote_object_from_id(
|
||||||
|
"https://patch.cx/objects/spoof_redirect"
|
||||||
|
)
|
||||||
|
|
||||||
|
assert id == "https://patch.cx/objects/spoof_redirect"
|
||||||
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
describe "fetching an object" do
|
describe "fetching an object" do
|
||||||
|
|
Loading…
Reference in a new issue