From 0ec62acb9dc1c6500033086b46c37adefb700c62 Mon Sep 17 00:00:00 2001 From: Oneric Date: Mon, 4 Mar 2024 18:39:08 +0100 Subject: [PATCH] Always insert Dedupe upload filter This actually was already intended before to eradict all future path-traversal-style exploits and to fix issues with some characters like akkoma#610 in 0b2ec0ccee. However, Dedupe and AnonymizeFilename got mixed up. The latter only anonymises the name in Content-Disposition headers GET parameters (with link_name), _not_ the upload path. Even without Dedupe, the upload path is prefixed by an UUID, so it _should_ already be hard to guess for attackers. But now we actually can be sure no path shenanigangs occur, uploads reliably work and save some disk space. While this makes the final path predictable, this prediction is not exploitable. Insertion of a back-reference to the upload itself requires pulling off a successfull preimage attack against SHA-256, which is deemed infeasible for the foreseeable futures. Dedupe was already included in the default list in config.exs since 28cfb2c37a, but this will get overridde by whatever the config generated by the "pleroma.instance gen" task chose. Upload+delete tests running in parallel using Dedupe might be flaky, but this was already true before and needs its own commit to fix eventually. --- CHANGELOG.md | 2 + config/config.exs | 2 +- docs/docs/configuration/cheatsheet.md | 17 +++--- lib/mix/tasks/pleroma/instance.ex | 21 +------ lib/pleroma/upload.ex | 2 +- test/mix/tasks/pleroma/instance_test.exs | 2 - test/mix/tasks/pleroma/uploads_test.exs | 1 - test/pleroma/object_test.exs | 55 ++++++++++--------- test/pleroma/upload_test.exs | 21 +------ .../web/activity_pub/activity_pub_test.exs | 8 --- .../controllers/mascot_controller_test.exs | 6 +- test/test_helper.exs | 4 ++ 12 files changed, 55 insertions(+), 86 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1efe1ecfc..9130a81ae 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). ## Changed - `Pleroma.Upload, :base_url` now MUST be configured explicitly; use of the same domain as the instance is **strongly** discouraged +- The `Dedupe` upload filter is now always active; + `AnonymizeFilenames` is again opt-in ## Fixed - Critical security issue allowing Akkoma to be used as a vector for diff --git a/config/config.exs b/config/config.exs index 723d173ec..a366961c0 100644 --- a/config/config.exs +++ b/config/config.exs @@ -61,7 +61,7 @@ # Upload configuration config :pleroma, Pleroma.Upload, uploader: Pleroma.Uploaders.Local, - filters: [Pleroma.Upload.Filter.Dedupe], + filters: [], link_name: false, proxy_remote: false, filename_display_max_length: 30, diff --git a/docs/docs/configuration/cheatsheet.md b/docs/docs/configuration/cheatsheet.md index a04160a1d..9c5bb9901 100644 --- a/docs/docs/configuration/cheatsheet.md +++ b/docs/docs/configuration/cheatsheet.md @@ -597,7 +597,7 @@ the source code is here: [kocaptcha](https://github.com/koto-bank/kocaptcha). Th * `uploader`: Which one of the [uploaders](#uploaders) to use. * `filters`: List of [upload filters](#upload-filters) to use. -* `link_name`: When enabled Akkoma will add a `name` parameter to the url of the upload, for example `https://instance.tld/media/corndog.png?name=corndog.png`. This is needed to provide the correct filename in Content-Disposition headers when using filters like `Pleroma.Upload.Filter.Dedupe` +* `link_name`: When enabled Akkoma will add a `name` parameter to the url of the upload, for example `https://instance.tld/media/corndog.png?name=corndog.png`. This is needed to provide the correct filename in Content-Disposition headers * `base_url`: The base URL to access a user-uploaded file; MUST be configured explicitly. Using a (sub)domain distinct from the instance endpoint is **strongly** recommended. * `proxy_remote`: If you're using a remote uploader, Akkoma will proxy media requests instead of redirecting to it. @@ -639,17 +639,18 @@ config :ex_aws, :s3, ### Upload filters -#### Pleroma.Upload.Filter.AnonymizeFilename - -This filter replaces the filename (not the path) of an upload. For complete obfuscation, add -`Pleroma.Upload.Filter.Dedupe` before AnonymizeFilename. - -* `text`: Text to replace filenames in links. If empty, `{random}.extension` will be used. You can get the original filename extension by using `{extension}`, for example `custom-file-name.{extension}`. - #### Pleroma.Upload.Filter.Dedupe +**Always** active; cannot be turned off. +Renames files to their hash and prevents duplicate files filling up the disk. No specific configuration. +#### Pleroma.Upload.Filter.AnonymizeFilename + +This filter replaces the declared filename (not the path) of an upload. + +* `text`: Text to replace filenames in links. If empty, `{random}.extension` will be used. You can get the original filename extension by using `{extension}`, for example `custom-file-name.{extension}`. + #### Pleroma.Upload.Filter.Exiftool This filter only strips the GPS and location metadata with Exiftool leaving color profiles and attributes intact. diff --git a/lib/mix/tasks/pleroma/instance.ex b/lib/mix/tasks/pleroma/instance.ex index b442fdb5b..44f6b6e70 100644 --- a/lib/mix/tasks/pleroma/instance.ex +++ b/lib/mix/tasks/pleroma/instance.ex @@ -36,8 +36,7 @@ def run(["gen" | rest]) do listen_ip: :string, listen_port: :string, strip_uploads: :string, - anonymize_uploads: :string, - dedupe_uploads: :string + anonymize_uploads: :string ], aliases: [ o: :output, @@ -195,14 +194,6 @@ def run(["gen" | rest]) do "n" ) === "y" - dedupe_uploads = - get_option( - options, - :dedupe_uploads, - "Do you want to deduplicate uploaded files? (y/n)", - "n" - ) === "y" - Config.put([:instance, :static_dir], static_dir) secret = :crypto.strong_rand_bytes(64) |> Base.encode64() |> binary_part(0, 64) @@ -240,8 +231,7 @@ def run(["gen" | rest]) do upload_filters: upload_filters(%{ strip: strip_uploads, - anonymize: anonymize_uploads, - dedupe: dedupe_uploads + anonymize: anonymize_uploads }) ) @@ -329,13 +319,6 @@ defp upload_filters(filters) when is_map(filters) do enabled_filters end - enabled_filters = - if filters.dedupe do - enabled_filters ++ [Pleroma.Upload.Filter.Dedupe] - else - enabled_filters - end - enabled_filters end end diff --git a/lib/pleroma/upload.ex b/lib/pleroma/upload.ex index 974d12533..1158e9449 100644 --- a/lib/pleroma/upload.ex +++ b/lib/pleroma/upload.ex @@ -66,7 +66,7 @@ defmodule Pleroma.Upload do path: String.t() } - @always_enabled_filters [Pleroma.Upload.Filter.AnonymizeFilename] + @always_enabled_filters [Pleroma.Upload.Filter.Dedupe] defstruct [:id, :name, :tempfile, :content_type, :width, :height, :blurhash, :path] diff --git a/test/mix/tasks/pleroma/instance_test.exs b/test/mix/tasks/pleroma/instance_test.exs index 522319371..17b2e3267 100644 --- a/test/mix/tasks/pleroma/instance_test.exs +++ b/test/mix/tasks/pleroma/instance_test.exs @@ -71,8 +71,6 @@ test "running gen" do "./test/../test/instance/static/", "--strip-uploads", "y", - "--dedupe-uploads", - "n", "--anonymize-uploads", "n" ]) diff --git a/test/mix/tasks/pleroma/uploads_test.exs b/test/mix/tasks/pleroma/uploads_test.exs index 67fb642c1..d00e25a37 100644 --- a/test/mix/tasks/pleroma/uploads_test.exs +++ b/test/mix/tasks/pleroma/uploads_test.exs @@ -16,7 +16,6 @@ defmodule Mix.Tasks.Pleroma.UploadsTest do Mix.shell(Mix.Shell.IO) end) - File.mkdir_p!("test/uploads") :ok end diff --git a/test/pleroma/object_test.exs b/test/pleroma/object_test.exs index 8320660a5..4b0fec1bd 100644 --- a/test/pleroma/object_test.exs +++ b/test/pleroma/object_test.exs @@ -22,6 +22,13 @@ defmodule Pleroma.ObjectTest do :ok end + # Only works for a single attachment but that's all we need here + defp get_attachment_filepath(note, uploads_dir) do + %{data: %{"attachment" => [%{"url" => [%{"href" => href}]}]}} = note + filename = href |> Path.basename() + "#{uploads_dir}/#{filename}" + end + test "returns an object by it's AP id" do object = insert(:note) found_object = Object.get_by_ap_id(object.data["id"]) @@ -95,14 +102,13 @@ test "Disabled via config" do {:ok, %Object{} = attachment} = Pleroma.Web.ActivityPub.ActivityPub.upload(file, actor: user.ap_id) - %{data: %{"attachment" => [%{"url" => [%{"href" => href}]}]}} = - note = insert(:note, %{user: user, data: %{"attachment" => [attachment.data]}}) + note = insert(:note, %{user: user, data: %{"attachment" => [attachment.data]}}) uploads_dir = Pleroma.Config.get!([Pleroma.Uploaders.Local, :uploads]) - path = href |> Path.dirname() |> Path.basename() + path = get_attachment_filepath(note, uploads_dir) - assert {:ok, ["an_image.jpg"]} == File.ls("#{uploads_dir}/#{path}") + assert File.exists?("#{path}") Object.delete(note) @@ -111,7 +117,7 @@ test "Disabled via config" do assert Object.get_by_id(note.id).data["deleted"] refute Object.get_by_id(attachment.id) == nil - assert {:ok, ["an_image.jpg"]} == File.ls("#{uploads_dir}/#{path}") + assert File.exists?("#{path}") end test "in subdirectories" do @@ -129,14 +135,13 @@ test "in subdirectories" do {:ok, %Object{} = attachment} = Pleroma.Web.ActivityPub.ActivityPub.upload(file, actor: user.ap_id) - %{data: %{"attachment" => [%{"url" => [%{"href" => href}]}]}} = - note = insert(:note, %{user: user, data: %{"attachment" => [attachment.data]}}) + note = insert(:note, %{user: user, data: %{"attachment" => [attachment.data]}}) uploads_dir = Pleroma.Config.get!([Pleroma.Uploaders.Local, :uploads]) - path = href |> Path.dirname() |> Path.basename() + path = get_attachment_filepath(note, uploads_dir) - assert {:ok, ["an_image.jpg"]} == File.ls("#{uploads_dir}/#{path}") + assert File.exists?("#{path}") Object.delete(note) @@ -145,7 +150,7 @@ test "in subdirectories" do assert Object.get_by_id(note.id).data["deleted"] assert Object.get_by_id(attachment.id) == nil - assert {:ok, []} == File.ls("#{uploads_dir}/#{path}") + refute File.exists?("#{path}") end test "with dedupe enabled" do @@ -168,13 +173,11 @@ test "with dedupe enabled" do {:ok, %Object{} = attachment} = Pleroma.Web.ActivityPub.ActivityPub.upload(file, actor: user.ap_id) - %{data: %{"attachment" => [%{"url" => [%{"href" => href}]}]}} = - note = insert(:note, %{user: user, data: %{"attachment" => [attachment.data]}}) + note = insert(:note, %{user: user, data: %{"attachment" => [attachment.data]}}) - filename = Path.basename(href) + path = get_attachment_filepath(note, uploads_dir) - assert {:ok, files} = File.ls(uploads_dir) - assert filename in files + assert File.exists?("#{path}") Object.delete(note) @@ -182,8 +185,8 @@ test "with dedupe enabled" do assert Object.get_by_id(note.id).data["deleted"] assert Object.get_by_id(attachment.id) == nil - assert {:ok, files} = File.ls(uploads_dir) - refute filename in files + # what if another test runs concurrently using the same image file? + refute File.exists?("#{path}") end test "with objects that have legacy data.url attribute" do @@ -203,14 +206,13 @@ test "with objects that have legacy data.url attribute" do {:ok, %Object{}} = Object.create(%{url: "https://google.com", actor: user.ap_id}) - %{data: %{"attachment" => [%{"url" => [%{"href" => href}]}]}} = - note = insert(:note, %{user: user, data: %{"attachment" => [attachment.data]}}) + note = insert(:note, %{user: user, data: %{"attachment" => [attachment.data]}}) uploads_dir = Pleroma.Config.get!([Pleroma.Uploaders.Local, :uploads]) - path = href |> Path.dirname() |> Path.basename() + path = get_attachment_filepath(note, uploads_dir) - assert {:ok, ["an_image.jpg"]} == File.ls("#{uploads_dir}/#{path}") + assert File.exists?("#{path}") Object.delete(note) @@ -219,7 +221,7 @@ test "with objects that have legacy data.url attribute" do assert Object.get_by_id(note.id).data["deleted"] assert Object.get_by_id(attachment.id) == nil - assert {:ok, []} == File.ls("#{uploads_dir}/#{path}") + refute File.exists?("#{path}") end test "With custom base_url" do @@ -238,14 +240,13 @@ test "With custom base_url" do {:ok, %Object{} = attachment} = Pleroma.Web.ActivityPub.ActivityPub.upload(file, actor: user.ap_id) - %{data: %{"attachment" => [%{"url" => [%{"href" => href}]}]}} = - note = insert(:note, %{user: user, data: %{"attachment" => [attachment.data]}}) + note = insert(:note, %{user: user, data: %{"attachment" => [attachment.data]}}) uploads_dir = Pleroma.Config.get!([Pleroma.Uploaders.Local, :uploads]) - path = href |> Path.dirname() |> Path.basename() + path = get_attachment_filepath(note, uploads_dir) - assert {:ok, ["an_image.jpg"]} == File.ls("#{uploads_dir}/#{path}") + assert File.exists?("#{path}") Object.delete(note) @@ -254,7 +255,7 @@ test "With custom base_url" do assert Object.get_by_id(note.id).data["deleted"] assert Object.get_by_id(attachment.id) == nil - assert {:ok, []} == File.ls("#{uploads_dir}/#{path}") + refute File.exists?("#{path}") end end diff --git a/test/pleroma/upload_test.exs b/test/pleroma/upload_test.exs index ad6065b43..27a2d1b97 100644 --- a/test/pleroma/upload_test.exs +++ b/test/pleroma/upload_test.exs @@ -188,7 +188,7 @@ test "copies the file to the configured folder with anonymizing filename" do refute data["name"] == "an [image.jpg" end - test "escapes invalid characters in url" do + test "mangles the filename" do File.cp!("test/fixtures/image.jpg", "test/fixtures/image_tmp.jpg") file = %Plug.Upload{ @@ -200,23 +200,8 @@ test "escapes invalid characters in url" do {:ok, data} = Upload.store(file) [attachment_url | _] = data["url"] - assert Path.basename(attachment_url["href"]) == "an%E2%80%A6%20image.jpg" - end - - test "escapes reserved uri characters" do - File.cp!("test/fixtures/image.jpg", "test/fixtures/image_tmp.jpg") - - file = %Plug.Upload{ - content_type: "image/jpeg", - path: Path.absname("test/fixtures/image_tmp.jpg"), - filename: ":?#[]@!$&\\'()*+,;=.jpg" - } - - {:ok, data} = Upload.store(file) - [attachment_url | _] = data["url"] - - assert Path.basename(attachment_url["href"]) == - "%3A%3F%23%5B%5D%40%21%24%26%5C%27%28%29%2A%2B%2C%3B%3D.jpg" + refute Path.basename(attachment_url["href"]) == "an%E2%80%A6%20image.jpg" + refute Path.basename(attachment_url["href"]) == "an… image.jpg" end end diff --git a/test/pleroma/web/activity_pub/activity_pub_test.exs b/test/pleroma/web/activity_pub/activity_pub_test.exs index 5d5388cf5..69b4ac257 100644 --- a/test/pleroma/web/activity_pub/activity_pub_test.exs +++ b/test/pleroma/web/activity_pub/activity_pub_test.exs @@ -1304,14 +1304,6 @@ test "returns reblogs for users for whom reblogs have not been muted" do %{test_file: test_file} end - test "strips / from filename", %{test_file: file} do - file = %Plug.Upload{file | filename: "../../../../../nested/bad.jpg"} - {:ok, %Object{} = object} = ActivityPub.upload(file) - [%{"href" => href}] = object.data["url"] - assert Regex.match?(~r"/bad.jpg$", href) - refute Regex.match?(~r"/nested/", href) - end - test "sets a description if given", %{test_file: file} do {:ok, %Object{} = object} = ActivityPub.upload(file, description: "a cool file") assert object.data["name"] == "a cool file" diff --git a/test/pleroma/web/pleroma_api/controllers/mascot_controller_test.exs b/test/pleroma/web/pleroma_api/controllers/mascot_controller_test.exs index 7f02bff8f..8829597eb 100644 --- a/test/pleroma/web/pleroma_api/controllers/mascot_controller_test.exs +++ b/test/pleroma/web/pleroma_api/controllers/mascot_controller_test.exs @@ -64,6 +64,10 @@ test "mascot retrieving" do assert json_response_and_validate_schema(ret_conn, 200) + %{"url" => uploaded_url} = Jason.decode!(ret_conn.resp_body) + + assert uploaded_url != nil and is_binary(uploaded_url) + user = User.get_cached_by_id(user.id) conn = @@ -72,6 +76,6 @@ test "mascot retrieving" do |> get("/api/v1/pleroma/mascot") assert %{"url" => url, "type" => "image"} = json_response_and_validate_schema(conn, 200) - assert url =~ "an_image" + assert url == uploaded_url end end diff --git a/test/test_helper.exs b/test/test_helper.exs index 0fc7a86b9..22a0f33ee 100644 --- a/test/test_helper.exs +++ b/test/test_helper.exs @@ -9,6 +9,10 @@ {:ok, _} = Application.ensure_all_started(:ex_machina) +# Prepare and later automatically cleanup upload dir +uploads_dir = Pleroma.Config.get([Pleroma.Uploaders.Local, :uploads], "test/uploads") +File.mkdir_p!(uploads_dir) + ExUnit.after_suite(fn _results -> uploads = Pleroma.Config.get([Pleroma.Uploaders.Local, :uploads], "test/uploads") File.rm_rf!(uploads)