Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use XChaCha20 Poly1305 in message encryptor and ignore sign_secret #36

Merged
merged 7 commits into from
Oct 6, 2023
Merged

Use XChaCha20 Poly1305 in message encryptor and ignore sign_secret #36

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
64676a4
No longer use sign_secret in MessageEncryptor
josevalim Oct 4, 2023
0bbd7e9
Use ChaCha20 & Poly1305
josevalim Oct 5, 2023
370ce26
Proper key size check
josevalim Oct 5, 2023
8c9efe9
Revert "Proper key size check"
josevalim Oct 5, 2023
8943fe6
More feedback
josevalim Oct 5, 2023
f632d1c
xchacha
josevalim Oct 5, 2023
cd4af5d
Simplify encoding
josevalim Oct 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions lib/plug/crypto/application.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
defmodule Plug.Crypto.Application do
@moduledoc false
use Application

def start(_, _) do
Expand Down
150 changes: 45 additions & 105 deletions lib/plug/crypto/message_encryptor.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,34 +9,37 @@ defmodule Plug.Crypto.MessageEncryptor do
This can be used in situations similar to the `Plug.Crypto.MessageVerifier`,
but where you don't want users to be able to determine the value of the payload.

The current algorithm used is AES-GCM-128.
The current algorithm used is ChaCha20-Poly1305.

## Example

iex> secret_key_base = "072d1e0157c008193fe48a670cce031faa4e..."
...> encrypted_cookie_salt = "encrypted cookie"
...> encrypted_signed_cookie_salt = "signed encrypted cookie"
...>
...> secret = KeyGenerator.generate(secret_key_base, encrypted_cookie_salt)
...> sign_secret = KeyGenerator.generate(secret_key_base, encrypted_signed_cookie_salt)
...>
...> data = "José"
...> encrypted = MessageEncryptor.encrypt(data, secret, sign_secret)
...> MessageEncryptor.decrypt(encrypted, secret, sign_secret)
...> encrypted = MessageEncryptor.encrypt(data, secret, "UNUSED")
...> MessageEncryptor.decrypt(encrypted, secret, "UNUSED")
{:ok, "José"}

"""

@doc """
Encrypts a message using authenticated encryption.

The `sign_secret` is currently only used on decryption
for backwards compatibility.

