From daa8ec3d6273b7002ddee041686adf7652ec8e18 Mon Sep 17 00:00:00 2001 From: William Pitcock Date: Sat, 17 Nov 2018 20:02:02 +0000 Subject: [PATCH 01/10] activitypub: factor out AP object fetching to it's own function and add ID-based containment --- lib/pleroma/web/activity_pub/activity_pub.ex | 32 +++++++++++++------ .../web/activity_pub/transmogrifier.ex | 13 ++++++++ 2 files changed, 35 insertions(+), 10 deletions(-) diff --git a/lib/pleroma/web/activity_pub/activity_pub.ex b/lib/pleroma/web/activity_pub/activity_pub.ex index 51b787272..98e9e2120 100644 --- a/lib/pleroma/web/activity_pub/activity_pub.ex +++ b/lib/pleroma/web/activity_pub/activity_pub.ex @@ -732,16 +732,7 @@ def fetch_object_from_id(id) do else Logger.info("Fetching #{id} via AP") - with true <- String.starts_with?(id, "http"), - {:ok, %{body: body, status_code: code}} when code in 200..299 <- - @httpoison.get( - id, - [Accept: "application/activity+json"], - follow_redirect: true, - timeout: 10000, - recv_timeout: 20000 - ), - {:ok, data} <- Jason.decode(body), + with {:ok, data} <- fetch_and_contain_remote_object_from_id(id), nil <- Object.normalize(data), params <- %{ "type" => "Create", @@ -771,6 +762,27 @@ def fetch_object_from_id(id) do end end + def fetch_and_contain_remote_object_from_id(id) do + Logger.info("Fetching #{id} via AP") + + with true <- String.starts_with?(id, "http"), + {:ok, %{body: body, status_code: code}} when code in 200..299 <- + @httpoison.get( + id, + [Accept: "application/activity+json"], + follow_redirect: true, + timeout: 10000, + recv_timeout: 20000 + ), + {:ok, data} <- Jason.decode(body), + :ok <- Transmogrifier.contain_origin_from_id(id, data) do + {:ok, data} + else + e -> + {:error, e} + end + end + def is_public?(activity) do "https://www.w3.org/ns/activitystreams#Public" in (activity.data["to"] ++ (activity.data["cc"] || [])) diff --git a/lib/pleroma/web/activity_pub/transmogrifier.ex b/lib/pleroma/web/activity_pub/transmogrifier.ex index d51d8626b..1f886839e 100644 --- a/lib/pleroma/web/activity_pub/transmogrifier.ex +++ b/lib/pleroma/web/activity_pub/transmogrifier.ex @@ -50,6 +50,19 @@ def contain_origin(id, %{"actor" => actor} = params) do 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. """ From 1a940cb46e1fb06b391043ae2efa3ac0d3c49fe0 Mon Sep 17 00:00:00 2001 From: William Pitcock Date: Sat, 17 Nov 2018 20:07:49 +0000 Subject: [PATCH 02/10] tests: add tests for contain_origin_from_id() --- test/web/activity_pub/transmogrifier_test.exs | 38 +++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/test/web/activity_pub/transmogrifier_test.exs b/test/web/activity_pub/transmogrifier_test.exs index 6320b5b6e..b8adf3b8a 100644 --- a/test/web/activity_pub/transmogrifier_test.exs +++ b/test/web/activity_pub/transmogrifier_test.exs @@ -918,4 +918,42 @@ 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 + end end From c88533209c20eeae51dcdc029db9483f8e69d096 Mon Sep 17 00:00:00 2001 From: William Pitcock Date: Sat, 17 Nov 2018 20:13:38 +0000 Subject: [PATCH 03/10] activitypub: user fetching: use fetch_and_contain_remote_object_from_id() --- lib/pleroma/web/activity_pub/activity_pub.ex | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/lib/pleroma/web/activity_pub/activity_pub.ex b/lib/pleroma/web/activity_pub/activity_pub.ex index 98e9e2120..ed579e336 100644 --- a/lib/pleroma/web/activity_pub/activity_pub.ex +++ b/lib/pleroma/web/activity_pub/activity_pub.ex @@ -628,9 +628,7 @@ def user_data_from_user_object(data) do end def fetch_and_prepare_user_from_ap_id(ap_id) do - with {:ok, %{status_code: 200, body: body}} <- - @httpoison.get(ap_id, [Accept: "application/activity+json"], follow_redirect: true), - {:ok, data} <- Jason.decode(body) do + with {:ok, data} <- fetch_and_contain_remote_object_from_id(ap_id) do user_data_from_user_object(data) else e -> Logger.error("Could not decode user at fetch #{ap_id}, #{inspect(e)}") From dc1d8e13b483d6e5df4ea6432aa3e37971bb3fb1 Mon Sep 17 00:00:00 2001 From: William Pitcock Date: Sat, 17 Nov 2018 20:20:45 +0000 Subject: [PATCH 04/10] tests: add a testcase for user collision --- test/support/httpoison_mock.ex | 8 ++++++++ test/web/activity_pub/transmogrifier_test.exs | 12 ++++++++++++ 2 files changed, 20 insertions(+) diff --git a/test/support/httpoison_mock.ex b/test/support/httpoison_mock.ex index ebd1e9c4d..e3310bb5d 100644 --- a/test/support/httpoison_mock.ex +++ b/test/support/httpoison_mock.ex @@ -756,6 +756,14 @@ def get("https://niu.moe/users/rye", [Accept: "application/activity+json"], _) d }} end + def get("https://n1u.moe/users/rye", [Accept: "application/activity+json"], _) do + {:ok, + %Response{ + status_code: 200, + body: File.read!("test/fixtures/httpoison_mock/rye.json") + }} + end + def get( "https://mst3k.interlinked.me/users/luciferMysticus", [Accept: "application/activity+json"], diff --git a/test/web/activity_pub/transmogrifier_test.exs b/test/web/activity_pub/transmogrifier_test.exs index b8adf3b8a..f8a82dd5b 100644 --- a/test/web/activity_pub/transmogrifier_test.exs +++ b/test/web/activity_pub/transmogrifier_test.exs @@ -955,5 +955,17 @@ test "contain_origin_from_id() allows matching IDs" do 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 end end From 55640c4804d1fe1c39b155f8acd5f820425fb7f6 Mon Sep 17 00:00:00 2001 From: William Pitcock Date: Sat, 17 Nov 2018 20:31:20 +0000 Subject: [PATCH 05/10] tests: add a test to verify the general fake direction protection works in all cases --- .../https__info.pleroma.site_activity4.json | 13 +++++++++++++ test/support/httpoison_mock.ex | 8 ++++++++ test/web/activity_pub/transmogrifier_test.exs | 7 +++++++ 3 files changed, 28 insertions(+) create mode 100644 test/fixtures/httpoison_mock/https__info.pleroma.site_activity4.json diff --git a/test/fixtures/httpoison_mock/https__info.pleroma.site_activity4.json b/test/fixtures/httpoison_mock/https__info.pleroma.site_activity4.json new file mode 100644 index 000000000..57a73b12a --- /dev/null +++ b/test/fixtures/httpoison_mock/https__info.pleroma.site_activity4.json @@ -0,0 +1,13 @@ +{ + "@context": "https://www.w3.org/ns/activitystreams", + "attributedTo": "http://mastodon.example.org/users/admin", + "attachment": [], + "content": "

this post was not actually written by Haelwenn

", + "id": "http://mastodon.example.org/users/admin/activities/1234", + "published": "2018-09-01T22:15:00Z", + "tag": [], + "to": [ + "https://www.w3.org/ns/activitystreams#Public" + ], + "type": "Note" +} diff --git a/test/support/httpoison_mock.ex b/test/support/httpoison_mock.ex index e3310bb5d..0be09b6ce 100644 --- a/test/support/httpoison_mock.ex +++ b/test/support/httpoison_mock.ex @@ -56,6 +56,14 @@ def get("https://info.pleroma.site/activity3.json", _, _) do }} end + def get("https://info.pleroma.site/activity4.json", _, _) do + {:ok, + %Response{ + status_code: 200, + body: File.read!("test/fixtures/httpoison_mock/https__info.pleroma.site_activity4.json") + }} + end + def get("https://info.pleroma.site/actor.json", _, _) do {:ok, %Response{ diff --git a/test/web/activity_pub/transmogrifier_test.exs b/test/web/activity_pub/transmogrifier_test.exs index f8a82dd5b..9174d9b76 100644 --- a/test/web/activity_pub/transmogrifier_test.exs +++ b/test/web/activity_pub/transmogrifier_test.exs @@ -967,5 +967,12 @@ test "users cannot be collided through fake direction spoofing attempts" do {: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 From 3d9266a8cbf7e1d0979ad7e17dd553851e73d81e Mon Sep 17 00:00:00 2001 From: William Pitcock Date: Sat, 17 Nov 2018 20:43:43 +0000 Subject: [PATCH 06/10] federator: do origin containment when processing inbound messages --- lib/pleroma/web/federator/federator.ex | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/pleroma/web/federator/federator.ex b/lib/pleroma/web/federator/federator.ex index 962cacfa3..33e6db9b9 100644 --- a/lib/pleroma/web/federator/federator.ex +++ b/lib/pleroma/web/federator/federator.ex @@ -101,8 +101,11 @@ def handle(:incoming_ap_doc, params) do params = Utils.normalize_params(params) + # NOTE: we use the actor ID to do the containment, this is fine because an + # actor shouldn't be acting on objects outside their own AP server. with {:ok, _user} <- ap_enabled_actor(params["actor"]), nil <- Activity.normalize(params["id"]), + :ok <- Transmogrifier.contain_origin_from_id(params["actor"], params), {:ok, _activity} <- Transmogrifier.handle_incoming(params) do else %Activity{} -> From 0d1375f2746eb927e516064df3fd9fd0ee7e9ff8 Mon Sep 17 00:00:00 2001 From: William Pitcock Date: Sat, 17 Nov 2018 21:00:37 +0000 Subject: [PATCH 07/10] federator: return :ok or :error depending on if an AP doc was accepted or not --- lib/pleroma/web/federator/federator.ex | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/lib/pleroma/web/federator/federator.ex b/lib/pleroma/web/federator/federator.ex index 33e6db9b9..6554fd2ef 100644 --- a/lib/pleroma/web/federator/federator.ex +++ b/lib/pleroma/web/federator/federator.ex @@ -106,15 +106,18 @@ def handle(:incoming_ap_doc, params) do with {:ok, _user} <- ap_enabled_actor(params["actor"]), nil <- Activity.normalize(params["id"]), :ok <- Transmogrifier.contain_origin_from_id(params["actor"], params), - {:ok, _activity} <- Transmogrifier.handle_incoming(params) do + {:ok, activity} <- Transmogrifier.handle_incoming(params) do + {:ok, activity} else %Activity{} -> Logger.info("Already had #{params["id"]}") + :error _e -> # Just drop those for now Logger.info("Unhandled activity") Logger.info(Poison.encode!(params, pretty: 2)) + :error end end From b1a6e8d80d47efdea5110e9d86e080a16b5aeaa8 Mon Sep 17 00:00:00 2001 From: William Pitcock Date: Sat, 17 Nov 2018 21:01:19 +0000 Subject: [PATCH 08/10] test: add sanity tests for federator handling of AP docs --- test/web/federator_test.exs | 38 +++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/test/web/federator_test.exs b/test/web/federator_test.exs index c709d1181..1d48931b5 100644 --- a/test/web/federator_test.exs +++ b/test/web/federator_test.exs @@ -61,4 +61,42 @@ test "with relays deactivated, it does not publish to the relay", %{ Pleroma.Config.put([:instance, :allow_relay], true) end end + + describe "Receive an activity" do + test "successfully processes incoming AP docs with correct origin" do + params = %{ + "@context" => "https://www.w3.org/ns/activitystreams", + "actor" => "http://mastodon.example.org/users/admin", + "type" => "Create", + "id" => "http://mastodon.example.org/users/admin/activities/1", + "object" => %{ + "type" => "Note", + "content" => "hi world!", + "id" => "http://mastodon.example.org/users/admin/objects/1", + "attributedTo" => "http://mastodon.example.org/users/admin", + }, + "to" => ["https://www.w3.org/ns/activitystreams#Public"] + } + + {:ok, _activity} = Federator.handle(:incoming_ap_doc, params) + end + + test "rejects incoming AP docs with incorrect origin" do + params = %{ + "@context" => "https://www.w3.org/ns/activitystreams", + "actor" => "https://niu.moe/users/rye", + "type" => "Create", + "id" => "http://mastodon.example.org/users/admin/activities/1", + "object" => %{ + "type" => "Note", + "content" => "hi world!", + "id" => "http://mastodon.example.org/users/admin/objects/1", + "attributedTo" => "http://mastodon.example.org/users/admin", + }, + "to" => ["https://www.w3.org/ns/activitystreams#Public"] + } + + :error = Federator.handle(:incoming_ap_doc, params) + end + end end From dfcfb184b10428af8d37492e64f271c0275fc2c9 Mon Sep 17 00:00:00 2001 From: William Pitcock Date: Sat, 17 Nov 2018 21:22:30 +0000 Subject: [PATCH 09/10] activitypub: transmogrifier: make deletes secure --- .../web/activity_pub/transmogrifier.ex | 11 +++++++--- test/web/activity_pub/transmogrifier_test.exs | 20 +++++++++++++++++++ 2 files changed, 28 insertions(+), 3 deletions(-) diff --git a/lib/pleroma/web/activity_pub/transmogrifier.ex b/lib/pleroma/web/activity_pub/transmogrifier.ex index 1f886839e..5864855b0 100644 --- a/lib/pleroma/web/activity_pub/transmogrifier.ex +++ b/lib/pleroma/web/activity_pub/transmogrifier.ex @@ -467,15 +467,20 @@ def handle_incoming( end end - # TODO: Make secure. + # TODO: We presently assume that any actor on the same origin domain as the object being + # deleted has the rights to delete that object. A better way to validate whether or not + # the object should be deleted is to refetch the object URI, which should return either + # an error or a tombstone. This would allow us to verify that a deletion actually took + # place. def handle_incoming( - %{"type" => "Delete", "object" => object_id, "actor" => actor, "id" => _id} = data + %{"type" => "Delete", "object" => object_id, "actor" => _actor, "id" => _id} = data ) do object_id = Utils.get_ap_id(object_id) with actor <- get_actor(data), - %User{} = _actor <- User.get_or_fetch_by_ap_id(actor), + %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, activity} <- ActivityPub.delete(object, false) do {:ok, activity} else diff --git a/test/web/activity_pub/transmogrifier_test.exs b/test/web/activity_pub/transmogrifier_test.exs index 9174d9b76..829da0a65 100644 --- a/test/web/activity_pub/transmogrifier_test.exs +++ b/test/web/activity_pub/transmogrifier_test.exs @@ -361,6 +361,26 @@ test "it works for incoming deletes" do refute Repo.get(Activity, activity.id) end + test "it fails for incoming deletes with spoofed origin" do + activity = insert(:note_activity) + + data = + File.read!("test/fixtures/mastodon-delete.json") + |> Poison.decode!() + + object = + data["object"] + |> Map.put("id", activity.data["object"]["id"]) + + data = + data + |> Map.put("object", object) + + :error = Transmogrifier.handle_incoming(data) + + assert Repo.get(Activity, activity.id) + end + test "it works for incoming unannounces with an existing notice" do user = insert(:user) {:ok, activity} = CommonAPI.post(user, %{"status" => "hey"}) From e10f839e9b413a58dfa2c55f136862ec0f56e314 Mon Sep 17 00:00:00 2001 From: William Pitcock Date: Sat, 17 Nov 2018 21:41:08 +0000 Subject: [PATCH 10/10] tests: federator: fix formatting --- test/web/federator_test.exs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/web/federator_test.exs b/test/web/federator_test.exs index 1d48931b5..02e1ca76e 100644 --- a/test/web/federator_test.exs +++ b/test/web/federator_test.exs @@ -73,7 +73,7 @@ test "successfully processes incoming AP docs with correct origin" do "type" => "Note", "content" => "hi world!", "id" => "http://mastodon.example.org/users/admin/objects/1", - "attributedTo" => "http://mastodon.example.org/users/admin", + "attributedTo" => "http://mastodon.example.org/users/admin" }, "to" => ["https://www.w3.org/ns/activitystreams#Public"] } @@ -91,7 +91,7 @@ test "rejects incoming AP docs with incorrect origin" do "type" => "Note", "content" => "hi world!", "id" => "http://mastodon.example.org/users/admin/objects/1", - "attributedTo" => "http://mastodon.example.org/users/admin", + "attributedTo" => "http://mastodon.example.org/users/admin" }, "to" => ["https://www.w3.org/ns/activitystreams#Public"] }