From 535a5ecad04c9c49105a77e7025fe9f4b4d23ba6 Mon Sep 17 00:00:00 2001 From: Mint Date: Sat, 2 Sep 2023 01:43:25 +0300 Subject: [PATCH] CommonAPI: Prevent users from accessing media of other users commit 1afde067b12ad0062c1820091ea9b0a680819281 upstream. --- .../check-attachment-attribution.security | 1 + lib/pleroma/scheduled_activity.ex | 6 ++- lib/pleroma/web/common_api.ex | 12 ++++++ lib/pleroma/web/common_api/activity_draft.ex | 2 +- lib/pleroma/web/common_api/utils.ex | 31 ++++++++------ test/pleroma/web/common_api/utils_test.exs | 41 +++++++++++++------ test/pleroma/web/common_api_test.exs | 18 ++++++++ .../views/scheduled_activity_view_test.exs | 2 +- .../chat_message_reference_view_test.exs | 2 +- 9 files changed, 85 insertions(+), 30 deletions(-) create mode 100644 changelog.d/check-attachment-attribution.security diff --git a/changelog.d/check-attachment-attribution.security b/changelog.d/check-attachment-attribution.security new file mode 100644 index 000000000..e0e46525b --- /dev/null +++ b/changelog.d/check-attachment-attribution.security @@ -0,0 +1 @@ +CommonAPI: Prevent users from accessing media of other users by creating a status with reused attachment ID diff --git a/lib/pleroma/scheduled_activity.ex b/lib/pleroma/scheduled_activity.ex index a7be58512..0ed51ad07 100644 --- a/lib/pleroma/scheduled_activity.ex +++ b/lib/pleroma/scheduled_activity.ex @@ -40,7 +40,11 @@ defp with_media_attachments( %{changes: %{params: %{"media_ids" => media_ids} = params}} = changeset ) when is_list(media_ids) do - media_attachments = Utils.attachments_from_ids(%{media_ids: media_ids}) + media_attachments = + Utils.attachments_from_ids( + %{media_ids: media_ids}, + User.get_cached_by_id(changeset.data.user_id) + ) params = params diff --git a/lib/pleroma/web/common_api.ex b/lib/pleroma/web/common_api.ex index 89cc0d6fe..44eb00075 100644 --- a/lib/pleroma/web/common_api.ex +++ b/lib/pleroma/web/common_api.ex @@ -33,6 +33,7 @@ def block(blocker, blocked) do def post_chat_message(%User{} = user, %User{} = recipient, content, opts \\ []) do with maybe_attachment <- opts[:media_id] && Object.get_by_id(opts[:media_id]), + :ok <- validate_chat_attachment_attribution(maybe_attachment, user), :ok <- validate_chat_content_length(content, !!maybe_attachment), {_, {:ok, chat_message_data, _meta}} <- {:build_object, @@ -71,6 +72,17 @@ defp format_chat_content(content) do text end + defp validate_chat_attachment_attribution(nil, _), do: :ok + + defp validate_chat_attachment_attribution(attachment, user) do + with :ok <- Object.authorize_access(attachment, user) do + :ok + else + e -> + e + end + end + defp validate_chat_content_length(_, true), do: :ok defp validate_chat_content_length(nil, false), do: {:error, :no_content} diff --git a/lib/pleroma/web/common_api/activity_draft.ex b/lib/pleroma/web/common_api/activity_draft.ex index 9af635da8..63ed48a27 100644 --- a/lib/pleroma/web/common_api/activity_draft.ex +++ b/lib/pleroma/web/common_api/activity_draft.ex @@ -111,7 +111,7 @@ defp full_payload(%{status: status, summary: summary} = draft) do end defp attachments(%{params: params} = draft) do - attachments = Utils.attachments_from_ids(params) + attachments = Utils.attachments_from_ids(params, draft.user) draft = %__MODULE__{draft | attachments: attachments} case Utils.validate_attachments_count(attachments) do diff --git a/lib/pleroma/web/common_api/utils.ex b/lib/pleroma/web/common_api/utils.ex index ff0814329..6410815ea 100644 --- a/lib/pleroma/web/common_api/utils.ex +++ b/lib/pleroma/web/common_api/utils.ex @@ -23,21 +23,21 @@ defmodule Pleroma.Web.CommonAPI.Utils do require Logger require Pleroma.Constants - def attachments_from_ids(%{media_ids: ids, descriptions: desc}) do - attachments_from_ids_descs(ids, desc) + def attachments_from_ids(%{media_ids: ids, descriptions: desc}, user) do + attachments_from_ids_descs(ids, desc, user) end - def attachments_from_ids(%{media_ids: ids}) do - attachments_from_ids_no_descs(ids) + def attachments_from_ids(%{media_ids: ids}, user) do + attachments_from_ids_no_descs(ids, user) end - def attachments_from_ids(_), do: [] + def attachments_from_ids(_, _), do: [] - def attachments_from_ids_no_descs([]), do: [] + def attachments_from_ids_no_descs([], _), do: [] - def attachments_from_ids_no_descs(ids) do + def attachments_from_ids_no_descs(ids, user) do Enum.map(ids, fn media_id -> - case get_attachment(media_id) do + case get_attachment(media_id, user) do %Object{data: data} -> data _ -> nil end @@ -45,21 +45,26 @@ def attachments_from_ids_no_descs(ids) do |> Enum.reject(&is_nil/1) end - def attachments_from_ids_descs([], _), do: [] + def attachments_from_ids_descs([], _, _), do: [] - def attachments_from_ids_descs(ids, descs_str) do + def attachments_from_ids_descs(ids, descs_str, user) do {_, descs} = Jason.decode(descs_str) Enum.map(ids, fn media_id -> - with %Object{data: data} <- get_attachment(media_id) do + with %Object{data: data} <- get_attachment(media_id, user) do Map.put(data, "name", descs[media_id]) end end) |> Enum.reject(&is_nil/1) end - defp get_attachment(media_id) do - Repo.get(Object, media_id) + defp get_attachment(media_id, user) do + with %Object{data: _data} = object <- Repo.get(Object, media_id), + :ok <- Object.authorize_access(object, user) do + object + else + _ -> nil + end end @spec get_to_and_cc(ActivityDraft.t()) :: {list(String.t()), list(String.t())} diff --git a/test/pleroma/web/common_api/utils_test.exs b/test/pleroma/web/common_api/utils_test.exs index d309c6ded..c52d3e9c5 100644 --- a/test/pleroma/web/common_api/utils_test.exs +++ b/test/pleroma/web/common_api/utils_test.exs @@ -586,41 +586,56 @@ test "returns recipients when object not found" do end end - describe "attachments_from_ids_descs/2" do + describe "attachments_from_ids_descs/3" do test "returns [] when attachment ids is empty" do - assert Utils.attachments_from_ids_descs([], "{}") == [] + assert Utils.attachments_from_ids_descs([], "{}", nil) == [] end test "returns list attachments with desc" do - object = insert(:note) + user = insert(:user) + object = insert(:note, %{user: user}) desc = Jason.encode!(%{object.id => "test-desc"}) - assert Utils.attachments_from_ids_descs(["#{object.id}", "34"], desc) == [ + assert Utils.attachments_from_ids_descs(["#{object.id}", "34"], desc, user) == [ Map.merge(object.data, %{"name" => "test-desc"}) ] end end - describe "attachments_from_ids/1" do + describe "attachments_from_ids/2" do test "returns attachments with descs" do - object = insert(:note) + user = insert(:user) + object = insert(:note, %{user: user}) desc = Jason.encode!(%{object.id => "test-desc"}) - assert Utils.attachments_from_ids(%{ - media_ids: ["#{object.id}"], - descriptions: desc - }) == [ + assert Utils.attachments_from_ids( + %{ + media_ids: ["#{object.id}"], + descriptions: desc + }, + user + ) == [ Map.merge(object.data, %{"name" => "test-desc"}) ] end test "returns attachments without descs" do - object = insert(:note) - assert Utils.attachments_from_ids(%{media_ids: ["#{object.id}"]}) == [object.data] + user = insert(:user) + object = insert(:note, %{user: user}) + assert Utils.attachments_from_ids(%{media_ids: ["#{object.id}"]}, user) == [object.data] end test "returns [] when not pass media_ids" do - assert Utils.attachments_from_ids(%{}) == [] + assert Utils.attachments_from_ids(%{}, nil) == [] + end + + test "returns [] when media_ids not belong to current user" do + user = insert(:user) + user2 = insert(:user) + + object = insert(:attachment, %{user: user}) + + assert Utils.attachments_from_ids(%{media_ids: ["#{object.id}"]}, user2) == [] end end diff --git a/test/pleroma/web/common_api_test.exs b/test/pleroma/web/common_api_test.exs index 5c9103e9f..e60691995 100644 --- a/test/pleroma/web/common_api_test.exs +++ b/test/pleroma/web/common_api_test.exs @@ -279,6 +279,24 @@ test "it reject messages via MRF" do assert {:reject, "[KeywordPolicy] Matches with rejected keyword"} == CommonAPI.post_chat_message(author, recipient, "GNO/Linux") end + + test "it reject messages with attachments not belonging to user" do + author = insert(:user) + not_author = insert(:user) + recipient = author + + attachment = insert(:attachment, %{user: not_author}) + + {:error, message} = + CommonAPI.post_chat_message( + author, + recipient, + "123", + media_id: attachment.id + ) + + assert message == :forbidden + end end describe "unblocking" do diff --git a/test/pleroma/web/mastodon_api/views/scheduled_activity_view_test.exs b/test/pleroma/web/mastodon_api/views/scheduled_activity_view_test.exs index e5e510d33..07a65a3bc 100644 --- a/test/pleroma/web/mastodon_api/views/scheduled_activity_view_test.exs +++ b/test/pleroma/web/mastodon_api/views/scheduled_activity_view_test.exs @@ -48,7 +48,7 @@ test "A scheduled activity with a media attachment" do id: to_string(scheduled_activity.id), media_attachments: %{media_ids: [upload.id]} - |> Utils.attachments_from_ids() + |> Utils.attachments_from_ids(user) |> Enum.map(&StatusView.render("attachment.json", %{attachment: &1})), params: %{ in_reply_to_id: to_string(activity.id), diff --git a/test/pleroma/web/pleroma_api/views/chat_message_reference_view_test.exs b/test/pleroma/web/pleroma_api/views/chat_message_reference_view_test.exs index 017c9c5c0..7ab3f5acd 100644 --- a/test/pleroma/web/pleroma_api/views/chat_message_reference_view_test.exs +++ b/test/pleroma/web/pleroma_api/views/chat_message_reference_view_test.exs @@ -24,7 +24,7 @@ test "it displays a chat message" do filename: "an_image.jpg" } - {:ok, upload} = ActivityPub.upload(file, actor: user.ap_id) + {:ok, upload} = ActivityPub.upload(file, actor: recipient.ap_id) {:ok, activity} = CommonAPI.post_chat_message(user, recipient, "kippis :firefox:", idempotency_key: "123")