elixir-plug / plug_crypto Goto Github PK

The result is encrypt and decrypt not working raising errors of not matching functions because of guards bit_size(256).

#8 makes the change. I think :crypto.exor is a faster operation.

This is unexpected:

iex> Plug.Crypto.sign "", "", ""                                
"SFMyNTY.g2gDbQAAAABuBgAWMyflcQFiAAFRgA.QFzpJfv3HeYdNU7fTjjbDhIVjrM9QTOFWWSytnA1v_E"

and this too:

iex> Plug.Crypto.encrypt "", "", ""
"QTEyOEdDTQ.i9CSgrZX2SkFNiK-20v3kLN-H3qHT2Dgq51fqV7YnXF-uR_shQPqBu0BpDg.9AaepQaobCgRQPL7.bdliPJ-iXyoFRcW_oKgn9HQOL3F4jw.wIX97k9OytAjfxiyIBR94A"

In my opinion none of the parameters should allow empty data.

This is also unexpected:

iex> Plug.Crypto.sign "", "", nil
"SFMyNTY.g2gDZAADbmlsbgYAgcUs5XEBYgABUYA.iVkYeAUWgvwfeEv5ZQjFhZgI7SobdUNMkwE2g8-OUZs"

and this too:

iex> Plug.Crypto.encrypt "", "", nil
"QTEyOEdDTQ.0qxI-t6Dfnmh_VIQwOLEDKj4xtjfjk-c0AaJzgKnYo5X8E8TLrdhQXPmD1s.XlTlQSSWdZyLRvkd.pfCwRXKFP6MGe8SyEfXJBNkQMV1EhI4.d53JPqhz8r3r9rtRD-tKWw"

I also think that here nil data should raise an error, as it raises when used as a secret or salt.

Have a look here:

https://hexdocs.pm/plug_crypto/Plug.Crypto.KeyGenerator.html

The "view source" links point to /plug/ instead of /plug_crypto/

This normal?

In MessageEncryptor, steps are taken as follows to encode session data:

1. Generate a 128-bit random key (the CEK) for encrypting the session. (ok)
2. Encrypt the session data with AES-128-GCM using the random key and AAD "A128GCM". (ok)
3. Encrypt the 128-bit random key from step 1 with AES-256-GCM using the 256-bit secret key as the key and the 256-bit signing key as AAD. (!)
4. Base-64 encode the generated token and format it for cookie storage (ok)

This isn't a security issue (I didn't find any issues with this implementation) but it is relevant to me because I am writing a session decoder, and it does not work without understanding that the CEK is encrypted using AES-256-GCM, so I think this should at least be mentioned in the comments in MessageEncryptor (and preferably, would also be mentioned in the module documentation).

From http://erlang.org/doc/apps/crypto/new_api.html:

The old functions - not recommended for new programs - are:

• block_encrypt/3
• block_encrypt/4
• block_decrypt/3
• block_decrypt/4

The new functions for encrypting or decrypting one single binary are:

• crypto_one_time/4
• crypto_one_time/5
• crypto_one_time_aead/6
• crypto_one_time_aead/7

The old API hasn't been deprecated, but the reference docs show a big red Don't box for old functions, e.g. http://erlang.org/doc/man/crypto.html#block_decrypt-3.

This could work in similar way to the PASETO where we have prefix:

• v1.public for Plug.Crypto.MessageVerifier messages
• v1.local for Plug.Crypto.MessageEncryptor messages

This would allow to change used algorithms in future if needed without introducing breaking changes.

Elixir 1.11.0 adds a few hard deprecations.

See https://github.com/elixir-lang/elixir/releases/tag/v1.11.0

[System] Deprecate System.stacktrace/0 in favor of STACKTRACE

==> plug_crypto
Compiling 5 files (.ex)
warning: System.stacktrace/0 is deprecated, use __STACKTRACE__ instead
  lib/plug/crypto/key_generator.ex:56

warning: System.stacktrace/0 is deprecated, use __STACKTRACE__ instead
  lib/plug/crypto/message_verifier.ex:24

warning: System.stacktrace/0 is deprecated, use __STACKTRACE__ instead
  lib/plug/crypto/message_verifier.ex:33

warning: System.stacktrace/0 is deprecated, use __STACKTRACE__ instead
  lib/plug/crypto/message_encryptor.ex:37

warning: System.stacktrace/0 is deprecated, use __STACKTRACE__ instead
  lib/plug/crypto/message_encryptor.ex:47

Generated plug_crypto app

I was reading through the MessageEncryptor implementation and have some concerns. Nothing appears to be outright broken, so I feel like it's appropriate to have this conversation in public.

The thing closest to being broken is that long keys are being truncated to 32 bytes:

This is pretty sketchy because you're reducing the user's key size without their awareness. AES has fixed key sizes, so it would be much better to return an error if a key of the wrong size is given. If you want to keep the API easy to use, you should pass keys through a KDF. Erlang's :crypto.pbkdf2_hmac/5 function would be suitable for this.

