PM-40154: feat: Create a new Shared Preferences that encrypts data via the AndroidKeystore by david-livefront · Pull Request #7158 · bitwarden/android

david-livefront · 2026-07-10T20:44:41Z

🎟️ Tracking

📔 Objective

This PR creates a new KeystoreEncryptedSharedPreferences that utilizes the AndroidKeystore to encrypt the data stored.

Notable changes:

This migrates us away from EncryptedSharedPreferences which is deprecated and is not FIPS compliant.
The keys are not stored as plaintext.
We currently only support the storing of Strings (this is the only type we need at the moment).

codecov · 2026-07-10T20:52:14Z

Codecov Report

❌ Patch coverage is 77.85714% with 31 lines in your changes missing coverage. Please review.
✅ Project coverage is 27.11%. Comparing base (c2255ba) to head (1249224).

Files with missing lines	Patch %	Lines
...en/data/auth/datasource/disk/AuthDiskSourceImpl.kt	0.00%	12 Missing ⚠️
...a/platform/datasource/disk/CookieDiskSourceImpl.kt	0.00%	8 Missing ⚠️
...asource/disk/KeystoreEncryptedSharedPreferences.kt	76.92%	3 Missing and 3 partials ⚠️
...e/data/manager/encryption/EncryptionManagerImpl.kt	87.50%	4 Missing ⚠️
...en/data/datasource/disk/BaseEncryptedDiskSource.kt	92.30%	0 Missing and 1 partial ⚠️

❗ There is a different number of reports uploaded between BASE (c2255ba) and HEAD (1249224). Click for more details.

HEAD has 3 uploads less than BASE

Flag BASE (c2255ba) HEAD (1249224)

app-data 1 0

app-ui-vault 1 0

app-ui-auth-tools 1 0

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #7158       +/-   ##
===========================================
- Coverage   85.70%   27.11%   -58.59%     
===========================================
  Files        1047     1023       -24     
  Lines       67560    66041     -1519     
  Branches     9789     9616      -173     
===========================================
- Hits        57900    17907    -39993     
- Misses       6147    47004    +40857     
+ Partials     3513     1130     -2383

