Merge branch 'release/2.5.5' into 'stable'v2.5.5

Release 2.5.5

See merge request pleroma/pleroma!3949

11 files changed, 92 insertions, 32 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 9d9aadc6e..32ec440de 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -14,7 +14,12 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
 
 ### Removed
 
-## 2.5.54
+## 2.5.5
+
+## Security
+- Prevent users from accessing media of other users by creating a status with reused attachment ID
+
+## 2.5.4
 
 ## Security
 - Fix XML External Entity (XXE) loading vulnerability allowing to fetch arbitary files from the server's filesystem
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 @@ 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})
+    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 @@ defmodule Pleroma.Web.CommonAPI 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 @@ defmodule Pleroma.Web.CommonAPI 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 @@ defmodule Pleroma.Web.CommonAPI.ActivityDraft 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 @@ defmodule Pleroma.Web.CommonAPI.Utils 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/mix.exs b/mix.exs
index 12f721364..e2aac0fc5 100644
--- a/mix.exs
+++ b/mix.exs
@@ -4,7 +4,7 @@ defmodule Pleroma.Mixfile do
   def project do
     [
       app: :pleroma,
-      version: version("2.5.4"),
+      version: version("2.5.5"),
       elixir: "~> 1.11",
       elixirc_paths: elixirc_paths(Mix.env()),
       compilers: [:phoenix, :gettext] ++ Mix.compilers(),
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 @@ defmodule Pleroma.Web.CommonAPI.UtilsTest 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 @@ defmodule Pleroma.Web.CommonAPITest 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 @@ defmodule Pleroma.Web.MastodonAPI.ScheduledActivityViewTest 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 @@ defmodule Pleroma.Web.PleromaAPI.ChatMessageReferenceViewTest 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")