Move actor check into fetch_and_contain_remote_object_from_id

This brings it in line with its name and closes an,
in practice harmless, verification hole.

This was/is the only user of contain_origin making it
safe to change the behaviour on actor-less objects.

Until now refetched objects did not ensure the new actor matches the
domain of the object. We refetch polls occasionally to retrieve
up-to-date vote counts. A malicious AP server could have switched out
the poll after initial posting with a completely different post
attribute to an actor from another server.
While we indeed fell for this spoof before the commit,
it fortunately seems to have had no ill effect in practice,
since the asociated Create activity is not changed. When exposing the
actor via our REST API, we read this info from the activity not the
object.

This at first thought still keeps one avenue for exploit open though:
the updated actor can be from our own domain and a third server be
instructed to fetch the object from us. However this is foiled by an
id mismatch. By necessity of being fetchable and our longstanding
same-domain check, the id must still be from the attacker’s server.
Even the most barebone authenticity check is able to sus this out.
This commit is contained in:
Oneric 2024-03-13 20:21:19 -01:00
parent c4cf4d7f0b
commit fee57eb376
4 changed files with 72 additions and 10 deletions

View file

@ -62,7 +62,7 @@ def contain_origin(id, %{"actor" => _actor} = params) do
def contain_origin(id, %{"attributedTo" => actor} = params), def contain_origin(id, %{"attributedTo" => actor} = params),
do: contain_origin(id, Map.put(params, "actor", actor)) do: contain_origin(id, Map.put(params, "actor", actor))
def contain_origin(_id, _data), do: :error def contain_origin(_id, _data), do: :ok
@doc """ @doc """
Check whether the object id is from the same host as another id Check whether the object id is from the same host as another id

View file

@ -143,7 +143,6 @@ def fetch_object_from_id(id, options \\ []) do
{_, {:ok, data}} <- {:fetch, fetch_and_contain_remote_object_from_id(id)}, {_, {:ok, data}} <- {:fetch, fetch_and_contain_remote_object_from_id(id)},
{_, nil} <- {:normalize, Object.normalize(data, fetch: false)}, {_, nil} <- {:normalize, Object.normalize(data, fetch: false)},
params <- prepare_activity_params(data), params <- prepare_activity_params(data),
{_, :ok} <- {:containment, Containment.contain_origin(id, params)},
{_, {:ok, activity}} <- {_, {:ok, activity}} <-
{:transmogrifier, Transmogrifier.handle_incoming(params, options)}, {:transmogrifier, Transmogrifier.handle_incoming(params, options)},
{_, _data, %Object{} = object} <- {_, _data, %Object{} = object} <-
@ -156,9 +155,6 @@ def fetch_object_from_id(id, options \\ []) do
{:scheme, false} -> {:scheme, false} ->
{:error, "URI Scheme Invalid"} {:error, "URI Scheme Invalid"}
{:containment, _} ->
{:error, "Object containment failed."}
{:transmogrifier, {:error, {:reject, e}}} -> {:transmogrifier, {:error, {:reject, e}}} ->
{:reject, e} {:reject, e}
@ -264,7 +260,8 @@ def fetch_and_contain_remote_object_from_id(id) when is_binary(id) do
with {:scheme, true} <- {:scheme, String.starts_with?(id, "http")}, with {:scheme, true} <- {:scheme, String.starts_with?(id, "http")},
{:ok, body} <- get_object(id), {:ok, body} <- get_object(id),
{:ok, data} <- safe_json_decode(body), {:ok, data} <- safe_json_decode(body),
:ok <- Containment.contain_origin_from_id(id, data) do {_, :ok} <- {:containment, Containment.contain_origin_from_id(id, data)},
{_, :ok} <- {:containment, Containment.contain_origin(id, data)} do
unless Instances.reachable?(id) do unless Instances.reachable?(id) do
Instances.set_reachable(id) Instances.set_reachable(id)
end end
@ -274,6 +271,9 @@ def fetch_and_contain_remote_object_from_id(id) when is_binary(id) do
{:scheme, _} -> {:scheme, _} ->
{:error, "Unsupported URI scheme"} {:error, "Unsupported URI scheme"}
{:containment, _} ->
{:error, "Object containment failed."}
{:error, e} -> {:error, e} ->
{:error, e} {:error, e}

