Merge pull request #239 from danschultzer/prevent-timing-attack

Prevent timing attacks on authentication

CHANGELOG.md

@@ -2,6 +2,9 @@
 
 ## v1.0.12 (TBA)
 
+* To prevent timing attacks, `Pow.Ecto.Context.authenticate/2` now verifies password on a blank user struct when no user can be found for the provided user id, but will always return nil. The blank user struct has a nil `:password_hash` value. The struct will be passed along with a blank password to the `verify_password/2` method in the user schema module.
+* To prevent timing attacks, when `Pow.Ecto.Schema.Changeset.verify_password/3` receives a struct with a nil `:password_hash` value, it'll hash a blank password, but always return false.
+* To prevent timing attacks, the UUID is always generated in `PowResetPassword.Plug.create_reset_token/2` whether the user exists or not.
 * `PowPersistentSession.Plug.Base` now accepts `:persistent_session_ttl` which will pass the TTL to the cache backend and used for the max age of the sesion cookie in `PowPersistentSession.Plug.Cookie`
 * Deprecated `:persistent_session_cookie_max_age` configuration setting
 

lib/extensions/reset_password/plug.ex

@@ -35,24 +35,27 @@ defmodule PowResetPassword.Plug do
   @doc """
   Finds a user for the provided params, creates a token, and stores the user
   for the token.
+
+  To prevent timing attacks, `Pow.UUID.generate/0` is called whether the user
+  exists or not.
   """
   @spec create_reset_token(Conn.t(), map()) :: {:ok, map(), Conn.t()} | {:error, map(), Conn.t()}
   def create_reset_token(conn, params) do
     config = Plug.fetch_config(conn)
+    token  = UUID.generate()
     user   =
       params
       |> Map.get("email")
       |> ResetPasswordContext.get_by_email(config)
 
-    maybe_create_reset_token(conn, user, config)
+      maybe_store_reset_token(conn, user, token, config)
   end
 
-  defp maybe_create_reset_token(conn, nil, _config) do
+  defp maybe_store_reset_token(conn, nil, _token, _config) do
     changeset = change_user(conn)
     {:error, %{changeset | action: :update}, conn}
   end
-  defp maybe_create_reset_token(conn, user, config) do
-    token = UUID.generate()
+  defp maybe_store_reset_token(conn, user, token, config) do
     {store, store_config} = store(config)
 
     store.put(store_config, token, user)

lib/pow/ecto/context.ex

@@ -90,7 +90,11 @@ defmodule Pow.Ecto.Context do
   User schema module and repo module will be fetched from the config. The user
   id field is fetched from the user schema module.
 
-  The method will return nil if either the user id or password is nil.
+  The method will return nil if either the fetched user, or password is nil.
+
+  To prevent timing attacks, a blank user struct will be passed to the
+  `verify_password/2` method for the user schema module to ensure that the
+  the response time will be equal as when a password is verified.
   """
   @spec authenticate(map(), Config.t()) :: user() | nil
   def authenticate(params, config) do
@@ -106,12 +110,18 @@ defmodule Pow.Ecto.Context do
   defp do_authenticate(user_id_field, login_value, password, config) do
     [{user_id_field, login_value}]
     |> get_by(config)
-    |> verify_password(password)
+    |> verify_password(password, config)
   end
 
-  defp verify_password(nil, _password), do: nil
-  defp verify_password(_user, nil), do: nil
-  defp verify_password(user, password) do
+  defp verify_password(nil, _password, config) do
+    user_mod = Config.user!(config)
+    user     = struct(user_mod, password_hash: nil)
+    user_mod.verify_password(user, "")
+
+    nil
+  end
+  defp verify_password(_user, nil, _config), do: false
+  defp verify_password(user, password, _config) do
     case user.__struct__.verify_password(user, password) do
       true -> user
       _    -> nil

lib/pow/ecto/schema/changeset.ex

@@ -141,9 +141,19 @@ defmodule Pow.Ecto.Schema.Changeset do
 
   The password will be verified by using the `:password_hash_methods` in the
   configuration.
+
+  To prevent timing attacks, a blank password will be passed to the hash method
+  in the `:password_hash_methods` configuration option if the `:password_hash`
+  is nil.
   """
   @spec verify_password(Ecto.Schema.t(), binary(), Config.t()) :: boolean()
