From fef773ca3524ed102e97014b590728b30edf9ab5 Mon Sep 17 00:00:00 2001 From: Oneric Date: Mon, 4 Mar 2024 17:50:22 +0100 Subject: [PATCH] Drop media base_url default and recommend different domain Same-domain setups enabled now at least two exploits, so they ought to be discouraged and definitely not be the default. --- CHANGELOG.md | 2 ++ docs/docs/configuration/cheatsheet.md | 3 ++- docs/docs/configuration/hardening.md | 10 ++++++++++ lib/mix/tasks/pleroma/instance.ex | 10 ++++++++++ lib/pleroma/upload.ex | 13 +++++++++++-- priv/templates/sample_config.eex | 4 +++- test/mix/tasks/pleroma/instance_test.exs | 3 +++ 7 files changed, 41 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1108c60e8..1efe1ecfc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). ## Added ## Changed +- `Pleroma.Upload, :base_url` now MUST be configured explicitly; + use of the same domain as the instance is **strongly** discouraged ## Fixed - Critical security issue allowing Akkoma to be used as a vector for diff --git a/docs/docs/configuration/cheatsheet.md b/docs/docs/configuration/cheatsheet.md index 3c9113f88..a04160a1d 100644 --- a/docs/docs/configuration/cheatsheet.md +++ b/docs/docs/configuration/cheatsheet.md @@ -598,7 +598,8 @@ 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` -* `base_url`: The base URL to access a user-uploaded file. Useful when you want to host the media files via another domain or are using a 3rd party S3 provider. +* `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. * `proxy_opts`: Proxy options, see `Pleroma.ReverseProxy` documentation. * `filename_display_max_length`: Set max length of a filename to display. 0 = no limit. Default: 30. diff --git a/docs/docs/configuration/hardening.md b/docs/docs/configuration/hardening.md index 521183f7d..f8ba048dd 100644 --- a/docs/docs/configuration/hardening.md +++ b/docs/docs/configuration/hardening.md @@ -17,6 +17,16 @@ This sets the Akkoma application server to only listen to the localhost interfac This sets the `secure` flag on Akkoma’s session cookie. This makes sure, that the cookie is only accepted over encrypted HTTPs connections. This implicitly renames the cookie from `pleroma_key` to `__Host-pleroma-key` which enforces some restrictions. (see [cookie prefixes](https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie#Cookie_prefixes)) +### `Pleroma.Upload, :uploader, :base_url` + +> Recommended value: *anything on a different domain than the instance endpoint; e.g. https://media.myinstance.net/* + +Uploads are user controlled and (unless you’re running a true single-user +instance) should therefore not be considered trusted. But the domain is used +as a pivilege boundary e.g. by HTTP content security policy and ActivityPub. +Having uploads on the same domain enabled several past vulnerabilities +able to be exploited by malicious users. + ### `:http_security` > Recommended value: `true` diff --git a/lib/mix/tasks/pleroma/instance.ex b/lib/mix/tasks/pleroma/instance.ex index 6a7d4f0d3..b442fdb5b 100644 --- a/lib/mix/tasks/pleroma/instance.ex +++ b/lib/mix/tasks/pleroma/instance.ex @@ -20,6 +20,7 @@ def run(["gen" | rest]) do output: :string, output_psql: :string, domain: :string, + media_url: :string, instance_name: :string, admin_email: :string, notify_email: :string, @@ -64,6 +65,14 @@ def run(["gen" | rest]) do ":" ) ++ [443] + media_url = + get_option( + options, + :media_url, + "What base url will uploads use? (e.g https://media.example.com/media)\n" <> + " Generally this should NOT use the same domain as the instance " + ) + name = get_option( options, @@ -207,6 +216,7 @@ def run(["gen" | rest]) do EEx.eval_file( template_dir <> "/sample_config.eex", domain: domain, + media_url: media_url, port: port, email: email, notify_email: notify_email, diff --git a/lib/pleroma/upload.ex b/lib/pleroma/upload.ex index 99b6b5215..974d12533 100644 --- a/lib/pleroma/upload.ex +++ b/lib/pleroma/upload.ex @@ -39,6 +39,8 @@ defmodule Pleroma.Upload do alias Pleroma.Web.ActivityPub.Utils require Logger + @mix_env Mix.env() + @type source :: Plug.Upload.t() | (data_uri_string :: String.t()) @@ -228,6 +230,13 @@ defp url_from_spec(%__MODULE__{name: name}, base_url, {:file, path}) do defp url_from_spec(_upload, _base_url, {:url, url}), do: url + if @mix_env == :test do + defp choose_base_url(prim, sec \\ nil), + do: prim || sec || Pleroma.Web.Endpoint.url() <> "/media/" + else + defp choose_base_url(prim, sec \\ nil), do: prim || sec + end + def base_url do uploader = Config.get([Pleroma.Upload, :uploader]) upload_base_url = Config.get([Pleroma.Upload, :base_url]) @@ -235,7 +244,7 @@ def base_url do case uploader do Pleroma.Uploaders.Local -> - upload_base_url || Pleroma.Web.Endpoint.url() <> "/media/" + choose_base_url(upload_base_url) Pleroma.Uploaders.S3 -> bucket = Config.get([Pleroma.Uploaders.S3, :bucket]) @@ -261,7 +270,7 @@ def base_url do end _ -> - public_endpoint || upload_base_url || Pleroma.Web.Endpoint.url() <> "/media/" + choose_base_url(public_endpoint, upload_base_url) end end end diff --git a/priv/templates/sample_config.eex b/priv/templates/sample_config.eex index 0068969ac..724c5a9d5 100644 --- a/priv/templates/sample_config.eex +++ b/priv/templates/sample_config.eex @@ -78,6 +78,8 @@ config :joken, default_signer: "<%= jwt_secret %>" config :pleroma, configurable_from_database: <%= db_configurable? %> +config :pleroma, Pleroma.Upload, <%= if Kernel.length(upload_filters) > 0 do -"config :pleroma, Pleroma.Upload, filters: #{inspect(upload_filters)}" +" filters: #{inspect(upload_filters)}," end %> + base_url: "<%= media_url %>" diff --git a/test/mix/tasks/pleroma/instance_test.exs b/test/mix/tasks/pleroma/instance_test.exs index 5a5a68053..522319371 100644 --- a/test/mix/tasks/pleroma/instance_test.exs +++ b/test/mix/tasks/pleroma/instance_test.exs @@ -39,6 +39,8 @@ test "running gen" do tmp_path() <> "setup.psql", "--domain", "test.pleroma.social", + "--media-url", + "https://media.pleroma.social/media", "--instance-name", "Pleroma", "--admin-email", @@ -92,6 +94,7 @@ test "running gen" do assert generated_config =~ "configurable_from_database: true" assert generated_config =~ "http: [ip: {127, 0, 0, 1}, port: 4000]" assert generated_config =~ "filters: [Pleroma.Upload.Filter.Exiftool]" + assert generated_config =~ "base_url: \"https://media.pleroma.social/media\"" assert File.read!(tmp_path() <> "setup.psql") == generated_setup_psql() assert File.exists?(Path.expand("./test/instance/static/robots.txt")) end