A custom authentication message can be provided.
It defaults to "A128GCM" for backwards compatibility.
"""
def encrypt(message, aad \\ "A128GCM", secret, sign_secret)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's probably hard to change or get rid of the default AAD without breaking backwards compatibility, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct. The default value doesn't matter much anyway I think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's probably fine to change the default here to <<>>, the default aad only matters during decrypt for backwards compatibility.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue is that, if we change it here, we need to change it in decrypt too, no? Otherwise we can't decrypt what we encrypt if they mismatch.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, gotcha, I was thinking along the lines whether it was even needed or not (as in: remove it entirely from encrypt and default to <<>> or some other fixed value going forward for v2+, but leave it for backwards compatibility under decrypt for now).

I wasn't sure if it's a feature people use or not.

Copy link
Contributor

@potatosalad potatosalad Oct 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that I'm thinking about it, couldn't we do something like this?

Old suggestion: 
def encrypt(message, secret) do
  encrypt(message, <<>>, secret, "UNUSED")
end

@deprecated "Use Plug.Crypto.MessageEncryptor.encrypt/2 instead"
def encrypt(message, secret, sign_secret) do
  encrypt(message, <<>>, secret, sign_secret)
end

@deprecated "Use Plug.Crypto.MessageEncryptor.encrypt/2 instead"
def encrypt(message, aad, secret, sign_secret) do
  # don't use sign_secret at all
  # ...
end

def decrypt(encrypted, secret) do
  decrypt(encrypted, nil, secret, "UNUSED")
end

@deprecated "Use Plug.Crypto.MessageEncryptor.decrypt/2 instead"
def decrypt(encrypted, secret, sign_secret) do
  decrypt(encrypted, nil, secret, sign_secret)
end

@deprecated "Use Plug.Crypto.MessageEncryptor.decrypt/2 instead"
def decrypt(encrypted, aad, secret, sign_secret) do
  case :binary.split(encrypted, ".", [:global]) do
    # Messages from Plug.Crypto v2.x
    ["XCP", iv, cipher_text, cipher_tag] ->
      aad = if is_nil(aad), do: <<>>, else: aad
      # ...

    # Messages from Plug.Crypto v1.x
    [protected, encrypted_key, iv, cipher_text, cipher_tag] ->
      aad = if is_nil(aad), do: "A128GCM", else: aad
      # ...
  end
end
New suggestion (maybe introduce %Opts{}?): 
defmodule Opts do
  defstruct aad: <<>>, legacy_sign_secret: nil
  @type t :: %__MODULE__{aad: nil | binary, legacy_sign_secret: nil | binary}
end

def encrypt(message, secret) do
  encrypt(message, secret, %Opts{})
end

def encrypt(message, secret, sign_secret) when is_binary(sign_secret) do
  # discard sign_secret, maybe warn user about deprecation?
  encrypt(message, secret)
end

def encrypt(message, secret, %Opts{aad: aad}) when is_binary(aad) do
  # XChaCha20-Poly1305 encrypt ...
end

@deprecated "Use Plug.Crypto.MessageEncryptor.encrypt/2,3 instead"
def encrypt(message, aad, secret, sign_secret) do
  # discard sign_secret, maybe warn user about deprecation?
  encrypt(message, secret, %Opts{aad: aad})
end

def decrypt(encrypted, secret) do
  decrypt(encrypted, secret, %Opts{aad: nil})
end

def decrypt(encrypted, secret, sign_secret) when is_binary(sign_secret) do
  decrypt(encrypted, secret, %Opts{aad: nil, legacy_sign_secret: sign_secret})
end

def decrypt(encrypted, secret, %Opts{aad: aad, legacy_sign_secret: sign_secret}) do
  case :binary.split(encrypted, ".", [:global]) do
    # Messages from Plug.Crypto v2.x
    [packed] ->
      case Base.url_decode64(packed, padding: false) do
        {:ok, <<2::8, iv::192-bits, cipher_tag::128-bits, cipher_text::bytes>>} ->
          aad = if is_nil(aad), do: <<>>, else: aad
          # XChaCha20-Poly1305 decrypt ...
        
        _ ->
          # ...
      end

    # Messages from Plug.Crypto v1.x
    [protected, encrypted_key, iv, cipher_text, cipher_tag] ->
      aad = if is_nil(aad), do: "A128GCM", else: aad
      # AES-128-GCM decrypt ...
  end
end

@deprecated "Use Plug.Crypto.MessageEncryptor.decrypt/2,3 instead"
def decrypt(encrypted, aad, secret, sign_secret) do
  decrypt(encrypted, secret, %Opts{aad: aad, legacy_sign_secret: sign_secret})
end

NOTE: aad doesn't seem to be used by any other libraries I can find on GH, with the exception of Livebook.Stamping.aead_encrypt/3.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it was recently added and Livebook was the main motivation for it. :) Unless having a non-empty default is problematic or harmful to performance, I would keep it. :)

when is_binary(message) and (is_binary(aad) or is_list(aad)) and byte_size(secret) > 0 and
when is_binary(message) and (is_binary(aad) or is_list(aad)) and
byte_size(secret) == 32 and
is_binary(sign_secret) do
aes128_gcm_encrypt(message, aad, secret, sign_secret)
iv = :crypto.strong_rand_bytes(12)
{cipher_text, cipher_tag} = block_encrypt(:chacha20_poly1305, secret, iv, {aad, message})
encode_token("CP20", iv, cipher_text, cipher_tag)
rescue
e -> reraise e, Plug.Crypto.prune_args_from_stacktrace(__STACKTRACE__)
end
Expand All @@ -45,67 +48,44 @@ defmodule Plug.Crypto.MessageEncryptor do
Decrypts a message using authenticated encryption.
"""
def decrypt(encrypted, aad \\ "A128GCM", secret, sign_secret)
when is_binary(encrypted) and (is_binary(aad) or is_list(aad)) and byte_size(secret) > 0 and
when is_binary(encrypted) and (is_binary(aad) or is_list(aad)) and
bit_size(secret) in [128, 192, 256] and
is_binary(sign_secret) do
aes128_gcm_decrypt(encrypted, aad, secret, sign_secret)
rescue
e -> reraise e, Plug.Crypto.prune_args_from_stacktrace(__STACKTRACE__)
end

