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

[8.x] Added support for GCM encryption #38190

Merged
merged 6 commits into from
Aug 6, 2021
Merged

[8.x] Added support for GCM encryption #38190

merged 6 commits into from
Aug 6, 2021

Conversation

nickakitch
Copy link
Contributor

This pull request adds support for 128-bit and 256-bit GCM encryption, which will allow Laravel developers to use this stronger encryption mode in their applications.

Galois/Counter Mode (GCM) encryption is a more secure and modern encryption cipher than CBC (which is currently default).

The pull request does not change the default encryption mode and is fully backwards compatible.

@nickakitch nickakitch changed the title Added support for GCM encryption [8.x] Added support for GCM encryption Jul 31, 2021
@taylorotwell
Copy link
Member

Can you explain what a "tag" is in this context?

@nickakitch
Copy link
Contributor Author

nickakitch commented Jul 31, 2021

Sure thing, the tag is generated during the encryption itself and essentially acts as a checksum hash that is used to verify the integrity of the encryption string.

The tag is required for GCM mode as it ensures the integrity of each encrypted block (of 128 or 256 bits), but is not available for older encryption methods.

The $tag variable is passed via reference to the OpenSSL encrypt function, which then assigns it a value. It will add 16 characters to the final mac value that is stored in the db.

In addition to the $tag, there are a number of security benefits that GCM mode has over CBC with a Mac. More information can be found below:

https://crypto.stackexchange.com/questions/6842/how-to-choose-between-aes-ccm-and-aes-gcm-for-storage-volume-encryption

@Krisell
Copy link
Contributor

Krisell commented Aug 19, 2021

The tag is another name for the Message Authentication Code (mac), and is used in GCM mode to provide authenticated encryption, meaning that ciphertext tampering can be detected and rejected before decrypting the data. This serves the exact same purpose as the already implemented HMAC in the previous CBC implementation, and using both of these (which this implementation does) is unnecessary and reduces performance.

So, when GCM mode is used, the HMAC part should be skipped. This seems to have been done correctly in the 2017 PR https://github.com/laravel/framework/pull/21963/files.

One of the reasons GCM is considered more secure is that it reduces the risk of implementation error, since the authentication part doesn't have to be implemented by the consumer. However Laravel implements Encrypt-then-mac correctly as far as I can see. https://en.wikipedia.org/wiki/Authenticated_encryption

I might try to make this into a PR in the coming weeks. There is no rush however as I don't see any downside to the current implementation except for performance degradation.

And a minor detail regarding the information above – the AES block size is 128 bits for both AES-128 and AES-256. It's the key length that differs.

@Krisell
Copy link
Contributor

Krisell commented Aug 20, 2021

There is also a bug in the generateKey method. If AES-128-GCM is used, the key:generate artisan command incorrectly generates a 256 bit key, causing a RuntimeException.

This is not critical, so I'll include that fix in the PR mentioned above, unless that takes longer than expected.

@driesvints
Copy link
Member

@Krisell thanks for spotting that. I sent in a PR for that: #38474

I might try to make this into a PR in the coming weeks. There is no rush however as I don't see any downside to the current implementation except for performance degradation.

Would very much welcome a PR for this 👍

@driesvints
Copy link
Member

@Krisell just saw you sent your PR roughly at the same time. I'll close mine.

LukeTowers added a commit to wintercms/storm that referenced this pull request Dec 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants