diff --git a/app/src/main/kotlin/org/libremail/data/attachment/AttachmentUriGrants.kt b/app/src/main/kotlin/org/libremail/data/attachment/AttachmentUriGrants.kt new file mode 100644 index 0000000..2a708de --- /dev/null +++ b/app/src/main/kotlin/org/libremail/data/attachment/AttachmentUriGrants.kt @@ -0,0 +1,68 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +package org.libremail.data.attachment + +import android.content.Context +import android.content.Intent +import android.net.Uri +import dagger.hilt.android.qualifiers.ApplicationContext +import org.libremail.data.local.dao.DraftDao +import org.libremail.data.local.dao.OutboxDao +import org.libremail.data.local.toOutgoingAttachments +import javax.inject.Inject +import javax.inject.Singleton + +/** + * Releases the persistable read grants the compose pickers take on attachment / inline-image URIs. + * `ComposeScreen` calls `takePersistableUriPermission` on every pick, but nothing released them, so + * the app accumulated indefinite read access to every file/photo ever attached and could hit the + * per-app persisted-grant cap — after which a silently-swallowed take fails a later draft's image + * reload (post-batch security review, Low). + * + * The picked bytes are copied into the app cache when a message is enqueued + * (`MailRepositoryImpl.copyAttachments`), so a grant is only truly needed to reload an image when a + * *draft* is reopened. Callers therefore invoke [releaseUnreferenced] once the referencing row is + * gone — a draft is deleted, or an outbox message is sent or cancelled — passing that row's URIs. + * A URI still referenced by another live draft or outbox row is kept; releasing a grant we do not + * actually hold is expected and swallowed. + */ +@Singleton +class AttachmentUriGrants @Inject constructor( + @ApplicationContext private val context: Context, + private val draftDao: DraftDao, + private val outboxDao: OutboxDao, +) { + /** + * Releases the persistable grant of each URI in [uris] that no *remaining* draft or outbox row + * still references. Call it after deleting the row that referenced them, so that row no longer + * counts toward the "still referenced" check. + */ + suspend fun releaseUnreferenced(uris: Collection) { + if (uris.isEmpty()) return + unreferencedUris(uris, referencedUris()).forEach(::release) + } + + /** Every attachment / inline-image URI still referenced by a draft or a queued outbox message. */ + private suspend fun referencedUris(): Set { + val fromDrafts = draftDao.getAll().flatMap { it.attachments.toOutgoingAttachments() } + val fromOutbox = outboxDao.getAll().flatMap { it.attachments.toOutgoingAttachments() } + return (fromDrafts + fromOutbox).mapTo(HashSet()) { it.uri } + } + + private fun release(uri: String) { + // Only a grant we actually hold can be released; a URI never persisted (or already released) + // throws SecurityException, which is expected here and deliberately ignored. + runCatching { + context.contentResolver.releasePersistableUriPermission( + Uri.parse(uri), + Intent.FLAG_GRANT_READ_URI_PERMISSION, + ) + } + } +} + +/** + * The distinct URIs in [candidates] not present in [referenced] — i.e. the grants safe to release. + * Kept as a pure top-level function so the release decision is unit-testable without Android types. + */ +internal fun unreferencedUris(candidates: Collection, referenced: Set): List = + candidates.distinct().filterNot { it in referenced } diff --git a/app/src/main/kotlin/org/libremail/data/local/dao/DraftDao.kt b/app/src/main/kotlin/org/libremail/data/local/dao/DraftDao.kt index 1ad234b..22af30c 100644 --- a/app/src/main/kotlin/org/libremail/data/local/dao/DraftDao.kt +++ b/app/src/main/kotlin/org/libremail/data/local/dao/DraftDao.kt @@ -19,6 +19,10 @@ interface DraftDao { @Query("SELECT * FROM drafts WHERE id = :id LIMIT 1") suspend fun getById(id: String): DraftEntity? + /** All drafts, used to check whether an attachment URI is still referenced before releasing its grant. */ + @Query("SELECT * FROM drafts") + suspend fun getAll(): List + @Insert(onConflict = OnConflictStrategy.REPLACE) suspend fun upsert(draft: DraftEntity) diff --git a/app/src/main/kotlin/org/libremail/data/local/dao/OutboxDao.kt b/app/src/main/kotlin/org/libremail/data/local/dao/OutboxDao.kt index 31e4e6f..4f8ab27 100644 --- a/app/src/main/kotlin/org/libremail/data/local/dao/OutboxDao.kt +++ b/app/src/main/kotlin/org/libremail/data/local/dao/OutboxDao.kt @@ -15,6 +15,9 @@ interface OutboxDao { @Query("SELECT * FROM outbox ORDER BY createdAt") suspend fun getAll(): List + @Query("SELECT * FROM outbox WHERE id = :id LIMIT 1") + suspend fun getById(id: String): OutboxEntity? + @Query("SELECT * FROM outbox ORDER BY createdAt") fun observeAll(): Flow> diff --git a/app/src/main/kotlin/org/libremail/data/repository/MailRepositoryImpl.kt b/app/src/main/kotlin/org/libremail/data/repository/MailRepositoryImpl.kt index 8463729..9c30e20 100644 --- a/app/src/main/kotlin/org/libremail/data/repository/MailRepositoryImpl.kt +++ b/app/src/main/kotlin/org/libremail/data/repository/MailRepositoryImpl.kt @@ -20,6 +20,7 @@ import kotlinx.coroutines.withContext import org.libremail.data.ReplyBuilder import org.libremail.data.SignatureBlock import org.libremail.data.Snippet +import org.libremail.data.attachment.AttachmentUriGrants import org.libremail.data.attachmentCacheDir import org.libremail.data.local.dao.AccountDao import org.libremail.data.local.dao.AttachmentDao @@ -32,6 +33,7 @@ import org.libremail.data.local.entity.MessageRouting import org.libremail.data.local.entity.OutboxEntity import org.libremail.data.local.toDomain import org.libremail.data.local.toEntity +import org.libremail.data.local.toOutgoingAttachments import org.libremail.data.local.toOutgoingAttachmentsJson import org.libremail.data.settings.AccountSettingsRepository import org.libremail.data.settings.SignatureRepository @@ -49,6 +51,7 @@ import org.libremail.domain.model.OutgoingAttachment import org.libremail.domain.model.OutgoingMessage import org.libremail.domain.model.ReplyMode import org.libremail.domain.model.UnreadCount +import org.libremail.domain.model.sanitizeAttachmentName import org.libremail.domain.repository.MailRepository import org.libremail.mail.ImapClient import java.io.File @@ -70,6 +73,7 @@ class MailRepositoryImpl @Inject constructor( private val sendScheduler: SendScheduler, private val accountSettingsRepository: AccountSettingsRepository, private val signatureRepository: SignatureRepository, + private val attachmentUriGrants: AttachmentUriGrants, ) : MailRepository { // Application-lifetime scope for fire-and-forget server pushes that must outlive the caller — e.g. @@ -421,7 +425,7 @@ class MailRepositoryImpl @Inject constructor( private fun copyAttachments(outboxId: String, attachments: List) { if (attachments.isEmpty()) return attachments.forEachIndexed { index, attachment -> - val safeName = attachment.name.substringAfterLast('/').substringAfterLast('\\').ifBlank { "attachment" } + val safeName = sanitizeAttachmentName(attachment.name) val dir = File(context.cacheDir, "outbox/$outboxId/$index").apply { mkdirs() } runCatching { context.contentResolver.openInputStream(Uri.parse(attachment.uri))?.use { input -> @@ -437,15 +441,24 @@ class MailRepositoryImpl @Inject constructor( override suspend fun saveDraft(draft: Draft) = draftDao.upsert(draft.toEntity()) - override suspend fun deleteDraft(id: String) = draftDao.delete(id) + override suspend fun deleteDraft(id: String) { + // Capture the draft's attachment URIs before the row is gone, then release any persistable grant + // no other live draft/outbox row still needs (security review): a deleted draft can never reopen. + val uris = draftDao.getById(id)?.attachments?.toOutgoingAttachments()?.map { it.uri }.orEmpty() + draftDao.delete(id) + attachmentUriGrants.releaseUnreferenced(uris) + } override fun observeOutbox(): Flow> = outboxDao.observeAll().map { rows -> rows.map { it.toDomain() } } override suspend fun cancelOutboxMessage(id: String) { + val uris = outboxDao.getById(id)?.attachments?.toOutgoingAttachments()?.map { it.uri }.orEmpty() outboxDao.delete(id) File(context.cacheDir, "outbox/$id").deleteRecursively() + // The queued copy is gone; release any picked-URI grant no other live draft/outbox row needs. + attachmentUriGrants.releaseUnreferenced(uris) } override suspend fun retryOutbox() = sendScheduler.sendNow() @@ -484,7 +497,7 @@ class MailRepositoryImpl @Inject constructor( * and avoids filename collisions between messages. */ private fun attachmentFile(messageId: String, partIndex: Int, filename: String): File { - val safeName = filename.substringAfterLast('/').substringAfterLast('\\').ifBlank { "attachment" } + val safeName = sanitizeAttachmentName(filename) return File(attachmentCacheDir(context.cacheDir, messageId), "$partIndex/$safeName") } } diff --git a/app/src/main/kotlin/org/libremail/data/sync/SendWorker.kt b/app/src/main/kotlin/org/libremail/data/sync/SendWorker.kt index bdd1eba..087253a 100644 --- a/app/src/main/kotlin/org/libremail/data/sync/SendWorker.kt +++ b/app/src/main/kotlin/org/libremail/data/sync/SendWorker.kt @@ -9,8 +9,10 @@ import androidx.work.WorkerParameters import dagger.Lazy import dagger.assisted.Assisted import dagger.assisted.AssistedInject +import org.libremail.data.attachment.AttachmentUriGrants import org.libremail.data.local.dao.AccountDao import org.libremail.data.local.dao.OutboxDao +import org.libremail.data.local.entity.OutboxEntity import org.libremail.data.local.toDomain import org.libremail.data.local.toOutgoingAttachments import org.libremail.data.security.EncryptedCacheGuard @@ -39,6 +41,8 @@ class SendWorker @AssistedInject constructor( private val graphSender: GraphSender, private val connectionFactory: Lazy, private val cacheGuard: EncryptedCacheGuard, + // Lazy for the same reason as the DAOs above: resolving it touches the Room DB. + private val attachmentUriGrants: Lazy, ) : CoroutineWorker(appContext, workerParams) { private companion object { @@ -50,6 +54,7 @@ class SendWorker @AssistedInject constructor( val outboxDao = this.outboxDao.get() val accountDao = this.accountDao.get() val connectionFactory = this.connectionFactory.get() + val attachmentUriGrants = this.attachmentUriGrants.get() val pending = outboxDao.getAll() if (pending.isEmpty()) return Result.success() @@ -60,6 +65,7 @@ class SendWorker @AssistedInject constructor( if (account == null) { outboxDao.delete(entity.id) // account removed — drop the queued message attachmentDir.deleteRecursively() + attachmentUriGrants.releaseUnreferenced(entity.attachmentUris()) continue } runCatching { @@ -87,6 +93,9 @@ class SendWorker @AssistedInject constructor( onSuccess = { outboxDao.delete(entity.id) attachmentDir.deleteRecursively() + // The picked bytes were staged at enqueue; with the row sent, drop the persistable + // grant unless a live draft/outbox row still references the same URI (security review). + attachmentUriGrants.releaseUnreferenced(entity.attachmentUris()) }, onFailure = { e -> if (e is GraphSendException && e.mayHaveSent) { @@ -167,4 +176,7 @@ class SendWorker @AssistedInject constructor( ?.sortedBy { it.name.toIntOrNull() ?: Int.MAX_VALUE } ?.mapNotNull { it.listFiles()?.firstOrNull() } .orEmpty() + + /** The picked content-URIs this queued message was built from, for releasing their persistable grants. */ + private fun OutboxEntity.attachmentUris(): List = attachments.toOutgoingAttachments().map { it.uri } } diff --git a/app/src/main/kotlin/org/libremail/domain/model/OutgoingMessage.kt b/app/src/main/kotlin/org/libremail/domain/model/OutgoingMessage.kt index d1c4bc5..1c53693 100644 --- a/app/src/main/kotlin/org/libremail/domain/model/OutgoingMessage.kt +++ b/app/src/main/kotlin/org/libremail/domain/model/OutgoingMessage.kt @@ -33,3 +33,12 @@ data class OutgoingAttachment( val contentId: String? = null, val isInline: Boolean = false, ) + +/** + * Sanitizes a picked or received attachment display name before it becomes an on-disk filename or a + * MIME `Content-Disposition` filename: drops any path prefix and strips ISO control characters + * (including CR/LF), so a crafted name can neither traverse directories nor inject header lines. + * Falls back to `"attachment"` when nothing usable remains. + */ +fun sanitizeAttachmentName(raw: String): String = + raw.substringAfterLast('/').substringAfterLast('\\').filterNot { it.isISOControl() }.ifBlank { "attachment" } diff --git a/app/src/main/kotlin/org/libremail/ui/compose/ComposeScreen.kt b/app/src/main/kotlin/org/libremail/ui/compose/ComposeScreen.kt index fdc0aad..4f66380 100644 --- a/app/src/main/kotlin/org/libremail/ui/compose/ComposeScreen.kt +++ b/app/src/main/kotlin/org/libremail/ui/compose/ComposeScreen.kt @@ -74,6 +74,7 @@ import kotlinx.coroutines.flow.collect import org.libremail.R import org.libremail.domain.model.Account import org.libremail.domain.model.OutgoingAttachment +import org.libremail.domain.model.sanitizeAttachmentName import org.libremail.ui.compose.format.FontRegistry @OptIn(ExperimentalMaterial3Api::class) @@ -410,7 +411,9 @@ private fun queryFileName(context: Context, uri: Uri): String { val name = context.contentResolver .query(uri, arrayOf(OpenableColumns.DISPLAY_NAME), null, null, null) ?.use { cursor -> if (cursor.moveToFirst()) cursor.getString(0) else null } - return name ?: uri.lastPathSegment?.substringAfterLast('/') ?: "attachment" + // Strip path separators / control chars so a crafted display name can't traverse dirs or inject + // into the on-disk name or the MIME Content-Disposition filename (security review). + return sanitizeAttachmentName(name ?: uri.lastPathSegment.orEmpty()) } @Composable diff --git a/app/src/test/kotlin/org/libremail/data/attachment/AttachmentUriGrantsTest.kt b/app/src/test/kotlin/org/libremail/data/attachment/AttachmentUriGrantsTest.kt new file mode 100644 index 0000000..b9ff6f3 --- /dev/null +++ b/app/src/test/kotlin/org/libremail/data/attachment/AttachmentUriGrantsTest.kt @@ -0,0 +1,50 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +package org.libremail.data.attachment + +import org.junit.Test +import kotlin.test.assertEquals + +/** + * The release decision that [AttachmentUriGrants] runs after a draft/outbox row is deleted: a picked + * URI's persistable grant is released only when no *remaining* draft or outbox row still references + * it. The Android release call itself (`releasePersistableUriPermission`) is a thin wrapper; the + * decision here is the part worth pinning. + */ +class AttachmentUriGrantsTest { + + @Test + fun `releases a uri referenced by no remaining row`() { + assertEquals( + listOf("content://a"), + unreferencedUris(candidates = listOf("content://a"), referenced = emptySet()), + ) + } + + @Test + fun `keeps a uri still referenced by another live draft or outbox row`() { + // content://shared is attached to a second draft that is still around, so its grant must stay. + assertEquals( + listOf("content://gone"), + unreferencedUris( + candidates = listOf("content://gone", "content://shared"), + referenced = setOf("content://shared"), + ), + ) + } + + @Test + fun `deduplicates candidate uris`() { + assertEquals( + listOf("content://a"), + unreferencedUris(candidates = listOf("content://a", "content://a"), referenced = emptySet()), + ) + } + + @Test + fun `releases nothing when every candidate is still referenced`() { + assertEquals( + emptyList(), + unreferencedUris(candidates = listOf("content://a"), referenced = setOf("content://a")), + ) + } +} diff --git a/app/src/test/kotlin/org/libremail/data/repository/MailRepositoryGrantsTest.kt b/app/src/test/kotlin/org/libremail/data/repository/MailRepositoryGrantsTest.kt new file mode 100644 index 0000000..ddab499 --- /dev/null +++ b/app/src/test/kotlin/org/libremail/data/repository/MailRepositoryGrantsTest.kt @@ -0,0 +1,114 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +package org.libremail.data.repository + +import android.content.Context +import io.mockk.Runs +import io.mockk.coEvery +import io.mockk.coVerify +import io.mockk.coVerifyOrder +import io.mockk.every +import io.mockk.just +import io.mockk.mockk +import kotlinx.coroutines.test.runTest +import org.junit.Test +import org.libremail.data.attachment.AttachmentUriGrants +import org.libremail.data.local.dao.DraftDao +import org.libremail.data.local.dao.OutboxDao +import org.libremail.data.local.entity.DraftEntity +import org.libremail.data.local.entity.OutboxEntity +import java.nio.file.Files + +/** + * Deleting a draft, or cancelling a queued outbox message, must hand the removed row's picked-URI + * grants to [AttachmentUriGrants] so they can be released once nothing references them (post-batch + * security review). The release itself is decided/executed in [AttachmentUriGrants]; here we pin that + * the repository captures the right URIs and calls it *after* the row is gone. + */ +class MailRepositoryGrantsTest { + + private val draftDao = mockk() + private val outboxDao = mockk() + private val attachmentUriGrants = mockk(relaxed = true) + private val context = mockk(relaxed = true) + + private val repository = MailRepositoryImpl( + context = context, + messageDao = mockk(relaxed = true), + accountDao = mockk(relaxed = true), + attachmentDao = mockk(relaxed = true), + outboxDao = outboxDao, + draftDao = draftDao, + folderDao = mockk(relaxed = true), + imapClient = mockk(relaxed = true), + connectionFactory = mockk(relaxed = true), + sendScheduler = mockk(relaxed = true), + accountSettingsRepository = mockk(relaxed = true), + signatureRepository = mockk(relaxed = true), + attachmentUriGrants = attachmentUriGrants, + ) + + @Test + fun `deleteDraft releases the deleted draft's URI grants after removing the row`() = runTest { + coEvery { draftDao.getById("d1") } returns draftEntity( + "d1", + """[{"uri":"content://pick/1","name":"a.png"}]""", + ) + coEvery { draftDao.delete("d1") } just Runs + + repository.deleteDraft("d1") + + // Order matters: the row must be gone before the "still referenced?" check runs, so a draft + // doesn't keep its own grant alive on the way out. + coVerifyOrder { + draftDao.delete("d1") + attachmentUriGrants.releaseUnreferenced(listOf("content://pick/1")) + } + } + + @Test + fun `deleteDraft of an attachment-less draft releases nothing`() = runTest { + coEvery { draftDao.getById("d2") } returns null // already gone: no attachments to release + coEvery { draftDao.delete("d2") } just Runs + + repository.deleteDraft("d2") + + coVerify { attachmentUriGrants.releaseUnreferenced(emptyList()) } + } + + @Test + fun `cancelOutboxMessage releases the cancelled message's URI grants`() = runTest { + every { context.cacheDir } returns Files.createTempDirectory("outbox").toFile() + coEvery { outboxDao.getById("o1") } returns outboxEntity( + "o1", + """[{"uri":"content://pick/2","name":"b.png"}]""", + ) + coEvery { outboxDao.delete("o1") } just Runs + + repository.cancelOutboxMessage("o1") + + coVerify { attachmentUriGrants.releaseUnreferenced(listOf("content://pick/2")) } + } + + private fun draftEntity(id: String, attachmentsJson: String) = DraftEntity( + id = id, + accountId = "acct", + toAddresses = "", + ccAddresses = "", + bccAddresses = "", + subject = "", + body = "", + updatedAt = 0L, + attachments = attachmentsJson, + ) + + private fun outboxEntity(id: String, attachmentsJson: String) = OutboxEntity( + id = id, + accountId = "acct", + toAddresses = "bob@example.org", + ccAddresses = "", + subject = "Hi", + body = "body", + createdAt = 0L, + attachments = attachmentsJson, + ) +} diff --git a/app/src/test/kotlin/org/libremail/data/repository/MailRepositoryImplTest.kt b/app/src/test/kotlin/org/libremail/data/repository/MailRepositoryImplTest.kt index 874e88c..624cea9 100644 --- a/app/src/test/kotlin/org/libremail/data/repository/MailRepositoryImplTest.kt +++ b/app/src/test/kotlin/org/libremail/data/repository/MailRepositoryImplTest.kt @@ -79,6 +79,8 @@ class MailRepositoryImplTest { sendScheduler = mockk(), accountSettingsRepository = accountSettingsRepository, signatureRepository = signatureRepository, + // Grant-release wiring (deleteDraft / cancelOutboxMessage) is covered by MailRepositoryGrantsTest. + attachmentUriGrants = mockk(relaxed = true), ) @Test diff --git a/app/src/test/kotlin/org/libremail/domain/model/SanitizeAttachmentNameTest.kt b/app/src/test/kotlin/org/libremail/domain/model/SanitizeAttachmentNameTest.kt new file mode 100644 index 0000000..19be63c --- /dev/null +++ b/app/src/test/kotlin/org/libremail/domain/model/SanitizeAttachmentNameTest.kt @@ -0,0 +1,48 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +package org.libremail.domain.model + +import org.junit.Test +import kotlin.test.assertEquals + +/** + * A picked/received attachment name becomes an on-disk filename and a MIME `Content-Disposition` + * filename, so it must not carry path separators (directory traversal) or CR/LF/control characters + * (header injection). [sanitizeAttachmentName] is the single chokepoint the compose picker and the + * outbox staging both run names through. + */ +class SanitizeAttachmentNameTest { + + @Test + fun `keeps an ordinary name unchanged`() { + assertEquals("report.pdf", sanitizeAttachmentName("report.pdf")) + } + + @Test + fun `strips CR and LF so a name cannot inject a header line`() { + assertEquals( + "invoice.pdfX-Evil: 1", + sanitizeAttachmentName("invoice.pdf\r\nX-Evil: 1"), + ) + } + + @Test + fun `strips other control characters`() { + // Char(9) = TAB and Char(0) = NUL are ISO control chars — removed, leaving the printable name. + val tab = Char(9) + val nul = Char(0) + assertEquals("ab.png", sanitizeAttachmentName("a" + tab + "b" + nul + ".png")) + } + + @Test + fun `drops any path prefix using either separator`() { + assertEquals("passwd", sanitizeAttachmentName("../../etc/passwd")) + assertEquals("evil.exe", sanitizeAttachmentName("C:\\Windows\\evil.exe")) + } + + @Test + fun `falls back to a default when nothing usable remains`() { + assertEquals("attachment", sanitizeAttachmentName("")) + assertEquals("attachment", sanitizeAttachmentName("\r\n")) + assertEquals("attachment", sanitizeAttachmentName("some/dir/")) + } +}