Feature/aris/crypto replay attack #6077
Conversation
…g the same eventId Improve code format
…ting the same event
| // Lets decrypt the original event | ||
| aliceSession.cryptoService().decryptEvent(sentEvents[0].root, timelineId) | ||
| // Lets try to decrypt the same event | ||
| aliceSession.cryptoService().decryptEvent(sentEvents[0].root, timelineId) |
There was a problem hiding this comment.
I'd do a try {.. decrypt } catch (error) { fail("Shouldn't throw a decryption error for same event") }
| Timber.tag(loggerTag.value).d("## decryptGroupMessage() eventId: $eventId") | ||
| Timber.tag(loggerTag.value).d("## decryptGroupMessage() mIndex: ${decryptResult.mIndex}") | ||
|
|
||
| if (timeline?.isNotBlank() == true) { |
There was a problem hiding this comment.
i think we might need some concurrency issues when accessing the timelineset. Maybe use a mutex for that?
There was a problem hiding this comment.
Its the way the current code works, I will have a look
| } | ||
| timelineSet.add(messageIndexKey) | ||
| } | ||
| replayAttackMap[messageIndexKey] = eventId |
There was a problem hiding this comment.
It feels strange to have the replayAttackMap global and shared by everyone.
I would have made inboundGroupSessionMessageIndexes a Map<timelineId, replayAttackMap>
There was a problem hiding this comment.
Yeap, its a good point will refactor it a bit
| @@ -537,6 +539,11 @@ internal class RoomSyncHandler @Inject constructor( | |||
| } | |||
| } | |||
|
|
|||
| private fun generateTimelineId(roomId: String, event: Event): String { | |||
| val threadIndicator = if (event.isThread()) "_thread_" else "_" | |||
There was a problem hiding this comment.
why do we make a different timeline id between thread and normal?
There was a problem hiding this comment.
For the fact that it's technically a different timeline, that can contain the same events with the main timeline in parallel. But maybe re-using the main timelineId is enough, will update it
| } | ||
|
|
||
| val messageIndexKey = senderKey + "|" + sessionId + "|" + roomId + "|" + decryptResult.mIndex | ||
| Timber.tag(loggerTag.value).d("##########################################################") |
There was a problem hiding this comment.
Reduce log visibility to verbose!
# Conflicts: # matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/crypto/MXOlmDevice.kt
|
|
||
| // Alice will send a message | ||
| val sentEvents = testHelper.sendTextMessage(aliceRoomPOV, "Hello I will be decrypted twice", 1) | ||
| Assert.assertTrue("Message should be sent", sentEvents.size == 1) |
There was a problem hiding this comment.
Can be better to use assertEqual for better test failure report (for instance "expected 1, actual 2", rather than "expected true, actual false" when using assertTrue
There was a problem hiding this comment.
Updated
| @@ -537,6 +538,10 @@ internal class RoomSyncHandler @Inject constructor( | |||
| } | |||
| } | |||
|
|
|||
| private fun generateTimelineId(roomId: String): String { | |||
| return "${RoomSyncHandler::class.java.simpleName}$roomId" | |||
There was a problem hiding this comment.
Strange to use RoomSyncHandler::class.java.simpleName here, can't it be a hard-coded prefix?
There was a problem hiding this comment.
Yes, it doesn't really matters. I hardcoded the prefix
Matrix SDKIntegration Tests Results:
|
This PR aims to reduce the UISI
DUPLICATED_MESSAGE_INDEX.timelineIdwhen decryption is called fromRoomSyncHandler