-  def verify_password(%{password_hash: nil}, _password, _config), do: false
+  def verify_password(%{password_hash: nil}, _password, config) do
+    config
+    |> password_hash_method()
+    |> apply([""])
+
+    false
+  end
   def verify_password(%{password_hash: password_hash}, password, config) do
     config
     |> password_verify_method()

test/extensions/reset_password/ecto/context_test.exs

@@ -23,9 +23,7 @@ defmodule PowResetPassword.Ecto.ContextTest do
 
   describe "update_password/2" do
     test "updates with compiled password hash methods" do
-      config = @config ++ [password_hash_methods: {&(&1 <> "123"), &(&1 == &2 <> "123")}]
-
-      assert {:ok, user} = Context.update_password(@user, %{password: @password, confirm_password: @password}, config)
+      assert {:ok, user} = Context.update_password(@user, %{password: @password, confirm_password: @password}, @config)
       assert Password.pbkdf2_verify(@password, user.password_hash)
     end
 

test/pow/ecto/context_test.exs

@@ -2,6 +2,28 @@ defmodule Pow.Ecto.ContextTest do
   use Pow.Test.Ecto.TestCase
   doctest Pow.Ecto.Context
 
+  defmodule TimingAttackUser do
+    @moduledoc false
+    use Ecto.Schema
+    use Pow.Ecto.Schema, password_hash_methods: {&__MODULE__.send_hash_password/1, &__MODULE__.send_verify_password/2}
+
+    schema "users" do
+      pow_user_fields()
+
+      timestamps()
+    end
+
+    def send_hash_password(password) do
+      send(self(), {:password_hash, password})
+      Pow.Ecto.Schema.Password.pbkdf2_hash(password)
+    end
+
+    def send_verify_password(password, password_hash) do
+      send(self(), {:password_verify, password, password_hash})
+      Pow.Ecto.Schema.Password.pbkdf2_verify(password, password_hash)
+    end
+  end
+
   alias Ecto.Changeset
   alias Pow.Ecto.{Context, Schema.Password}
   alias Pow.Test.Ecto.{Repo, Users, Users.User, Users.UsernameUser}
@@ -71,6 +93,17 @@ defmodule Pow.Ecto.ContextTest do
       assert Users.authenticate(@valid_params) == user
       assert Users.authenticate(:test_macro) == :ok
     end
+
+    test "prevents timing attack" do
+      config = [repo: Repo, user: TimingAttackUser]
+
+      refute Context.authenticate(Map.put(@valid_params, "email", "[email protected]"), config)
+      assert_received {:password_hash, ""}
+      refute Context.authenticate(Map.put(@valid_params, "password", "invalid"), config)
+      assert_received {:password_verify, "invalid", _any}
+      assert Context.authenticate(@valid_params, config)
+      assert_received {:password_verify, "secret1234", _any}
+    end
   end
 
   describe "create/2" do

test/pow/ecto/schema/changeset_test.exs

@@ -184,11 +184,36 @@ defmodule Pow.Ecto.Schema.ChangesetTest do
     end
   end
 
-  test "User.verify_password/2" do
-    refute User.verify_password(%User{}, "secret1234")
+  describe "User.verify_password/2" do
+    test "verifies" do
+      refute User.verify_password(%User{}, "secret1234")
 
-    password_hash = Password.pbkdf2_hash("secret1234")
-    refute User.verify_password(%User{password_hash: password_hash}, "invalid")
-    assert User.verify_password(%User{password_hash: password_hash}, "secret1234")
+      password_hash = Password.pbkdf2_hash("secret1234")
+      refute User.verify_password(%User{password_hash: password_hash}, "invalid")
+      assert User.verify_password(%User{password_hash: password_hash}, "secret1234")
+    end
+
+    test "prevents timing attacks" do
+      config = [
+        password_hash_methods: {
+          fn password ->
+            send(self(), {:password_hash, password})
+
+            ""
+          end,
+          fn password, password_hash ->
+            send(self(), {:password_verify, password, password_hash})
+
+            false
+          end
+        }
+      ]
+
+      refute Changeset.verify_password(%User{password_hash: nil}, "secret1234", config)
+      assert_received {:password_hash, ""}
+
+      refute Changeset.verify_password(%User{password_hash: "hash"}, "secret1234", config)
+      assert_received {:password_verify, "secret1234", "hash"}
+    end
   end
 end