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.

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

ssl: add aes gcm cipher #372

Merged
merged 6 commits into from Sep 4, 2014

Conversation

Projects
None yet
5 participants
@RoadRunnr
Copy link

commented May 18, 2014

This adds support for the AES GCM cipher suites. AES-GCM is much faster than normal AES+MAC. On Intel CPU's with AES-NI it is comparable to RC4 in terms of throughput.

The cipher support in crypto is based on OpenSSL's EVP contexts. The EVP context where chosen over the more generic CRYPTO_gcm128_xxx functions for performance. The CRYPTO_gcm128_xxx functions use a generic implementation while the EVP context can use hardware optimized variants (AES-NI or padlock instructions) on supported CPUs'

Some benchmarks on an Intel(R) Core(TM) i7-2760QM CPU @ 2.40GHz:

The 'numbers' are in 1000s of bytes per second processed.
type             16 bytes     64 bytes    256 bytes   1024 bytes   8192 bytes

openssl speed aes-128-cbc (generic functions):
aes-128 cbc     105336.34k   112751.42k   114742.10k   115345.41k   113988.95k

openssl speed -evp aes-128-cbc (EVP with AES-NI):
aes-128-cbc     615445.91k   660332.93k   670884.44k   665051.48k   669777.92k

openssl speed -evp rc4:
rc4             348612.51k   609902.38k   748361.73k   782705.19k   788460.89k

openssl speed -evp aes-128-gcm (EVP with AES-NI):
aes-128-gcm     310758.55k   785113.43k  1156720.81k  1277712.38k  1323431.25k
@RoadRunnr

This comment has been minimized.

Copy link
Author

commented May 18, 2014

There is one open question with this change: how to fix the cipher suite filtering?

Currently, in tls_v1 there is a differentiation between EC and non-EC support. This change requires and additional level of selection, AES-GCM suport available or not? This is already supported by ssl_cipher:filter_suites/1. But the call from ssl:cipher_suites/1 down to tls_v1:suites is not passed through that filter function.

It seems that that was a deliberate design decision. So what is the desired way of implementing this filtering?

@OTP-Maintainer

This comment has been minimized.

Copy link

commented May 18, 2014

Patch has passed first testings and has been assigned to be reviewed

@IngelaAndin

This comment has been minimized.

Copy link
Contributor

commented May 28, 2014

I think that this clause should be changed to
binary_cipher_suites(Version, []) -> % Defaults to all supported suites
ssl_cipher:suites(Version);

binary_cipher_suites(Version, []) -> % Defaults to all supported suites
ssl_cipher:filter_suites(ssl_cipher:suites(Version));

@RoadRunnr

This comment has been minimized.

Copy link
Author

commented May 28, 2014

updated with a cipher filter

Note: #371 or #385 is a prerequisite for this change, otherwise the ssl test suite fails for the anon and psk ciphers.

@IngelaAndin

This comment has been minimized.

Copy link
Contributor

commented May 28, 2014

Could you add to the description for the functions decipher, decipher_aead, cipher and cipher_aead to explain the difference e.i. that they handle ciphers suites that uses HMAC or authenticated encryption with associated data (AEAD). Maybe we should rename cipher to cipher_hmac and decipher to decipher_hmac to make it more clear?!

@IngelaAndin

This comment has been minimized.

Copy link
Contributor

commented May 29, 2014

Or maybe cipher_aead could be made part of cipher (and same for decipher_aead) with a new function calculate_message_auth that depending on case calculates HMAC (calc_hmac) or the ADD (calc_add) and a new function to handle the HMAC or ADD where (if I remeber your code correctly)the function will do nothing in the ADD case.

@RoadRunnr

This comment has been minimized.

Copy link
Author

commented May 31, 2014

Or maybe cipher_aead could be made part of cipher (and same for decipher_aead) with a new function calculate_message_auth that depending on case calculates HMAC (calc_hmac) or the ADD (calc_add) and a new function to handle the HMAC or ADD where (if I remeber your code correctly)the function will do nothing in the ADD case.

