### 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
%{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
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,
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}
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
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
|> 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())}
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(),
--- /dev/null
+diff --git a/CHANGELOG.md b/CHANGELOG.md
+index 9d9aadc6e8a0162d8944622f783a6301fefd6cfa..32ec440de55b707d01be37e21f1517542c9cf7d9 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 0000000000000000000000000000000000000000..e0e46525b38d15e58423debbf9d799e85276d0ca
+--- /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 a7be585123d4be67e7513626b07c2e33c8b18388..0ed51ad07d43c0cc434e532852ce6a7ec1089d10 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 89cc0d6fe82acfd418e8e5928bc4b44d72b17f79..44eb00075ee694005241c1eabfbb846e4668d665 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 9af635da8abafb9d9a4c1b59c00cc63261955d48..63ed48a27bbd4a4f2fdfd97776acadcf0fa72c74 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 ff0814329583726d43ad52e6082662aee1212b45..6410815ea89153a65f20bacc24a5a5aed82a1aa0 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/mix.exs b/mix.exs
+index 12f721364dd75744651e5044936d195684d8cf08..e2aac0fc541f2820841a16feff06b058b18ed2ec 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 d309c6deda789fd7818b29e95425b77413b06f7c..c52d3e9c557b6040bb457f40d9ebd02c31975a64 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 5c9103e9fc742b8082a432d8ac9ca2ce1929ec32..e60691995c51bdd829f0f67a3abbddc9fe919db9 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 e5e510d33b88244b9b088ed93b98bfb2c411dd8b..07a65a3bc1bd09ece6afa2557a719b7550dfae2a 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 017c9c5c0d2fdf4686d920ea7f05084a6ddf44d7..7ab3f5acdc7971f9ac324db07be30dbff60d680a 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")
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
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
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),
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")
projects / anni / commitdiff
