From 0c2b33458d1cccabfc4b5f0db53f963ea673ecb9 Mon Sep 17 00:00:00 2001 From: Oneric Date: Wed, 24 Apr 2024 17:46:18 +0200 Subject: [PATCH] Restrict media usage to owners MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In Mastodon media can only be used by owners and only be associated with a single post. We currently allow media to be associated with several posts and until now did not limit their usage in posts to media owners. However, media update and GET lookup was already limited to owners. (In accordance with allowing media reuse, we also still allow GET lookups of media already used in a post unlike Mastodon) Allowing reuse isn’t problematic per se, but allowing use by non-owners can be problematic if media ids of private-scoped posts can be guessed since creating a new post with this media id will reveal the uploaded file content and alt text. Given media ids are currently just part of a sequentieal series shared with some other objects, guessing media ids is with some persistence indeed feasible. E.g. sampline some public media ids from a real-world instance with 112 total and 61 monthly-active users: 17.465.096 at t0 17.472.673 at t1 = t0 + 4h 17.473.248 at t2 = t1 + 20min This gives about 30 new ids per minute of which most won't be local media but remote and local posts, poll answers etc. Assuming the default ratelimit of 15 post actions per 10s, scraping all media for the 4h interval takes about 84 minutes and scraping the 20min range mere 6.3 minutes. (Until the preceding commit, post updates were not rate limited at all, allowing even faster scraping.) If an attacker can infer (e.g. via reply to a follower-only post not accessbile to the attacker) some sensitive information was uploaded during a specific time interval and has some pointers regarding the nature of the information, identifying the specific upload out of all scraped media for this timerange is not impossible. Thus restrict media usage to owners. Checking ownership just in ActivitDraft would already be sufficient, since when a scheduled status actually gets posted it goes through ActivityDraft again, but would erroneously return a success status when scheduling an illegal post. Independently discovered and fixed by mint in Pleroma https://git.pleroma.social/pleroma/pleroma/-/commit/1afde067b12ad0062c1820091ea9b0a680819281 --- CHANGELOG.md | 3 + lib/pleroma/scheduled_activity.ex | 43 +++++++++---- lib/pleroma/web/common_api/activity_draft.ex | 11 +++- lib/pleroma/web/common_api/utils.ex | 26 ++++---- test/pleroma/web/common_api/utils_test.exs | 8 ++- .../controllers/status_controller_test.exs | 64 +++++++++++++++++++ .../views/scheduled_activity_view_test.exs | 3 +- 7 files changed, 125 insertions(+), 33 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2284f5c8d..48e487bf1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,9 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). ## Unreleased +## Fixed +- Issue allowing non-owners to use media objects in posts + ## 2024.04 ## Added diff --git a/lib/pleroma/scheduled_activity.ex b/lib/pleroma/scheduled_activity.ex index 3e1c51abb..5292b1491 100644 --- a/lib/pleroma/scheduled_activity.ex +++ b/lib/pleroma/scheduled_activity.ex @@ -40,19 +40,29 @@ defmodule Pleroma.ScheduledActivity do %{changes: %{params: %{"media_ids" => media_ids} = params}} = changeset ) when is_list(media_ids) do - media_attachments = Utils.attachments_from_ids(%{media_ids: media_ids}) + user = User.get_by_id(changeset.data.user_id) - params = - params - |> Map.put("media_attachments", media_attachments) - |> Map.put("media_ids", media_ids) + case Utils.attachments_from_ids(user, %{media_ids: media_ids}) do + media_attachments when is_list(media_attachments) -> + params = + params + |> Map.put("media_attachments", media_attachments) + |> Map.put("media_ids", media_ids) - put_change(changeset, :params, params) + put_change(changeset, :params, params) + + {:error, _} = e -> + e + + e -> + {:error, e} + end end defp with_media_attachments(changeset), do: changeset defp update_changeset(%ScheduledActivity{} = scheduled_activity, attrs) do + # note: should this ever allow swapping media attachments, make sure ownership is checked scheduled_activity |> cast(attrs, [:scheduled_at]) |> validate_required([:scheduled_at]) @@ -115,13 +125,22 @@ defmodule Pleroma.ScheduledActivity do @doc """ Creates ScheduledActivity and add to queue to perform at scheduled_at date """ - @spec create(User.t(), map()) :: {:ok, ScheduledActivity.t()} | {:error, Ecto.Changeset.t()} + @spec create(User.t(), map()) :: {:ok, ScheduledActivity.t()} | {:error, any()} def create(%User{} = user, attrs) do - Multi.new() - |> Multi.insert(:scheduled_activity, new(user, attrs)) - |> maybe_add_jobs(Config.get([ScheduledActivity, :enabled])) - |> Repo.transaction() - |> transaction_response + case new(user, attrs) do + %Ecto.Changeset{} = sched_data -> + Multi.new() + |> Multi.insert(:scheduled_activity, sched_data) + |> maybe_add_jobs(Config.get([ScheduledActivity, :enabled])) + |> Repo.transaction() + |> transaction_response + + {:error, _} = e -> + e + + e -> + {:error, e} + end end defp maybe_add_jobs(multi, true) do diff --git a/lib/pleroma/web/common_api/activity_draft.ex b/lib/pleroma/web/common_api/activity_draft.ex index 4555efd61..2d24edec4 100644 --- a/lib/pleroma/web/common_api/activity_draft.ex +++ b/lib/pleroma/web/common_api/activity_draft.ex @@ -92,9 +92,14 @@ defmodule Pleroma.Web.CommonAPI.ActivityDraft do end end - defp attachments(%{params: params} = draft) do - attachments = Utils.attachments_from_ids(params) - %__MODULE__{draft | attachments: attachments} + defp attachments(%{params: params, user: user} = draft) do + case Utils.attachments_from_ids(user, params) do + attachments when is_list(attachments) -> + %__MODULE__{draft | attachments: attachments} + + {:error, reason} -> + add_error(draft, reason) + end end defp in_reply_to(%{params: %{in_reply_to_status_id: ""}} = draft), do: draft diff --git a/lib/pleroma/web/common_api/utils.ex b/lib/pleroma/web/common_api/utils.ex index d80109a98..6d2113100 100644 --- a/lib/pleroma/web/common_api/utils.ex +++ b/lib/pleroma/web/common_api/utils.ex @@ -22,24 +22,24 @@ defmodule Pleroma.Web.CommonAPI.Utils do require Logger require Pleroma.Constants - def attachments_from_ids(%{media_ids: ids}) do - attachments_from_ids(ids) + def attachments_from_ids(user, %{media_ids: ids}) do + attachments_from_ids(user, ids, []) end - def attachments_from_ids([]), do: [] + def attachments_from_ids(_, _), do: [] - def attachments_from_ids(ids) when is_list(ids) do - Enum.map(ids, fn media_id -> - case get_attachment(media_id) do - %Object{data: data} -> data - _ -> nil - end - end) - |> Enum.reject(&is_nil/1) + defp attachments_from_ids(_user, [], acc), do: Enum.reverse(acc) + + defp attachments_from_ids(user, [media_id | ids], acc) do + with {_, %Object{} = object} <- {:get, get_attachment(media_id)}, + :ok <- Object.authorize_access(object, user) do + attachments_from_ids(user, ids, [object.data | acc]) + else + {:get, _} -> attachments_from_ids(user, ids, acc) + {:error, reason} -> {:error, reason} + end end - def attachments_from_ids(_), do: [] - defp get_attachment(media_id) do Repo.get(Object, media_id) end diff --git a/test/pleroma/web/common_api/utils_test.exs b/test/pleroma/web/common_api/utils_test.exs index d98eb76ad..10abb89e5 100644 --- a/test/pleroma/web/common_api/utils_test.exs +++ b/test/pleroma/web/common_api/utils_test.exs @@ -592,12 +592,14 @@ defmodule Pleroma.Web.CommonAPI.UtilsTest do describe "attachments_from_ids/1" do 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(user, %{media_ids: ["#{object.id}"]}) == [object.data] end test "returns [] when not pass media_ids" do - assert Utils.attachments_from_ids(%{}) == [] + user = insert(:user) + assert Utils.attachments_from_ids(user, %{}) == [] end end diff --git a/test/pleroma/web/mastodon_api/controllers/status_controller_test.exs b/test/pleroma/web/mastodon_api/controllers/status_controller_test.exs index f58045640..a15fd42fc 100644 --- a/test/pleroma/web/mastodon_api/controllers/status_controller_test.exs +++ b/test/pleroma/web/mastodon_api/controllers/status_controller_test.exs @@ -220,6 +220,28 @@ defmodule Pleroma.Web.MastodonAPI.StatusControllerTest do assert json_response_and_validate_schema(conn, 200) end + test "refuses to post non-owned media", %{conn: conn} do + other_user = insert(:user) + + file = %Plug.Upload{ + content_type: "image/jpeg", + path: Path.absname("test/fixtures/image.jpg"), + filename: "an_image.jpg" + } + + {:ok, upload} = ActivityPub.upload(file, actor: other_user.ap_id) + + conn = + conn + |> put_req_header("content-type", "application/json") + |> post("/api/v1/statuses", %{ + "status" => "mew", + "media_ids" => [to_string(upload.id)] + }) + + assert json_response(conn, 422) == %{"error" => "forbidden"} + end + test "posting a status with an invalid language", %{conn: conn} do conn = conn @@ -569,6 +591,29 @@ defmodule Pleroma.Web.MastodonAPI.StatusControllerTest do assert %{"type" => "image"} = media_attachment end + test "refuses to schedule post with non-owned media", %{conn: conn} do + other_user = insert(:user) + + file = %Plug.Upload{ + content_type: "image/jpeg", + path: Path.absname("test/fixtures/image.jpg"), + filename: "an_image.jpg" + } + + {:ok, upload} = ActivityPub.upload(file, actor: other_user.ap_id) + + conn = + conn + |> put_req_header("content-type", "application/json") + |> post("/api/v1/statuses", %{ + "status" => "mew", + "scheduled_at" => DateTime.add(DateTime.utc_now(), 6, :minute), + "media_ids" => [to_string(upload.id)] + }) + + assert json_response(conn, 403) == %{"error" => "Access denied"} + end + test "skips the scheduling and creates the activity if scheduled_at is earlier than 5 minutes from now", %{conn: conn} do scheduled_at = @@ -2406,6 +2451,25 @@ defmodule Pleroma.Web.MastodonAPI.StatusControllerTest do assert [%{"id" => ^attachment_id}] = response["media_attachments"] end + test "it does not update to non-owned attachments", %{conn: conn, user: user} do + other_user = insert(:user) + attachment = insert(:attachment, user: other_user) + attachment_id = to_string(attachment.id) + + {:ok, activity} = CommonAPI.post(user, %{status: "mew mew #abc", spoiler_text: "#def"}) + + conn = + conn + |> put_req_header("content-type", "application/json") + |> put("/api/v1/statuses/#{activity.id}", %{ + "status" => "mew mew #abc", + "spoiler_text" => "#def", + "media_ids" => [attachment_id] + }) + + assert json_response(conn, 400) == %{"error" => "internal_server_error"} + end + test "it does not update visibility", %{conn: conn, user: user} do {:ok, activity} = CommonAPI.post(user, %{ 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 e323f3a1f..cc893f94e 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 @@ -47,8 +47,7 @@ defmodule Pleroma.Web.MastodonAPI.ScheduledActivityViewTest do expected = %{ id: to_string(scheduled_activity.id), media_attachments: - %{media_ids: [upload.id]} - |> Utils.attachments_from_ids() + Utils.attachments_from_ids(user, %{media_ids: [upload.id]}) |> Enum.map(&StatusView.render("attachment.json", %{attachment: &1})), params: %{ in_reply_to_id: to_string(activity.id),