Mhh, I first tried an approach along those lines. However, in the DTLS case a different method is needed to generate the AAD data. While this is doable with a fun, it leads to code that is really hard to comprehend.
So, instead I opted for the current, slightly redundant, but much easier to understand version.

@IngelaAndin

This comment has been minimized.

Copy link
Contributor

commented Jun 3, 2014

I have cherry-picked your filtering commit so that it can make 17.1. The other changes will alas not be able to make 17.1 as there is too little time left and we have not had time to look at the crypto parts at all yet. But we will get there, and do appreciate your contributions. And if we are keeping the cipher_aead it would be good if you could formulate the reason you described above in a comment that could be understood when reading the code and knowing nothing about this pull request.

@IngelaAndin

This comment has been minimized.

Copy link
Contributor

commented Jun 3, 2014

I also think that the abbreviation AEAD should be explaind in the crypto documentation.

@sverker

This comment has been minimized.

Copy link
Contributor

commented Jun 12, 2014

crypto review:

  • The mutable context is not thread safe. Use a ErlNifMutex similar to the hmac_context.
  • Move up the call to enif_open_resource_type next to the call for the hmac_context, to be called
    even at upgrade. Also add ERL_NIF_RT_TAKEOVER to the flags argument.
  • Question: For other crypto functions IVEC binary arguments are copied as they are changed by openssl. For other streaming functions, the IVEC is also carried along as part of the context. The patch neither copies or carries, is this correct behaviour?
  • I think aead should be added to some of the generic crypto functions, probably stream_init/encrypt/decrypt, and not pollute the API with specific functions.
@RoadRunnr

This comment has been minimized.

Copy link
Author

commented Jun 12, 2014

  • Question: For other crypto functions IVEC binary arguments are copied as they are changed by openssl. For other streaming functions, the IVEC is also carried along as part of the context. The patch neither copies or carries, is this correct behaviour?

Yes, that is correct. In GCM mode this field is actually the GCMnounce (RFC 5244, Section 3). In TLS, 4 bytes of it are fixed (salt), the other 8 bytes are a counter include in each packet.

I'll rename the argument to make it clearer.

  • I think aead should be added to some of the generic crypto functions, probably stream_init/encrypt/decrypt, and not pollute the API with specific functions.

AEAD is a block cipher operation mode, putting it in the stream cipher API seems wrong.

What about this:

block_encrypt(Ctx, Nounce, AAD, Data) -> {Cipher, Tag}
block_decrypt(Ctx, Nounce, AAD, {Cipher, Tag}) -> Text | 'error'
@sverker

This comment has been minimized.

Copy link
Contributor

commented Jun 12, 2014

I suggested stream_* as you have a context that (I thought) could be passed on to subsequent calls. The block_* functions are for stateless one-shot encrypt/decrypt.

@RoadRunnr

This comment has been minimized.

Copy link
Author

commented Jun 15, 2014

Updated to incorporate the review comments.

This version implements AES-GCM without using the OpenSSL EVP interface. This will sacrifice some of the possible optimizations for an simpler interface.

There is already a different review that implements all ciphers using the EVP interfaces. Adding AES-GCM in that context makes IMHO more sense.

@OTP-Maintainer

This comment has been minimized.

Copy link

commented Jun 15, 2014

Patch has passed first testings and has been assigned to be reviewed


I am a script, I am not human


@RoadRunnr

This comment has been minimized.

Copy link
Author

commented Jun 16, 2014

I have another set of changes that build on top of this review that add support for the RFC draft-agl-tls-chacha20poly1305-04 Chacha20/Poly1305 Suites. Due to the somewhat experimental nature of those, I would like to get some feedback on whether they would be acceptable or not?

The changes in question can be view here: https://github.com/RoadRunnr/otp/compare/ssl%2Fadd-aes-gcm-cipher...ssl%2Fadd-chacha20-cipher?expand=1