# Encrypts and authenticates a message using AES128-GCM mode.
#
# A random 128-bit content encryption key (CEK) is generated for
# every message which is then encrypted with `aes_gcm_key_wrap/3`.
defp aes128_gcm_encrypt(plain_text, aad, secret, sign_secret) when bit_size(secret) > 256 do
aes128_gcm_encrypt(plain_text, aad, binary_part(secret, 0, 32), sign_secret)
end

defp aes128_gcm_encrypt(plain_text, aad, secret, sign_secret)
when is_binary(plain_text) and bit_size(secret) in [128, 192, 256] and
is_binary(sign_secret) do
key = :crypto.strong_rand_bytes(16)
iv = :crypto.strong_rand_bytes(12)
{cipher_text, cipher_tag} = block_encrypt(:aes_gcm, key, iv, {aad, plain_text})
encrypted_key = aes_gcm_key_wrap(key, secret, sign_secret)
encode_token("A128GCM", encrypted_key, iv, cipher_text, cipher_tag)
end

# Verifies and decrypts a message using AES128-GCM mode.
#
# Decryption will never be performed prior to verification.
#
# The encrypted content encryption key (CEK) is decrypted
# with `aes_gcm_key_unwrap/3`.
defp aes128_gcm_decrypt(cipher_text, aad, secret, sign_secret) when bit_size(secret) > 256 do
aes128_gcm_decrypt(cipher_text, aad, binary_part(secret, 0, 32), sign_secret)
end

defp aes128_gcm_decrypt(cipher_text, aad, secret, sign_secret)
when is_binary(cipher_text) and bit_size(secret) in [128, 192, 256] and
is_binary(sign_secret) do
case decode_token(cipher_text) do
{"A128GCM", encrypted_key, iv, cipher_text, cipher_tag}
when bit_size(iv) === 96 and bit_size(cipher_tag) === 128 ->
encrypted_key
|> aes_gcm_key_unwrap(secret, sign_secret)
|> case do
{:ok, key} ->
block_decrypt(:aes_gcm, key, iv, {aad, cipher_text, cipher_tag})

_ ->
:error
case :binary.split(encrypted, ".", [:global]) do
# Messages from Plug.Crypto v2.x
["CP20", iv, cipher_text, cipher_tag] ->
with {:ok, iv} when bit_size(iv) === 96 <- Base.url_decode64(iv, padding: false),
{:ok, cipher_text} <- Base.url_decode64(cipher_text, padding: false),
{:ok, cipher_tag} when bit_size(cipher_tag) === 128 <-
Base.url_decode64(cipher_tag, padding: false),
plain_text when is_binary(plain_text) <-
block_decrypt(:chacha20_poly1305, secret, iv, {aad, cipher_text, cipher_tag}) do
{:ok, plain_text}
else
_ -> :error
end
|> case do
plain_text when is_binary(plain_text) ->
{:ok, plain_text}

_ ->
:error
# Messages from Plug.Crypto v1.x
[protected, encrypted_key, iv, cipher_text, cipher_tag] ->
with {:ok, "A128GCM"} <- Base.url_decode64(protected, padding: false),
{:ok, encrypted_key} <- Base.url_decode64(encrypted_key, padding: false),
{:ok, iv} when bit_size(iv) === 96 <- Base.url_decode64(iv, padding: false),
{:ok, cipher_text} <- Base.url_decode64(cipher_text, padding: false),
{:ok, cipher_tag} when bit_size(cipher_tag) === 128 <-
Base.url_decode64(cipher_tag, padding: false),
{:ok, key} <- aes_gcm_key_unwrap(encrypted_key, secret, sign_secret),
plain_text when is_binary(plain_text) <-
block_decrypt(:aes_gcm, key, iv, {aad, cipher_text, cipher_tag}) do
{:ok, plain_text}
else
_ -> :error
end

