Skip to content

Commit

Permalink
Crypto fixes
Browse files Browse the repository at this point in the history
  • Loading branch information
Anderas committed Nov 7, 2022
1 parent da1cf57 commit 4661d13
Show file tree
Hide file tree
Showing 14 changed files with 142 additions and 68 deletions.
39 changes: 23 additions & 16 deletions MatrixSDK/Crypto/Algorithms/RoomEvent/MXRoomEventDecryption.swift
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,10 @@ protocol MXRoomEventDecrypting: Actor {
/// Note: room key could be contained in `m.room_key` or `m.forwarded_room_key`
func handlePossibleRoomKeyEvent(_ event: MXEvent)

/// Retry decrypting all previously undecrypted events
/// Retry decrypting events with specific session ids
///
/// Note: this may be useful if we have just imported keys from backup / file
func retryAllUndecryptedEvents()
func retryUndecryptedEvents(sessionIds: [String])

/// Reset the store of undecrypted events
func resetUndecryptedEvents()
Expand All @@ -46,7 +46,7 @@ actor MXRoomEventDecryption: MXRoomEventDecrypting {

private let handler: MXCryptoRoomEventDecrypting
private var undecryptedEvents: [SessionId: [EventId: MXEvent]]
private let log = MXNamedLog(name: "MXCryptoRoomEventDecryptor")
private let log = MXNamedLog(name: "MXRoomEventDecryption")

init(handler: MXCryptoRoomEventDecrypting) {
self.handler = handler
Expand Down Expand Up @@ -82,14 +82,14 @@ actor MXRoomEventDecryption: MXRoomEventDecrypting {
retryDecryption(events: events)
}

func retryAllUndecryptedEvents() {
let allEvents = undecryptedEvents
func retryUndecryptedEvents(sessionIds: [String]) {
let events = sessionIds
.flatMap {
$0.value.map {
undecryptedEvents[$0]?.map {
$0.value
}
} ?? []
}
retryDecryption(events: allEvents)
retryDecryption(events: events)
}

func resetUndecryptedEvents() {
Expand All @@ -100,11 +100,15 @@ actor MXRoomEventDecryption: MXRoomEventDecrypting {

private func decrypt(event: MXEvent) -> MXEventDecryptionResult {
guard
let sessionId = sessionId(for: event),
event.content?["algorithm"] as? String == kMXCryptoMegolmAlgorithm
event.isEncrypted && event.clear == nil,
event.content?["algorithm"] as? String == kMXCryptoMegolmAlgorithm,
let sessionId = sessionId(for: event)
else {
log.debug("Ignoring unencrypted or non-room event")
return MXEventDecryptionResult()

let result = MXEventDecryptionResult()
result.clearEvent = event.clear?.jsonDictionary()
return result
}

do {
Expand All @@ -117,8 +121,9 @@ actor MXRoomEventDecryption: MXRoomEventDecrypting {
// hardcoded non-localized error message. Will be changed in future PR
} catch DecryptionError.Megolm(message: "decryption failed because the room key is missing") {
if undecryptedEvents[sessionId] == nil {
log.error("Failed to decrypt event due to missing room keys (further errors for the same key will be supressed)", context: [
"session_id": sessionId
log.error("Failed to decrypt one or more events due to missing room keys", context: [
"session_id": sessionId,
"details": "further errors for the same key will be supressed"
])
}

Expand Down Expand Up @@ -178,7 +183,8 @@ actor MXRoomEventDecryption: MXRoomEventDecrypting {
guard result.clearEvent != nil else {
log.error("Event still not decryptable", context: [
"event_id": event.eventId ?? "unknown",
"session_id": sessionId(for: event)
"session_id": sessionId(for: event),
"error": result.error.localizedDescription
])
continue
}
Expand Down Expand Up @@ -207,8 +213,9 @@ actor MXRoomEventDecryption: MXRoomEventDecrypting {
}

private func sessionId(for event: MXEvent) -> String? {
guard event.isEncrypted, let sessionId = event.content["session_id"] as? String else {
log.error("Event is not encrypted or is missing session id")
let sessionId = event.content["session_id"] ?? event.wireContent["session_id"]
guard let sessionId = sessionId as? String else {
log.failure("Event is missing session id")
return nil
}
return sessionId
Expand Down
4 changes: 2 additions & 2 deletions MatrixSDK/Crypto/CrossSigning/MXCrossSigningV2.swift
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ class MXCrossSigningV2: NSObject, MXCrossSigning {

Task {
do {
try await crossSigning.manuallyVerifyDevice(userId: crossSigning.userId, deviceId: deviceId)
try await crossSigning.verifyDevice(userId: crossSigning.userId, deviceId: deviceId)

log.debug("Successfully cross-signed a device")
await MainActor.run {
Expand All @@ -170,7 +170,7 @@ class MXCrossSigningV2: NSObject, MXCrossSigning {

Task {
do {
try await crossSigning.manuallyVerifyUser(userId: userId)
try await crossSigning.verifyUser(userId: userId)
log.debug("Successfully cross-signed a user")

await MainActor.run {
Expand Down
30 changes: 23 additions & 7 deletions MatrixSDK/Crypto/CryptoMachine/MXCryptoMachine.swift
Original file line number Diff line number Diff line change
Expand Up @@ -108,11 +108,18 @@ class MXCryptoMachine {
log.debug("Keys successfully uploaded")
}

private static func storeURL(for userId: String) throws -> URL {
func deleteAllData() throws {
let url = try Self.storeURL(for: machine.userId())
try FileManager.default.removeItem(at: url)
}
}

extension MXCryptoMachine {
static func storeURL(for userId: String) throws -> URL {
let container: URL
if let sharedContainerURL = FileManager.default.applicationGroupContainerURL() {
container = sharedContainerURL
} else if let url = FileManager.default.urls(for: .documentDirectory, in: .userDomainMask).first {
} else if let url = platformDirectoryURL() {
container = url
} else {
throw Error.invalidStorage
Expand All @@ -123,9 +130,18 @@ class MXCryptoMachine {
.appendingPathComponent(userId)
}

func deleteAllData() throws {
let url = try Self.storeURL(for: machine.userId())
try FileManager.default.removeItem(at: url)
private static func platformDirectoryURL() -> URL? {
#if os(OSX)
guard
let applicationSupport = FileManager.default.urls(for: .applicationSupportDirectory, in: .userDomainMask).first,
let identifier = Bundle.main.bundleIdentifier
else {
return nil
}
return applicationSupport.appendingPathComponent(identifier)
#else
return FileManager.default.urls(for: .documentDirectory, in: .userDomainMask).first
#endif
}
}

Expand Down Expand Up @@ -334,12 +350,12 @@ extension MXCryptoMachine: MXCryptoUserIdentitySource {
)
}

func manuallyVerifyUser(userId: String) async throws {
func verifyUser(userId: String) async throws {
let request = try machine.verifyIdentity(userId: userId)
try await requests.uploadSignatures(request: request)
}

func manuallyVerifyDevice(userId: String, deviceId: String) async throws {
func verifyDevice(userId: String, deviceId: String) async throws {
let request = try machine.verifyDevice(userId: userId, deviceId: deviceId)
try await requests.uploadSignatures(request: request)
}
Expand Down
4 changes: 2 additions & 2 deletions MatrixSDK/Crypto/CryptoMachine/MXCryptoProtocols.swift
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,8 @@ protocol MXCryptoUserIdentitySource: MXCryptoIdentity {
func isUserVerified(userId: String) -> Bool
func isUserTracked(userId: String) -> Bool
func downloadKeys(users: [String]) async throws
func manuallyVerifyUser(userId: String) async throws
func manuallyVerifyDevice(userId: String, deviceId: String) async throws
func verifyUser(userId: String) async throws
func verifyDevice(userId: String, deviceId: String) async throws
func setLocalTrust(userId: String, deviceId: String, trust: LocalTrust) throws
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,11 +37,7 @@ extension MXEventDecryptionResult {
senderCurve25519Key = event.senderCurve25519Key
claimedEd25519Key = event.claimedEd25519Key
forwardingCurve25519KeyChain = event.forwardingCurve25519Chain

// Untrusted state not yet fully implemented, will equal to:
// `isUntrusted = event.verificationState == VerificationState.untrusted`
MXLog.warning("[MXEventDecryptionResult] trust not yet implemeneted")
isUntrusted = false
isUntrusted = event.verificationState == VerificationState.untrusted
}
}

Expand Down
14 changes: 13 additions & 1 deletion MatrixSDK/Crypto/Data/MXMegolmSessionData.m
Original file line number Diff line number Diff line change
Expand Up @@ -52,9 +52,21 @@ + (id)modelFromJSON:(NSDictionary *)JSONDictionary

- (NSDictionary *)JSONDictionary
{
if (!_senderKey || !_roomId || !_sessionId || !_sessionKey || !_algorithm)
{
NSDictionary *details = @{
@"sender_key": _senderKey ?: @"unknown",
@"room_id": _roomId ?: @"unknown",
@"session_id": _sessionId ?: @"unknown",
@"algorithm": _algorithm ?: @"unknown",
};
MXLogErrorDetails(@"[MXMegolmSessionData] JSONDictionary: some properties are missing", details);
return nil;
}

return @{
@"sender_key": _senderKey,
@"sender_claimed_keys": _senderClaimedKeys,
@"sender_claimed_keys": _senderClaimedKeys ?: @[],
@"room_id": _roomId,
@"session_id": _sessionId,
@"session_key":_sessionKey,
Expand Down
11 changes: 9 additions & 2 deletions MatrixSDK/Crypto/KeyBackup/Engine/MXCryptoKeyBackupEngine.swift
Original file line number Diff line number Diff line change
Expand Up @@ -297,7 +297,7 @@ class MXCryptoKeyBackupEngine: NSObject, MXKeyBackupEngine {

do {
let result = try backup.importDecryptedKeys(roomKeys: sessions, progressListener: self)
await roomEventDecryptor.retryAllUndecryptedEvents()
await roomEventDecryptor.retryUndecryptedEvents(sessionIds: sessions.map(\.sessionId))

let duration2 = Date().timeIntervalSince(date2) * 1000
log.debug("Successfully imported \(result.imported) out of \(result.total) sessions in \(duration2) ms")
Expand Down Expand Up @@ -364,7 +364,14 @@ class MXCryptoKeyBackupEngine: NSObject, MXKeyBackupEngine {

func importRoomKeys(_ data: Data, passphrase: String) async throws -> KeysImportResult {
let result = try backup.importRoomKeys(data, passphrase: passphrase, progressListener: self)
await roomEventDecryptor.retryAllUndecryptedEvents()
let sessionIds = result.keys
.flatMap { (roomId, senders) in
senders.flatMap { (sender, sessionIds) in
sessionIds
}
}

await roomEventDecryptor.retryUndecryptedEvents(sessionIds: sessionIds)
return result
}

Expand Down
33 changes: 14 additions & 19 deletions MatrixSDK/Crypto/MXCryptoV2.swift
Original file line number Diff line number Diff line change
Expand Up @@ -386,9 +386,11 @@ private class MXCryptoV2: NSObject, MXCrypto {

func handle(_ syncResponse: MXSyncResponse, onComplete: @escaping () -> Void) {
let toDeviceCount = syncResponse.toDevice?.events.count ?? 0
let devicesChanged = syncResponse.deviceLists?.changed?.count ?? 0
let devicesLeft = syncResponse.deviceLists?.left?.count ?? 0

MXLog.debug("[MXCryptoV2] --------------------------------")
log.debug("Handling new sync response with \(toDeviceCount) to-device event(s)")
log.debug("Handling new sync response with \(toDeviceCount) to-device event(s), \(devicesChanged) device(s) changed, \(devicesLeft) device(s) left")

Task(priority: .medium) {
do {
Expand Down Expand Up @@ -447,7 +449,7 @@ private class MXCryptoV2: NSObject, MXCrypto {

Task {
do {
try await machine.manuallyVerifyDevice(userId: userId, deviceId: deviceId)
try await machine.verifyDevice(userId: userId, deviceId: deviceId)
log.debug("Successfully marked device as verified")
await MainActor.run {
success?()
Expand Down Expand Up @@ -486,22 +488,16 @@ private class MXCryptoV2: NSObject, MXCrypto {
return
}

log.debug("Setting user verification status manually")

Task {
do {
try await machine.manuallyVerifyUser(userId: userId)
log.debug("Successfully marked user as verified")
await MainActor.run {
success?()
}
} catch {
log.error("Failed marking user as verified", context: error)
await MainActor.run {
failure?(error)
}
log.debug("Signing user")
crossSigning.signUser(
withUserId: userId,
success: {
success?()
},
failure: {
failure?($0)
}
}
)
}

public func trustLevel(forUser userId: String) -> MXUserTrustLevel {
Expand Down Expand Up @@ -726,8 +722,7 @@ private class MXCryptoV2: NSObject, MXCrypto {

private func getRoomUserIds(for room: MXRoom) async throws -> [String] {
return try await room.members()?.members
.compactMap(\.userId)
.filter { $0 != machine.userId } ?? []
.compactMap(\.userId) ?? []
}

private func crossSigningInfo(userIds: [String]) -> [String: MXCrossSigningInfo] {
Expand Down
8 changes: 7 additions & 1 deletion MatrixSDK/MXSession.m
Original file line number Diff line number Diff line change
Expand Up @@ -2082,7 +2082,7 @@ - (void)handleBackgroundSyncCacheIfRequiredWithCompletion:(void (^)(void))comple
{
BOOL isInValidState = _state == MXSessionStateStoreDataReady || _state == MXSessionStatePaused;
if (!isInValidState) {
NSString *message = [NSString stringWithFormat:@"[MXSession] state %@ is not valid to handle background sync cache, investigate why the method was called", [MXTools readableSessionState:_state]];
NSString *message = [NSString stringWithFormat:@"[MXSession] handleBackgroundSyncCacheIfRequired: state %@ is not valid to handle background sync cache, investigate why the method was called", [MXTools readableSessionState:_state]];
MXLogFailure(message);
if (completion)
{
Expand Down Expand Up @@ -4928,6 +4928,12 @@ - (void)onDidDecryptEvent:(NSNotification *)notification
MXRoomSummary *summary = [self roomSummaryWithRoomId:event.roomId];
if (summary)
{
if (!summary.lastMessage)
{
[summary resetLastMessage:nil failure:nil commit:YES];
return;
}

[self eventWithEventId:summary.lastMessage.eventId
inRoom:summary.roomId
success:^(MXEvent *lastEvent) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -137,10 +137,10 @@ class MXRoomEventDecryptionUnitTests: XCTestCase {

// MARK: - Retry all

func test_retryAllUndecryptedEvents() async {
func test_retryUndecryptedEvents() async {
let events = await prepareEventsForRedecryption()

await roomDecryptor.retryAllUndecryptedEvents()
await roomDecryptor.retryUndecryptedEvents(sessionIds: ["123", "456"])
await waitForDecryption()

XCTAssertNotNil(events[0].clear)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,20 +23,33 @@ import MatrixSDKCrypto

class MXCryptoMachineUnitTests: XCTestCase {

var userId = "@alice:matrix.org"
var restClient: MXRestClient!
var machine: MXCryptoMachine!

override func setUp() {
restClient = MXRestClientStub()
machine = try! MXCryptoMachine(
userId: "@alice:matrix.org",
userId: userId,
deviceId: "ABCD",
restClient: restClient,
getRoomAction: {
MXRoom(roomId: $0, andMatrixSession: nil)
})
}

override func tearDown() {
do {
let url = try MXCryptoMachine.storeURL(for: userId)
guard FileManager.default.fileExists(atPath: url.path) else {
return
}
try FileManager.default.removeItem(at: url)
} catch {
XCTFail("Cannot tear down test - \(error)")
}
}

func test_handleSyncResponse_canProcessEmptyResponse() throws {
let result = try machine.handleSyncResponse(
toDevice: nil,
Expand Down
Loading

0 comments on commit 4661d13

Please sign in to comment.