From a2de80091d3d588e029f362645b7166d2430768d Mon Sep 17 00:00:00 2001 From: Onuray Sahin Date: Fri, 5 Feb 2021 11:56:42 +0300 Subject: [PATCH] Code review fixes. --- .../internal/crypto/DefaultCryptoService.kt | 15 ++++- .../crypto/IncomingGossipingRequestManager.kt | 13 +++-- .../crypto/algorithms/IMXEncrypting.kt | 37 ------------ .../crypto/algorithms/IMXGroupEncryption.kt | 57 +++++++++++++++++++ .../algorithms/megolm/MXMegolmEncryption.kt | 3 +- .../crypto/algorithms/olm/MXOlmEncryption.kt | 13 ----- .../session/room/DefaultRoomService.kt | 23 +++++++- .../session/room/timeline/DefaultTimeline.kt | 23 +------- .../room/typing/DefaultTypingService.kt | 15 ++++- .../session/room/typing/SendTypingTask.kt | 16 +----- 10 files changed, 119 insertions(+), 96 deletions(-) create mode 100644 matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/crypto/algorithms/IMXGroupEncryption.kt diff --git a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/crypto/DefaultCryptoService.kt b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/crypto/DefaultCryptoService.kt index 821ac18e3a..72cf8b261d 100755 --- a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/crypto/DefaultCryptoService.kt +++ b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/crypto/DefaultCryptoService.kt @@ -53,6 +53,7 @@ import org.matrix.android.sdk.api.session.room.model.RoomMemberContent import org.matrix.android.sdk.internal.crypto.actions.MegolmSessionDataImporter import org.matrix.android.sdk.internal.crypto.actions.SetDeviceVerificationAction import org.matrix.android.sdk.internal.crypto.algorithms.IMXEncrypting +import org.matrix.android.sdk.internal.crypto.algorithms.IMXGroupEncryption import org.matrix.android.sdk.internal.crypto.algorithms.IMXWithHeldExtension import org.matrix.android.sdk.internal.crypto.algorithms.megolm.MXMegolmEncryptionFactory import org.matrix.android.sdk.internal.crypto.algorithms.olm.MXOlmEncryptionFactory @@ -667,7 +668,12 @@ internal class DefaultCryptoService @Inject constructor( override fun discardOutboundSession(roomId: String) { cryptoCoroutineScope.launch(coroutineDispatchers.crypto) { - roomEncryptorsStore.get(roomId)?.discardSessionKey() + val roomEncryptor = roomEncryptorsStore.get(roomId) + if (roomEncryptor is IMXGroupEncryption) { + roomEncryptor.discardSessionKey() + } else { + Timber.e("## CRYPTO | discardOutboundSession() for:$roomId: Unable to handle IMXGroupEncryption") + } } } @@ -1298,7 +1304,12 @@ internal class DefaultCryptoService @Inject constructor( getEncryptionAlgorithm(roomId)?.let { safeAlgorithm -> val userIds = getRoomUserIds(roomId) if (setEncryptionInRoom(roomId, safeAlgorithm, false, userIds)) { - roomEncryptorsStore.get(roomId)?.ensureOutboundSession(getRoomUserIds(roomId)) + val roomEncryptor = roomEncryptorsStore.get(roomId) + if (roomEncryptor is IMXGroupEncryption) { + roomEncryptor.ensureOutboundSession(getRoomUserIds(roomId)) + } else { + Timber.e("## CRYPTO | ensureOutboundSession() for:$roomId: Unable to handle IMXGroupEncryption for $safeAlgorithm") + } } } } diff --git a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/crypto/IncomingGossipingRequestManager.kt b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/crypto/IncomingGossipingRequestManager.kt index c075c90eb9..4a0a274f4f 100644 --- a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/crypto/IncomingGossipingRequestManager.kt +++ b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/crypto/IncomingGossipingRequestManager.kt @@ -28,6 +28,7 @@ import org.matrix.android.sdk.api.session.crypto.keyshare.GossipingRequestListen import org.matrix.android.sdk.api.session.events.model.Event import org.matrix.android.sdk.api.session.events.model.EventType import org.matrix.android.sdk.api.session.events.model.toModel +import org.matrix.android.sdk.internal.crypto.algorithms.IMXGroupEncryption import org.matrix.android.sdk.internal.crypto.crosssigning.toBase64NoPadding import org.matrix.android.sdk.internal.crypto.keysbackup.util.extractCurveKeyFromRecoveryKey import org.matrix.android.sdk.internal.crypto.model.rest.GossipingDefaultContent @@ -290,12 +291,16 @@ internal class IncomingGossipingRequestManager @Inject constructor( .also { cryptoStore.updateGossipingRequestState(request, GossipingRequestState.REJECTED) } cryptoCoroutineScope.launch(coroutineDispatchers.crypto) { - val isSuccess = roomEncryptor.reshareKey(sessionId, userId, deviceId, senderKey) + if (roomEncryptor is IMXGroupEncryption) { + val isSuccess = roomEncryptor.reshareKey(sessionId, userId, deviceId, senderKey) - if (isSuccess) { - cryptoStore.updateGossipingRequestState(request, GossipingRequestState.ACCEPTED) + if (isSuccess) { + cryptoStore.updateGossipingRequestState(request, GossipingRequestState.ACCEPTED) + } else { + cryptoStore.updateGossipingRequestState(request, GossipingRequestState.UNABLE_TO_PROCESS) + } } else { - cryptoStore.updateGossipingRequestState(request, GossipingRequestState.UNABLE_TO_PROCESS) + Timber.e("## CRYPTO | handleKeyRequestFromOtherUser() from:$userId: Unable to handle IMXGroupEncryption.reshareKey for $alg") } } cryptoStore.updateGossipingRequestState(request, GossipingRequestState.RE_REQUESTED) diff --git a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/crypto/algorithms/IMXEncrypting.kt b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/crypto/algorithms/IMXEncrypting.kt index 008ef1b084..5294429198 100644 --- a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/crypto/algorithms/IMXEncrypting.kt +++ b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/crypto/algorithms/IMXEncrypting.kt @@ -32,41 +32,4 @@ internal interface IMXEncrypting { * @return the encrypted content */ suspend fun encryptEventContent(eventContent: Content, eventType: String, userIds: List): Content - - /** - * In Megolm, each recipient maintains a record of the ratchet value which allows - * them to decrypt any messages sent in the session after the corresponding point - * in the conversation. If this value is compromised, an attacker can similarly - * decrypt past messages which were encrypted by a key derived from the - * compromised or subsequent ratchet values. This gives 'partial' forward - * secrecy. - * - * To mitigate this issue, the application should offer the user the option to - * discard historical conversations, by winding forward any stored ratchet values, - * or discarding sessions altogether. - */ - fun discardSessionKey() - - /** - * Re-shares a session key with devices if the key has already been - * sent to them. - * - * @param sessionId The id of the outbound session to share. - * @param userId The id of the user who owns the target device. - * @param deviceId The id of the target device. - * @param senderKey The key of the originating device for the session. - * - * @return true in case of success - */ - suspend fun reshareKey(sessionId: String, - userId: String, - deviceId: String, - senderKey: String): Boolean - - /** - * Ensure the outbound session - * - * @param usersInRoom the users in the room - */ - suspend fun ensureOutboundSession(usersInRoom: List) } diff --git a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/crypto/algorithms/IMXGroupEncryption.kt b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/crypto/algorithms/IMXGroupEncryption.kt new file mode 100644 index 0000000000..97b005659d --- /dev/null +++ b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/crypto/algorithms/IMXGroupEncryption.kt @@ -0,0 +1,57 @@ +/* + * Copyright 2020 The Matrix.org Foundation C.I.C. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.matrix.android.sdk.internal.crypto.algorithms + +internal interface IMXGroupEncryption { + + /** + * In Megolm, each recipient maintains a record of the ratchet value which allows + * them to decrypt any messages sent in the session after the corresponding point + * in the conversation. If this value is compromised, an attacker can similarly + * decrypt past messages which were encrypted by a key derived from the + * compromised or subsequent ratchet values. This gives 'partial' forward + * secrecy. + * + * To mitigate this issue, the application should offer the user the option to + * discard historical conversations, by winding forward any stored ratchet values, + * or discarding sessions altogether. + */ + fun discardSessionKey() + + /** + * Re-shares a session key with devices if the key has already been + * sent to them. + * + * @param sessionId The id of the outbound session to share. + * @param userId The id of the user who owns the target device. + * @param deviceId The id of the target device. + * @param senderKey The key of the originating device for the session. + * + * @return true in case of success + */ + suspend fun reshareKey(sessionId: String, + userId: String, + deviceId: String, + senderKey: String): Boolean + + /** + * Ensure the outbound session + * + * @param usersInRoom the users in the room + */ + suspend fun ensureOutboundSession(usersInRoom: List) +} diff --git a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/crypto/algorithms/megolm/MXMegolmEncryption.kt b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/crypto/algorithms/megolm/MXMegolmEncryption.kt index 42c19925ae..be85fcec8e 100644 --- a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/crypto/algorithms/megolm/MXMegolmEncryption.kt +++ b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/crypto/algorithms/megolm/MXMegolmEncryption.kt @@ -30,6 +30,7 @@ import org.matrix.android.sdk.internal.crypto.MXOlmDevice import org.matrix.android.sdk.internal.crypto.actions.EnsureOlmSessionsForDevicesAction import org.matrix.android.sdk.internal.crypto.actions.MessageEncrypter import org.matrix.android.sdk.internal.crypto.algorithms.IMXEncrypting +import org.matrix.android.sdk.internal.crypto.algorithms.IMXGroupEncryption import org.matrix.android.sdk.internal.crypto.keysbackup.DefaultKeysBackupService import org.matrix.android.sdk.internal.crypto.model.CryptoDeviceInfo import org.matrix.android.sdk.internal.crypto.model.MXUsersDevicesMap @@ -61,7 +62,7 @@ internal class MXMegolmEncryption( private val taskExecutor: TaskExecutor, private val coroutineDispatchers: MatrixCoroutineDispatchers, private val cryptoCoroutineScope: CoroutineScope -) : IMXEncrypting { +) : IMXEncrypting, IMXGroupEncryption { // OutboundSessionInfo. Null if we haven't yet started setting one up. Note // that even if this is non-null, it may not be ready for use (in which diff --git a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/crypto/algorithms/olm/MXOlmEncryption.kt b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/crypto/algorithms/olm/MXOlmEncryption.kt index acea6ff3fe..65f78e11f0 100644 --- a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/crypto/algorithms/olm/MXOlmEncryption.kt +++ b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/crypto/algorithms/olm/MXOlmEncryption.kt @@ -76,17 +76,4 @@ internal class MXOlmEncryption( deviceListManager.downloadKeys(users, false) ensureOlmSessionsForUsersAction.handle(users) } - - override fun discardSessionKey() { - // No need for olm - } - - override suspend fun reshareKey(sessionId: String, userId: String, deviceId: String, senderKey: String): Boolean { - // No need for olm - return false - } - - override suspend fun ensureOutboundSession(usersInRoom: List) { - // NOOP - } } diff --git a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/room/DefaultRoomService.kt b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/room/DefaultRoomService.kt index 383dd876d3..c60150004f 100644 --- a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/room/DefaultRoomService.kt +++ b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/room/DefaultRoomService.kt @@ -20,6 +20,11 @@ import androidx.lifecycle.LiveData import androidx.lifecycle.Transformations import com.zhuinden.monarchy.Monarchy import org.matrix.android.sdk.api.MatrixCallback +import org.matrix.android.sdk.api.MatrixConfiguration +import org.matrix.android.sdk.api.NoOpMatrixCallback +import org.matrix.android.sdk.api.crypto.OutboundSessionKeySharingStrategy +import org.matrix.android.sdk.api.extensions.orFalse +import org.matrix.android.sdk.api.session.crypto.CryptoService import org.matrix.android.sdk.api.session.events.model.Event import org.matrix.android.sdk.api.session.room.Room import org.matrix.android.sdk.api.session.room.RoomService @@ -39,6 +44,7 @@ import org.matrix.android.sdk.internal.session.room.alias.DeleteRoomAliasTask import org.matrix.android.sdk.internal.session.room.alias.GetRoomIdByAliasTask import org.matrix.android.sdk.internal.session.room.alias.RoomAliasDescription import org.matrix.android.sdk.internal.session.room.create.CreateRoomTask +import org.matrix.android.sdk.internal.session.room.membership.LoadRoomMembersTask import org.matrix.android.sdk.internal.session.room.membership.RoomChangeMembershipStateDataSource import org.matrix.android.sdk.internal.session.room.membership.RoomMemberHelper import org.matrix.android.sdk.internal.session.room.membership.joining.JoinRoomTask @@ -65,7 +71,10 @@ internal class DefaultRoomService @Inject constructor( private val roomGetter: RoomGetter, private val roomSummaryDataSource: RoomSummaryDataSource, private val roomChangeMembershipStateDataSource: RoomChangeMembershipStateDataSource, - private val taskExecutor: TaskExecutor + private val taskExecutor: TaskExecutor, + private val loadRoomMembersTask: LoadRoomMembersTask, + private val matrixConfiguration: MatrixConfiguration, + private val cryptoService: CryptoService ) : RoomService { override fun createRoom(createRoomParams: CreateRoomParams, callback: MatrixCallback): Cancelable { @@ -105,6 +114,18 @@ internal class DefaultRoomService @Inject constructor( } override fun onRoomDisplayed(roomId: String): Cancelable { + // Ensure to load all room members + loadRoomMembersTask + .configureWith(LoadRoomMembersTask.Params(roomId)) { + this.callback = NoOpMatrixCallback() + } + .executeBy(taskExecutor) + + // Ensure to share the outbound session keys with all members + if (roomGetter.getRoom(roomId)?.isEncrypted().orFalse() + && matrixConfiguration.outboundSessionKeySharingStrategy == OutboundSessionKeySharingStrategy.WhenEnteringRoom) { + cryptoService.ensureOutboundSession(roomId) + } return updateBreadcrumbsTask .configureWith(UpdateBreadcrumbsTask.Params(roomId)) .executeBy(taskExecutor) diff --git a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/room/timeline/DefaultTimeline.kt b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/room/timeline/DefaultTimeline.kt index e75beb4b21..1be5d55cad 100644 --- a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/room/timeline/DefaultTimeline.kt +++ b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/room/timeline/DefaultTimeline.kt @@ -24,13 +24,8 @@ import io.realm.RealmQuery import io.realm.RealmResults import io.realm.Sort import org.matrix.android.sdk.api.MatrixCallback -import org.matrix.android.sdk.api.MatrixConfiguration -import org.matrix.android.sdk.api.NoOpMatrixCallback -import org.matrix.android.sdk.api.crypto.OutboundSessionKeySharingStrategy import org.matrix.android.sdk.api.extensions.orFalse import org.matrix.android.sdk.api.extensions.tryOrNull -import org.matrix.android.sdk.api.session.Session -import org.matrix.android.sdk.api.session.crypto.CryptoService import org.matrix.android.sdk.api.session.events.model.EventType import org.matrix.android.sdk.api.session.events.model.RelationType import org.matrix.android.sdk.api.session.events.model.toModel @@ -54,7 +49,6 @@ import org.matrix.android.sdk.internal.database.query.filterEvents import org.matrix.android.sdk.internal.database.query.findAllInRoomWithSendStates import org.matrix.android.sdk.internal.database.query.where import org.matrix.android.sdk.internal.database.query.whereRoomId -import org.matrix.android.sdk.internal.session.room.membership.LoadRoomMembersTask import org.matrix.android.sdk.internal.task.TaskExecutor import org.matrix.android.sdk.internal.task.configureWith import org.matrix.android.sdk.internal.util.Debouncer @@ -83,11 +77,7 @@ internal class DefaultTimeline( private val hiddenReadReceipts: TimelineHiddenReadReceipts, private val timelineInput: TimelineInput, private val eventDecryptor: TimelineEventDecryptor, - private val realmSessionProvider: RealmSessionProvider, - private val loadRoomMembersTask: LoadRoomMembersTask, - private val session: Session, - private val matrixConfiguration: MatrixConfiguration, - private val cryptoService: CryptoService + private val realmSessionProvider: RealmSessionProvider ) : Timeline, TimelineHiddenReadReceipts.Delegate, TimelineInput.Listener { @@ -189,17 +179,6 @@ internal class DefaultTimeline( hiddenReadReceipts.start(realm, filteredEvents, nonFilteredEvents, this) } - loadRoomMembersTask - .configureWith(LoadRoomMembersTask.Params(roomId)) { - this.callback = NoOpMatrixCallback() - } - .executeBy(taskExecutor) - - if (session.getRoom(roomId)?.isEncrypted().orFalse() - && matrixConfiguration.outboundSessionKeySharingStrategy == OutboundSessionKeySharingStrategy.WhenEnteringRoom) { - cryptoService.ensureOutboundSession(roomId) - } - isReady.set(true) } } diff --git a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/room/typing/DefaultTypingService.kt b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/room/typing/DefaultTypingService.kt index 39b7967bc1..279ab3f334 100644 --- a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/room/typing/DefaultTypingService.kt +++ b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/room/typing/DefaultTypingService.kt @@ -21,8 +21,13 @@ import dagger.assisted.Assisted import dagger.assisted.AssistedInject import dagger.assisted.AssistedFactory import org.matrix.android.sdk.api.MatrixCallback +import org.matrix.android.sdk.api.MatrixConfiguration +import org.matrix.android.sdk.api.crypto.OutboundSessionKeySharingStrategy +import org.matrix.android.sdk.api.extensions.orFalse +import org.matrix.android.sdk.api.session.crypto.CryptoService import org.matrix.android.sdk.api.session.room.typing.TypingService import org.matrix.android.sdk.api.util.Cancelable +import org.matrix.android.sdk.internal.session.room.RoomGetter import org.matrix.android.sdk.internal.task.TaskExecutor import org.matrix.android.sdk.internal.task.configureWith import timber.log.Timber @@ -36,7 +41,10 @@ import timber.log.Timber internal class DefaultTypingService @AssistedInject constructor( @Assisted private val roomId: String, private val taskExecutor: TaskExecutor, - private val sendTypingTask: SendTypingTask + private val sendTypingTask: SendTypingTask, + private val matrixConfiguration: MatrixConfiguration, + private val cryptoService: CryptoService, + private val roomGetter: RoomGetter ) : TypingService { @AssistedFactory @@ -69,6 +77,11 @@ internal class DefaultTypingService @AssistedInject constructor( currentTask?.cancel() + if (roomGetter.getRoom(roomId)?.isEncrypted().orFalse() + && matrixConfiguration.outboundSessionKeySharingStrategy == OutboundSessionKeySharingStrategy.WhenTyping) { + cryptoService.ensureOutboundSession(roomId) + } + val params = SendTypingTask.Params(roomId, true) currentTask = sendTypingTask .configureWith(params) diff --git a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/room/typing/SendTypingTask.kt b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/room/typing/SendTypingTask.kt index aa8dcf6013..3b56d04872 100644 --- a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/room/typing/SendTypingTask.kt +++ b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/room/typing/SendTypingTask.kt @@ -21,11 +21,6 @@ import org.matrix.android.sdk.internal.network.executeRequest import org.matrix.android.sdk.internal.session.room.RoomAPI import org.matrix.android.sdk.internal.task.Task import kotlinx.coroutines.delay -import org.matrix.android.sdk.api.MatrixConfiguration -import org.matrix.android.sdk.api.crypto.OutboundSessionKeySharingStrategy -import org.matrix.android.sdk.api.extensions.orFalse -import org.matrix.android.sdk.api.session.Session -import org.matrix.android.sdk.api.session.crypto.CryptoService import org.matrix.android.sdk.internal.network.GlobalErrorReceiver import javax.inject.Inject @@ -43,21 +38,12 @@ internal interface SendTypingTask : Task { internal class DefaultSendTypingTask @Inject constructor( private val roomAPI: RoomAPI, @UserId private val userId: String, - private val globalErrorReceiver: GlobalErrorReceiver, - private val matrixConfiguration: MatrixConfiguration, - private val session: Session, - private val cryptoService: CryptoService + private val globalErrorReceiver: GlobalErrorReceiver ) : SendTypingTask { override suspend fun execute(params: SendTypingTask.Params) { delay(params.delay ?: -1) - if (params.isTyping - && session.getRoom(params.roomId)?.isEncrypted().orFalse() - && matrixConfiguration.outboundSessionKeySharingStrategy == OutboundSessionKeySharingStrategy.WhenTyping) { - cryptoService.ensureOutboundSession(params.roomId) - } - executeRequest(globalErrorReceiver) { apiCall = roomAPI.sendTypingState( params.roomId,