Use AES in GCM mode instead of CFB mode - #12
Conversation
| // AESEncrypt encrypts text and given key with AES. | ||
| func AESEncrypt(key, text []byte) ([]byte, error) { | ||
| // AESEncrypt encrypts plaintext with the given key and nonce using AES in GCM mode. | ||
| func AESEncrypt(key, nonce, plaintext []byte) ([]byte, error) { |
There was a problem hiding this comment.
I changed the signature to take in a nonce because the nonce (or IV if you want to call it that) has to be 100% unique. For example, if you go to encrypt a piece of data the encryption/decryption for that piece of data should use its own randomly generated nonce. If you go to encrypt N other pieces of data the encryption/decryption for each piece of data should use its own randomly generated nonce.
There was a problem hiding this comment.
Not automatically generating and appending the nonce leaves every caller with the task of generating the nonce itself, thus knowing about the actual encryption algorithm this function uses.
AES/GCM requires a 96 bit nonce (gcm.NonceSize()), other ciphers might require a different nonce length.
Please rename this function to AESGCMEncrypt.
There was a problem hiding this comment.
How can the nonce be automatically generated and encapsulated in here? It needs to be unique for every encryption and be passed over for decryption purposes. I guess there could be methods in here to generate and fetch the nonce, but the client would still need to call them before using encrypt/decrypt. Another option would be to change the structure of this code was so you got an instance. Then you could make use of a counter that's incremented every time encrypt is called.
There was a problem hiding this comment.
Encryption:
- Read the 96-bit nonce with
crypto/rand - Use it to encrypt your stuff
- Return
nonce || ciphertextwhere||stands for concatenation
Decryption:
- Extract the nonce from the "ciphertext" (first 96 bits)
- Use it to decrypt the ciphertext
- Return the plaintext
Even though the nonce is only 12 bytes long, creating it randomly instead of using a counter is no problem unless you intend to encrypt more than 2^32 messages using the same key.
There was a problem hiding this comment.
Quoting NIST Special Publication 800-38D, 8.3 "Constraints on the Number of Invocations":
The total number of invocations of the authenticated encryption function shall not exceed 2^32, including all IV lengths and all instances of the authenticated encryption function with the given key.
There was a problem hiding this comment.
Ah, I see what you mean about appending it to the ciphertext. Sounds like a good idea.
|
Do you have any tests anywhere for this stuff? |
|
Test file is https://github.com/Unknwon/com/blob/master/string_test.go (no tests for this |
|
Oops, my mistake...thought I caught and committed that already. Just fixed it. |
|
All good, merging. |
| iv := ciphertext[:aes.BlockSize] | ||
| if _, err := io.ReadFull(rand.Reader, iv); err != nil { | ||
|
|
||
| b := base64.StdEncoding.EncodeToString(plaintext) |
There was a problem hiding this comment.
Why do you encrypt the base64-encoded string representation of plaintext and not plaintext directly?
Base64-encoding creates a lot of unnecessary overhead.
There was a problem hiding this comment.
I left this as it was, maybe a better question for @unknwon
There was a problem hiding this comment.
@aaronjwood hmm, not sure what's the primary purpose. Please change to whatever suit the best.
| // AESEncrypt encrypts text and given key with AES. | ||
| func AESEncrypt(key, text []byte) ([]byte, error) { | ||
| // AESEncrypt encrypts plaintext with the given key and nonce using AES in GCM mode. | ||
| func AESEncrypt(key, nonce, plaintext []byte) ([]byte, error) { |
There was a problem hiding this comment.
Not automatically generating and appending the nonce leaves every caller with the task of generating the nonce itself, thus knowing about the actual encryption algorithm this function uses.
AES/GCM requires a 96 bit nonce (gcm.NonceSize()), other ciphers might require a different nonce length.
Please rename this function to AESGCMEncrypt.
|
Just realized we would have a API break here (Macaron won't compile correctly now)... maybe we should leave |
|
What's the problem with changing the API? If one intends to use the most recent version of this library, then changes to the code base are required. There's no need to have |
|
@leonklingele it is not possible to accept/remember the nonce here: https://github.com/go-macaron/macaron/blob/master/context.go#L421-L429 without breaking the API. |
|
That's why I call to prepend / append the nonce directly to the resulting ciphertext in |
|
@unknwon I'm making changes to Macaron right now that will fix the API break. Will send over a PR in a sec... |
|
There are definitely some limitations/reduced security with keeping the API the same. For example, there is only ever one nonce generated with my PR above. Also, the key used for AES is still a basic hash of data instead of a derived key... |
One issue with this is that the nonce used by setting a secure cookie will need to be used when getting it. I think ideally those funcs (or at least |
|
Good luck implementing this. Why make life so hard? |
|
@leonklingele I think it would be fine if we could change the API and move away from the hashing stuff. There just needs to be coordination in place for the nonce and key, and the key needs to be generated/managed differently. Why would that be so hard? |
|
@unknwon are you okay if I make more breaking API changes that were discussed here? |
3 participants
#11