_ ->
:error
end
rescue
e -> reraise e, Plug.Crypto.prune_args_from_stacktrace(__STACKTRACE__)
end

defp block_encrypt(cipher, key, iv, {aad, payload}) do
Expand All @@ -132,32 +112,11 @@ defmodule Plug.Crypto.MessageEncryptor do
"Please make sure it was compiled with the correct OpenSSL/BoringSSL bindings"
end

# Wraps a decrypted content encryption key (CEK) with secret and
# sign_secret using AES GCM mode. Accepts keys of 128, 192, or
# 256 bits based on the length of the secret key.
#
# See: https://tools.ietf.org/html/rfc7518#section-4.7
defp aes_gcm_key_wrap(cek, secret, sign_secret) when bit_size(secret) > 256 do
aes_gcm_key_wrap(cek, binary_part(secret, 0, 32), sign_secret)
end

defp aes_gcm_key_wrap(cek, secret, sign_secret)
when bit_size(cek) in [128, 192, 256] and bit_size(secret) in [128, 192, 256] and
is_binary(sign_secret) do
iv = :crypto.strong_rand_bytes(12)
{cipher_text, cipher_tag} = block_encrypt(:aes_gcm, secret, iv, {sign_secret, cek})
cipher_text <> cipher_tag <> iv
end

# Unwraps an encrypted content encryption key (CEK) with secret and
# sign_secret using AES GCM mode. Accepts keys of 128, 192, or 256
# bits based on the length of the secret key.
#
# See: https://tools.ietf.org/html/rfc7518#section-4.7
defp aes_gcm_key_unwrap(wrapped_cek, secret, sign_secret) when bit_size(secret) > 256 do
aes_gcm_key_unwrap(wrapped_cek, binary_part(secret, 0, 32), sign_secret)
end

defp aes_gcm_key_unwrap(wrapped_cek, secret, sign_secret)
when bit_size(secret) in [128, 192, 256] and is_binary(sign_secret) do
wrapped_cek
Expand All @@ -175,37 +134,18 @@ defmodule Plug.Crypto.MessageEncryptor do
:error
end
|> case do
cek when bit_size(cek) in [128, 192, 256] ->
{:ok, cek}

_ ->
:error
cek when bit_size(cek) in [128, 192, 256] -> {:ok, cek}
_ -> :error
end
end

defp encode_token(protected, encrypted_key, iv, cipher_text, cipher_tag) do
Base.url_encode64(protected, padding: false)
|> Kernel.<>(".")
|> Kernel.<>(Base.url_encode64(encrypted_key, padding: false))
defp encode_token(head, iv, cipher_text, cipher_tag) do
josevalim marked this conversation as resolved.
Show resolved Hide resolved
head
|> Kernel.<>(".")
|> Kernel.<>(Base.url_encode64(iv, padding: false))
|> Kernel.<>(".")
|> Kernel.<>(Base.url_encode64(cipher_text, padding: false))
|> Kernel.<>(".")
|> Kernel.<>(Base.url_encode64(cipher_tag, padding: false))
end

defp decode_token(token) do
with [protected, encrypted_key, iv, cipher_text, cipher_tag] <-
String.split(token, ".", parts: 5),
{:ok, protected} <- Base.url_decode64(protected, padding: false),
{:ok, encrypted_key} <- Base.url_decode64(encrypted_key, padding: false),
{:ok, iv} <- Base.url_decode64(iv, padding: false),
{:ok, cipher_text} <- Base.url_decode64(cipher_text, padding: false),
{:ok, cipher_tag} <- Base.url_decode64(cipher_tag, padding: false) do
{protected, encrypted_key, iv, cipher_text, cipher_tag}
else
_ -> :error
end
end
end
57 changes: 12 additions & 45 deletions test/plug/crypto/message_encryptor_test.exs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,26 +7,13 @@ defmodule Plug.Crypto.MessageEncryptorTest do