ChaCha20/Poly1305 is currently supported in Chrome for Android (tested and works on 4.4.3), on Google Servers (tested and works), in OpenBSD builds using libressl (not tested) and in unofficial libressl ports (https://github.com/busterb/libressl)

Some additional information on Chacha20/Poly1305:

@sverker

This comment has been minimized.

Copy link
Contributor

commented Jun 19, 2014

'notsup' should not be returned as an atom from crypto API, but rather as an exception with erlang:error(notsup). Look how the other are doing it.

@RoadRunnr

This comment has been minimized.

Copy link
Author

commented Jul 13, 2014

Updated:

  • crypto API now uses erlang:error(notsup) as return
  • reworked the generic parts of TLS AEAD support to account for ChaCha20/Poly1305
@OTP-Maintainer

This comment has been minimized.

Copy link

commented Jul 15, 2014

Patch has passed first testings and has been assigned to be reviewed


I am a script, I am not human


@marcusarendt

This comment has been minimized.

Copy link

commented Aug 15, 2014

This fails to build on a number of platforms (eg, Windows, Solaris 2.10 & 2.11, Darwin 13.3.0, SLES 10 SP2, SLES 11 etc) with the following error message (or similar):

crypto.c:55:27: openssl/modes.h: No such file or directory

@RoadRunnr RoadRunnr force-pushed the RoadRunnr:ssl/add-aes-gcm-cipher branch from e4ec216 to 68369bf Aug 21, 2014

@RoadRunnr

This comment has been minimized.

Copy link
Author

commented Aug 21, 2014

updated with OpenSSL version protection for the modes.h include

@marcusarendt

This comment has been minimized.

Copy link

commented Aug 26, 2014

When running our internal tests Dialyzer complains:

ssl.erl:357: (call_to_missing)
Call to missing or unexported function ssl_cipher:anonymous_suites/0
ssl_cipher.erl:326: (call)
The call ssl_cipher:anonymous_suites(0)
breaks the contract (ssl_record:ssl_version()) -> [cipher_suite()]
ssl_cipher.erl:1440: (guard_fail)
Guard test 'or'('false','false') can never succeed
ssl_cipher.erl:1443: (pattern_match)
The pattern 'chacha20_poly1305' can never match the type 'aes_128_cbc' | 'aes_256_cbc'
ssl_cipher.erl:1455: (guard_fail)
Guard test 'or'('false','false') can never succeed
ssl_cipher.erl:1472: (pattern_match)
The pattern 'aes_128_gcm' can never match the type '3des_ede_cbc' | 'aes_128_cbc' | 'aes_256_cbc' | 'des_cbc' | 'null' | 'rc4_128'
ssl_cipher.erl:1474: (pattern_match)
The pattern 'aes_256_gcm' can never match the type '3des_ede_cbc' | 'aes_128_cbc' | 'aes_256_cbc' | 'des_cbc' | 'null' | 'rc4_128'
ssl_cipher.erl:1476: (pattern_match)
The pattern 'chacha20_poly1305' can never match the type '3des_ede_cbc' | 'aes_128_cbc' | 'aes_256_cbc' | 'des_cbc' | 'null' | 'rc4_128'
ssl_cipher.erl:1515: (guard_fail)
Guard test 'or'('false','false') can never succeed

@marcusarendt

This comment has been minimized.

Copy link

commented Sep 3, 2014

Any update?

@RoadRunnr

This comment has been minimized.

Copy link
Author

commented Sep 3, 2014

don't have much time to work on this right now, hopefully I can spend a few minutes on the weekend...

@RoadRunnr RoadRunnr force-pushed the RoadRunnr:ssl/add-aes-gcm-cipher branch from 68369bf to 7603a40 Sep 3, 2014

@RoadRunnr

This comment has been minimized.

Copy link
Author

commented Sep 3, 2014

found a few seconds, updated to fix the specs

@marcusarendt marcusarendt merged commit 7603a40 into erlang:master Sep 4, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.