From df2173343accec7a7a311d85df2f13d5141b7bc7 Mon Sep 17 00:00:00 2001 From: rinpatch Date: Fri, 28 Feb 2020 17:29:53 +0300 Subject: [PATCH 1/5] pagination: limit the number of elements returned at one time to 40 --- lib/pleroma/pagination.ex | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/lib/pleroma/pagination.ex b/lib/pleroma/pagination.ex index 4535ca7c5..43fb7babf 100644 --- a/lib/pleroma/pagination.ex +++ b/lib/pleroma/pagination.ex @@ -13,6 +13,7 @@ defmodule Pleroma.Pagination do alias Pleroma.Repo @default_limit 20 + @max_limit 40 @page_keys ["max_id", "min_id", "limit", "since_id", "order"] def page_keys, do: @page_keys @@ -130,7 +131,11 @@ defp restrict(query, :offset, %{offset: offset}, _table_binding) do end defp restrict(query, :limit, options, _table_binding) do - limit = Map.get(options, :limit, @default_limit) + limit = + case Map.get(options, :limit, @default_limit) do + limit when limit < @max_limit -> limit + _ -> @max_limit + end query |> limit(^limit) From 4d416343fae4a9e0b1654b12bd476017be63a7e9 Mon Sep 17 00:00:00 2001 From: rinpatch Date: Fri, 28 Feb 2020 17:35:01 +0300 Subject: [PATCH 2/5] rate limiter: Fix a race condition When multiple requests are processed by rate limiter plug at the same time and the bucket is not yet initialized, both would try to initialize the bucket resulting in an internal server error. --- .../plugs/rate_limiter/limiter_supervisor.ex | 10 ++++-- .../plugs/rate_limiter/rate_limiter.ex | 15 ++++++--- test/plugs/rate_limiter_test.exs | 31 +++++++++++++++++++ 3 files changed, 49 insertions(+), 7 deletions(-) diff --git a/lib/pleroma/plugs/rate_limiter/limiter_supervisor.ex b/lib/pleroma/plugs/rate_limiter/limiter_supervisor.ex index 187582ede..884268d96 100644 --- a/lib/pleroma/plugs/rate_limiter/limiter_supervisor.ex +++ b/lib/pleroma/plugs/rate_limiter/limiter_supervisor.ex @@ -7,8 +7,8 @@ def start_link(init_arg) do DynamicSupervisor.start_link(__MODULE__, init_arg, name: __MODULE__) end - def add_limiter(limiter_name, expiration) do - {:ok, _pid} = + def add_or_return_limiter(limiter_name, expiration) do + result = DynamicSupervisor.start_child( __MODULE__, %{ @@ -28,6 +28,12 @@ def add_limiter(limiter_name, expiration) do ]} } ) + + case result do + {:ok, _pid} = result -> result + {:error, {:already_started, pid}} -> {:ok, pid} + _ -> result + end end @impl true diff --git a/lib/pleroma/plugs/rate_limiter/rate_limiter.ex b/lib/pleroma/plugs/rate_limiter/rate_limiter.ex index 9c362a392..b9cbe9716 100644 --- a/lib/pleroma/plugs/rate_limiter/rate_limiter.ex +++ b/lib/pleroma/plugs/rate_limiter/rate_limiter.ex @@ -171,7 +171,7 @@ defp check_rate(action_settings) do {:error, value} {:error, :no_cache} -> - initialize_buckets(action_settings) + initialize_buckets!(action_settings) check_rate(action_settings) end end @@ -250,11 +250,16 @@ defp attach_selected_params(input, %{conn_params: conn_params, opts: plug_opts}) |> String.replace_leading(":", "") end - defp initialize_buckets(%{name: _name, limits: nil}), do: :ok + defp initialize_buckets!(%{name: _name, limits: nil}), do: :ok - defp initialize_buckets(%{name: name, limits: limits}) do - LimiterSupervisor.add_limiter(anon_bucket_name(name), get_scale(:anon, limits)) - LimiterSupervisor.add_limiter(user_bucket_name(name), get_scale(:user, limits)) + defp initialize_buckets!(%{name: name, limits: limits}) do + {:ok, _pid} = + LimiterSupervisor.add_or_return_limiter(anon_bucket_name(name), get_scale(:anon, limits)) + + {:ok, _pid} = + LimiterSupervisor.add_or_return_limiter(user_bucket_name(name), get_scale(:user, limits)) + + :ok end defp attach_identity(base, %{mode: :user, conn_info: conn_info}), diff --git a/test/plugs/rate_limiter_test.exs b/test/plugs/rate_limiter_test.exs index 104d67611..8cdc8d1a2 100644 --- a/test/plugs/rate_limiter_test.exs +++ b/test/plugs/rate_limiter_test.exs @@ -242,4 +242,35 @@ test "different users are counted independently" do refute conn_2.halted end end + + test "doesn't crash due to a race condition when multiple requests are made at the same time and the bucket is not yet initialized" do + limiter_name = :test_race_condition + Pleroma.Config.put([:rate_limit, limiter_name], {1000, 5}) + Pleroma.Config.put([Pleroma.Web.Endpoint, :http, :ip], {8, 8, 8, 8}) + + opts = RateLimiter.init(name: limiter_name) + + conn = conn(:get, "/") + conn_2 = conn(:get, "/") + + %Task{pid: pid1} = + task1 = + Task.async(fn -> + receive do + :process2_up -> + RateLimiter.call(conn, opts) + end + end) + + task2 = + Task.async(fn -> + send(pid1, :process2_up) + RateLimiter.call(conn_2, opts) + end) + + Task.await(task1) + Task.await(task2) + + refute {:err, :not_found} == RateLimiter.inspect_bucket(conn, limiter_name, opts) + end end From ffcebe7e22b4c5ccaf3ba63f3ed2885ac55a6b4d Mon Sep 17 00:00:00 2001 From: rinpatch Date: Fri, 28 Feb 2020 17:44:59 +0300 Subject: [PATCH 3/5] timeline controller: rate limit timelines to 3 requests per 500ms per timeline per ip/user --- config/config.exs | 1 + config/description.exs | 6 ++++++ docs/configuration/cheatsheet.md | 1 + .../mastodon_api/controllers/timeline_controller.ex | 11 +++++++++++ 4 files changed, 19 insertions(+) diff --git a/config/config.exs b/config/config.exs index 0dde1fc85..9c4eb70a3 100644 --- a/config/config.exs +++ b/config/config.exs @@ -599,6 +599,7 @@ config :pleroma, :rate_limit, authentication: {60_000, 15}, + timeline: {500, 3}, search: [{1000, 10}, {1000, 30}], app_account_creation: {1_800_000, 25}, relations_actions: {10_000, 10}, diff --git a/config/description.exs b/config/description.exs index bcb69bc41..9fdcfcd96 100644 --- a/config/description.exs +++ b/config/description.exs @@ -2465,6 +2465,12 @@ description: "For the search requests (account & status search etc.)", suggestions: [{1000, 10}, [{10_000, 10}, {10_000, 50}]] }, + %{ + key: :timeline, + type: [:tuple, {:list, :tuple}], + description: "For requests to timelines (each timeline has it's own limiter)", + suggestions: [{1000, 10}, [{10_000, 10}, {10_000, 50}]] + }, %{ key: :app_account_creation, type: [:tuple, {:list, :tuple}], diff --git a/docs/configuration/cheatsheet.md b/docs/configuration/cheatsheet.md index ac55a0b32..1cffae977 100644 --- a/docs/configuration/cheatsheet.md +++ b/docs/configuration/cheatsheet.md @@ -343,6 +343,7 @@ Means that: Supported rate limiters: * `:search` - Account/Status search. +* `:timeline` - Timeline requests (each timeline has it's own limiter). * `:app_account_creation` - Account registration from the API. * `:relations_actions` - Following/Unfollowing in general. * `:relation_id_action` - Following/Unfollowing for a specific user. diff --git a/lib/pleroma/web/mastodon_api/controllers/timeline_controller.ex b/lib/pleroma/web/mastodon_api/controllers/timeline_controller.ex index 29964a1d4..f58c1f93c 100644 --- a/lib/pleroma/web/mastodon_api/controllers/timeline_controller.ex +++ b/lib/pleroma/web/mastodon_api/controllers/timeline_controller.ex @@ -10,9 +10,20 @@ defmodule Pleroma.Web.MastodonAPI.TimelineController do alias Pleroma.Pagination alias Pleroma.Plugs.OAuthScopesPlug + alias Pleroma.Plugs.RateLimiter alias Pleroma.User alias Pleroma.Web.ActivityPub.ActivityPub + # XXX: Ideally these would be generated instead of copypasted, + # but I haven't been able to overcome an issue with guards when + # trying to generate these. + # See: https://elixirforum.com/t/trouble-plugging-plugs-with-generated-options-in-guards-in-a-phoenix-controller/29465 + plug(RateLimiter, [name: :timeline, bucket_name: :direct_timeline] when action == :direct) + plug(RateLimiter, [name: :timeline, bucket_name: :public_timeline] when action == :public) + plug(RateLimiter, [name: :timeline, bucket_name: :home_timeline] when action == :home) + plug(RateLimiter, [name: :timeline, bucket_name: :hashtag_timeline] when action == :hashtag) + plug(RateLimiter, [name: :timeline, bucket_name: :list_timeline] when action == :list) + plug(OAuthScopesPlug, %{scopes: ["read:statuses"]} when action in [:home, :direct]) plug(OAuthScopesPlug, %{scopes: ["read:lists"]} when action == :list) From e6ccf121292292d8851688822e951d6651ef3bf3 Mon Sep 17 00:00:00 2001 From: rinpatch Date: Fri, 28 Feb 2020 17:59:16 +0300 Subject: [PATCH 4/5] changelog: entries for timeline DoS fixes --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 12f7e8fab..37df345ed 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,9 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). ## [Unreleased] +### Security +- Mastodon API: Fix being able to request enourmous amount of statuses in timelines leading to DoS. Now limited to 40 per request. + ### Removed - **Breaking**: Removed 1.0+ deprecated configurations `Pleroma.Upload, :strip_exif` and `:instance, :dedupe_media` - **Breaking**: OStatus protocol support @@ -56,6 +59,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). - Admin API: Render whole status in grouped reports - Mastodon API: User timelines will now respect blocks, unless you are getting the user timeline of somebody you blocked (which would be empty otherwise). - Mastodon API: Favoriting / Repeating a post multiple times will now return the identical response every time. Before, executing that action twice would return an error ("already favorited") on the second try. +- Mastodon API: Limit timeline requests to 3 per timeline per 500ms per user/ip by default. ### Added From b5465bf385800d52998bca472a19ea1b9db4c252 Mon Sep 17 00:00:00 2001 From: rinpatch Date: Sun, 1 Mar 2020 02:03:46 +0300 Subject: [PATCH 5/5] timeline controller: add a TODO for replacing copypaste with a macro --- .../web/mastodon_api/controllers/timeline_controller.ex | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/pleroma/web/mastodon_api/controllers/timeline_controller.ex b/lib/pleroma/web/mastodon_api/controllers/timeline_controller.ex index f58c1f93c..a3110c722 100644 --- a/lib/pleroma/web/mastodon_api/controllers/timeline_controller.ex +++ b/lib/pleroma/web/mastodon_api/controllers/timeline_controller.ex @@ -14,10 +14,10 @@ defmodule Pleroma.Web.MastodonAPI.TimelineController do alias Pleroma.User alias Pleroma.Web.ActivityPub.ActivityPub - # XXX: Ideally these would be generated instead of copypasted, - # but I haven't been able to overcome an issue with guards when - # trying to generate these. - # See: https://elixirforum.com/t/trouble-plugging-plugs-with-generated-options-in-guards-in-a-phoenix-controller/29465 + # TODO: Replace with a macro when there is a Phoenix release with + # https://github.com/phoenixframework/phoenix/commit/2e8c63c01fec4dde5467dbbbf9705ff9e780735e + # in it + plug(RateLimiter, [name: :timeline, bucket_name: :direct_timeline] when action == :direct) plug(RateLimiter, [name: :timeline, bucket_name: :public_timeline] when action == :public) plug(RateLimiter, [name: :timeline, bucket_name: :home_timeline] when action == :home)