From 695dd340dec21dc9ddb72f908431f5d2c83ca6a6 Mon Sep 17 00:00:00 2001 From: Lain Soykaf Date: Tue, 27 Oct 2026 21:37:20 +0400 Subject: [PATCH] fix(federation): reject actors that point at other hosts A remote actor could set its inbox or shared inbox to a third-party host, so our signed deliveries went there. It could also claim another actor's followers collection, which the visibility checks trust. Validate fetched actors and incoming actor Updates: inbox, endpoints.sharedInbox, followers, following, featured and the publicKey id must be on the same host as the actor id. Based on Akkoma f07eb4cb5. The Bridgy fixture gets the key id that current Bridgy Fed serves. --- changelog.d/actor-containment.security | 1 + lib/pleroma/object/containment.ex | 10 +++ lib/pleroma/web/activity_pub/activity_pub.ex | 2 + .../web/activity_pub/object_validator.ex | 19 +++++- .../object_validators/user_validator.ex | 52 +++++++++++++++ test/fixtures/bridgy/actor.json | 2 +- .../web/activity_pub/activity_pub_test.exs | 25 +++++++ .../object_validators/user_validator_test.exs | 65 +++++++++++++++++++ .../user_update_handling_test.exs | 33 ++++++++-- 9 files changed, 201 insertions(+), 8 deletions(-) create mode 100644 changelog.d/actor-containment.security create mode 100644 lib/pleroma/web/activity_pub/object_validators/user_validator.ex create mode 100644 test/pleroma/web/activity_pub/object_validators/user_validator_test.exs diff --git a/changelog.d/actor-containment.security b/changelog.d/actor-containment.security new file mode 100644 index 000000000..58dbca4bc --- /dev/null +++ b/changelog.d/actor-containment.security @@ -0,0 +1 @@ +Reject remote actors whose inbox, shared inbox, followers, following or featured collection, or key id is on another host diff --git a/lib/pleroma/object/containment.ex b/lib/pleroma/object/containment.ex index c4cc1f014..b50d560d1 100644 --- a/lib/pleroma/object/containment.ex +++ b/lib/pleroma/object/containment.ex @@ -112,4 +112,14 @@ defmodule Pleroma.Object.Containment do do: contain_origin(id, object) def contain_child(_), do: :ok + + @doc "Checks that two URIs are on the same host. URIs without a host never match." + def same_origin(id, other_id) when is_binary(id) and is_binary(other_id) do + case {URI.parse(id), URI.parse(other_id)} do + {%URI{host: host}, %URI{host: host}} when is_binary(host) and host != "" -> :ok + _ -> :error + end + end + + def same_origin(_id, _other_id), do: :error end diff --git a/lib/pleroma/web/activity_pub/activity_pub.ex b/lib/pleroma/web/activity_pub/activity_pub.ex index 1279e0cb4..18cb2cadc 100644 --- a/lib/pleroma/web/activity_pub/activity_pub.ex +++ b/lib/pleroma/web/activity_pub/activity_pub.ex @@ -21,6 +21,7 @@ defmodule Pleroma.Web.ActivityPub.ActivityPub do alias Pleroma.Upload alias Pleroma.User alias Pleroma.Web.ActivityPub.MRF + alias Pleroma.Web.ActivityPub.ObjectValidators.UserValidator alias Pleroma.Web.ActivityPub.Transmogrifier alias Pleroma.Web.Streamer alias Pleroma.Web.WebFinger @@ -1924,6 +1925,7 @@ defmodule Pleroma.Web.ActivityPub.ActivityPub do def user_data_from_user_object(data, additional \\ []) do with {:ok, data} <- MRF.filter(data), + {:ok, data, _} <- UserValidator.validate(data, []), {:ok, data} <- object_to_user_data(data, additional) do {:ok, data} else diff --git a/lib/pleroma/web/activity_pub/object_validator.ex b/lib/pleroma/web/activity_pub/object_validator.ex index 0254d6dc1..468298a4d 100644 --- a/lib/pleroma/web/activity_pub/object_validator.ex +++ b/lib/pleroma/web/activity_pub/object_validator.ex @@ -11,7 +11,7 @@ defmodule Pleroma.Web.ActivityPub.ObjectValidator do @behaviour Pleroma.Web.ActivityPub.ObjectValidator.Validating - import Pleroma.Constants, only: [activity_types: 0, object_types: 0] + import Pleroma.Constants, only: [activity_types: 0, actor_types: 0, object_types: 0] alias Pleroma.Activity alias Pleroma.EctoType.ActivityPub.ObjectValidators @@ -37,6 +37,7 @@ defmodule Pleroma.Web.ActivityPub.ObjectValidator do alias Pleroma.Web.ActivityPub.ObjectValidators.QuestionValidator alias Pleroma.Web.ActivityPub.ObjectValidators.UndoValidator alias Pleroma.Web.ActivityPub.ObjectValidators.UpdateValidator + alias Pleroma.Web.ActivityPub.ObjectValidators.UserValidator @impl true def validate(object, meta) @@ -226,7 +227,8 @@ defmodule Pleroma.Web.ActivityPub.ObjectValidator do end def validate(%{"type" => type} = object, meta) when type == "Update" do - with {:ok, object} <- + with :ok <- validate_updated_actor(object, meta), + {:ok, object} <- object |> UpdateValidator.cast_and_validate(meta) |> Ecto.Changeset.apply_action(:insert) do @@ -247,6 +249,19 @@ defmodule Pleroma.Web.ActivityPub.ObjectValidator do def validate(o, m), do: {:error, {:validator_not_set, {o, m}}} + defp validate_updated_actor(%{"object" => %{"type" => type} = actor}, meta) + when type in actor_types() do + with {_, false} <- {:local, Access.get(meta, :local, false)}, + {:ok, _, _} <- UserValidator.validate(actor, meta) do + :ok + else + {:local, true} -> :ok + error -> error + end + end + + defp validate_updated_actor(_, _), do: :ok + def cast_and_apply_and_stringify_with_history(object) do do_separate_with_history(object, fn object -> with {:ok, object_data} <- cast_and_apply(object), diff --git a/lib/pleroma/web/activity_pub/object_validators/user_validator.ex b/lib/pleroma/web/activity_pub/object_validators/user_validator.ex new file mode 100644 index 000000000..183be4940 --- /dev/null +++ b/lib/pleroma/web/activity_pub/object_validators/user_validator.ex @@ -0,0 +1,52 @@ +# Pleroma: A lightweight social networking server +# Copyright © 2017-2022 Pleroma Authors +# SPDX-License-Identifier: AGPL-3.0-only + +defmodule Pleroma.Web.ActivityPub.ObjectValidators.UserValidator do + @moduledoc """ + Checks that a remote actor document only points at its own host. + + We deliver to the inbox and shared inbox, use the collections for + addressing and visibility checks, and map signature key ids back to + the actor. If an actor could point any of these at another host, it + could redirect our signed deliveries or claim another actor's + collections. Optional fields are only checked when present. + + Based on Akkoma's UserValidator. + """ + + @behaviour Pleroma.Web.ActivityPub.ObjectValidator.Validating + + alias Pleroma.Object.Containment + + @collections ["inbox", "followers", "following", "featured"] + + @impl true + def validate(%{"id" => id} = data, meta) when is_binary(id) do + fields = + Enum.map(@collections, &{&1, data[&1]}) ++ + [ + {"sharedInbox", get_in_map(data, ["endpoints", "sharedInbox"])}, + {"publicKey", get_in_map(data, ["publicKey", "id"])} + ] + + case Enum.find(fields, fn {_, value} -> not same_origin?(id, value) end) do + nil -> {:ok, data, meta} + {field, _} -> {:error, "#{field} is not on the same host as the actor"} + end + end + + def validate(_data, _meta), do: {:error, "Actor has no id"} + + defp same_origin?(_id, nil), do: true + defp same_origin?(id, %{"id" => value}), do: same_origin?(id, value) + defp same_origin?(id, value), do: Containment.same_origin(id, value) == :ok + + defp get_in_map(data, [key | rest]) do + case data do + %{^key => value} when rest == [] -> value + %{^key => value} -> get_in_map(value, rest) + _ -> nil + end + end +end diff --git a/test/fixtures/bridgy/actor.json b/test/fixtures/bridgy/actor.json index 5b2d8982b..b4e859a82 100644 --- a/test/fixtures/bridgy/actor.json +++ b/test/fixtures/bridgy/actor.json @@ -70,7 +70,7 @@ "preferredUsername": "jk.nipponalba.scot", "summary": "", "publicKey": { - "id": "jk.nipponalba.scot", + "id": "https://fed.brid.gy/jk.nipponalba.scot#key", "publicKeyPem": "-----BEGIN PUBLIC KEY-----\nMIGfMA0GCSqGSIb3DQEBAQUAA4GNADCBiQKBgQDdarxwzxnNbJ2hneWOYHkYJowk\npyigQtxlUd0VjgSQHwxU9kWqfbrHBVADyTtcqi/4dAzQd3UnCI1TPNnn4LPZY9PW\noiWd3Zl1/EfLFxO7LU9GS7fcSLQkyj5JNhSlN3I8QPudZbybrgRDVZYooDe1D+52\n5KLGqC2ajrIVOiDRTQIDAQAB\n-----END PUBLIC KEY-----" }, "inbox": "https://fed.brid.gy/jk.nipponalba.scot/inbox", diff --git a/test/pleroma/web/activity_pub/activity_pub_test.exs b/test/pleroma/web/activity_pub/activity_pub_test.exs index ed6db558b..125a2b4f9 100644 --- a/test/pleroma/web/activity_pub/activity_pub_test.exs +++ b/test/pleroma/web/activity_pub/activity_pub_test.exs @@ -244,6 +244,31 @@ defmodule Pleroma.Web.ActivityPub.ActivityPubTest do } end + test "rejects actors whose inbox is on another host" do + ap_id = "https://example.com/users/evil" + + user_data = + "test/fixtures/users_mock/user.json" + |> File.read!() + |> String.replace("{{nickname}}", "evil") + |> Jason.decode!() + |> Map.put("inbox", "https://victim.example/inbox") + |> Map.put("featured", nil) + |> Jason.encode!() + + Tesla.Mock.mock(fn + %{method: :get, url: ^ap_id} -> + %Tesla.Env{ + status: 200, + body: user_data, + headers: [{"content-type", "application/activity+json"}] + } + end) + + assert {:error, _} = ActivityPub.make_user_from_ap_id(ap_id) + refute User.get_by_ap_id(ap_id) + end + test "fetches user featured collection" do ap_id = "https://example.com/users/lain" diff --git a/test/pleroma/web/activity_pub/object_validators/user_validator_test.exs b/test/pleroma/web/activity_pub/object_validators/user_validator_test.exs new file mode 100644 index 000000000..91b33b9e2 --- /dev/null +++ b/test/pleroma/web/activity_pub/object_validators/user_validator_test.exs @@ -0,0 +1,65 @@ +# Pleroma: A lightweight social networking server +# Copyright © 2017-2022 Pleroma Authors +# SPDX-License-Identifier: AGPL-3.0-only + +defmodule Pleroma.Web.ActivityPub.ObjectValidators.UserValidatorTest do + use ExUnit.Case, async: true + + alias Pleroma.Web.ActivityPub.ObjectValidators.UserValidator + + defp actor do + "test/fixtures/users_mock/user.json" + |> File.read!() + |> String.replace("{{nickname}}", "lain") + |> Jason.decode!() + end + + test "accepts an actor whose endpoints and collections are on its own host" do + data = actor() + assert {:ok, ^data, []} = UserValidator.validate(data, []) + end + + test "accepts an actor without optional fields" do + data = Map.drop(actor(), ["endpoints", "featured", "followers", "following", "publicKey"]) + assert {:ok, _, _} = UserValidator.validate(data, []) + end + + test "accepts all actor fixtures" do + for path <- Path.wildcard("test/fixtures/**/*.json"), + {:ok, %{"inbox" => _, "id" => id} = data} when is_binary(id) <- + [Jason.decode(File.read!(path))] do + assert {{:ok, _, _}, ^path} = {UserValidator.validate(data, []), path} + end + end + + for field <- ["inbox", "followers", "following", "featured"] do + test "rejects an actor whose #{field} is on another host" do + data = Map.put(actor(), unquote(field), "https://victim.example/users/lain/x") + assert {:error, _} = UserValidator.validate(data, []) + end + end + + test "rejects an actor whose sharedInbox is on another host" do + data = put_in(actor(), ["endpoints", "sharedInbox"], "https://victim.example/inbox") + assert {:error, _} = UserValidator.validate(data, []) + end + + test "rejects an actor whose collection is an object on another host" do + data = Map.put(actor(), "followers", %{"id" => "https://victim.example/users/v/followers"}) + assert {:error, _} = UserValidator.validate(data, []) + end + + test "rejects an actor whose key id is on another host" do + data = put_in(actor(), ["publicKey", "id"], "https://victim.example/users/lain#main-key") + assert {:error, _} = UserValidator.validate(data, []) + end + + test "rejects an actor whose key id has no host" do + data = put_in(actor(), ["publicKey", "id"], "lain") + assert {:error, _} = UserValidator.validate(data, []) + end + + test "rejects an actor without an id" do + assert {:error, _} = UserValidator.validate(Map.delete(actor(), "id"), []) + end +end diff --git a/test/pleroma/web/activity_pub/transmogrifier/user_update_handling_test.exs b/test/pleroma/web/activity_pub/transmogrifier/user_update_handling_test.exs index cf995e143..b39bc8780 100644 --- a/test/pleroma/web/activity_pub/transmogrifier/user_update_handling_test.exs +++ b/test/pleroma/web/activity_pub/transmogrifier/user_update_handling_test.exs @@ -11,8 +11,11 @@ defmodule Pleroma.Web.ActivityPub.Transmogrifier.UserUpdateHandlingTest do import Pleroma.Factory + # matches the host of the actor in mastodon-update.json + @gargron "http://mastodon.example.org/users/gargron" + test "it works for incoming update activities" do - user = insert(:user, local: false) + user = insert(:user, local: false, ap_id: @gargron) update_data = File.read!("test/fixtures/mastodon-update.json") |> Jason.decode!() @@ -50,8 +53,28 @@ defmodule Pleroma.Web.ActivityPub.Transmogrifier.UserUpdateHandlingTest do assert user.bio == "