To give a concrete example of why this is concerning, imagine a user storing their key in PEM format for some reason:

-----BEGIN AES-256-GCM KEY-----
lvAGFJD1EpnaH+IkgrbWzQSpAWH/yT78/dN76cDzXOE=
-----END AES-256-GCM KEY-----

Passing that into MessageEncryptor, their key is now functionally -----BEGIN AES-256-GCM KEY-----\n.

My other concerns are with the overall encryption scheme. In pseudocode, the scheme is

def encrypt(message, aad // "A128GCM", secret, sign_secret)
  k1 = rand_bytes(16)
  iv1 = rand_bytes(12)
  {ct1, tag1} = aes_gcm(%{key: k1, iv: iv1, aad: "A128GCM", pt: message})

  iv2 = rand_bytes(12)
  {ct2, tag2} = aes_gcm(%{key: secret, iv: iv2, aad: sign_secret, pt: k1})

  [ct2, tag2, iv2, iv1, ct1, tag1]
    |> Enum.map(&Base.encode64/1)
    |> String.join(".")
end

There's a fair bit of undue complexity here. Most notably, sign_secret is serving no purpose as far as I can tell. GCM is an authenticated mode, so the ciphertext is already protected from tampering. I can't think of a scenario where users would benefit from the ability to roll these secrets separately. Requiring sign_secret does make the API more difficult to use though. Unless there's some purpose I'm missing, I'd recommend removing it. If there is a purpose, it would be nice to have it docummented.

I also question the key wrapping scheme. If you were expecting users to decompose the returned blob and store the first and second ciphertexsts separately, there's concievably some value to it. This is meant to be a black-box API though, so I don't imagine users are doing that. As with the sign_secret, I'd recommend docummenting any purpose to the key wrapping scheme and removing it if there is none.

In the same pseudocode, I think you'd be better served by the following implementation:

def encrypt(message, aad // "", secret, sign_secret)
  iv = rand_bytes(12)
  {ct, tag} = aes_gcm(%{key: secret, iv: iv, aad: aad, pt: message})

  [iv, ct, tag]
    |> Enum.map(&Base.encode64/1)
    |> String.join(".")
end

Hi,

I'd like to ask a quick question about how the derived key size check is handled.

I am not a cryptographer (that's why I ask), but I think it doesn't conform to the standard. The standard reads:

[...]
hLen: length in octets of pseudorandom function output, a positive integer
[....]
dkLen: intended length in octets of the derived key, a positive integer, at most (2^32 - 1) * hLen
[...]
If dkLen > (2^32 - 1) * hLen, output "derived key too long" and stop.

If it were to conform to the standard, it would do:

length > (max = @max_length * :crypto.hash_info(digest).size) ->
	raise ArgumentError, "length must be less than or equal to #{max} for #{digest}"

I assume.

Please tell me what you think! ^^

Cheers,
srfsh

mix test

12:33:08.706 [info]  Application plug exited: exited in: Plug.start(:normal, [])
    ** (EXIT) an exception was raised:
        ** (UndefinedFunctionError) function Plug.start/2 is undefined (module Plug is not available)
            Plug.start(:normal, [])
            (kernel 6.5.1) application_master.erl:277: :application_master.start_it_old/4

12:33:08.708 [info]  Application plug_crypto exited: :stopped

12:33:08.709 [info]  Application mime exited: :stopped

12:33:08.710 [info]  Application phoenix_pubsub exited: :stopped

12:33:08.710 [info]  Application telemetry exited: :stopped

12:33:08.711 [info]  Application eex exited: :stopped
** (MatchError) no match of right hand side value: {:error, {:plug, {:bad_return, {{Plug, :start, [:normal, []]}, {:EXIT, {:undef, [{Plug, :start, [:normal, []], []}, {:application_master, :start_it_old, 4, [file: 'application_master.erl', line: 277]}]}}}}}}
    (phoenix 1.4.12) lib/mix/tasks/compile.phoenix.ex:11: Mix.Tasks.Compile.Phoenix.run/1
    (mix 1.10.0) lib/mix/task.ex:330: Mix.Task.run_task/3
    (mix 1.10.0) lib/mix/tasks/compile.all.ex:76: Mix.Tasks.Compile.All.run_compiler/2
    (mix 1.10.0) lib/mix/tasks/compile.all.ex:56: Mix.Tasks.Compile.All.do_compile/4
    (mix 1.10.0) lib/mix/tasks/compile.all.ex:27: anonymous fn/2 in Mix.Tasks.Compile.All.run/1
    (mix 1.10.0) lib/mix/tasks/compile.all.ex:43: Mix.Tasks.Compile.All.with_logger_app/2
    (mix 1.10.0) lib/mix/task.ex:330: Mix.Task.run_task/3
    (mix 1.10.0) lib/mix/tasks/compile.ex:96: Mix.Tasks.Compile.run/1

Using elixir 1.10.0