From e8caecb5c7f49a829b857131ff98a46c705e3a80 Mon Sep 17 00:00:00 2001 From: William Pitcock Date: Sat, 1 Dec 2018 22:29:41 +0000 Subject: [PATCH] object: move object containment out of transmogrifier into it's own module --- lib/pleroma/object/containment.ex | 64 +++++++++++++++++ lib/pleroma/web/activity_pub/activity_pub.ex | 5 +- .../web/activity_pub/transmogrifier.ex | 71 +++---------------- test/object/containment_test.exs | 66 +++++++++++++++++ test/web/activity_pub/transmogrifier_test.exs | 57 --------------- 5 files changed, 144 insertions(+), 119 deletions(-) create mode 100644 lib/pleroma/object/containment.ex create mode 100644 test/object/containment_test.exs diff --git a/lib/pleroma/object/containment.ex b/lib/pleroma/object/containment.ex new file mode 100644 index 000000000..010b768bd --- /dev/null +++ b/lib/pleroma/object/containment.ex @@ -0,0 +1,64 @@ +defmodule Pleroma.Object.Containment do + @moduledoc """ + # Object Containment + + This module contains some useful functions for containing objects to specific + origins and determining those origins. They previously lived in the + ActivityPub `Transmogrifier` module. + + Object containment is an important step in validating remote objects to prevent + spoofing, therefore removal of object containment functions is NOT recommended. + """ + + require Logger + + def get_actor(%{"actor" => actor}) when is_binary(actor) do + actor + end + + def get_actor(%{"actor" => actor}) when is_list(actor) do + if is_binary(Enum.at(actor, 0)) do + Enum.at(actor, 0) + else + Enum.find(actor, fn %{"type" => type} -> type in ["Person", "Service", "Application"] end) + |> Map.get("id") + end + end + + def get_actor(%{"actor" => %{"id" => id}}) when is_bitstring(id) do + id + end + + def get_actor(%{"actor" => nil, "attributedTo" => actor}) when not is_nil(actor) do + get_actor(%{"actor" => actor}) + end + + @doc """ + Checks that an imported AP object's actor matches the domain it came from. + """ + def contain_origin(id, %{"actor" => nil}), do: :error + + def contain_origin(id, %{"actor" => actor} = params) do + id_uri = URI.parse(id) + actor_uri = URI.parse(get_actor(params)) + + if id_uri.host == actor_uri.host do + :ok + else + :error + end + end + + def contain_origin_from_id(id, %{"id" => nil}), do: :error + + def contain_origin_from_id(id, %{"id" => other_id} = params) do + id_uri = URI.parse(id) + other_uri = URI.parse(other_id) + + if id_uri.host == other_uri.host do + :ok + else + :error + end + end +end diff --git a/lib/pleroma/web/activity_pub/activity_pub.ex b/lib/pleroma/web/activity_pub/activity_pub.ex index 34a84b045..517cf4b46 100644 --- a/lib/pleroma/web/activity_pub/activity_pub.ex +++ b/lib/pleroma/web/activity_pub/activity_pub.ex @@ -1,5 +1,6 @@ defmodule Pleroma.Web.ActivityPub.ActivityPub do alias Pleroma.{Activity, Repo, Object, Upload, User, Notification} + alias Pleroma.Object.Containment alias Pleroma.Web.ActivityPub.{Transmogrifier, MRF} alias Pleroma.Web.WebFinger alias Pleroma.Web.Federator @@ -739,7 +740,7 @@ def fetch_object_from_id(id) do "actor" => data["actor"] || data["attributedTo"], "object" => data }, - :ok <- Transmogrifier.contain_origin(id, params), + :ok <- Containment.contain_origin(id, params), {:ok, activity} <- Transmogrifier.handle_incoming(params) do {:ok, Object.normalize(activity.data["object"])} else @@ -773,7 +774,7 @@ def fetch_and_contain_remote_object_from_id(id) do recv_timeout: 20000 ), {:ok, data} <- Jason.decode(body), - :ok <- Transmogrifier.contain_origin_from_id(id, data) do + :ok <- Containment.contain_origin_from_id(id, data) do {:ok, data} else e -> diff --git a/lib/pleroma/web/activity_pub/transmogrifier.ex b/lib/pleroma/web/activity_pub/transmogrifier.ex index 5e3d40d9f..1b5e57294 100644 --- a/lib/pleroma/web/activity_pub/transmogrifier.ex +++ b/lib/pleroma/web/activity_pub/transmogrifier.ex @@ -4,6 +4,7 @@ defmodule Pleroma.Web.ActivityPub.Transmogrifier do """ alias Pleroma.User alias Pleroma.Object + alias Pleroma.Object.Containment alias Pleroma.Activity alias Pleroma.Repo alias Pleroma.Web.ActivityPub.ActivityPub @@ -13,56 +14,6 @@ defmodule Pleroma.Web.ActivityPub.Transmogrifier do require Logger - def get_actor(%{"actor" => actor}) when is_binary(actor) do - actor - end - - def get_actor(%{"actor" => actor}) when is_list(actor) do - if is_binary(Enum.at(actor, 0)) do - Enum.at(actor, 0) - else - Enum.find(actor, fn %{"type" => type} -> type in ["Person", "Service", "Application"] end) - |> Map.get("id") - end - end - - def get_actor(%{"actor" => %{"id" => id}}) when is_bitstring(id) do - id - end - - def get_actor(%{"actor" => nil, "attributedTo" => actor}) when not is_nil(actor) do - get_actor(%{"actor" => actor}) - end - - @doc """ - Checks that an imported AP object's actor matches the domain it came from. - """ - def contain_origin(id, %{"actor" => nil}), do: :error - - def contain_origin(id, %{"actor" => actor} = params) do - id_uri = URI.parse(id) - actor_uri = URI.parse(get_actor(params)) - - if id_uri.host == actor_uri.host do - :ok - else - :error - end - end - - def contain_origin_from_id(id, %{"id" => nil}), do: :error - - def contain_origin_from_id(id, %{"id" => other_id} = params) do - id_uri = URI.parse(id) - other_uri = URI.parse(other_id) - - if id_uri.host == other_uri.host do - :ok - else - :error - end - end - @doc """ Modifies an incoming AP object (mastodon format) to our internal format. """ @@ -99,7 +50,7 @@ def fix_addressing(map) do def fix_actor(%{"attributedTo" => actor} = object) do object - |> Map.put("actor", get_actor(%{"actor" => actor})) + |> Map.put("actor", Containment.get_actor(%{"actor" => actor})) end def fix_likes(%{"likes" => likes} = object) @@ -277,7 +228,7 @@ def handle_incoming(%{"id" => id}) when not (is_binary(id) and length(id) > 8), # - emoji def handle_incoming(%{"type" => "Create", "object" => %{"type" => objtype} = object} = data) when objtype in ["Article", "Note", "Video", "Page"] do - actor = get_actor(data) + actor = Containment.get_actor(data) data = Map.put(data, "actor", actor) @@ -360,7 +311,7 @@ defp get_follow_activity(follow_object, followed) do def handle_incoming( %{"type" => "Accept", "object" => follow_object, "actor" => actor, "id" => id} = data ) do - with actor <- get_actor(data), + with actor <- Containment.get_actor(data), %User{} = followed <- User.get_or_fetch_by_ap_id(actor), {:ok, follow_activity} <- get_follow_activity(follow_object, followed), {:ok, follow_activity} <- Utils.update_follow_state(follow_activity, "accept"), @@ -386,7 +337,7 @@ def handle_incoming( def handle_incoming( %{"type" => "Reject", "object" => follow_object, "actor" => actor, "id" => id} = data ) do - with actor <- get_actor(data), + with actor <- Containment.get_actor(data), %User{} = followed <- User.get_or_fetch_by_ap_id(actor), {:ok, follow_activity} <- get_follow_activity(follow_object, followed), {:ok, follow_activity} <- Utils.update_follow_state(follow_activity, "reject"), @@ -410,7 +361,7 @@ def handle_incoming( def handle_incoming( %{"type" => "Like", "object" => object_id, "actor" => actor, "id" => id} = data ) do - with actor <- get_actor(data), + with actor <- Containment.get_actor(data), %User{} = actor <- User.get_or_fetch_by_ap_id(actor), {:ok, object} <- get_obj_helper(object_id) || fetch_obj_helper(object_id), {:ok, activity, _object} <- ActivityPub.like(actor, object, id, false) do @@ -423,7 +374,7 @@ def handle_incoming( def handle_incoming( %{"type" => "Announce", "object" => object_id, "actor" => actor, "id" => id} = data ) do - with actor <- get_actor(data), + with actor <- Containment.get_actor(data), %User{} = actor <- User.get_or_fetch_by_ap_id(actor), {:ok, object} <- get_obj_helper(object_id) || fetch_obj_helper(object_id), {:ok, activity, _object} <- ActivityPub.announce(actor, object, id, false) do @@ -477,10 +428,10 @@ def handle_incoming( ) do object_id = Utils.get_ap_id(object_id) - with actor <- get_actor(data), + with actor <- Containment.get_actor(data), %User{} = actor <- User.get_or_fetch_by_ap_id(actor), {:ok, object} <- get_obj_helper(object_id) || fetch_obj_helper(object_id), - :ok <- contain_origin(actor.ap_id, object.data), + :ok <- Containment.contain_origin(actor.ap_id, object.data), {:ok, activity} <- ActivityPub.delete(object, false) do {:ok, activity} else @@ -496,7 +447,7 @@ def handle_incoming( "id" => id } = data ) do - with actor <- get_actor(data), + with actor <- Containment.get_actor(data), %User{} = actor <- User.get_or_fetch_by_ap_id(actor), {:ok, object} <- get_obj_helper(object_id) || fetch_obj_helper(object_id), {:ok, activity, _} <- ActivityPub.unannounce(actor, object, id, false) do @@ -566,7 +517,7 @@ def handle_incoming( "id" => id } = data ) do - with actor <- get_actor(data), + with actor <- Containment.get_actor(data), %User{} = actor <- User.get_or_fetch_by_ap_id(actor), {:ok, object} <- get_obj_helper(object_id) || fetch_obj_helper(object_id), {:ok, activity, _, _} <- ActivityPub.unlike(actor, object, id, false) do diff --git a/test/object/containment_test.exs b/test/object/containment_test.exs new file mode 100644 index 000000000..fcedb2283 --- /dev/null +++ b/test/object/containment_test.exs @@ -0,0 +1,66 @@ +defmodule Pleroma.Object.ContainmentTest do + use Pleroma.DataCase + + alias Pleroma.User + alias Pleroma.Object.Containment + alias Pleroma.Web.ActivityPub.ActivityPub + + import Pleroma.Factory + + describe "general origin containment" do + test "contain_origin_from_id() catches obvious spoofing attempts" do + data = %{ + "id" => "http://example.com/~alyssa/activities/1234.json" + } + + :error = + Containment.contain_origin_from_id( + "http://example.org/~alyssa/activities/1234.json", + data + ) + end + + test "contain_origin_from_id() allows alternate IDs within the same origin domain" do + data = %{ + "id" => "http://example.com/~alyssa/activities/1234.json" + } + + :ok = + Containment.contain_origin_from_id( + "http://example.com/~alyssa/activities/1234", + data + ) + end + + test "contain_origin_from_id() allows matching IDs" do + data = %{ + "id" => "http://example.com/~alyssa/activities/1234.json" + } + + :ok = + Containment.contain_origin_from_id( + "http://example.com/~alyssa/activities/1234.json", + data + ) + end + + test "users cannot be collided through fake direction spoofing attempts" do + user = + insert(:user, %{ + nickname: "rye@niu.moe", + local: false, + ap_id: "https://niu.moe/users/rye", + follower_address: User.ap_followers(%User{nickname: "rye@niu.moe"}) + }) + + {:error, _} = User.get_or_fetch_by_ap_id("https://n1u.moe/users/rye") + end + + test "all objects with fake directions are rejected by the object fetcher" do + {:error, _} = + ActivityPub.fetch_and_contain_remote_object_from_id( + "https://info.pleroma.site/activity4.json" + ) + end + end +end diff --git a/test/web/activity_pub/transmogrifier_test.exs b/test/web/activity_pub/transmogrifier_test.exs index ef89752f5..a71a2c5b1 100644 --- a/test/web/activity_pub/transmogrifier_test.exs +++ b/test/web/activity_pub/transmogrifier_test.exs @@ -945,61 +945,4 @@ test "it rejects activities which reference objects that have an incorrect attri :error = Transmogrifier.handle_incoming(data) end end - - describe "general origin containment" do - test "contain_origin_from_id() catches obvious spoofing attempts" do - data = %{ - "id" => "http://example.com/~alyssa/activities/1234.json" - } - - :error = - Transmogrifier.contain_origin_from_id( - "http://example.org/~alyssa/activities/1234.json", - data - ) - end - - test "contain_origin_from_id() allows alternate IDs within the same origin domain" do - data = %{ - "id" => "http://example.com/~alyssa/activities/1234.json" - } - - :ok = - Transmogrifier.contain_origin_from_id( - "http://example.com/~alyssa/activities/1234", - data - ) - end - - test "contain_origin_from_id() allows matching IDs" do - data = %{ - "id" => "http://example.com/~alyssa/activities/1234.json" - } - - :ok = - Transmogrifier.contain_origin_from_id( - "http://example.com/~alyssa/activities/1234.json", - data - ) - end - - test "users cannot be collided through fake direction spoofing attempts" do - user = - insert(:user, %{ - nickname: "rye@niu.moe", - local: false, - ap_id: "https://niu.moe/users/rye", - follower_address: User.ap_followers(%User{nickname: "rye@niu.moe"}) - }) - - {:error, _} = User.get_or_fetch_by_ap_id("https://n1u.moe/users/rye") - end - - test "all objects with fake directions are rejected by the object fetcher" do - {:error, _} = - ActivityPub.fetch_and_contain_remote_object_from_id( - "https://info.pleroma.site/activity4.json" - ) - end - end end