From 18a0c923d0da4c8fb6e33b383dabd1d06bb22968 Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Thu, 3 Aug 2023 13:08:37 -0400 Subject: [PATCH 01/14] Resolve information disclosure vulnerability through emoji pack archive download endpoint The pack name has been sanitized so an attacker cannot upload a media file called pack.json with their own handcrafted list of emoji files as arbitrary files on the filesystem and then call the emoji pack archive download endpoint with a pack name crafted to the location of the media file they uploaded which tricks Pleroma into generating a zip file of the target files the attacker wants to download. The attack only works if the Pleroma instance does not have the AnonymizeFilename upload filter enabled, which is currently the default. Reported by: graf@poast.org --- changelog.d/emoji-pack-sanitization.security | 1 + lib/pleroma/emoji/pack.ex | 1 + test/pleroma/emoji/pack_test.exs | 4 ++++ 3 files changed, 6 insertions(+) create mode 100644 changelog.d/emoji-pack-sanitization.security diff --git a/changelog.d/emoji-pack-sanitization.security b/changelog.d/emoji-pack-sanitization.security new file mode 100644 index 000000000..f3218abd4 --- /dev/null +++ b/changelog.d/emoji-pack-sanitization.security @@ -0,0 +1 @@ +Emoji pack loader sanitizes pack names diff --git a/lib/pleroma/emoji/pack.ex b/lib/pleroma/emoji/pack.ex index a361ea200..6e58f8898 100644 --- a/lib/pleroma/emoji/pack.ex +++ b/lib/pleroma/emoji/pack.ex @@ -285,6 +285,7 @@ def update_metadata(name, data) do @spec load_pack(String.t()) :: {:ok, t()} | {:error, :file.posix()} def load_pack(name) do + name = Path.basename(name) pack_file = Path.join([emoji_path(), name, "pack.json"]) with {:ok, _} <- File.stat(pack_file), diff --git a/test/pleroma/emoji/pack_test.exs b/test/pleroma/emoji/pack_test.exs index 18b99da75..00001abfc 100644 --- a/test/pleroma/emoji/pack_test.exs +++ b/test/pleroma/emoji/pack_test.exs @@ -90,4 +90,8 @@ test "add emoji file", %{pack: pack} do assert updated_pack.files_count == 1 end + + test "load_pack/1 ignores path traversal in a forged pack name", %{pack: pack} do + assert {:ok, ^pack} = Pack.load_pack("../../../../../dump_pack") + end end From 4befb3b1d02f32eb2c56f12e4684a7bb3167b0ee Mon Sep 17 00:00:00 2001 From: "Haelwenn (lanodan) Monnier" Date: Thu, 22 Jun 2023 00:46:52 +0200 Subject: [PATCH 02/14] Config: Restrict permissions of OTP config file --- lib/pleroma/config/release_runtime_provider.ex | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/lib/pleroma/config/release_runtime_provider.ex b/lib/pleroma/config/release_runtime_provider.ex index 91e5f1a54..9ec0f975e 100644 --- a/lib/pleroma/config/release_runtime_provider.ex +++ b/lib/pleroma/config/release_runtime_provider.ex @@ -20,6 +20,20 @@ def load(config, opts) do with_runtime_config = if File.exists?(config_path) do + # + %File.Stat{mode: mode} = File.lstat!(config_path) + + if Bitwise.band(mode, 0o007) > 0 do + raise "Configuration at #{config_path} has world-permissions, execute the following: chmod o= #{config_path}" + end + + if Bitwise.band(mode, 0o020) > 0 do + raise "Configuration at #{config_path} has group-wise write permissions, execute the following: chmod g-w #{config_path}" + end + + # Note: Elixir doesn't provides a getuid(2) + # so cannot forbid group-read only when config is owned by us + runtime_config = Config.Reader.read!(config_path) with_defaults From bd7381f2f4139c26d9dbb8aad77ce77be7777532 Mon Sep 17 00:00:00 2001 From: "Haelwenn (lanodan) Monnier" Date: Thu, 22 Jun 2023 00:58:05 +0200 Subject: [PATCH 03/14] instance gen: Reduce permissions of pleroma directories and config files --- lib/mix/tasks/pleroma/instance.ex | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/lib/mix/tasks/pleroma/instance.ex b/lib/mix/tasks/pleroma/instance.ex index 5c93f19ff..5d8b254a2 100644 --- a/lib/mix/tasks/pleroma/instance.ex +++ b/lib/mix/tasks/pleroma/instance.ex @@ -266,12 +266,20 @@ def run(["gen" | rest]) do config_dir = Path.dirname(config_path) psql_dir = Path.dirname(psql_path) + # Note: Distros requiring group read (0o750) on those directories should + # pre-create the directories. [config_dir, psql_dir, static_dir, uploads_dir] |> Enum.reject(&File.exists?/1) - |> Enum.map(&File.mkdir_p!/1) + |> Enum.each(fn dir -> + File.mkdir_p!(dir) + File.chmod!(dir, 0o700) + end) shell_info("Writing config to #{config_path}.") + # Sadly no fchmod(2) equivalent in Elixir… + File.touch!(config_path) + File.chmod!(config_path, 0o640) File.write(config_path, result_config) shell_info("Writing the postgres script to #{psql_path}.") File.write(psql_path, result_psql) @@ -290,8 +298,7 @@ def run(["gen" | rest]) do else shell_error( "The task would have overwritten the following files:\n" <> - (Enum.map(will_overwrite, &"- #{&1}\n") |> Enum.join("")) <> - "Rerun with `--force` to overwrite them." + Enum.map_join(will_overwrite, &"- #{&1}\n") <> "Rerun with `--force` to overwrite them." ) end end From 22df32b3f5cfe9fe0a4a97ff799df72c091b676e Mon Sep 17 00:00:00 2001 From: "Haelwenn (lanodan) Monnier" Date: Thu, 22 Jun 2023 01:00:25 +0200 Subject: [PATCH 04/14] changelog: Entry for config permissions restrictions Closes: https://git.pleroma.social/pleroma/pleroma/-/issues/3135 --- changelog.d/otp_perms.security | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/otp_perms.security diff --git a/changelog.d/otp_perms.security b/changelog.d/otp_perms.security new file mode 100644 index 000000000..a3da1c677 --- /dev/null +++ b/changelog.d/otp_perms.security @@ -0,0 +1 @@ +- Reduced permissions of config files and directories, distros requiring greater permissions like group-read need to pre-create the directories \ No newline at end of file From 76e408e42d1da123a955b85490f05f6d810172f9 Mon Sep 17 00:00:00 2001 From: "Haelwenn (lanodan) Monnier" Date: Fri, 4 Aug 2023 07:16:50 +0200 Subject: [PATCH 05/14] release_runtime_provider_test: chmod config for hardened permissions Git doesn't manages file permissions precisely enough for us. --- test/pleroma/config/release_runtime_provider_test.exs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/test/pleroma/config/release_runtime_provider_test.exs b/test/pleroma/config/release_runtime_provider_test.exs index 4e0d4c838..8ff578e63 100644 --- a/test/pleroma/config/release_runtime_provider_test.exs +++ b/test/pleroma/config/release_runtime_provider_test.exs @@ -17,6 +17,8 @@ test "loads release defaults config and warns about non-existent runtime config" end test "merged runtime config" do + assert :ok == File.chmod!("test/fixtures/config/temp.secret.exs", 0o640) + merged = ReleaseRuntimeProvider.load([], config_path: "test/fixtures/config/temp.secret.exs") @@ -25,6 +27,8 @@ test "merged runtime config" do end test "merged exported config" do + assert :ok == File.chmod!("test/fixtures/config/temp.exported_from_db.secret.exs", 0o640) + ExUnit.CaptureIO.capture_io(fn -> merged = ReleaseRuntimeProvider.load([], @@ -37,6 +41,9 @@ test "merged exported config" do end test "runtime config is merged with exported config" do + assert :ok == File.chmod!("test/fixtures/config/temp.secret.exs", 0o640) + assert :ok == File.chmod!("test/fixtures/config/temp.exported_from_db.secret.exs", 0o640) + merged = ReleaseRuntimeProvider.load([], config_path: "test/fixtures/config/temp.secret.exs", From c37561214a803f9011d5ec6af8b8c07e547c19ff Mon Sep 17 00:00:00 2001 From: "Haelwenn (lanodan) Monnier" Date: Fri, 4 Aug 2023 06:49:19 +0200 Subject: [PATCH 06/14] Force the use of amd64 runners for jobs using ci-base --- .gitlab-ci.yml | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 8b0381d11..91e568a32 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -32,7 +32,13 @@ before_script: after_script: - rm -rf _build/*/lib/pleroma +.using-ci-base: + tags: + - amd64 + build: + extends: + - .using-ci-base stage: build only: changes: &build_changes_policy @@ -44,6 +50,8 @@ build: - mix compile --force spec-build: + extends: + - .using-ci-base stage: test only: changes: @@ -57,6 +65,8 @@ spec-build: - mix pleroma.openapi_spec spec.json benchmark: + extends: + - .using-ci-base stage: benchmark when: manual variables: @@ -71,6 +81,8 @@ benchmark: - mix pleroma.load_testing unit-testing: + extends: + - .using-ci-base stage: test only: changes: *build_changes_policy @@ -94,6 +106,8 @@ unit-testing: path: coverage.xml unit-testing-erratic: + extends: + - .using-ci-base stage: test retry: 2 allow_failure: true @@ -129,6 +143,8 @@ unit-testing-erratic: # - mix test --trace --only federated unit-testing-rum: + extends: + - .using-ci-base stage: test only: changes: *build_changes_policy @@ -162,6 +178,8 @@ lint: - mix format --check-formatted analysis: + extends: + - .using-ci-base stage: test only: changes: *build_changes_policy From 5ac2b7417d052a493b38ee05b393ae7c78e89484 Mon Sep 17 00:00:00 2001 From: "Haelwenn (lanodan) Monnier" Date: Fri, 4 Aug 2023 09:16:08 +0200 Subject: [PATCH 07/14] test: Fix warnings --- .../activity_pub/transmogrifier/emoji_react_handling_test.exs | 2 +- test/pleroma/web/mastodon_api/update_credentials_test.exs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/test/pleroma/web/activity_pub/transmogrifier/emoji_react_handling_test.exs b/test/pleroma/web/activity_pub/transmogrifier/emoji_react_handling_test.exs index 9d99df27c..83bf59c6f 100644 --- a/test/pleroma/web/activity_pub/transmogrifier/emoji_react_handling_test.exs +++ b/test/pleroma/web/activity_pub/transmogrifier/emoji_react_handling_test.exs @@ -65,7 +65,7 @@ test "it works for incoming unqualified emoji reactions" do object = Object.get_by_ap_id(data["object"]) assert object.data["reaction_count"] == 1 - assert match?([[emoji, _]], object.data["reactions"]) + assert match?([[^emoji, _]], object.data["reactions"]) end test "it reject invalid emoji reactions" do diff --git a/test/pleroma/web/mastodon_api/update_credentials_test.exs b/test/pleroma/web/mastodon_api/update_credentials_test.exs index 57fa0f047..40f79d103 100644 --- a/test/pleroma/web/mastodon_api/update_credentials_test.exs +++ b/test/pleroma/web/mastodon_api/update_credentials_test.exs @@ -375,7 +375,7 @@ test "updates the user's background, upload_limit, returns a HTTP 413", %{ "pleroma_background_image" => new_background_oversized }) - assert user_response = json_response_and_validate_schema(res, 413) + assert _user_response = json_response_and_validate_schema(res, 413) assert user.background == %{} clear_config([:instance, :upload_limit], upload_limit) From 57f74537486cf7f721679f125741de9008478b00 Mon Sep 17 00:00:00 2001 From: "Haelwenn (lanodan) Monnier" Date: Fri, 4 Aug 2023 05:13:28 +0200 Subject: [PATCH 08/14] Release 2.5.3 --- CHANGELOG.md | 6 ++++++ mix.exs | 2 +- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f6fc6aaee..468ec1012 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,12 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). ### Removed +## 2.5.3 + +### Security +- Emoji pack loader sanitizes pack names +- Reduced permissions of config files and directories, distros requiring greater permissions like group-read need to pre-create the directories + ## 2.5.2 ### Security diff --git a/mix.exs b/mix.exs index 79fd9c9ef..d1cdb151d 100644 --- a/mix.exs +++ b/mix.exs @@ -4,7 +4,7 @@ defmodule Pleroma.Mixfile do def project do [ app: :pleroma, - version: version("2.5.2"), + version: version("2.5.3"), elixir: "~> 1.11", elixirc_paths: elixirc_paths(Mix.env()), compilers: [:phoenix, :gettext] ++ Mix.compilers(), From fc10e07ffbc9d81c7a2ac38a3f9175f2edf2bd1f Mon Sep 17 00:00:00 2001 From: Mae Date: Fri, 4 Aug 2023 22:24:17 +0100 Subject: [PATCH 09/14] Prevent XML parser from loading external entities --- lib/pleroma/web/xml.ex | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/lib/pleroma/web/xml.ex b/lib/pleroma/web/xml.ex index b699446b0..380a80ab8 100644 --- a/lib/pleroma/web/xml.ex +++ b/lib/pleroma/web/xml.ex @@ -29,7 +29,10 @@ def parse_document(text) do {doc, _rest} = text |> :binary.bin_to_list() - |> :xmerl_scan.string(quiet: true) + |> :xmerl_scan.string( + quiet: true, + fetch_fun: fn _, _ -> raise "Resolving external entities not supported" end + ) {:ok, doc} rescue From 77d57c974ad83fcea77e424d53dc16a27e5d88b6 Mon Sep 17 00:00:00 2001 From: FloatingGhost Date: Fri, 4 Aug 2023 22:24:32 +0100 Subject: [PATCH 10/14] Add unit test for external entity loading --- test/fixtures/xml_external_entities.xml | 3 +++ test/pleroma/web/web_finger_test.exs | 23 +++++++++++++++++++++++ test/pleroma/web/xml_test.exs | 10 ++++++++++ 3 files changed, 36 insertions(+) create mode 100644 test/fixtures/xml_external_entities.xml create mode 100644 test/pleroma/web/xml_test.exs diff --git a/test/fixtures/xml_external_entities.xml b/test/fixtures/xml_external_entities.xml new file mode 100644 index 000000000..d5ff87134 --- /dev/null +++ b/test/fixtures/xml_external_entities.xml @@ -0,0 +1,3 @@ + + ]> +&xxe; diff --git a/test/pleroma/web/web_finger_test.exs b/test/pleroma/web/web_finger_test.exs index fafef54fe..be5e08776 100644 --- a/test/pleroma/web/web_finger_test.exs +++ b/test/pleroma/web/web_finger_test.exs @@ -180,5 +180,28 @@ test "respects xml content-type" do {:ok, _data} = WebFinger.finger("pekorino@pawoo.net") end + + test "refuses to process XML remote entities" do + Tesla.Mock.mock(fn + %{ + url: "https://pawoo.net/.well-known/webfinger?resource=acct:pekorino@pawoo.net" + } -> + {:ok, + %Tesla.Env{ + status: 200, + body: File.read!("test/fixtures/xml_external_entities.xml"), + headers: [{"content-type", "application/xrd+xml"}] + }} + + %{url: "https://pawoo.net/.well-known/host-meta"} -> + {:ok, + %Tesla.Env{ + status: 200, + body: File.read!("test/fixtures/tesla_mock/pawoo.net_host_meta") + }} + end) + + assert :error = WebFinger.finger("pekorino@pawoo.net") + end end end diff --git a/test/pleroma/web/xml_test.exs b/test/pleroma/web/xml_test.exs new file mode 100644 index 000000000..89d4709b6 --- /dev/null +++ b/test/pleroma/web/xml_test.exs @@ -0,0 +1,10 @@ +defmodule Pleroma.Web.XMLTest do + use Pleroma.DataCase, async: true + + alias Pleroma.Web.XML + + test "refuses to load external entities from XML" do + data = File.read!("test/fixtures/xml_external_entities.xml") + assert(:error == XML.parse_document(data)) + end +end From cc848b78dca51fcd7e785eb92a7a3a4d5d1c419e Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Fri, 4 Aug 2023 22:44:09 -0400 Subject: [PATCH 11/14] Document and test that XXE processing is disabled https://vuln.be/post/xxe-in-erlang-and-elixir/ --- changelog.d/akkoma-xml-remote-entities.security | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/akkoma-xml-remote-entities.security diff --git a/changelog.d/akkoma-xml-remote-entities.security b/changelog.d/akkoma-xml-remote-entities.security new file mode 100644 index 000000000..b3c86bee1 --- /dev/null +++ b/changelog.d/akkoma-xml-remote-entities.security @@ -0,0 +1 @@ +Restrict XML parser from processing external entitites (XXE) From b631180b38ac63029f08bef137b13231bcf57b59 Mon Sep 17 00:00:00 2001 From: "Haelwenn (lanodan) Monnier" Date: Sat, 5 Aug 2023 08:27:42 +0200 Subject: [PATCH 12/14] Release 2.5.4 --- CHANGELOG.md | 5 +++++ changelog.d/akkoma-xml-remote-entities.security | 2 +- mix.exs | 2 +- 3 files changed, 7 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 468ec1012..9d9aadc6e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,11 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). ### Removed +## 2.5.54 + +## Security +- Fix XML External Entity (XXE) loading vulnerability allowing to fetch arbitary files from the server's filesystem + ## 2.5.3 ### Security diff --git a/changelog.d/akkoma-xml-remote-entities.security b/changelog.d/akkoma-xml-remote-entities.security index b3c86bee1..5e6725e5b 100644 --- a/changelog.d/akkoma-xml-remote-entities.security +++ b/changelog.d/akkoma-xml-remote-entities.security @@ -1 +1 @@ -Restrict XML parser from processing external entitites (XXE) +Fix XML External Entity (XXE) loading vulnerability allowing to fetch arbitary files from the server's filesystem diff --git a/mix.exs b/mix.exs index d1cdb151d..12f721364 100644 --- a/mix.exs +++ b/mix.exs @@ -4,7 +4,7 @@ defmodule Pleroma.Mixfile do def project do [ app: :pleroma, - version: version("2.5.3"), + version: version("2.5.4"), elixir: "~> 1.11", elixirc_paths: elixirc_paths(Mix.env()), compilers: [:phoenix, :gettext] ++ Mix.compilers(), From 535a5ecad04c9c49105a77e7025fe9f4b4d23ba6 Mon Sep 17 00:00:00 2001 From: Mint Date: Sat, 2 Sep 2023 01:43:25 +0300 Subject: [PATCH 13/14] 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") From 385492577d11e9667064d7f7e0dacdc00457064a Mon Sep 17 00:00:00 2001 From: "Haelwenn (lanodan) Monnier" Date: Fri, 23 Dec 2022 18:46:14 +0100 Subject: [PATCH 14/14] mix: version 2.5.5 --- CHANGELOG.md | 7 ++++++- mix.exs | 2 +- 2 files changed, 7 insertions(+), 2 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/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(),