Properly initialize AEAD cipher flags in OpenSSL backend #20853
Conversation
jordikroon
force-pushed
the
fix/gh20851
branch
from
2cff66b to
7de1357
Compare
jordikroon
changed the title
|
Please review commits 1 by 1 since I am not sure if the change from The second commit allows AAD to be null since it behaves differently than when only an empty string is given. The matches other implementations like |
bukka
left a comment
bukka
left a comment
There was a problem hiding this comment.
This looks reasonable. Just some minor things really.
| /* Only pass AAD to OpenSSL if caller provided it. | ||
| This makes NULL mean zero AAD items, while "" with len 0 means one empty AAD item. */ | ||
| if (mode->is_aead && aad != NULL && !EVP_CipherUpdate(cipher_ctx, NULL, &i, (const unsigned char *)aad, (int)aad_len)) { |
There was a problem hiding this comment.
Is this for SIV or is this for all modes. I thought that empty string won't make difference but I guess it might.
In general allowing null should be fine for master so it's not really a problem. Just want to know the reasoning as I haven't checked this propertly.
There was a problem hiding this comment.
Only SIV produces different outputs for aad = null and add = "". Other modes produce the same output.
From my understanding this is because in SIV mode aad could be a vector list. Thus null here means an empty vector list. And I don't believe we need to support multiple vectors as aad. Because then we would need to change $aad to string|array.
There was a problem hiding this comment.
ah ok. It would be good to check if skipping actually works for ccm, gcm and ocb (using null argument). It might make sense to potentially limit it just to SIV and treat null as "" for other modes.
bukka
left a comment
bukka
left a comment
There was a problem hiding this comment.
It looks good now. It would be just nice to convert the test so it's consistent with other AEAD tests.
| @@ -0,0 +1,47 @@ | |||
| --TEST-- | |||
There was a problem hiding this comment.
Could you actually change it to a single test for encryption and decryption that will look like https://github.com/jordikroon/php-src/blob/ca12f45a76f3242953e7e27a9b64e509e9f518e2/ext/openssl/tests/openssl_decrypt_gcm.phpt
and
https://github.com/jordikroon/php-src/blob/ca12f45a76f3242953e7e27a9b64e509e9f518e2/ext/openssl/tests/openssl_encrypt_gcm.phpt
Please name it in a similar way and use the standard test vectors. It will also match the ccm and ocb tests...
2 participants
Fixes #20851
Add support for AEAD ciphers like AES-SIV by detecting and initializing AEAD flags during cipher mode loading.
Includes test case for AES-256-SIV encryption/decryption roundtrip.