From a7fb4867149c46d6ee3a3e1bc69292e84c66ad45 Mon Sep 17 00:00:00 2001 From: Steve <263163432+mptfire@users.noreply.github.com> Date: Tue, 13 Oct 2026 13:11:17 +0200 Subject: [PATCH 1/3] fix(security): gate cached private-key operations on device unlock Prepared by Codex security review. Original local review commit: c289f020c7ca50c5c3192f310cbf18e368e28bc4 --- .../greenart7c3/nostrsigner/models/Account.kt | 77 +++++++++++++++---- .../nostrsigner/service/ApplicationBackup.kt | 1 + .../nostrsigner/service/DeviceUnlockPolicy.kt | 9 +++ .../models/AccountCachedKeyLockTest.kt | 48 ++++++++++++ .../service/ApplicationBackupCryptoTest.kt | 1 + .../service/DeviceUnlockPolicyTest.kt | 19 +++++ .../service/SubscriptionTestUtils.kt | 1 + 7 files changed, 143 insertions(+), 13 deletions(-) create mode 100644 app/src/main/java/com/greenart7c3/nostrsigner/service/DeviceUnlockPolicy.kt create mode 100644 app/src/test/java/com/greenart7c3/nostrsigner/models/AccountCachedKeyLockTest.kt create mode 100644 app/src/test/java/com/greenart7c3/nostrsigner/service/DeviceUnlockPolicyTest.kt diff --git a/app/src/main/java/com/greenart7c3/nostrsigner/models/Account.kt b/app/src/main/java/com/greenart7c3/nostrsigner/models/Account.kt index 29dbb401..dd237e23 100644 --- a/app/src/main/java/com/greenart7c3/nostrsigner/models/Account.kt +++ b/app/src/main/java/com/greenart7c3/nostrsigner/models/Account.kt @@ -1,5 +1,6 @@ package com.greenart7c3.nostrsigner.models +import android.app.KeyguardManager import android.widget.Toast import androidx.compose.runtime.Immutable import androidx.compose.runtime.Stable @@ -7,6 +8,7 @@ import androidx.compose.ui.platform.Clipboard import com.greenart7c3.nostrsigner.Amber import com.greenart7c3.nostrsigner.DataStoreAccess import com.greenart7c3.nostrsigner.R +import com.greenart7c3.nostrsigner.service.DeviceUnlockPolicy import com.greenart7c3.nostrsigner.service.nip44v3.Nip44v3 import com.greenart7c3.nostrsigner.ui.setSensitiveClip import com.vitorpamplona.quartz.nip01Core.core.Event @@ -42,6 +44,13 @@ class Account( // Watches name/picture to mark the account saveable; injectable so Compose // previews can build an Account without the Amber Application singleton. scope: CoroutineScope = Amber.instance.applicationIOScope, + private val keyAccessCheck: () -> Unit = { + val required = Amber.instance.settings.requireUnlockedDevice + if (required) { + val keyguard = Amber.instance.getSystemService(KeyguardManager::class.java) + DeviceUnlockPolicy.requireAccess(required, keyguard?.isDeviceLocked ?: true) + } + }, ) { var signPolicy: Int = signPolicy set(value) { @@ -70,38 +79,74 @@ class Account( } } - suspend fun sign(eventTemplate: EventTemplate): T = signer.sign(eventTemplate) + internal fun requirePrivateKeyAccess() = keyAccessCheck() + + suspend fun sign(eventTemplate: EventTemplate): T { + requirePrivateKeyAccess() + return signer.sign(eventTemplate) + } fun signSync( createdAt: Long, kind: Int, tags: Array>, content: String, - ): T = signer.signerSync.sign(createdAt, kind, tags, content) + ): T { + requirePrivateKeyAccess() + return signer.signerSync.sign(createdAt, kind, tags, content) + } - fun nip49Encrypt(password: String): String = Nip49().encrypt(signer.keyPair.privKey!!.toHexKey(), password) + fun nip49Encrypt(password: String): String { + requirePrivateKeyAccess() + return Nip49().encrypt(signer.keyPair.privKey!!.toHexKey(), password) + } - suspend fun nip44Encrypt(plainText: String, toPublicKey: String): String = signer.nip44Encrypt(plainText, toPublicKey) + suspend fun nip44Encrypt(plainText: String, toPublicKey: String): String { + requirePrivateKeyAccess() + return signer.nip44Encrypt(plainText, toPublicKey) + } - suspend fun nip04Encrypt(plainText: String, toPublicKey: String): String = signer.nip04Encrypt(plainText, toPublicKey) + suspend fun nip04Encrypt(plainText: String, toPublicKey: String): String { + requirePrivateKeyAccess() + return signer.nip04Encrypt(plainText, toPublicKey) + } - suspend fun nip44Decrypt(cipherText: String, fromPublicKey: String): String = signer.nip44Decrypt(cipherText, fromPublicKey) + suspend fun nip44Decrypt(cipherText: String, fromPublicKey: String): String { + requirePrivateKeyAccess() + return signer.nip44Decrypt(cipherText, fromPublicKey) + } - suspend fun nip04Decrypt(cipherText: String, fromPublicKey: String): String = signer.nip04Decrypt(cipherText, fromPublicKey) + suspend fun nip04Decrypt(cipherText: String, fromPublicKey: String): String { + requirePrivateKeyAccess() + return signer.nip04Decrypt(cipherText, fromPublicKey) + } - fun nip44v3Encrypt(plainText: ByteArray, toPublicKey: String, kind: Int, scope: String): String = Nip44v3.encrypt(plainText, signer.keyPair.privKey!!, toPublicKey.hexToByteArray(), kind, scope) + fun nip44v3Encrypt(plainText: ByteArray, toPublicKey: String, kind: Int, scope: String): String { + requirePrivateKeyAccess() + return Nip44v3.encrypt(plainText, signer.keyPair.privKey!!, toPublicKey.hexToByteArray(), kind, scope) + } - fun nip44v3Decrypt(cipherText: String, fromPublicKey: String, kind: Int, scope: String): ByteArray = Nip44v3.decrypt(cipherText, signer.keyPair.privKey!!, fromPublicKey.hexToByteArray(), kind, scope) + fun nip44v3Decrypt(cipherText: String, fromPublicKey: String, kind: Int, scope: String): ByteArray { + requirePrivateKeyAccess() + return Nip44v3.decrypt(cipherText, signer.keyPair.privKey!!, fromPublicKey.hexToByteArray(), kind, scope) + } - suspend fun decrypt(encryptedContent: String, fromPublicKey: String): String = signer.decrypt(encryptedContent, fromPublicKey) + suspend fun decrypt(encryptedContent: String, fromPublicKey: String): String { + requirePrivateKeyAccess() + return signer.decrypt(encryptedContent, fromPublicKey) + } - suspend fun signPsbt(psbtHex: String): String = signer.signPsbt(psbtHex) + suspend fun signPsbt(psbtHex: String): String { + requirePrivateKeyAccess() + return signer.signPsbt(psbtHex) + } suspend fun seedWords() = runCatching { DataStoreAccess.getEncryptedKey(Amber.instance, npub, DataStoreAccess.SEED_WORDS) }.getOrNull() ?: "" fun decryptZapEvent( data: String, ): String? { + requirePrivateKeyAccess() val event = Event.fromJson(data) as LnZapRequestEvent return try { PrivateZapRequestBuilder().decryptZapEvent( @@ -114,9 +159,15 @@ class Account( } } - suspend fun createMessageNIP17(template: EventTemplate): NIP17Factory.Result = NIP17Factory().createMessageNIP17(template, signer) + suspend fun createMessageNIP17(template: EventTemplate): NIP17Factory.Result { + requirePrivateKeyAccess() + return NIP17Factory().createMessageNIP17(template, signer) + } - suspend fun getNsec(): String = signer.keyPair.privKey!!.toNsec() + suspend fun getNsec(): String { + requirePrivateKeyAccess() + return signer.keyPair.privKey!!.toNsec() + } fun copyToClipboard(clipboardManager: Clipboard) { Amber.instance.applicationIOScope.launch { diff --git a/app/src/main/java/com/greenart7c3/nostrsigner/service/ApplicationBackup.kt b/app/src/main/java/com/greenart7c3/nostrsigner/service/ApplicationBackup.kt index ca459bf8..8b62229e 100644 --- a/app/src/main/java/com/greenart7c3/nostrsigner/service/ApplicationBackup.kt +++ b/app/src/main/java/com/greenart7c3/nostrsigner/service/ApplicationBackup.kt @@ -115,6 +115,7 @@ internal fun backupKeyPair(identityPrivKey: ByteArray): KeyPair { } internal fun backupSigner(account: Account): NostrSignerInternal { + account.requirePrivateKeyAccess() val identityPrivKey = checkNotNull(account.signer.keyPair.privKey) { "ApplicationBackup: account has no private key" } diff --git a/app/src/main/java/com/greenart7c3/nostrsigner/service/DeviceUnlockPolicy.kt b/app/src/main/java/com/greenart7c3/nostrsigner/service/DeviceUnlockPolicy.kt new file mode 100644 index 00000000..b800383e --- /dev/null +++ b/app/src/main/java/com/greenart7c3/nostrsigner/service/DeviceUnlockPolicy.kt @@ -0,0 +1,9 @@ +package com.greenart7c3.nostrsigner.service + +object DeviceUnlockPolicy { + fun requireAccess(requireUnlockedDevice: Boolean, isDeviceLocked: Boolean) { + if (requireUnlockedDevice && isDeviceLocked) { + throw SecurityException("Private-key operations require an unlocked device") + } + } +} diff --git a/app/src/test/java/com/greenart7c3/nostrsigner/models/AccountCachedKeyLockTest.kt b/app/src/test/java/com/greenart7c3/nostrsigner/models/AccountCachedKeyLockTest.kt new file mode 100644 index 00000000..e38289e5 --- /dev/null +++ b/app/src/test/java/com/greenart7c3/nostrsigner/models/AccountCachedKeyLockTest.kt @@ -0,0 +1,48 @@ +package com.greenart7c3.nostrsigner.models + +import com.greenart7c3.nostrsigner.service.DeviceUnlockPolicy +import com.vitorpamplona.quartz.nip01Core.core.Event +import com.vitorpamplona.quartz.nip01Core.signers.NostrSignerInternal +import io.mockk.mockk +import io.mockk.verify +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.SupervisorJob +import kotlinx.coroutines.cancel +import kotlinx.coroutines.flow.MutableStateFlow +import kotlinx.coroutines.runBlocking +import org.junit.Assert.assertThrows +import org.junit.Test + +class AccountCachedKeyLockTest { + @Test + fun `cached account cannot sign or decrypt after device lock`() { + val scope = CoroutineScope(SupervisorJob() + Dispatchers.Unconfined) + val signer = mockk() + var locked = false + val account = Account( + signer = signer, + hexKey = "owner", + npub = "npub-owner", + name = MutableStateFlow(""), + picture = MutableStateFlow(""), + signPolicy = 1, + didBackup = false, + scope = scope, + keyAccessCheck = { DeviceUnlockPolicy.requireAccess(true, locked) }, + ) + try { + account.requirePrivateKeyAccess() + locked = true + assertThrows(SecurityException::class.java) { + account.signSync(0, 1, emptyArray(), "synthetic-event") + } + assertThrows(SecurityException::class.java) { + runBlocking { account.nip44Decrypt("synthetic-ciphertext", "peer") } + } + verify { signer wasNot Called } + } finally { + scope.cancel() + } + } +} diff --git a/app/src/test/java/com/greenart7c3/nostrsigner/service/ApplicationBackupCryptoTest.kt b/app/src/test/java/com/greenart7c3/nostrsigner/service/ApplicationBackupCryptoTest.kt index f2eb76e9..3c633300 100644 --- a/app/src/test/java/com/greenart7c3/nostrsigner/service/ApplicationBackupCryptoTest.kt +++ b/app/src/test/java/com/greenart7c3/nostrsigner/service/ApplicationBackupCryptoTest.kt @@ -45,6 +45,7 @@ class ApplicationBackupCryptoTest { signPolicy = 1, didBackup = true, scope = scope, + keyAccessCheck = {}, ) } diff --git a/app/src/test/java/com/greenart7c3/nostrsigner/service/DeviceUnlockPolicyTest.kt b/app/src/test/java/com/greenart7c3/nostrsigner/service/DeviceUnlockPolicyTest.kt new file mode 100644 index 00000000..addfd501 --- /dev/null +++ b/app/src/test/java/com/greenart7c3/nostrsigner/service/DeviceUnlockPolicyTest.kt @@ -0,0 +1,19 @@ +package com.greenart7c3.nostrsigner.service + +import org.junit.Assert.assertThrows +import org.junit.Test + +class DeviceUnlockPolicyTest { + @Test + fun `opt in blocks operations after the device locks`() { + DeviceUnlockPolicy.requireAccess(true, false) + assertThrows(SecurityException::class.java) { DeviceUnlockPolicy.requireAccess(true, true) } + DeviceUnlockPolicy.requireAccess(true, false) + } + + @Test + fun `default policy preserves authorized background operation`() { + DeviceUnlockPolicy.requireAccess(false, true) + DeviceUnlockPolicy.requireAccess(false, false) + } +} diff --git a/app/src/test/java/com/greenart7c3/nostrsigner/service/SubscriptionTestUtils.kt b/app/src/test/java/com/greenart7c3/nostrsigner/service/SubscriptionTestUtils.kt index 5148ed56..80cb252c 100644 --- a/app/src/test/java/com/greenart7c3/nostrsigner/service/SubscriptionTestUtils.kt +++ b/app/src/test/java/com/greenart7c3/nostrsigner/service/SubscriptionTestUtils.kt @@ -38,5 +38,6 @@ fun newTestAccount(scope: CoroutineScope): Account { signPolicy = 0, didBackup = false, scope = scope, + keyAccessCheck = {}, ) } From 53701fc6f2f7da3c5c82e722e9939cc946954cf8 Mon Sep 17 00:00:00 2001 From: Codex Security Review Date: Tue, 13 Oct 2026 22:24:49 +0200 Subject: [PATCH 2/3] fix(security): gate seedWords, fail closed before settings load, fix test - seedWords() returns the cached recovery mnemonic and was the one Account private-key entry point left ungated. - The default keyAccessCheck read requireUnlockedDevice before async settings load landed, so the gate ran on a default-valued guess in the startup window; mirror WebDavService's check(isSettingsLoaded). - AccountCachedKeyLockTest was missing io.mockk.Called for the wasNot Called verification and now also covers the seedWords gate. --- .../java/com/greenart7c3/nostrsigner/models/Account.kt | 8 +++++++- .../nostrsigner/models/AccountCachedKeyLockTest.kt | 4 ++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/app/src/main/java/com/greenart7c3/nostrsigner/models/Account.kt b/app/src/main/java/com/greenart7c3/nostrsigner/models/Account.kt index dd237e23..c61d59b0 100644 --- a/app/src/main/java/com/greenart7c3/nostrsigner/models/Account.kt +++ b/app/src/main/java/com/greenart7c3/nostrsigner/models/Account.kt @@ -45,6 +45,9 @@ class Account( // previews can build an Account without the Amber Application singleton. scope: CoroutineScope = Amber.instance.applicationIOScope, private val keyAccessCheck: () -> Unit = { + // Until settings load, requireUnlockedDevice's default (false) is a guess, not a + // decision — the same pre-load window WebDavService fails closed on. + check(Amber.instance.isSettingsLoaded) { "Settings are not loaded yet" } val required = Amber.instance.settings.requireUnlockedDevice if (required) { val keyguard = Amber.instance.getSystemService(KeyguardManager::class.java) @@ -141,7 +144,10 @@ class Account( return signer.signPsbt(psbtHex) } - suspend fun seedWords() = runCatching { DataStoreAccess.getEncryptedKey(Amber.instance, npub, DataStoreAccess.SEED_WORDS) }.getOrNull() ?: "" + suspend fun seedWords(): String { + requirePrivateKeyAccess() + return runCatching { DataStoreAccess.getEncryptedKey(Amber.instance, npub, DataStoreAccess.SEED_WORDS) }.getOrNull() ?: "" + } fun decryptZapEvent( data: String, diff --git a/app/src/test/java/com/greenart7c3/nostrsigner/models/AccountCachedKeyLockTest.kt b/app/src/test/java/com/greenart7c3/nostrsigner/models/AccountCachedKeyLockTest.kt index e38289e5..bdbcbbc3 100644 --- a/app/src/test/java/com/greenart7c3/nostrsigner/models/AccountCachedKeyLockTest.kt +++ b/app/src/test/java/com/greenart7c3/nostrsigner/models/AccountCachedKeyLockTest.kt @@ -3,6 +3,7 @@ package com.greenart7c3.nostrsigner.models import com.greenart7c3.nostrsigner.service.DeviceUnlockPolicy import com.vitorpamplona.quartz.nip01Core.core.Event import com.vitorpamplona.quartz.nip01Core.signers.NostrSignerInternal +import io.mockk.Called import io.mockk.mockk import io.mockk.verify import kotlinx.coroutines.CoroutineScope @@ -40,6 +41,9 @@ class AccountCachedKeyLockTest { assertThrows(SecurityException::class.java) { runBlocking { account.nip44Decrypt("synthetic-ciphertext", "peer") } } + assertThrows(SecurityException::class.java) { + runBlocking { account.seedWords() } + } verify { signer wasNot Called } } finally { scope.cancel() From 599a7f82968b260875eeded82ce83f254b25876f Mon Sep 17 00:00:00 2001 From: Steve <263163432+mptfire@users.noreply.github.com> Date: Wed, 14 Oct 2026 11:01:42 +0200 Subject: [PATCH 3/3] ci: validate lint, free release, and security regressions Prepared by Codex security review. Preserve the existing security fix and follow-up commits; add the missing validation gates without changing app code. --- .github/workflows/build.yml | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index e072d2a6..282b7910 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -37,12 +37,24 @@ jobs: - name: Linter (gradle) run: ./gradlew ktlintCheck + - name: Android lint (gradle) + run: ./gradlew lint --no-daemon + - name: Test (gradle) run: ./gradlew test --no-daemon + - name: Python security regressions + run: | + if [ -d tests ]; then + python3 -m unittest discover -s tests -p 'test_*.py' -v + fi + - name: Build APK (gradle) run: ./gradlew assembleDebug --no-daemon + - name: Build free release APK (gradle) + run: ./gradlew :app:assembleFreeRelease --no-daemon + - name: Upload APK uses: actions/upload-artifact@v7 with: