Implement AES-GCM support for TLS1.2#37
Implement AES-GCM support for TLS1.2#37colmmacc merged 8 commits intoaws:masterfrom baldwinmatt:aead-cipher-suites
Conversation
This commit adds support for AES-GCM mode for AES128 and AES256. The new cipher suites are added as version 20150214 and made the new default. closes #5
|
This has been tested with ./bin/s2nd localhost 8443 and ./bin/s2nc www.amazon.com 443 and in both cases we are able to negotiate and transmit data in both directions |
crypto/s2n_aead_cipher_aes_gcm.c
Outdated
There was a problem hiding this comment.
Since s2n is a TLS implementation, should it be authoritative about these sizes, rather than relying on OpenSSL to get it right? I feel like it's ok for OpenSSL to be authoritative about the crypto, but we should probably define TLS basics ourselves.
There was a problem hiding this comment.
agreed - i was thinking about this, glad you agree ;)
|
Awesome change! implementing this is not at all easy and it's a big deal to get AEAD in now. Using OpenSSL-1.0.2's libcrypto will get us some great performance benefits too. Its AES implementation is significantly faster than predecessors. |
Define TLS constants in S2N so as not to rely on openssl Refactored the AAD derivation into a single implementation Added some extra checks on size Reordered code to prefer AEAD mode
|
Added a new commit which addresses most things - still working on the negative tests and fixing the AES256/SHA384 implementations. |
crypto/s2n_aead_cipher_aes_gcm.c
Outdated
There was a problem hiding this comment.
Thanks for the update! Sorry for not catching it earlier, but I think we need to use != 1 here. The docs say that these functions return 1 on success, so that's the most explicit thing that we can check. If they return say, 2, we should assume something weird is going on.
There was a problem hiding this comment.
good call - fixed
|
Added data tampering tests |
|
Removed the AES256/SHA384 cipher squites as they require changes to the PRF |
There was a problem hiding this comment.
It's a memory leak unless you call EVP_CIPHER_CTX_cleanup.
There was a problem hiding this comment.
you are right, but calling that destroys the key.
do we need a corresponding destroy_(en|de)cryption_key method? as we currently have this problem with the CBC mode ciphers leaking too.
There was a problem hiding this comment.
Destroy sounds useful - there's definitely times where things keep intermediate forms of keys, and cipher states and need allocation and deallocation. Even if it's a no-op for a bunch of the suites that's no problem.
|
Added the destroy_key API |
This commit adds support for AES-GCM mode for AES128 and AES256.
The new cipher suites are added as version 20150214 and made the new default.
closes #5