apple / apple/foundationdb

StreamCipher implementation is not valid AES-GCM

Open
#12,511 0 comments 0 reactions 0 assignees View on GitHub
backup-restore security
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.