@right String.duplicate("abcdefgh", 4)
@wrong String.duplicate("12345678", 4)
@large String.duplicate(@right, 2)

test "it encrypts/decrypts a message" do
data = <<0, "hełłoworld", 0>>
encrypted = ME.encrypt(data, "right aad", @right, @right)

decrypted = ME.decrypt(encrypted, "right aad", @wrong, @wrong)
assert decrypted == :error

decrypted = ME.decrypt(encrypted, "right aad", @right, @wrong)
assert decrypted == :error

decrypted = ME.decrypt(encrypted, "right aad", @wrong, @right)
assert decrypted == :error

decrypted = ME.decrypt(encrypted, "wrong aad", @right, @right)
assert decrypted == :error

decrypted = ME.decrypt(encrypted, "right aad", @right, @right)
assert decrypted == {:ok, data}
encrypted = ME.encrypt(data, "right aad", @right, "UNUSED")
assert ME.decrypt(encrypted, "right aad", @wrong, "UNUSED") == :error
assert ME.decrypt(encrypted, "wrong aad", @right, "UNUSED") == :error
assert ME.decrypt(encrypted, "right aad", @right, "UNUSED") == {:ok, data}
end

test "it encrypts/decrypts with iodata aad" do
Expand All @@ -35,34 +22,14 @@ defmodule Plug.Crypto.MessageEncryptorTest do
assert ME.decrypt(encrypted, ["right", ?\s, "aad"], @right, @right) == {:ok, data}
end

test "it uses only the first 32 bytes to encrypt/decrypt" do
data = <<0, "helloworld", 0>>
encrypted = ME.encrypt(<<0, "helloworld", 0>>, @large, @large)

decrypted = ME.decrypt(encrypted, @large, @large)
assert decrypted == {:ok, data}

decrypted = ME.decrypt(encrypted, @right, @large)
assert decrypted == {:ok, data}
@old_message "QTEyOEdDTQ.L85cCXPvSqswNJoxmP5QTopFY83qCPj9czxkwct8b0HDHdC8Qwruhkq3SWw.mmqfbc2dfaMMi6Xi.n1qvYhAUYI0r7-QB6Vw.0jV2tT3U-AQMAQSch2rNsw"

decrypted = ME.decrypt(encrypted, @large, @right)
assert decrypted == :error

decrypted = ME.decrypt(encrypted, @right, @right)
assert decrypted == :error

encrypted = ME.encrypt(<<0, "helloworld", 0>>, @right, @large)

decrypted = ME.decrypt(encrypted, @large, @large)
assert decrypted == {:ok, data}

decrypted = ME.decrypt(encrypted, @right, @large)
assert decrypted == {:ok, data}

decrypted = ME.decrypt(encrypted, @large, @right)
assert decrypted == :error

decrypted = ME.decrypt(encrypted, @right, @right)
assert decrypted == :error
test "it decodes messages from earlier versions" do
data = <<0, "hełłoworld", 0>>
assert ME.decrypt(@old_message, "right aad", @right, @right) == {:ok, data}
assert ME.decrypt(@old_message, "wrong aad", @right, @right) == :error
assert ME.decrypt(@old_message, "right aad", @wrong, @right) == :error
assert ME.decrypt(@old_message, "right aad", @right, @wrong) == :error
assert ME.decrypt(@old_message, "right aad", @wrong, @wrong) == :error
end
end
6 changes: 0 additions & 6 deletions test/plug/crypto_test.exs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,11 +211,5 @@ defmodule Plug.CryptoTest do
signed2 = encrypt(@key, "secret", 1, signed_at: 0, key_digest: :sha512)
assert signed1 != signed2
end

test "passes key_length options to key generator" do
signed1 = encrypt(@key, "secret", 1, signed_at: 0, key_length: 16)
signed2 = encrypt(@key, "secret", 1, signed_at: 0, key_length: 32)
assert signed1 != signed2
end
end
end
Loading