[8.x] Remove unnecessary double MAC for AEAD ciphers #38475
Conversation
| @@ -70,7 +82,7 @@ public static function supported($key, $cipher) | |||
| */ | |||
| public static function generateKey($cipher) | |||
| { | |||
| return random_bytes($cipher === 'AES-128-CBC' ? 16 : 32); | |||
There was a problem hiding this comment.
This was incorrect for AES-128-GCM, causing key:generate to output a 256 bit key and the application to throw a RuntimeException.
driesvints
changed the title
| { | ||
| return hash_hmac('sha256', $tag.$iv.$value, $this->key); | ||
| return hash_hmac('sha256', $iv.$value, $this->key); |
There was a problem hiding this comment.
Note that the only change here and in validMac is reverting the unnecessary tag addition in https://github.com/laravel/framework/pull/38190/files
Krisell
force-pushed
the
encryption-aead-fixes
branch
2 times, most recently
from
2bd55ef to
e60732f
Compare
| @@ -184,7 +194,6 @@ public function decryptString($payload) | |||
| * | |||
| * @param string $iv | |||
| * @param mixed $value | |||
| * @param string $tag | |||
Krisell
force-pushed
the
encryption-aead-fixes
branch
from
e60732f to
8abf9c6
Compare
Krisell
force-pushed
the
encryption-aead-fixes
branch
from
8abf9c6 to
3800323
Compare
|
Removing the |
|
Could the following be a problem? |
|
No I don't think that's a high risk. |
This PR does two things:
generateKeymethod (wrong result forAES-128-GCM, see comment in code below).hmaccomputation for AEAD ciphers, improving performance and reducing encrypted data amount.These two problems were introduced recently when support for GCM ciphers was added, in #38190
GCM is a so called Authenticated Encryption scheme (AEAD), which means that a Message Authentication Code (
mac) is included in the algorithm and handled byopenssl_encrypt/openssl_decrypt. This is calledtagin the code, and serves the exact same purpose as themacthat is also present from before (and necessary in the CBC-case, which is not natively authenticated).This PR removes the HMAC computation in the GCM case, since the
tagalready ensures data integrity (before decryption is attempted). This simplifies the code (very important for cryptographic implementations), improves performance and reduces the amount of data stored/transmitted. As an example, the XSRF and session id cookies are reduced by around 25 % (but the reduction for larger data is of course much smaller).The changes are backwards compatible with #38190, and of course with any data stored before that. In fact, the only change to the decryption part is skipping the MAC validation in the case of an AEAD-cipher being used.
This was actually all done correctly in a 2017 PR by bukka, #21963, but that was never merged. I did use that PR as inspiration for this, but I tried to keep the changes to a minimum to increase chances of merging. One larger change is generalizing the list of supported ciphers, and adding the AEAD support as a property. This should simplify the process of adding additional ciphers in the future, and simplified other parts of the code.