Flag	Coverage Δ
app-data	`?`
app-ui-auth-tools	`?`
app-ui-platform	`17.05% <0.00%> (-0.04%)`	⬇️
app-ui-vault	`?`
authenticator	`6.11% <12.85%> (+0.01%)`	⬆️
lib-core-network-bridge	`4.13% <50.71%> (+0.10%)`	⬆️
lib-data-ui	`1.18% <14.28%> (+0.03%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

quexten

If we had a tech breakdown beforehand, I'd recommend implementing this in the SDK based on our modern encryption primitives instead. Given the stage, I'll comment the minimum to make this reasonably secure. I recommend dropping the separate MAC key, and switching from CBC to GCM. Currently, the API is very easy to misuse, and you generally do not want to expose separate authentication and encryption (without authentication) APIs.

quexten · 2026-07-17T06:17:39Z

+        alias: String,
+        bytes: ByteArray,
+    ): Result<ByteArray> = keystoreManager
+        .getOrCreateKey(alias = KeyAlias.Aes(name = alias))


In general, it is dangerous cryptographic design to offer separate encrypt and sign (actually, this should be authenticate, sign usually is used in asymmetric contexts) primitives. You should only ever offer authenticated encryption that does both.

quexten · 2026-07-17T06:29:52Z

+        key: String,
+        value: ByteArray,
+    ): SharedPreferences.Editor {
+        val encryptedKey = encryptionManager


Oh.. are you deliberately not encrypting the keys, but just want to auth them, but the values should be encrypted and authenticated?

@david-livefront Could you clarify what the intent of protecting the "Key" entries here is? The values should be encrypted and authenticated because they contain secret values, and tampering with them may affect the security of the app, so much is clear. Thus we use authenticated encryption for them. But what about the keys? Is there a perceived threat here?

Depending on what the answer to this is I would recommend either:

Use AES-GCM for values as authenticated encryption, do not encrypt or authenticate the key entries whatsoever

(if you decide that the keys are worth protecting integrity of)
2. Use AES-GCM for the values, put the key into the authenticated additional data of AES-GCM, when encrypting the value

This binds together the value + key and makes it not possible to swap between different value + key pairs.

However, so far I believe 1. to be the correct path forward unless I'm missing context.

@quexten We are encrypting the keys for two main reasons.

The previous implementation encrypted the keys and I was attempting to match behavior . They used AES256_SIV for this because it is required for the keys to be deterministically encrypted (otherwise we can never retrieve the data). AES256_SIV is not a compliant option, so I had to come up with something else.

The only threat model I can think of if that if someone were to gain access to the file and attempted to brute force the data. The keys provide information on what each value is, and therefore, which values to brute force first. If this is not a concern, then I can easily drop addition logic and simplify everything.

CC @vvolkgang

Right.

The only threat model I can think of if that if someone were to gain access to the file and attempted to brute force the data. The keys provide information on what each value is, and therefore, which values to brute force first. If this is not a concern, then I can easily drop addition logic and simplify everything.

Since we are encrypting with a high-entropy key stored in the key-chain, we can assume brute-force is not possible for an aes256-gcm key.

With the current design, you would still know what key is which, since we are authenticating but not encrypting them

-> I would recommend dropping the mac on the key and leaving it plaintext, and encrypting the values only. Ideally, we would context bind them so they cannot be swapped between keys, but doing so here would further complicate this PR / it is not blocking from my perspective, and would increase the scope, so I would recommend not doing so here.

Thanks @quexten I have updated the PR.

The sign function is completely gone

We leave the keys as plaintext

We now use AES/GCM/NoPadding for encrypting the values

quexten · 2026-07-20T07:55:32Z

+        alias: String,
+        bytes: ByteArray,
+    ): Result<ByteArray> = keystoreManager
+        .getOrCreateKey(alias = alias)


I don't understand the kotlin syntax too well here. Is the IV generated by the javax.crypto.cipher api?

I don't have a kotlin dev environment set up currently. @david-livefront could you confirm that the java crypto API here samples a random fresh IV on every invocation? If so I'm happy to approve the PR.

(IV / in gcm's case "nonce" re-use leads to a catastrophic cryptographic break, so we want to ensure nonces are correctly randomly sampled).

That is correct, by default a new random IV is generated everytime.

david-livefront · 2026-07-20T14:13:18Z

+                )
+            }
+
+            is InitCipher.Encrypt -> init(Cipher.ENCRYPT_MODE, this@generateCipher)


@quexten Here where we initialize the cipher to encrypt data, it is creating a new IV since we are not providing one.

Above, on line 73, we provide the stored IV for decryption.

david-livefront requested review from SaintPatrck and vvolkgang July 10, 2026 20:44

github-actions Bot added app:password-manager Bitwarden Password Manager app context app:authenticator Bitwarden Authenticator app context labels Jul 10, 2026

david-livefront force-pushed the PM-40154-keystore-encrypted-shared-prefs branch from d7c24bb to aa9e67c Compare July 10, 2026 20:48

vvolkgang reviewed Jul 13, 2026

View reviewed changes

Comment thread data/src/main/kotlin/com/bitwarden/data/datasource/disk/BaseEncryptedDiskSource.kt Outdated

david-livefront force-pushed the PM-40154-keystore-encrypted-shared-prefs branch 6 times, most recently from 18d2899 to 0405c99 Compare July 15, 2026 23:55

david-livefront changed the title ~~PM-40154: Create a new Shared Preferences that encrypts data via the AndroidKeystore~~ PM-40154: feat: Create a new Shared Preferences that encrypts data via the AndroidKeystore Jul 15, 2026

david-livefront added the t:feature-tool Change Type - Internal tool feature or enhancement label Jul 16, 2026

david-livefront force-pushed the PM-40154-keystore-encrypted-shared-prefs branch from 0405c99 to ffcb25d Compare July 16, 2026 16:50

github-actions Bot added t:feature Change Type - Feature Development and removed t:feature-tool Change Type - Internal tool feature or enhancement labels Jul 16, 2026

david-livefront commented Jul 16, 2026

View reviewed changes

Comment thread core/src/main/kotlin/com/bitwarden/core/data/manager/encryption/EncryptionManagerImpl.kt Outdated

david-livefront force-pushed the PM-40154-keystore-encrypted-shared-prefs branch from ffcb25d to bafe9cf Compare July 16, 2026 17:38

quexten requested changes Jul 17, 2026

View reviewed changes

quexten reviewed Jul 17, 2026

View reviewed changes

david-livefront force-pushed the PM-40154-keystore-encrypted-shared-prefs branch 3 times, most recently from e7abca5 to 4eb424e Compare July 17, 2026 18:45

quexten reviewed Jul 20, 2026

View reviewed changes

david-livefront commented Jul 20, 2026

View reviewed changes

david-livefront force-pushed the PM-40154-keystore-encrypted-shared-prefs branch from 4eb424e to d90db9e Compare July 21, 2026 19:34

david-livefront added 2 commits July 21, 2026 15:27

Add encryption layer for new KeystoreEncryptedSharedPreferences

d83a54b

PM-40154: feat: Migrate to new keystore encrypted shared preferences

1249224

david-livefront force-pushed the PM-40154-keystore-encrypted-shared-prefs branch from d90db9e to 1249224 Compare July 21, 2026 20:27

Uh oh!

Conversation

david-livefront commented Jul 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🎟️ Tracking

📔 Objective

Uh oh!

codecov Bot commented Jul 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

quexten left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

quexten Jul 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

david-livefront commented Jul 10, 2026 •

edited

Loading

codecov Bot commented Jul 10, 2026 •

edited

Loading

quexten Jul 17, 2026 •

edited

Loading