From fee57eb376a446ee394812762df148d7b4d19b39 Mon Sep 17 00:00:00 2001 From: Oneric Date: Wed, 13 Mar 2024 20:21:19 -0100 Subject: [PATCH] Move actor check into fetch_and_contain_remote_object_from_id MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- lib/pleroma/object/containment.ex | 2 +- lib/pleroma/object/fetcher.ex | 10 ++--- test/pleroma/object/containment_test.exs | 50 ++++++++++++++++++++++-- test/pleroma/object/fetcher_test.exs | 20 ++++++++++ 4 files changed, 72 insertions(+), 10 deletions(-) diff --git a/lib/pleroma/object/containment.ex b/lib/pleroma/object/containment.ex index 219bc3892..85b777179 100644 --- a/lib/pleroma/object/containment.ex +++ b/lib/pleroma/object/containment.ex @@ -62,7 +62,7 @@ def contain_origin(id, %{"actor" => _actor} = params) do def contain_origin(id, %{"attributedTo" => actor} = params), do: contain_origin(id, Map.put(params, "actor", actor)) - def contain_origin(_id, _data), do: :error + def contain_origin(_id, _data), do: :ok @doc """ Check whether the object id is from the same host as another id diff --git a/lib/pleroma/object/fetcher.ex b/lib/pleroma/object/fetcher.ex index 9c0bf7124..16d8194e7 100644 --- a/lib/pleroma/object/fetcher.ex +++ b/lib/pleroma/object/fetcher.ex @@ -143,7 +143,6 @@ def fetch_object_from_id(id, options \\ []) do {_, {:ok, data}} <- {:fetch, fetch_and_contain_remote_object_from_id(id)}, {_, nil} <- {:normalize, Object.normalize(data, fetch: false)}, params <- prepare_activity_params(data), - {_, :ok} <- {:containment, Containment.contain_origin(id, params)}, {_, {:ok, activity}} <- {:transmogrifier, Transmogrifier.handle_incoming(params, options)}, {_, _data, %Object{} = object} <- @@ -156,9 +155,6 @@ def fetch_object_from_id(id, options \\ []) do {:scheme, false} -> {:error, "URI Scheme Invalid"} - {:containment, _} -> - {:error, "Object containment failed."} - {:transmogrifier, {:error, {: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")}, {:ok, body} <- get_object(id), {: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 Instances.set_reachable(id) end @@ -274,6 +271,9 @@ def fetch_and_contain_remote_object_from_id(id) when is_binary(id) do {:scheme, _} -> {:error, "Unsupported URI scheme"} + {:containment, _} -> + {:error, "Object containment failed."} + {:error, e} -> {:error, e} diff --git a/test/pleroma/object/containment_test.exs b/test/pleroma/object/containment_test.exs index fb2fb7d49..ad1660069 100644 --- a/test/pleroma/object/containment_test.exs +++ b/test/pleroma/object/containment_test.exs @@ -17,16 +17,58 @@ defmodule Pleroma.Object.ContainmentTest do end describe "general origin containment" do - test "works for completely actorless posts" do - assert :error == - Containment.contain_origin("https://glaceon.social/users/monorail", %{ + test "handles completly actorless objects gracefully" do + assert :ok == + Containment.contain_origin("https://glaceon.social/statuses/123", %{ "deleted" => "2019-10-30T05:48:50.249606Z", "formerType" => "Note", - "id" => "https://glaceon.social/users/monorail/statuses/103049757364029187", + "id" => "https://glaceon.social/statuses/123", "type" => "Tombstone" }) 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 data = %{ "id" => "http://example.com/~alyssa/activities/1234.json" diff --git a/test/pleroma/object/fetcher_test.exs b/test/pleroma/object/fetcher_test.exs index 7f6f9c031..b289e869d 100644 --- a/test/pleroma/object/fetcher_test.exs +++ b/test/pleroma/object/fetcher_test.exs @@ -84,6 +84,19 @@ defp spoofed_object_with_ids( 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 -> apply(HttpRequestMock, :request, [env]) end) @@ -217,6 +230,13 @@ test "it accepts same-domain redirects" do assert id == "https://patch.cx/objects/spoof_redirect" 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 describe "fetching an object" do