Use builtin :crypto functions (when available) for PBKDF2 and secure constant-time comparison (#37)

Author: potatosalad

lib/plug/crypto.ex

@@ -6,7 +6,6 @@ defmodule Plug.Crypto do
   `Plug.Crypto.MessageEncryptor`, and `Plug.Crypto.MessageVerifier`.
   """
 
-  import Bitwise
   alias Plug.Crypto.{KeyGenerator, MessageVerifier, MessageEncryptor}
 
   @doc """
@@ -113,16 +112,12 @@ defmodule Plug.Crypto do
   @spec masked_compare(binary(), binary(), binary()) :: boolean()
   def masked_compare(left, right, mask)
       when is_binary(left) and is_binary(right) and is_binary(mask) do
-    byte_size(left) == byte_size(right) and masked_compare(left, right, mask, 0)
+    byte_size(left) == byte_size(right) and byte_size(right) == byte_size(mask) and
+      crypto_exor_hash_equals(left, right, mask)
   end
 
-  defp masked_compare(<<x, left::binary>>, <<y, right::binary>>, <<z, mask::binary>>, acc) do
-    xorred = bxor(x, bxor(y, z))
-    masked_compare(left, right, mask, acc ||| xorred)
-  end
-
-  defp masked_compare(<<>>, <<>>, <<>>, acc) do
-    acc === 0
+  defp crypto_exor_hash_equals(x, y, z) do
+    crypto_hash_equals(mask(x, y), z)
   end
 
   @doc """
@@ -132,16 +127,28 @@ defmodule Plug.Crypto do
   """
   @spec secure_compare(binary(), binary()) :: boolean()
   def secure_compare(left, right) when is_binary(left) and is_binary(right) do
-    byte_size(left) == byte_size(right) and secure_compare(left, right, 0)
+    byte_size(left) == byte_size(right) and crypto_hash_equals(left, right)
   end
 
-  defp secure_compare(<<x, left::binary>>, <<y, right::binary>>, acc) do
-    xorred = bxor(x, y)
-    secure_compare(left, right, acc ||| xorred)
-  end
+  # TODO: remove when we require OTP 25.0
+  if Code.ensure_loaded?(:crypto) and function_exported?(:crypto, :hash_equals, 2) do
+    defp crypto_hash_equals(x, y) do
+      :crypto.hash_equals(x, y)
+    end
+  else
+    defp crypto_hash_equals(x, y) do
+      legacy_secure_compare(x, y, 0)
+    end
+
+    defp legacy_secure_compare(<<x, left::binary>>, <<y, right::binary>>, acc) do
+      import Bitwise
+      xorred = bxor(x, y)
+      legacy_secure_compare(left, right, acc ||| xorred)
+    end
 
-  defp secure_compare(<<>>, <<>>, acc) do
-    acc === 0
+    defp legacy_secure_compare(<<>>, <<>>, acc) do
+      acc === 0
+    end
   end
 
   @doc """

lib/plug/crypto/key_generator.ex

@@ -31,15 +31,20 @@ defmodule Plug.Crypto.KeyGenerator do
 
   """
   def generate(secret, salt, opts \\ []) do
+    force_legacy_mode = Keyword.get(opts, :force_legacy_mode, false)
     iterations = Keyword.get(opts, :iterations, 1000)
     length = Keyword.get(opts, :length, 32)
     digest = Keyword.get(opts, :digest, :sha256)
     cache = Keyword.get(opts, :cache)
-    generate(secret, salt, iterations, length, digest, cache)
+    generate(secret, salt, iterations, length, digest, force_legacy_mode, cache)
   end
 
   @doc false
   def generate(secret, salt, iterations, length, digest, cache) do
+    generate(secret, salt, iterations, length, digest, false, cache)
+  end
+
+  defp generate(secret, salt, iterations, length, digest, force_legacy_mode, cache) do
     cond do
       not is_integer(iterations) or iterations < 1 ->
         raise ArgumentError, "iterations must be an integer >= 1"
@@ -49,13 +54,23 @@ defmodule Plug.Crypto.KeyGenerator do
 
       true ->
         with_cache(cache, {secret, salt, iterations, length, digest}, fn ->
-          generate(hmac_fun(digest, secret), salt, iterations, length, 1, [], 0)
+          pbkdf2_hmac(digest, secret, salt, iterations, length, force_legacy_mode)
         end)
     end
   rescue
     e -> reraise e, Plug.Crypto.prune_args_from_stacktrace(__STACKTRACE__)
   end
 
+  defp pbkdf2_hmac(digest, secret, salt, iterations, length, force_legacy_mode) do
+    # TODO: remove when we require OTP 24.2
+    if not force_legacy_mode and Code.ensure_loaded?(:crypto) and
+         function_exported?(:crypto, :pbkdf2_hmac, 5) do
+      :crypto.pbkdf2_hmac(digest, secret, salt, iterations, length)
+    else
+      legacy_pbkdf2_hmac(hmac_fun(digest, secret), salt, iterations, length, 1, [], 0)
+    end
+  end
+
   defp with_cache(nil, _key, fun), do: fun.()
 
   defp with_cache(ets, key, fun) do
@@ -70,19 +85,19 @@ defmodule Plug.Crypto.KeyGenerator do
     end
   end
 
-  defp generate(_fun, _salt, _iterations, max_length, _block_index, acc, length)
+  defp legacy_pbkdf2_hmac(_fun, _salt, _iterations, max_length, _block_index, acc, length)
        when length >= max_length do
     acc
     |> IO.iodata_to_binary()
     |> binary_part(0, max_length)
   end
 
-  defp generate(fun, salt, iterations, max_length, block_index, acc, length) do
+  defp legacy_pbkdf2_hmac(fun, salt, iterations, max_length, block_index, acc, length) do
     initial = fun.(<<salt::binary, block_index::integer-size(32)>>)
     block = iterate(fun, iterations - 1, initial, initial)
     length = byte_size(block) + length
 
-    generate(fun, salt, iterations, max_length, block_index + 1, [acc | block], length)
+    legacy_pbkdf2_hmac(fun, salt, iterations, max_length, block_index + 1, [acc | block], length)
   end
 
   defp iterate(_fun, 0, _prev, acc), do: acc

test/plug/crypto/key_generator_test.exs

@@ -1,7 +1,6 @@
 defmodule Plug.Crypto.KeyGeneratorTest do
   use ExUnit.Case, async: true
 
-  import Plug.Crypto.KeyGenerator
   import Bitwise
 
   @max_length bsl(1, 32) - 1
@@ -64,5 +63,49 @@ defmodule Plug.Crypto.KeyGeneratorTest do
              "6e88be8bad7eae9d9e10aa061224034fed48d03fcbad968b56006784539d5214ce970d912ec2049b04231d47c2eb88506945b26b2325e6adfeeba08895ff9587"
   end
 
+  test "digest :sha works" do
+    key = generate("password", "salt", iterations: 1, length: 16, digest: :sha)
+    assert byte_size(key) == 16
+    assert to_hex(key) == "0c60c80f961f0e71f3a9b524af601206"
+  end
+
+  test "digest :sha224 works" do
+    key = generate("password", "salt", iterations: 1, length: 16, digest: :sha224)
+    assert byte_size(key) == 16
+    assert to_hex(key) == "3c198cbdb9464b7857966bd05b7bc92b"
+  end
+
+  test "digest :sha256 works" do
+    key = generate("password", "salt", iterations: 1, length: 16, digest: :sha256)
+    assert byte_size(key) == 16
+    assert to_hex(key) == "120fb6cffcf8b32c43e7225256c4f837"
+  end
+
+  test "digest :sha384 works" do
+    key = generate("password", "salt", iterations: 1, length: 16, digest: :sha384)
+    assert byte_size(key) == 16
+    assert to_hex(key) == "c0e14f06e49e32d73f9f52ddf1d0c5c7"
+  end
+
+  test "digest :sha512 works" do
+    key = generate("password", "salt", iterations: 1, length: 16, digest: :sha512)
+    assert byte_size(key) == 16
+    assert to_hex(key) == "867f70cf1ade02cff3752599a3a53dc4"
+  end
+
+  def generate(secret, salt, opts \\ []) do
+    key = Plug.Crypto.KeyGenerator.generate(secret, salt, opts)
+
+    legacy_key =
+      Plug.Crypto.KeyGenerator.generate(
+        secret,
+        salt,
+        Keyword.merge(opts, force_legacy_mode: true)
+      )
+
+    assert key == legacy_key
+    key
+  end
+
   def to_hex(value), do: Base.encode16(value, case: :lower)
 end