View file

@ -17,16 +17,58 @@ defmodule Pleroma.Object.ContainmentTest do
end end
describe "general origin containment" do describe "general origin containment" do
test "works for completely actorless posts" do test "handles completly actorless objects gracefully" do
assert :error == assert :ok ==
Containment.contain_origin("https://glaceon.social/users/monorail", %{ Containment.contain_origin("https://glaceon.social/statuses/123", %{
"deleted" => "2019-10-30T05:48:50.249606Z", "deleted" => "2019-10-30T05:48:50.249606Z",
"formerType" => "Note", "formerType" => "Note",
"id" => "https://glaceon.social/users/monorail/statuses/103049757364029187", "id" => "https://glaceon.social/statuses/123",
"type" => "Tombstone" "type" => "Tombstone"
}) })
end end
test "errors for spoofed actors" do
assert :error ==
Containment.contain_origin("https://glaceon.social/statuses/123", %{
"actor" => "https://otp.akkoma.dev/users/you",
"id" => "https://glaceon.social/statuses/123",
"type" => "Note"
})
end
test "errors for spoofed attributedTo" do
assert :error ==
Containment.contain_origin("https://glaceon.social/statuses/123", %{
"attributedTo" => "https://otp.akkoma.dev/users/you",
"id" => "https://glaceon.social/statuses/123",
"type" => "Note"
})
end
test "accepts valid actors" do
assert :ok ==
Containment.contain_origin("https://glaceon.social/statuses/123", %{
"actor" => "https://glaceon.social/users/monorail",
"attributedTo" => "https://glaceon.social/users/monorail",
"id" => "https://glaceon.social/statuses/123",
"type" => "Note"
})
assert :ok ==
Containment.contain_origin("https://glaceon.social/statuses/123", %{
"actor" => "https://glaceon.social/users/monorail",
"id" => "https://glaceon.social/statuses/123",
"type" => "Note"
})
assert :ok ==
Containment.contain_origin("https://glaceon.social/statuses/123", %{
"attributedTo" => "https://glaceon.social/users/monorail",
"id" => "https://glaceon.social/statuses/123",
"type" => "Note"
})
end
test "contain_origin_from_id() catches obvious spoofing attempts" do test "contain_origin_from_id() catches obvious spoofing attempts" do
data = %{ data = %{
"id" => "http://example.com/~alyssa/activities/1234.json" "id" => "http://example.com/~alyssa/activities/1234.json"

View file

@ -84,6 +84,19 @@ defp spoofed_object_with_ids(
body: spoofed_object_with_ids("https://patch.cx/objects/spoof_redirect") body: spoofed_object_with_ids("https://patch.cx/objects/spoof_redirect")
} }
# Spoof: Actor from another domain
%{method: :get, url: "https://patch.cx/objects/spoof_foreign_actor"} ->
%Tesla.Env{
status: 200,
url: "https://patch.cx/objects/spoof_foreign_actor",
headers: [{"content-type", "application/activity+json"}],
body:
spoofed_object_with_ids(
"https://patch.cx/objects/spoof_foreign_actor",
"https://not.patch.cx/users/rin"
)
}
env -> env ->
apply(HttpRequestMock, :request, [env]) apply(HttpRequestMock, :request, [env])
end) end)
@ -217,6 +230,13 @@ test "it accepts same-domain redirects" do
assert id == "https://patch.cx/objects/spoof_redirect" assert id == "https://patch.cx/objects/spoof_redirect"
end end
test "it does not fetch a spoofed object with a foreign actor" do
assert {:error, "Object containment failed."} =
Fetcher.fetch_and_contain_remote_object_from_id(
"https://patch.cx/objects/spoof_foreign_actor"
)
end
end end
describe "fetching an object" do describe "fetching an object" do