Skip to content

Commit 0d99151

Browse files
authored
FF remove synchronized from EncryptionHelper (#5991)
Task/Issue URL: https://app.asana.com/1/137249556945/task/1210112602367622?focus=true ### Description Use mutex instead of synchronized in encrypt/decrypt to prevent thread starvation ### Steps to test this PR _Feature 1_ - [ ] Smoke test autofill ### UI changes | Before | After | | ------ | ----- | !(Upload before screenshot)|(Upload after screenshot)|
1 parent e657ba8 commit 0d99151

File tree

3 files changed

+89
-18
lines changed
  • autofill

3 files changed

+89
-18
lines changed

autofill/autofill-impl/src/main/java/com/duckduckgo/autofill/impl/securestorage/encryption/EncryptionHelper.kt

Lines changed: 70 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -17,26 +17,31 @@
1717
package com.duckduckgo.autofill.impl.securestorage.encryption
1818

1919
import android.security.keystore.KeyProperties
20+
import com.duckduckgo.autofill.api.AutofillFeature
2021
import com.duckduckgo.autofill.impl.securestorage.SecureStorageException
2122
import com.duckduckgo.autofill.impl.securestorage.SecureStorageException.InternalSecureStorageException
2223
import com.duckduckgo.autofill.impl.securestorage.encryption.EncryptionHelper.EncryptedBytes
24+
import com.duckduckgo.common.utils.DispatcherProvider
2325
import com.duckduckgo.di.scopes.AppScope
2426
import com.squareup.anvil.annotations.ContributesBinding
2527
import java.lang.Exception
2628
import java.security.Key
2729
import javax.crypto.Cipher
2830
import javax.crypto.spec.GCMParameterSpec
2931
import javax.inject.Inject
32+
import kotlinx.coroutines.sync.Mutex
33+
import kotlinx.coroutines.sync.withLock
34+
import kotlinx.coroutines.withContext
3035

3136
interface EncryptionHelper {
3237
@Throws(SecureStorageException::class)
33-
fun encrypt(
38+
suspend fun encrypt(
3439
raw: ByteArray,
3540
key: Key,
3641
): EncryptedBytes
3742

3843
@Throws(SecureStorageException::class)
39-
fun decrypt(
44+
suspend fun decrypt(
4045
toDecrypt: EncryptedBytes,
4146
key: Key,
4247
): ByteArray
@@ -53,12 +58,45 @@ interface EncryptionHelper {
5358
}
5459

5560
@ContributesBinding(AppScope::class)
56-
class RealEncryptionHelper @Inject constructor() : EncryptionHelper {
61+
class RealEncryptionHelper @Inject constructor(
62+
private val autofillFeature: AutofillFeature,
63+
private val dispatcherProvider: DispatcherProvider,
64+
) : EncryptionHelper {
5765
private val encryptionCipher = Cipher.getInstance(TRANSFORMATION)
5866
private val decryptionCipher = Cipher.getInstance(TRANSFORMATION)
5967

68+
private val encryptMutex = Mutex()
69+
private val decryptMutex = Mutex()
70+
71+
override suspend fun encrypt(
72+
raw: ByteArray,
73+
key: Key,
74+
): EncryptedBytes = withContext(dispatcherProvider.io()) {
75+
return@withContext if (autofillFeature.createAsyncPreferences().isEnabled()) {
76+
encryptAsync(raw, key)
77+
} else {
78+
encryptSync(raw, key)
79+
}
80+
}
81+
6082
@Synchronized
61-
override fun encrypt(
83+
private fun encryptSync(
84+
raw: ByteArray,
85+
key: Key,
86+
): EncryptedBytes {
87+
return innerEncrypt(raw, key)
88+
}
89+
90+
private suspend fun encryptAsync(
91+
raw: ByteArray,
92+
key: Key,
93+
): EncryptedBytes {
94+
encryptMutex.withLock {
95+
return innerEncrypt(raw, key)
96+
}
97+
}
98+
99+
private fun innerEncrypt(
62100
raw: ByteArray,
63101
key: Key,
64102
): EncryptedBytes {
@@ -73,8 +111,35 @@ class RealEncryptionHelper @Inject constructor() : EncryptionHelper {
73111
return EncryptedBytes(encrypted, iv)
74112
}
75113

114+
override suspend fun decrypt(
115+
toDecrypt: EncryptedBytes,
116+
key: Key,
117+
): ByteArray = withContext(dispatcherProvider.io()) {
118+
return@withContext if (autofillFeature.createAsyncPreferences().isEnabled()) {
119+
decryptAsync(toDecrypt, key)
120+
} else {
121+
decryptSync(toDecrypt, key)
122+
}
123+
}
124+
76125
@Synchronized
77-
override fun decrypt(
126+
private fun decryptSync(
127+
toDecrypt: EncryptedBytes,
128+
key: Key,
129+
): ByteArray {
130+
return innerDecrypt(toDecrypt, key)
131+
}
132+
133+
private suspend fun decryptAsync(
134+
toDecrypt: EncryptedBytes,
135+
key: Key,
136+
): ByteArray {
137+
decryptMutex.withLock {
138+
return innerDecrypt(toDecrypt, key)
139+
}
140+
}
141+
142+
private fun innerDecrypt(
78143
toDecrypt: EncryptedBytes,
79144
key: Key,
80145
): ByteArray {

autofill/autofill-impl/src/test/java/com/duckduckgo/autofill/impl/securestorage/Fakes.kt

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -47,15 +47,15 @@ class FakeEncryptionHelper constructor(
4747
private val expectedEncryptedIv: String,
4848
private val expectedDecryptedData: String,
4949
) : EncryptionHelper {
50-
override fun encrypt(
50+
override suspend fun encrypt(
5151
raw: ByteArray,
5252
key: Key,
5353
): EncryptedBytes = EncryptedBytes(
5454
expectedEncryptedData.decodeBase64()!!.toByteArray(),
5555
expectedEncryptedIv.decodeBase64()!!.toByteArray(),
5656
)
5757

58-
override fun decrypt(
58+
override suspend fun decrypt(
5959
toDecrypt: EncryptedBytes,
6060
key: Key,
6161
): ByteArray = expectedDecryptedData.decodeBase64()!!.toByteArray()

autofill/autofill-store/src/main/java/com/duckduckgo/securestorage/store/keys/SecureStorageKeyStore.kt

Lines changed: 17 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -70,9 +70,7 @@ class RealSecureStorageKeyStore constructor(
7070
private val encryptedPreferencesSync: SharedPreferences? by lazy { encryptedPreferencesSync() }
7171

7272
private suspend fun getEncryptedPreferences(): SharedPreferences? {
73-
return withContext(dispatcherProvider.io()) {
74-
if (autofillFeature.createAsyncPreferences().isEnabled()) encryptedPreferencesDeferred.await() else encryptedPreferencesSync
75-
}
73+
return if (autofillFeature.createAsyncPreferences().isEnabled()) encryptedPreferencesDeferred.await() else encryptedPreferencesSync
7674
}
7775

7876
private suspend fun encryptedPreferencesAsync(): SharedPreferences? {
@@ -114,20 +112,28 @@ class RealSecureStorageKeyStore constructor(
114112
keyName: String,
115113
keyValue: ByteArray?,
116114
) {
117-
getEncryptedPreferences()?.edit(commit = true) {
118-
if (keyValue == null) {
119-
remove(keyName)
120-
} else {
121-
putString(keyName, keyValue.toByteString().base64())
115+
withContext(dispatcherProvider.io()) {
116+
getEncryptedPreferences()?.edit(commit = true) {
117+
if (keyValue == null) {
118+
remove(keyName)
119+
} else {
120+
putString(keyName, keyValue.toByteString().base64())
121+
}
122122
}
123123
}
124124
}
125125

126-
override suspend fun getKey(keyName: String): ByteArray? = getEncryptedPreferences()?.getString(keyName, null)?.run {
127-
this.decodeBase64()?.toByteArray()
126+
override suspend fun getKey(keyName: String): ByteArray? {
127+
return withContext(dispatcherProvider.io()) {
128+
return@withContext getEncryptedPreferences()?.getString(keyName, null)?.run {
129+
this.decodeBase64()?.toByteArray()
130+
}
131+
}
128132
}
129133

130-
override suspend fun canUseEncryption(): Boolean = getEncryptedPreferences() != null
134+
override suspend fun canUseEncryption(): Boolean = withContext(dispatcherProvider.io()) {
135+
getEncryptedPreferences() != null
136+
}
131137

132138
companion object {
133139
const val FILENAME = "com.duckduckgo.securestorage.store"

0 commit comments

Comments
 (0)