Use XChaCha20 Poly1305 in message encryptor and ignore sign_secret #36

josevalim · 2023-10-04T22:32:42Z

No description provided.

This reverts commit 370ce26.

josevalim · 2023-10-05T07:14:42Z

lib/plug/crypto/message_encryptor.ex

  A custom authentication message can be provided.
  It defaults to "A128GCM" for backwards compatibility.
  """
  def encrypt(message, aad \\ "A128GCM", secret, sign_secret)
-      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
+             bit_size(secret) in [128, 192, 256] and


My only question for now is if this still applies to CHACHA20 Poly1305. What is their supported key (secret) sizes?

Only 256-bit keys are supported (it's roughly the security equivalent of switching to AES-256-GCM).

potatosalad · 2023-10-05T12:50:02Z

XChaCha20-Poly1305 requires an extra step prior to encrypt/decrypt as shown here.

The extra functions needed are hchacha20/2 and xchacha20_subkey_and_nonce/2, I can help port them to Elixir if that's helpful, too.

Some questions:

Do we want to maintain backwards compatibility with older encrypted messages for a period of time?
For any secret that is not exactly 256-bits, do we want to fallback to using HKDF? Below is a really basic example implementation based on SHA-256.

# IKM = Input Keying Material
# PRK = Pseudorandom Key
# OKM = Output Keying Material
def hkdf(ikm, salt, info, output_length) do
  prk = extract(ikm, salt)
  okm = expand(prk, info, output_length)
  okm
end

defp extract(ikm, <<>>), do: extract(ikm, <<0::256>>)
defp extract(ikm, salt), do: :crypto.mac(:hmac, :sha256, salt, ikm)

defp expand(prk, info, output_length) do
  rounds = calculate_rounds(output_length)
  <<okm::bytes-size(output_length), _::bytes>> = expand(prk, info, 1, rounds, <<>>, <<>>)
  okm
end

defp expand(_prk, _info, i, rounds, _prev, next) when i > rounds do
  next
end

defp expand(prk, info, i, rounds, prev, next) do
  ti = :crypto.mac(:hmac, :sha256, prk, <<prev::bytes, info::bytes, i::8>>)
  expand(prk, info, i + 1, rounds, ti, <<next::bytes, ti::bytes>>)
end

defp calculate_rounds(x) do
  n = div(x, 32)

  if rem(x, 32) === 0 do
    n
  else
    n + 1
  end
end

Usage could look like this:

defp maybe_derive_subkey(secret_key) when bit_size(secret_key) === 256,
  do: secret_key
defp maybe_derive_subkey(secret_key) when is_binary(secret_key),
  do: hkdf(secret_key, <<>>, "Plug.Crypto", 32)

btoews

Thanks for putting this together so quickly. This seems like a big improvement to me.

lib/plug/crypto/message_encryptor.ex

btoews · 2023-10-05T13:20:07Z

lib/plug/crypto/message_encryptor.ex

      {: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)


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

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

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

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.

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.

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.

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. :)

lib/plug/crypto/message_encryptor.ex

josevalim · 2023-10-05T13:43:13Z

@btoews feedback incorporated.

@potatosalad thanks, I will incorporate those additions soon. Meanwhile, answers:

Do we want to maintain backwards compatibility with older encrypted messages for a period of time?

Yeah, we are currently doing that (I also included a test)

For any secret that is not exactly 256-bits, do we want to fallback to using HKDF? Below is a really basic example implementation based on SHA-256.

This code has always assumed proper key lengths, so I plan to continue doing that. The high-level functions in Plug.Crypto do that and a bit more. :)

josevalim · 2023-10-05T13:59:04Z

@potatosalad I have pushed xchacha, thank you for sharing. Please let me know how it looks like :)

lib/plug/crypto/message_encryptor.ex

potatosalad

LGTM. Based on past changes to the algorithms used in 2014 (elixir-plug/plug#72) and 2016 (elixir-plug/plug#420), maybe this will be revisited again in another 2-7 years when some future encryption best practices have changed 😄

potatosalad · 2023-10-05T14:33:28Z

lib/plug/crypto/message_encryptor.ex

+      :crypto.crypto_one_time(:chacha20, key, nonce, mask, true)
+
+    <<
+      x00::32-unsigned-little-integer,


I just now realized that it can be written as <<x::32-unsigned-little-integer>> instead of <<x::unsigned-little-integer-size(32)>> 😮

potatosalad · 2023-10-05T17:04:04Z

lib/plug/crypto/message_encryptor.ex

-  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)
+  # Messages from Plug.Crypto v1.x
+  defp unguarded_decrypt("QTEyOEdDTQ." <> rest, aad, secret, sign_secret) do


josevalim · 2023-10-05T17:04:46Z

@btoews @potatosalad I have incorporated all of your feedback. I would love if you could do another careful pass and let me know if you are happy with it (if you feel comfortable, feel free to approve it too).

potatosalad

LGTM. I wish there was a way to get rid of the aad = "A128GCM" nicely without breaking backwards compatibility so that it won't confuse folks a few years in the future, but it otherwise won't affect anything performance-wise. The introduction of something like %Opts{} was the only thing I could think of that would preserve backwards compatibility while also allowing us to eventually drop the default aad and sign_secret in the future.

This was a fast turnaround, I hope sleep is a little easier tonight 😄

potatosalad · 2023-10-05T18:43:02Z

As a nice side-effect of switching to XChaCha20-Poly1305 and simplifying the encoded format, the overhead size has shrunk nearly 50% compared to the old AES-128-GCM with key-wrapping implementation.

Old Format Overhead: 111-bytes (crypto overhead) + ceil(n / 3) * 4-bytes (base64 overhead, worst case)
New Format Overhead: 57-bytes (crypto overhead) + ceil(n / 3) * 4-bytes (base64 overhead, worst case)

Example tokens:

# 14-bytes
msg = <<0, "hełłoworld", 0>>
# 130-bytes = 111-bytes (crypto overhead) + 19-bytes (base64 overhead)
old = "QTEyOEdDTQ.L85cCXPvSqswNJoxmP5QTopFY83qCPj9czxkwct8b0HDHdC8Qwruhkq3SWw.mmqfbc2dfaMMi6Xi.n1qvYhAUYI0r7-QB6Vw.0jV2tT3U-AQMAQSch2rNsw"
# 76-bytes = 57-bytes (crypto overhead) + 19-bytes (base64 overhead)
new = "XCP.8A4k2vrqYJBKsaaU1RodMRyjOQjBVyoQjTENTQSpuTcWThFE5eOj4OeM1oQAmbTKQ52LqKT7"

josevalim · 2023-10-06T09:01:16Z

💚 💙 💜 💛 ❤️

No longer use sign_secret in MessageEncryptor

64676a4

josevalim mentioned this pull request Oct 4, 2023

MessageEncryptor design concerns #35

Closed

josevalim force-pushed the main branch from a3df6d4 to 1245751 Compare October 4, 2023 22:44

josevalim added 3 commits October 5, 2023 09:09

Use ChaCha20 & Poly1305

0bbd7e9

Proper key size check

370ce26

Revert "Proper key size check"

8c9efe9

This reverts commit 370ce26.

josevalim commented Oct 5, 2023

View reviewed changes

josevalim changed the title ~~No longer use sign_secret in MessageEncryptor~~ Use ChaCha20 pOly1305 in message encryptor and ignore sign_secret Oct 5, 2023

josevalim changed the title ~~Use ChaCha20 pOly1305 in message encryptor and ignore sign_secret~~ Use ChaCha20 Poly1305 in message encryptor and ignore sign_secret Oct 5, 2023

btoews approved these changes Oct 5, 2023

View reviewed changes

More feedback

8943fe6

xchacha

f632d1c

josevalim changed the title ~~Use ChaCha20 Poly1305 in message encryptor and ignore sign_secret~~ Use XChaCha20 Poly1305 in message encryptor and ignore sign_secret Oct 5, 2023

potatosalad reviewed Oct 5, 2023

View reviewed changes

lib/plug/crypto/message_encryptor.ex Outdated Show resolved Hide resolved

potatosalad approved these changes Oct 5, 2023

View reviewed changes

potatosalad reviewed Oct 5, 2023

View reviewed changes

Simplify encoding

cd4af5d

potatosalad reviewed Oct 5, 2023

View reviewed changes

potatosalad approved these changes Oct 5, 2023

View reviewed changes

josevalim merged commit ae684cd into main Oct 6, 2023
2 checks passed

josevalim deleted the jv-no-sign-secret-encrypt branch October 6, 2023 09:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use XChaCha20 Poly1305 in message encryptor and ignore sign_secret #36

Use XChaCha20 Poly1305 in message encryptor and ignore sign_secret #36

josevalim commented Oct 4, 2023

josevalim Oct 5, 2023

potatosalad Oct 5, 2023

potatosalad commented Oct 5, 2023 •

edited

Loading

btoews left a comment

btoews Oct 5, 2023

josevalim Oct 5, 2023

potatosalad Oct 5, 2023

josevalim Oct 5, 2023

potatosalad Oct 5, 2023

potatosalad Oct 5, 2023 •

edited

Loading

josevalim Oct 5, 2023

josevalim commented Oct 5, 2023

josevalim commented Oct 5, 2023

potatosalad left a comment •

edited

Loading

potatosalad Oct 5, 2023

potatosalad Oct 5, 2023

josevalim commented Oct 5, 2023

potatosalad left a comment

potatosalad commented Oct 5, 2023

josevalim commented Oct 6, 2023

Use XChaCha20 Poly1305 in message encryptor and ignore sign_secret #36

Use XChaCha20 Poly1305 in message encryptor and ignore sign_secret #36

Conversation

josevalim commented Oct 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

potatosalad commented Oct 5, 2023 • edited Loading

btoews left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

potatosalad Oct 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josevalim commented Oct 5, 2023

josevalim commented Oct 5, 2023

potatosalad left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josevalim commented Oct 5, 2023

potatosalad left a comment

Choose a reason for hiding this comment

potatosalad commented Oct 5, 2023

josevalim commented Oct 6, 2023

potatosalad commented Oct 5, 2023 •

edited

Loading

potatosalad Oct 5, 2023 •

edited

Loading

potatosalad left a comment •

edited

Loading