From 3708cc558337a06cc94370121d1e122f4f75757e Mon Sep 17 00:00:00 2001 From: Greyson Parrelli Date: Fri, 23 Dec 2022 11:26:36 -0500 Subject: [PATCH] Add additional protections around recipientIds and threadIds matching. --- .../securesms/database/ThreadTable.kt | 14 +++++- .../securesms/jobs/IndividualSendJob.java | 19 +++++--- .../securesms/sms/MessageSender.java | 44 +++++-------------- 3 files changed, 36 insertions(+), 41 deletions(-) diff --git a/app/src/main/java/org/thoughtcrime/securesms/database/ThreadTable.kt b/app/src/main/java/org/thoughtcrime/securesms/database/ThreadTable.kt index a369dc31f..c10226014 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/database/ThreadTable.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/database/ThreadTable.kt @@ -12,6 +12,7 @@ import org.jsoup.helper.StringUtil import org.signal.core.util.CursorUtil import org.signal.core.util.SqlUtil import org.signal.core.util.delete +import org.signal.core.util.exists import org.signal.core.util.logging.Log import org.signal.core.util.or import org.signal.core.util.readToList @@ -1078,7 +1079,11 @@ class ThreadTable(context: Context, databaseHelper: SignalDatabase) : DatabaseTa Log.i(TAG, "Using remapped threadId: " + candidateId + " -> " + remapped.get()) remapped.get() } else { - candidateId + if (areThreadIdAndRecipientAssociated(candidateId, recipient)) { + candidateId + } else { + throw IllegalArgumentException() + } } } else { getOrCreateThreadIdFor(recipient, distributionType) @@ -1094,6 +1099,13 @@ class ThreadTable(context: Context, databaseHelper: SignalDatabase) : DatabaseTa return threadId ?: createThreadForRecipient(recipient.id, recipient.isGroup, distributionType) } + fun areThreadIdAndRecipientAssociated(threadId: Long, recipient: Recipient): Boolean { + return readableDatabase + .exists(TABLE_NAME) + .where("$ID = ? AND $RECIPIENT_ID = ?", threadId, recipient.id) + .run() + } + fun getThreadIdFor(recipientId: RecipientId): Long? { return readableDatabase .select(ID) diff --git a/app/src/main/java/org/thoughtcrime/securesms/jobs/IndividualSendJob.java b/app/src/main/java/org/thoughtcrime/securesms/jobs/IndividualSendJob.java index 3246b92ed..fe82fb980 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/jobs/IndividualSendJob.java +++ b/app/src/main/java/org/thoughtcrime/securesms/jobs/IndividualSendJob.java @@ -84,18 +84,25 @@ public class IndividualSendJob extends PushSendJob { this.messageId = messageId; } + public static Job create(long messageId, @NonNull Recipient recipient, boolean hasMedia) { + if (!recipient.hasServiceId()) { + throw new AssertionError("No ServiceId!"); + } + + if (recipient.isGroup()) { + throw new AssertionError("This job does not send group messages!"); + } + + return new IndividualSendJob(messageId, recipient, hasMedia); + } + @WorkerThread public static void enqueue(@NonNull Context context, @NonNull JobManager jobManager, long messageId, @NonNull Recipient recipient) { try { - if (!recipient.hasServiceId()) { - throw new AssertionError("No ServiceId!"); - } - OutgoingMessage message = SignalDatabase.messages().getOutgoingMessage(messageId); Set attachmentUploadIds = enqueueCompressingAndUploadAttachmentsChains(jobManager, message); - jobManager.add(new IndividualSendJob(messageId, recipient, attachmentUploadIds.size() > 0), attachmentUploadIds, recipient.getId().toQueueKey()); - + jobManager.add(IndividualSendJob.create(messageId, recipient, attachmentUploadIds.size() > 0), attachmentUploadIds, recipient.getId().toQueueKey()); } catch (NoSuchMessageException | MmsException e) { Log.w(TAG, "Failed to enqueue message.", e); SignalDatabase.messages().markAsSentFailed(messageId); diff --git a/app/src/main/java/org/thoughtcrime/securesms/sms/MessageSender.java b/app/src/main/java/org/thoughtcrime/securesms/sms/MessageSender.java index 9e98e4c4c..3ac2733bc 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/sms/MessageSender.java +++ b/app/src/main/java/org/thoughtcrime/securesms/sms/MessageSender.java @@ -260,23 +260,16 @@ public class MessageSender { Preconditions.checkArgument(message.getAttachments().isEmpty(), "If the media is pre-uploaded, there should be no attachments on the message."); try { - ThreadTable threadTable = SignalDatabase.threads(); - MessageTable mmsDatabase = SignalDatabase.messages(); + ThreadTable threadTable = SignalDatabase.threads(); + MessageTable mmsDatabase = SignalDatabase.messages(); AttachmentTable attachmentDatabase = SignalDatabase.attachments(); - long allocatedThreadId; - - if (threadId == -1) { - allocatedThreadId = threadTable.getOrCreateThreadIdFor(message.getRecipient(), message.getDistributionType()); - } else { - allocatedThreadId = threadId; - } - - Recipient recipient = message.getRecipient(); - long messageId = mmsDatabase.insertMessageOutbox(applyUniversalExpireTimerIfNecessary(context, recipient, message, allocatedThreadId), - allocatedThreadId, - false, - insertListener); + Recipient recipient = message.getRecipient(); + long allocatedThreadId = threadTable.getOrCreateValidThreadId(message.getRecipient(), threadId); + long messageId = mmsDatabase.insertMessageOutbox(applyUniversalExpireTimerIfNecessary(context, recipient, message, allocatedThreadId), + allocatedThreadId, + false, + insertListener); List attachmentIds = Stream.of(preUploadResults).map(PreUploadResult::getAttachmentId).toList(); List jobIds = Stream.of(preUploadResults).map(PreUploadResult::getJobIds).flatMap(Stream::of).toList(); @@ -406,7 +399,7 @@ public class MessageSender { } else if (recipient.isDistributionList()) { jobManager.add(new PushDistributionListSendJob(messageId, recipient.getId(), true, Collections.emptySet()), messageDependsOnIds, recipient.getId().toQueueKey()); } else { - jobManager.add(new IndividualSendJob(messageId, recipient, true), messageDependsOnIds, recipient.getId().toQueueKey()); + jobManager.add(IndividualSendJob.create(messageId, recipient, true), messageDependsOnIds, recipient.getId().toQueueKey()); } } } @@ -532,7 +525,7 @@ public class MessageSender { JobManager jobManager = ApplicationDependencies.getJobManager(); if (uploadJobIds.size() > 0) { - Job mediaSend = new IndividualSendJob(messageId, recipient, true); + Job mediaSend = IndividualSendJob.create(messageId, recipient, true); jobManager.add(mediaSend, uploadJobIds); } else { IndividualSendJob.enqueue(context, jobManager, messageId, recipient); @@ -561,28 +554,11 @@ public class MessageSender { } } - private static void sendSms(Recipient recipient, long messageId) { - JobManager jobManager = ApplicationDependencies.getJobManager(); - jobManager.add(new SmsSendJob(messageId, recipient)); - } - private static void sendMms(Context context, long messageId) { JobManager jobManager = ApplicationDependencies.getJobManager(); MmsSendJob.enqueue(context, jobManager, messageId); } - private static boolean isPushTextSend(Context context, Recipient recipient, boolean keyExchange) { - if (!SignalStore.account().isRegistered()) { - return false; - } - - if (keyExchange) { - return false; - } - - return isPushDestination(context, recipient); - } - private static boolean isPushMediaSend(Context context, Recipient recipient) { if (!SignalStore.account().isRegistered()) { return false;