From 336e37d98f1b86c0332c9f260e27455a14714fa6 Mon Sep 17 00:00:00 2001 From: Ekaterina Vaartis Date: Fri, 21 Dec 2018 00:32:37 +0300 Subject: [PATCH 1/6] Make captcha (kocaptcha) stateless Also rename seconds_retained to seconds_valid since that's how it is now. Put it down from 180 to 20 seconds. The answer data is now stored in an encrypted text transfered to the client and back, so no ETS is needed --- config/config.exs | 2 +- docs/config.md | 2 +- lib/pleroma/captcha/captcha.ex | 29 +------ lib/pleroma/captcha/captcha_service.ex | 12 +-- lib/pleroma/captcha/kocaptcha.ex | 90 ++++++++++++---------- lib/pleroma/web/twitter_api/twitter_api.ex | 16 ++-- test/captcha_test.exs | 18 +++-- test/support/captcha_mock.ex | 5 +- 8 files changed, 82 insertions(+), 92 deletions(-) diff --git a/config/config.exs b/config/config.exs index 65d7346de..b8ef448b4 100644 --- a/config/config.exs +++ b/config/config.exs @@ -12,7 +12,7 @@ config :pleroma, Pleroma.Captcha, enabled: false, - seconds_retained: 180, + seconds_valid: 20, method: Pleroma.Captcha.Kocaptcha config :pleroma, Pleroma.Captcha.Kocaptcha, endpoint: "https://captcha.kotobank.ch" diff --git a/docs/config.md b/docs/config.md index c35769f21..cfeca8eb4 100644 --- a/docs/config.md +++ b/docs/config.md @@ -168,7 +168,7 @@ Web Push Notifications configuration. You can use the mix task `mix web_push.gen ## Pleroma.Captcha * `enabled`: Whether the captcha should be shown on registration * `method`: The method/service to use for captcha -* `seconds_retained`: The time in seconds for which the captcha is valid (stored in the cache) +* `seconds_valid`: The time in seconds for which the captcha is valid ### Pleroma.Captcha.Kocaptcha Kocaptcha is a very simple captcha service with a single API endpoint, diff --git a/lib/pleroma/captcha/captcha.ex b/lib/pleroma/captcha/captcha.ex index 5630f6b57..61a0f907f 100644 --- a/lib/pleroma/captcha/captcha.ex +++ b/lib/pleroma/captcha/captcha.ex @@ -1,8 +1,6 @@ defmodule Pleroma.Captcha do use GenServer - @ets_options [:ordered_set, :private, :named_table, {:read_concurrency, true}] - @doc false def start_link() do GenServer.start_link(__MODULE__, [], name: __MODULE__) @@ -10,14 +8,6 @@ def start_link() do @doc false def init(_) do - # Create a ETS table to store captchas - ets_name = Module.concat(method(), Ets) - ^ets_name = :ets.new(Module.concat(method(), Ets), @ets_options) - - # Clean up old captchas every few minutes - seconds_retained = Pleroma.Config.get!([__MODULE__, :seconds_retained]) - Process.send_after(self(), :cleanup, 1000 * seconds_retained) - {:ok, nil} end @@ -31,8 +21,8 @@ def new() do @doc """ Ask the configured captcha service to validate the captcha """ - def validate(token, captcha) do - GenServer.call(__MODULE__, {:validate, token, captcha}) + def validate(token, captcha, answer_data) do + GenServer.call(__MODULE__, {:validate, token, captcha, answer_data}) end @doc false @@ -47,19 +37,8 @@ def handle_call(:new, _from, state) do end @doc false - def handle_call({:validate, token, captcha}, _from, state) do - {:reply, method().validate(token, captcha), state} - end - - @doc false - def handle_info(:cleanup, state) do - :ok = method().cleanup() - - seconds_retained = Pleroma.Config.get!([__MODULE__, :seconds_retained]) - # Schedule the next clenup - Process.send_after(self(), :cleanup, 1000 * seconds_retained) - - {:noreply, state} + def handle_call({:validate, token, captcha, answer_data}, _from, state) do + {:reply, method().validate(token, captcha, answer_data), state} end defp method, do: Pleroma.Config.get!([__MODULE__, :method]) diff --git a/lib/pleroma/captcha/captcha_service.ex b/lib/pleroma/captcha/captcha_service.ex index 8d0b76f86..6f36d29b0 100644 --- a/lib/pleroma/captcha/captcha_service.ex +++ b/lib/pleroma/captcha/captcha_service.ex @@ -14,15 +14,15 @@ defmodule Pleroma.Captcha.Service do Arguments: * `token` the captcha is associated with * `captcha` solution of the captcha to validate + * `answer_data` is the data needed to validate the answer (presumably encrypted) Returns: `true` if captcha is valid, `false` if not """ - @callback validate(token :: String.t(), captcha :: String.t()) :: boolean - - @doc """ - This function is called periodically to clean up old captchas - """ - @callback cleanup() :: :ok + @callback validate( + token :: String.t(), + captcha :: String.t(), + answer_data :: String.t() + ) :: :ok | {:error, String.t()} end diff --git a/lib/pleroma/captcha/kocaptcha.ex b/lib/pleroma/captcha/kocaptcha.ex index 51900d123..f881c7b65 100644 --- a/lib/pleroma/captcha/kocaptcha.ex +++ b/lib/pleroma/captcha/kocaptcha.ex @@ -1,11 +1,11 @@ defmodule Pleroma.Captcha.Kocaptcha do + alias Plug.Crypto.KeyGenerator + alias Plug.Crypto.MessageEncryptor alias Calendar.DateTime alias Pleroma.Captcha.Service @behaviour Service - @ets __MODULE__.Ets - @impl Service def new() do endpoint = Pleroma.Config.get!([__MODULE__, :endpoint]) @@ -18,50 +18,56 @@ def new() do json_resp = Poison.decode!(res.body) token = json_resp["token"] + answer_md5 = json_resp["md5"] - true = - :ets.insert( - @ets, - {token, json_resp["md5"], DateTime.now_utc() |> DateTime.Format.unix()} - ) + secret_key_base = Pleroma.Config.get!([Pleroma.Web.Endpoint, :secret_key_base]) - %{type: :kocaptcha, token: token, url: endpoint <> json_resp["url"]} - end - end + # This make salt a little different for two keys + secret = KeyGenerator.generate(secret_key_base, token <> "_encrypt") + sign_secret = KeyGenerator.generate(secret_key_base, token <> "_sign") + # Basicallty copy what Phoenix.Token does here, add the time to + # the actual data and make it a binary to then encrypt it + encrypted_captcha_answer = + %{ + at: DateTime.now_utc(), + answer_md5: answer_md5 + } + |> :erlang.term_to_binary() + |> MessageEncryptor.encrypt(secret, sign_secret) - @impl Service - def validate(token, captcha) do - with false <- is_nil(captcha), - [{^token, saved_md5, _}] <- :ets.lookup(@ets, token), - true <- :crypto.hash(:md5, captcha) |> Base.encode16() == String.upcase(saved_md5) do - # Clear the saved value - :ets.delete(@ets, token) - - true - else - _ -> false - end - end - - @impl Service - def cleanup() do - seconds_retained = Pleroma.Config.get!([Pleroma.Captcha, :seconds_retained]) - # If the time in ETS is less than current_time - seconds_retained, then the time has - # already passed - delete_after = - DateTime.subtract!(DateTime.now_utc(), seconds_retained) |> DateTime.Format.unix() - - :ets.select_delete( - @ets, - [ - { - {:_, :_, :"$1"}, - [{:<, :"$1", {:const, delete_after}}], - [true] + %{ + type: :kocaptcha, + token: token, + url: endpoint <> json_resp["url"], + answer_data: encrypted_captcha_answer } - ] - ) + end + end - :ok + @impl Service + def validate(token, captcha, answer_data) do + secret_key_base = Pleroma.Config.get!([Pleroma.Web.Endpoint, :secret_key_base]) + secret = KeyGenerator.generate(secret_key_base, token <> "_encrypt") + sign_secret = KeyGenerator.generate(secret_key_base, token <> "_sign") + + # If the time found is less than (current_time - seconds_valid), then the time has already passed. + # Later we check that the time found is more than the presumed invalidatation time, that means + # that the data is still valid and the captcha can be checked + seconds_valid = Pleroma.Config.get!([Pleroma.Captcha, :seconds_valid]) + valid_if_after = DateTime.subtract!(DateTime.now_utc(), seconds_valid) + + with {:ok, data} <- MessageEncryptor.decrypt(answer_data, secret, sign_secret), + %{at: at, answer_md5: answer_md5} <- :erlang.binary_to_term(data) do + if DateTime.after?(at, valid_if_after) do + if not is_nil(captcha) and + :crypto.hash(:md5, captcha) |> Base.encode16() == String.upcase(answer_md5), + do: :ok, + else: {:error, "Invalid CAPTCHA"} + else + {:error, "CAPTCHA expired"} + end + else + _ -> {:error, "Invalid answer data"} + end end end diff --git a/lib/pleroma/web/twitter_api/twitter_api.ex b/lib/pleroma/web/twitter_api/twitter_api.ex index d816dc3bc..9e15f2c33 100644 --- a/lib/pleroma/web/twitter_api/twitter_api.ex +++ b/lib/pleroma/web/twitter_api/twitter_api.ex @@ -136,22 +136,28 @@ def register_user(params) do password: params["password"], password_confirmation: params["confirm"], captcha_solution: params["captcha_solution"], - captcha_token: params["captcha_token"] + captcha_token: params["captcha_token"], + captcha_answer_data: params["captcha_answer_data"] } captcha_enabled = Pleroma.Config.get([Pleroma.Captcha, :enabled]) # true if captcha is disabled or enabled and valid, false otherwise captcha_ok = if !captcha_enabled do - true + :ok else - Pleroma.Captcha.validate(params[:captcha_token], params[:captcha_solution]) + Pleroma.Captcha.validate( + params[:captcha_token], + params[:captcha_solution], + params[:captcha_answer_data] + ) end # Captcha invalid - if not captcha_ok do + if captcha_ok != :ok do + {:error, error} = captcha_ok # I have no idea how this error handling works - {:error, %{error: Jason.encode!(%{captcha: ["Invalid CAPTCHA"]})}} + {:error, %{error: Jason.encode!(%{captcha: [error]})}} else registrations_open = Pleroma.Config.get([:instance, :registrations_open]) diff --git a/test/captcha_test.exs b/test/captcha_test.exs index 54ffbd92f..93b8930da 100644 --- a/test/captcha_test.exs +++ b/test/captcha_test.exs @@ -25,16 +25,18 @@ defmodule Pleroma.CaptchaTest do end test "new and validate" do - assert Kocaptcha.new() == %{ - type: :kocaptcha, - token: "afa1815e14e29355e6c8f6b143a39fa2", - url: "https://captcha.kotobank.ch/captchas/afa1815e14e29355e6c8f6b143a39fa2.png" - } + new = Kocaptcha.new() + assert new[:type] == :kocaptcha + assert new[:token] == "afa1815e14e29355e6c8f6b143a39fa2" + + assert new[:url] == + "https://captcha.kotobank.ch/captchas/afa1815e14e29355e6c8f6b143a39fa2.png" assert Kocaptcha.validate( - "afa1815e14e29355e6c8f6b143a39fa2", - "7oEy8c" - ) + new[:token], + "7oEy8c", + new[:answer_data] + ) == :ok end end end diff --git a/test/support/captcha_mock.ex b/test/support/captcha_mock.ex index 898aa17b8..410318dc4 100644 --- a/test/support/captcha_mock.ex +++ b/test/support/captcha_mock.ex @@ -6,8 +6,5 @@ defmodule Pleroma.Captcha.Mock do def new(), do: %{type: :mock} @impl Service - def validate(_token, _captcha), do: true - - @impl Service - def cleanup(), do: :ok + def validate(_token, _captcha, _data), do: :ok end From b386e560ba22ca93b56ba74ffc134fe7e6de8b2d Mon Sep 17 00:00:00 2001 From: Ekaterina Vaartis Date: Sat, 22 Dec 2018 22:39:08 +0300 Subject: [PATCH 2/6] Move the encryption out of kocaptcha into general captcha module That way there won't be a need to reimplement it for other captcha services --- lib/pleroma/captcha/captcha.ex | 53 +++++++++++++++++++++++- lib/pleroma/captcha/captcha_service.ex | 11 +++-- lib/pleroma/captcha/kocaptcha.ex | 56 ++++---------------------- 3 files changed, 67 insertions(+), 53 deletions(-) diff --git a/lib/pleroma/captcha/captcha.ex b/lib/pleroma/captcha/captcha.ex index 61a0f907f..04769d4b2 100644 --- a/lib/pleroma/captcha/captcha.ex +++ b/lib/pleroma/captcha/captcha.ex @@ -1,4 +1,8 @@ defmodule Pleroma.Captcha do + alias Plug.Crypto.KeyGenerator + alias Plug.Crypto.MessageEncryptor + alias Calendar.DateTime + use GenServer @doc false @@ -32,13 +36,58 @@ def handle_call(:new, _from, state) do if !enabled do {:reply, %{type: :none}, state} else - {:reply, method().new(), state} + new_captcha = method().new() + + secret_key_base = Pleroma.Config.get!([Pleroma.Web.Endpoint, :secret_key_base]) + + # This make salt a little different for two keys + token = new_captcha[:token] + secret = KeyGenerator.generate(secret_key_base, token <> "_encrypt") + sign_secret = KeyGenerator.generate(secret_key_base, token <> "_sign") + # Basicallty copy what Phoenix.Token does here, add the time to + # the actual data and make it a binary to then encrypt it + encrypted_captcha_answer = + %{ + at: DateTime.now_utc(), + answer_data: new_captcha[:answer_data] + } + |> :erlang.term_to_binary() + |> MessageEncryptor.encrypt(secret, sign_secret) + + IO.inspect(%{new_captcha | answer_data: encrypted_captcha_answer}) + + { + :reply, + # Repalce the answer with the encrypted answer + %{new_captcha | answer_data: encrypted_captcha_answer}, + state + } end end @doc false def handle_call({:validate, token, captcha, answer_data}, _from, state) do - {:reply, method().validate(token, captcha, answer_data), state} + secret_key_base = Pleroma.Config.get!([Pleroma.Web.Endpoint, :secret_key_base]) + secret = KeyGenerator.generate(secret_key_base, token <> "_encrypt") + sign_secret = KeyGenerator.generate(secret_key_base, token <> "_sign") + + # If the time found is less than (current_time - seconds_valid), then the time has already passed. + # Later we check that the time found is more than the presumed invalidatation time, that means + # that the data is still valid and the captcha can be checked + seconds_valid = Pleroma.Config.get!([Pleroma.Captcha, :seconds_valid]) + valid_if_after = DateTime.subtract!(DateTime.now_utc(), seconds_valid) + + result = + with {:ok, data} <- MessageEncryptor.decrypt(answer_data, secret, sign_secret), + %{at: at, answer_data: answer_md5} <- :erlang.binary_to_term(data) do + if DateTime.after?(at, valid_if_after), + do: method().validate(token, captcha, answer_md5), + else: {:error, "CAPTCHA expired"} + else + _ -> {:error, "Invalid answer data"} + end + + {:reply, result, state} end defp method, do: Pleroma.Config.get!([__MODULE__, :method]) diff --git a/lib/pleroma/captcha/captcha_service.ex b/lib/pleroma/captcha/captcha_service.ex index 6f36d29b0..6c5ab6c36 100644 --- a/lib/pleroma/captcha/captcha_service.ex +++ b/lib/pleroma/captcha/captcha_service.ex @@ -4,9 +4,14 @@ defmodule Pleroma.Captcha.Service do Returns: - Service-specific data for using the newly created captcha + Type/Name of the service, the token to identify the captcha, + the data of the answer and service-specific data to use the newly created captcha """ - @callback new() :: map + @callback new() :: %{ + type: atom(), + token: String.t(), + answer_data: any() + } @doc """ Validated the provided captcha solution. @@ -23,6 +28,6 @@ defmodule Pleroma.Captcha.Service do @callback validate( token :: String.t(), captcha :: String.t(), - answer_data :: String.t() + answer_data :: any() ) :: :ok | {:error, String.t()} end diff --git a/lib/pleroma/captcha/kocaptcha.ex b/lib/pleroma/captcha/kocaptcha.ex index f881c7b65..cd0eb6f21 100644 --- a/lib/pleroma/captcha/kocaptcha.ex +++ b/lib/pleroma/captcha/kocaptcha.ex @@ -1,8 +1,4 @@ defmodule Pleroma.Captcha.Kocaptcha do - alias Plug.Crypto.KeyGenerator - alias Plug.Crypto.MessageEncryptor - alias Calendar.DateTime - alias Pleroma.Captcha.Service @behaviour Service @@ -17,57 +13,21 @@ def new() do {:ok, res} -> json_resp = Poison.decode!(res.body) - token = json_resp["token"] - answer_md5 = json_resp["md5"] - - secret_key_base = Pleroma.Config.get!([Pleroma.Web.Endpoint, :secret_key_base]) - - # This make salt a little different for two keys - secret = KeyGenerator.generate(secret_key_base, token <> "_encrypt") - sign_secret = KeyGenerator.generate(secret_key_base, token <> "_sign") - # Basicallty copy what Phoenix.Token does here, add the time to - # the actual data and make it a binary to then encrypt it - encrypted_captcha_answer = - %{ - at: DateTime.now_utc(), - answer_md5: answer_md5 - } - |> :erlang.term_to_binary() - |> MessageEncryptor.encrypt(secret, sign_secret) - %{ type: :kocaptcha, - token: token, + token: json_resp["token"], url: endpoint <> json_resp["url"], - answer_data: encrypted_captcha_answer + answer_data: json_resp["md5"] } end end @impl Service - def validate(token, captcha, answer_data) do - secret_key_base = Pleroma.Config.get!([Pleroma.Web.Endpoint, :secret_key_base]) - secret = KeyGenerator.generate(secret_key_base, token <> "_encrypt") - sign_secret = KeyGenerator.generate(secret_key_base, token <> "_sign") - - # If the time found is less than (current_time - seconds_valid), then the time has already passed. - # Later we check that the time found is more than the presumed invalidatation time, that means - # that the data is still valid and the captcha can be checked - seconds_valid = Pleroma.Config.get!([Pleroma.Captcha, :seconds_valid]) - valid_if_after = DateTime.subtract!(DateTime.now_utc(), seconds_valid) - - with {:ok, data} <- MessageEncryptor.decrypt(answer_data, secret, sign_secret), - %{at: at, answer_md5: answer_md5} <- :erlang.binary_to_term(data) do - if DateTime.after?(at, valid_if_after) do - if not is_nil(captcha) and - :crypto.hash(:md5, captcha) |> Base.encode16() == String.upcase(answer_md5), - do: :ok, - else: {:error, "Invalid CAPTCHA"} - else - {:error, "CAPTCHA expired"} - end - else - _ -> {:error, "Invalid answer data"} - end + def validate(_token, captcha, answer_data) do + # Here the token is unsed, because the unencrypted captcha answer is just passed to method + if not is_nil(captcha) and + :crypto.hash(:md5, captcha) |> Base.encode16() == String.upcase(answer_data), + do: :ok, + else: {:error, "Invalid CAPTCHA"} end end From d112990776383b289c98916b3482a2a8ab337221 Mon Sep 17 00:00:00 2001 From: Ekaterina Vaartis Date: Sat, 22 Dec 2018 23:07:44 +0300 Subject: [PATCH 3/6] Specifically disable captcha for automatic tests, it makes them fail --- config/test.exs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/config/test.exs b/config/test.exs index 51aace407..67ed4737f 100644 --- a/config/test.exs +++ b/config/test.exs @@ -9,7 +9,8 @@ # Disable captha for tests config :pleroma, Pleroma.Captcha, - enabled: true, + # It should not be enabled for automatic tests + enabled: false, # A fake captcha service for tests method: Pleroma.Captcha.Mock From 448af3601a365e4f24a2db54af366d075af7e90f Mon Sep 17 00:00:00 2001 From: Ekaterina Vaartis Date: Thu, 27 Dec 2018 00:12:20 +0300 Subject: [PATCH 4/6] Up captcha timer to 60 secs again, save used captchas in cachex --- config/config.exs | 2 +- lib/pleroma/application.ex | 10 ++++++++++ lib/pleroma/captcha/captcha.ex | 21 ++++++++++++++++++--- 3 files changed, 29 insertions(+), 4 deletions(-) diff --git a/config/config.exs b/config/config.exs index b8ef448b4..9cb81cf47 100644 --- a/config/config.exs +++ b/config/config.exs @@ -12,7 +12,7 @@ config :pleroma, Pleroma.Captcha, enabled: false, - seconds_valid: 20, + seconds_valid: 60, method: Pleroma.Captcha.Kocaptcha config :pleroma, Pleroma.Captcha.Kocaptcha, endpoint: "https://captcha.kotobank.ch" diff --git a/lib/pleroma/application.ex b/lib/pleroma/application.ex index e15991957..bdd0ee26f 100644 --- a/lib/pleroma/application.ex +++ b/lib/pleroma/application.ex @@ -25,6 +25,16 @@ def start(_type, _args) do supervisor(Pleroma.Repo, []), worker(Pleroma.Emoji, []), worker(Pleroma.Captcha, []), + worker( + Cachex, + [ + :used_captcha_cache, + [ + ttl_interval: :timer.seconds(60 * 2) + ] + ], + id: :cachex_used_captcha_cache + ), worker( Cachex, [ diff --git a/lib/pleroma/captcha/captcha.ex b/lib/pleroma/captcha/captcha.ex index 04769d4b2..c7abafeb1 100644 --- a/lib/pleroma/captcha/captcha.ex +++ b/lib/pleroma/captcha/captcha.ex @@ -80,9 +80,24 @@ def handle_call({:validate, token, captcha, answer_data}, _from, state) do result = with {:ok, data} <- MessageEncryptor.decrypt(answer_data, secret, sign_secret), %{at: at, answer_data: answer_md5} <- :erlang.binary_to_term(data) do - if DateTime.after?(at, valid_if_after), - do: method().validate(token, captcha, answer_md5), - else: {:error, "CAPTCHA expired"} + try do + if DateTime.before?(at, valid_if_after), do: throw({:error, "CAPTCHA expired"}) + + if not is_nil(Cachex.get!(:used_captcha_cache, token)), + do: throw({:error, "CAPTCHA already used"}) + + res = method().validate(token, captcha, answer_md5) + # Throw if an error occurs + if res != :ok, do: throw(res) + + # Mark this captcha as used + {:ok, _} = + Cachex.put(:used_captcha_cache, token, true, ttl: :timer.seconds(seconds_valid)) + + :ok + catch + :throw, e -> e + end else _ -> {:error, "Invalid answer data"} end From 708a22891921d54c9c716ac250e75ae6a889eb7b Mon Sep 17 00:00:00 2001 From: vaartis Date: Thu, 27 Dec 2018 07:42:03 +0000 Subject: [PATCH 5/6] Set ttl_interval to the seconds_valid time --- lib/pleroma/application.ex | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/pleroma/application.ex b/lib/pleroma/application.ex index bdd0ee26f..8dbacf258 100644 --- a/lib/pleroma/application.ex +++ b/lib/pleroma/application.ex @@ -30,7 +30,7 @@ def start(_type, _args) do [ :used_captcha_cache, [ - ttl_interval: :timer.seconds(60 * 2) + ttl_interval: :timer.seconds(Pleroma.Config.get!([Pleroma.Captcha, :seconds_valid])) ] ], id: :cachex_used_captcha_cache From 816db3f494c6fcc60d0a700dfc473a9cc49c84a0 Mon Sep 17 00:00:00 2001 From: vaartis Date: Sat, 29 Dec 2018 17:44:26 +0000 Subject: [PATCH 6/6] Remove the debugging IO.inspect --- lib/pleroma/captcha/captcha.ex | 2 -- 1 file changed, 2 deletions(-) diff --git a/lib/pleroma/captcha/captcha.ex b/lib/pleroma/captcha/captcha.ex index c7abafeb1..424ad4add 100644 --- a/lib/pleroma/captcha/captcha.ex +++ b/lib/pleroma/captcha/captcha.ex @@ -54,8 +54,6 @@ def handle_call(:new, _from, state) do |> :erlang.term_to_binary() |> MessageEncryptor.encrypt(secret, sign_secret) - IO.inspect(%{new_captcha | answer_data: encrypted_captcha_answer}) - { :reply, # Repalce the answer with the encrypted answer