StreamCipher implementation is not valid AES-GCM
- Dominant language
- C++
- Stars
- 16.7k
- Forks
- 1.6k
- Avg merge
- 1d 20h
- Merged PRs (30d)
- 126
Description
While iterating on encrypted backups, we identified that the encryption scheme in FoundationDB is not a correct implementation of AES-GCM.
In particular, we are not correctly generating and verifying authentication tags for the ciphertext, which means the encryption scheme is unauthenticated and vulnerable to tampering attacks. One way you can see this is by attempting to decrypt a FoundationDB snapshot in java. Java's encryption libraries are more fool-proof / defensive in their design and will refuse to disable authentication in decryption. What this means concretely is that an attacker with access to e.g. an encrypted FoundationDB backup could conceivably alter data at a given offset to corrupt or mask a piece of data.
See:
* https://wiki.openssl.org/index.php/EVP_Authenticated_Encryption_and_Decryption
* https://andrea.corbellini.name/2023/03/09/authenticated-encryption/
My intuition is that this is a minor security issue, it's difficult to imagine a practical attack vector given backups are stored in secure blob storage and likely server-side encrypted. That said, the bug should probably be fixed since StreamCipher may be extended to new uses over time where the lack of authentication becomes more severe.
Unfortunately, the fix is non-trivial because it changes the semantics of encryption. Previously, we could rely on `len(ciphertext) == len(plaintext)`. With the addition of authentication tags, this is no longer true. This is also a breaking change.
I have a first draft of what a fix could look like here:
https://github.com/millasml/foundationdb/commit/3bbebe8f65a602d3203692475fc3c1e8c58572a2
What I would propose doing:
* Change StreamCipher so that `authenticationEnabled` is a flag that must be specified.
* Add a knob to backups so that authenticated encryption becomes possible, default to false for back compat
* (?) I'm not sure if you have a process for rolling out a breaking change like this and changing the default behavior of encrypted backups - possibly in a minor version upgrade?
Contributor guide
Research direction
The issue identifies StreamCipher and encrypted FoundationDB backups as the scope; start by reviewing the linked draft commit and the cited AES-GCM guidance. Done means authentication tags are generated and verified while preserving the proposed backup compatibility behavior, but the rollout and default change still need a project decision.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp
- Domain
- cryptography, databases, security
- Issue type
- Bug
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100