Some bio

" end + test "it rejects updates that move the shared inbox to another host" do + user = insert(:user, local: false, ap_id: @gargron) + + update_data = File.read!("test/fixtures/mastodon-update.json") |> Jason.decode!() + + object = + update_data["object"] + |> Map.put("actor", user.ap_id) + |> Map.put("id", user.ap_id) + |> put_in(["endpoints", "sharedInbox"], "https://victim.example/inbox") + + update_data = + update_data + |> Map.put("actor", user.ap_id) + |> Map.put("object", object) + + assert {:error, _} = Transmogrifier.handle_incoming(update_data) + assert User.get_cached_by_ap_id(user.ap_id).shared_inbox == user.shared_inbox + end + test "it selects an actor type after an extension type" do - user = insert(:user, local: false) + user = insert(:user, local: false, ap_id: @gargron) update_data = File.read!("test/fixtures/mastodon-update.json") |> Jason.decode!() object = @@ -70,7 +93,7 @@ defmodule Pleroma.Web.ActivityPub.Transmogrifier.UserUpdateHandlingTest do end test "it works with alsoKnownAs" do - %{ap_id: actor} = insert(:user, local: false) + %{ap_id: actor} = insert(:user, local: false, ap_id: @gargron) assert User.get_cached_by_ap_id(actor).also_known_as == [] @@ -97,7 +120,7 @@ defmodule Pleroma.Web.ActivityPub.Transmogrifier.UserUpdateHandlingTest do end test "it works with custom profile fields" do - user = insert(:user, local: false) + user = insert(:user, local: false, ap_id: @gargron) assert user.fields == [] @@ -155,7 +178,7 @@ defmodule Pleroma.Web.ActivityPub.Transmogrifier.UserUpdateHandlingTest do end test "it works for incoming update activities which lock the account" do - user = insert(:user, local: false) + user = insert(:user, local: false, ap_id: @gargron) update_data = File.read!("test/fixtures/mastodon-update.json") |> Jason.decode!()