From dd14120c1942ec9d6868d215825c5e0475979260 Mon Sep 17 00:00:00 2001 From: Thilo Molitor Date: Tue, 20 Aug 2024 23:59:15 +0200 Subject: [PATCH 01/10] Improve PLAIN only warning message when using advanced account menu --- Monal/Classes/xmpp.m | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Monal/Classes/xmpp.m b/Monal/Classes/xmpp.m index 97e1e8341..684b53cf4 100644 --- a/Monal/Classes/xmpp.m +++ b/Monal/Classes/xmpp.m @@ -2955,7 +2955,7 @@ -(void) handleFeaturesBeforeAuth:(MLXMLNode*) parsedStanza withForceSasl2:(BOOL) //leave that in for translators, we might use it at a later time while(!NSLocalizedString(@"This server isn't additionally hardened against man-in-the-middle attacks on the TLS encryption layer by using authentication methods that are secure against such attacks! This indicates an ongoing attack if the server is supposed to support SASL2 and SCRAM and is harmless otherwise. Use the advanced account creation menu and turn on the PLAIN switch there if you still want to log in to this server.", @"")); - clearPipelineCacheOrReportSevereError(NSLocalizedString(@"This server lacks support for SASL2 and SCRAM, additionally hardening authentication against man-in-the-middle attacks on the TLS encryption layer. Since this server is listed as supporting both at https://github.com/monal-im/SCRAM_PreloadList, an ongoing MITM attack is highly likely! You should try again once you are in a clean networking environment.", @"")); + clearPipelineCacheOrReportSevereError(NSLocalizedString(@"This server lacks support for SASL2 and SCRAM, additionally hardening authentication against man-in-the-middle attacks on the TLS encryption layer. Since this server is listed as supporting both at https://github.com/monal-im/SCRAM_PreloadList (or you intentionally left the PLAIN switch off when using the advanced account creation menu), an ongoing MITM attack is very likely! Try again once you are in a clean network environment.", @"")); return; } } From d2fa56fe6fbfbe9e98f23d54c2496ada2a4d593b Mon Sep 17 00:00:00 2001 From: Thilo Molitor Date: Sun, 25 Aug 2024 15:50:40 +0200 Subject: [PATCH 02/10] Don't crash in oghtmlparser on big sites Some sites ignore the byterange, make sure to not crash for those sites if they are big. Also limit the size to 64kb instead of 512kb. --- Monal/Classes/chatViewController.m | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/Monal/Classes/chatViewController.m b/Monal/Classes/chatViewController.m index d5fc278cd..ace3440d5 100644 --- a/Monal/Classes/chatViewController.m +++ b/Monal/Classes/chatViewController.m @@ -3010,7 +3010,7 @@ -(void) loadPreviewWithUrlForRow:(NSIndexPath *) indexPath withResultHandler:(mo return; } //limit to 512KB of html - if(contentLength.intValue > 524288) + if(contentLength.intValue > 65536) { DDLogWarn(@"Now loading preview HTML for %@ with byte range 0-512k...", row.url); [self downloadPreviewWithRow:indexPath usingByterange:YES andResultHandler:resultHandler]; @@ -3051,7 +3051,7 @@ -(void) downloadPreviewWithRow:(NSIndexPath*) indexPath usingByterange:(BOOL) us request.requiresDNSSECValidation = YES; [request setValue:@"facebookexternalhit/1.1" forHTTPHeaderField:@"User-Agent"]; //required on some sites for og tags e.g. youtube if(useByterange) - [request setValue:@"bytes=0-524288" forHTTPHeaderField:@"Range"]; + [request setValue:@"bytes=0-65536" forHTTPHeaderField:@"Range"]; request.timeoutInterval = 10; NSURLSession* session = [HelperTools createEphemeralURLSession]; [[session dataTaskWithRequest:request completionHandler:^(NSData* _Nullable data, NSURLResponse* _Nullable response, NSError* _Nullable error) { @@ -3060,10 +3060,14 @@ -(void) downloadPreviewWithRow:(NSIndexPath*) indexPath usingByterange:(BOOL) us else { NSString* body = [[NSString alloc] initWithData:data encoding:NSUTF8StringEncoding]; - NSURL* baseURL = [NSURL URLWithString:[NSString stringWithFormat:@"%@://%@%@", row.url.scheme, row.url.host, row.url.path]]; - MLOgHtmlParser* ogParser = [[MLOgHtmlParser alloc] initWithHtml:body andBaseUrl:baseURL]; + MLOgHtmlParser* ogParser = nil; NSString* text = nil; NSURL* image = nil; + if([body length] <= 65536) + { + NSURL* baseURL = [NSURL URLWithString:[NSString stringWithFormat:@"%@://%@%@", row.url.scheme, row.url.host, row.url.path]]; + ogParser = [[MLOgHtmlParser alloc] initWithHtml:body andBaseUrl:baseURL]; + } if(ogParser != nil) { text = [ogParser getOgTitle]; From 4ed5ad82f7b2c6d460e5262e022ad8e4f61bdbbc Mon Sep 17 00:00:00 2001 From: lissine Date: Fri, 23 Aug 2024 10:42:40 +0100 Subject: [PATCH 03/10] Improve the discovery of room names Make the discovery of a room's name more reliable by getting it from the identity element, which MUST be returned by the MUC service, instead of relying on muc#roomconfig_roomname, which MAY be returned. see https://xmpp.org/extensions/xep-0045.html#disco-roominfo --- Monal/Classes/MLMucProcessor.m | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Monal/Classes/MLMucProcessor.m b/Monal/Classes/MLMucProcessor.m index 0235708d8..54faa8388 100644 --- a/Monal/Classes/MLMucProcessor.m +++ b/Monal/Classes/MLMucProcessor.m @@ -1417,7 +1417,7 @@ -(void) publishAvatar:(UIImage* _Nullable) image forMuc:(NSString*) room } //extract further muc infos - NSString* mucName = [iqNode findFirst:@"{http://jabber.org/protocol/disco#info}query/\\{http://jabber.org/protocol/muc#roominfo}result@muc#roomconfig_roomname\\"]; + NSString* mucName = [iqNode findFirst:@"{http://jabber.org/protocol/disco#info}query/identity@name"]; NSString* mucType = @"channel"; //both are needed for omemo, see discussion with holger 2021-01-02/03 -- Thilo Molitor //see also: https://docs.modernxmpp.org/client/groupchat/ From 63ed1e6c1e262574d2fda3ee84d5d0ac29cf9ea9 Mon Sep 17 00:00:00 2001 From: Thilo Molitor Date: Wed, 28 Aug 2024 06:00:06 +0200 Subject: [PATCH 04/10] Fix crash in app delegate --- Monal/Classes/MonalAppDelegate.m | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/Monal/Classes/MonalAppDelegate.m b/Monal/Classes/MonalAppDelegate.m index 278f740e1..a32ddbb05 100644 --- a/Monal/Classes/MonalAppDelegate.m +++ b/Monal/Classes/MonalAppDelegate.m @@ -1269,6 +1269,10 @@ -(void) showConnectionStatus:(NSNotification*) notification { dispatch_async(dispatch_get_main_queue(), ^{ xmpp* xmppAccount = notification.object; + //ignore errors with unknown accounts + //(possibly meaning an account we currently try to create --> the creating ui will take care of this already) + if(xmppAccount == nil) + return; if(![notification.userInfo[@"isSevere"] boolValue]) DDLogError(@"Minor XMPP Error(%@): %@", xmppAccount.connectionProperties.identity.jid, notification.userInfo[@"message"]); NotificationBanner* banner = [[NotificationBanner alloc] initWithTitle:xmppAccount.connectionProperties.identity.jid subtitle:notification.userInfo[@"message"] leftView:nil rightView:nil style:([notification.userInfo[@"isSevere"] boolValue] ? BannerStyleDanger : BannerStyleWarning) colors:nil]; From b8f220e946dcb8dc8eb0ebb56065083644f4aa13 Mon Sep 17 00:00:00 2001 From: Thilo Molitor Date: Tue, 8 Oct 2024 10:26:38 +0200 Subject: [PATCH 05/10] Don't try to resume a non-paused timer Non-paused timers have a _remainingTime of zero, resuming them will fast-forward them to firing now. That's not what resume should do --> log a warning in those cases and ignore the resume instead. --- Monal/Classes/MLDelayableTimer.m | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/Monal/Classes/MLDelayableTimer.m b/Monal/Classes/MLDelayableTimer.m index 3658412e8..bf3ed5e69 100644 --- a/Monal/Classes/MLDelayableTimer.m +++ b/Monal/Classes/MLDelayableTimer.m @@ -77,9 +77,15 @@ -(void) pause DDLogWarn(@"Tried to pause already fired timer: %@", self); return; } + NSTimeInterval remaining = _wrappedTimer.fireDate.timeIntervalSinceNow; + if(remaining == 0) + { + DDLogWarn(@"Tried to pause timer the exact second its firing: %@", self); + return; + } DDLogDebug(@"Pausing timer: %@", self); - _remainingTime = _wrappedTimer.fireDate.timeIntervalSinceNow; _wrappedTimer.fireDate = NSDate.distantFuture; //postpone timer virtually indefinitely + _remainingTime = remaining; } } @@ -91,6 +97,11 @@ -(void) resume DDLogWarn(@"Tried to resume already fired timer: %@", self); return; } + if(_remainingTime == 0) + { + DDLogWarn(@"Tried to resume non-paused timer: %@", self); + return; + } DDLogDebug(@"Resuming timer: %@", self); _wrappedTimer.fireDate = [NSDate dateWithTimeIntervalSinceNow:_remainingTime]; _remainingTime = 0; From e8181531ec5edcf4cb83585ddfe0096dde3daf30 Mon Sep 17 00:00:00 2001 From: Thilo Molitor Date: Wed, 9 Oct 2024 09:53:35 +0200 Subject: [PATCH 06/10] Fix deadlock between @synchronized in MLStream and receiveQueue The `@synchronized(self.shared_state)` in `generateEvent:` of `MLStream.m` was held while calling the delegate from the networking runloop. If we now tried to call disconnect in the receiveQueue while still running the delegate handler (something that can happen because we are sync dispatching to the receive queue from the delegate), that disconnect tries to close the MLStream which tries to acquire the @synchronized lock on the shared MLStream state which is still held by the networking runloop waiting for the sync receiveQueue dispatch to complete. (which in turn won't complete because the receiveQueue is still waiting for the @synchronized in the `[MLStream close]`). Order of events: 1. Add a block doing a disconnect (or reconnect) to the receiveQueue 2. Call the MLStream delegate with the NSStreamEventOpenCompleted event This results in the following deadlock cycle: 1. The sync dispatch in the delegate waits for the receiveQueue to become available while holding the @synchronized lock on the shared MLStream state 2. The disconnect blocks the the receiveQueue while waiting for the @synchronized lock to the shared MLStream state to become available This bug was made more likely to happen when resuming the app from suspended state by a bug in the MLDelayableTimer implementation fixed in commit 83e0d34fdf2480c88b2e6afd2ebf7aad80c5e4d1 --- Monal/Classes/MLStream.m | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/Monal/Classes/MLStream.m b/Monal/Classes/MLStream.m index 4a2c6562b..06699e89a 100644 --- a/Monal/Classes/MLStream.m +++ b/Monal/Classes/MLStream.m @@ -689,20 +689,24 @@ -(void) generateEvent:(NSStreamEvent) event return; //schedule the delegate calls in the runloop that was registered CFRunLoopPerformBlock([self.shared_state.runLoop getCFRunLoop], (__bridge CFStringRef)self.shared_state.runLoopMode, ^{ + //make sure to NOT hold the @synchronized lock when calling the delegate to not introduce deadlocks + BOOL handleEvent = NO; @synchronized(self.shared_state) { if(event == NSStreamEventOpenCompleted && self.open_called && self.shared_state.open) - [self->_delegate stream:self handleEvent:event]; + handleEvent = YES; else if(event == NSStreamEventHasBytesAvailable && self.open_called && self.shared_state.open) - [self->_delegate stream:self handleEvent:event]; + handleEvent = YES; else if(event == NSStreamEventHasSpaceAvailable && self.open_called && self.shared_state.open) - [self->_delegate stream:self handleEvent:event]; + handleEvent = YES; else if(event == NSStreamEventErrorOccurred) - [self->_delegate stream:self handleEvent:event]; + handleEvent = YES; else if(event == NSStreamEventEndEncountered && self.open_called && self.shared_state.open) - [self->_delegate stream:self handleEvent:event]; - else - DDLogVerbose(@"Ignored event %ld", (long)event); + handleEvent = YES; } + if(handleEvent) + [self->_delegate stream:self handleEvent:event]; + else + DDLogVerbose(@"Ignored event %ld", (long)event); }); //trigger wakeup of runloop to execute the block as soon as possible CFRunLoopWakeUp([self.shared_state.runLoop getCFRunLoop]); From e3d1ca02c301dc080c00f9ae8257b3c3cccda51e Mon Sep 17 00:00:00 2001 From: Matthew Fennell Date: Thu, 26 Sep 2024 01:05:12 +0100 Subject: [PATCH 07/10] Fix crash when opening settings on iOS 18 If you open the settings page on iOS 18.0, the app will crash. On iOS 17.4, it works fine. The crash happens because UIKit complains that the account cell is initialised twice. CRASH(NSInternalInconsistencyException): View was already initialized: We initialise the cell for the first time in MLSettingsTableViewController:tableView: MLSwitchCell* cell = [tableView dequeueReusableCellWithIdentifier:@"AccountCell" forIndexPath:indexPath]; Just below, we call AccountListController:initContactCell, which initialises the cell for the second time: cell = [cell initWithStyle:UITableViewCellStyleSubtitle reuseIdentifier:@"AccountCell"]; We seem to do this so we can mostly resuse the styling defined in the nib file for AccountCell, but additionally add the subtitle styling that lets us show the time that the account was last connected. This approach is recommended here: https://stackoverflow.com/a/40809039 However, this seems incorrect since it leads to the cell being initialised twice. Probably Apple were not asserting this condition before, but are now. So, remove the subtitle from the cell, so that we can fix this crash for now, and re-add the subtitle later when rewriting the view in SwiftUI. --- Monal/Classes/AccountListController.m | 9 --------- 1 file changed, 9 deletions(-) diff --git a/Monal/Classes/AccountListController.m b/Monal/Classes/AccountListController.m index 1ebdb7467..3f623f3a0 100644 --- a/Monal/Classes/AccountListController.m +++ b/Monal/Classes/AccountListController.m @@ -76,7 +76,6 @@ -(void) refreshAccountList -(void) initContactCell:(MLSwitchCell*) cell forAccNo:(NSUInteger) accNo { [cell initTapCell:@"\n\n"]; - cell = [cell initWithStyle:UITableViewCellStyleSubtitle reuseIdentifier:@"AccountCell"]; NSDictionary* account = [self.accountList objectAtIndex:accNo]; MLAssert(account != nil, ([NSString stringWithFormat:@"Expected non nil account in row %lu", (unsigned long)accNo])); if([(NSString*)[account objectForKey:@"domain"] length] > 0) { @@ -89,7 +88,6 @@ -(void) initContactCell:(MLSwitchCell*) cell forAccNo:(NSUInteger) accNo } UIImageView* accessory = [[UIImageView alloc] initWithFrame:CGRectMake(0, 0, 30, 30)]; - cell.detailTextLabel.text = nil; if([[account objectForKey:@"enabled"] boolValue] == YES) { @@ -98,17 +96,11 @@ -(void) initContactCell:(MLSwitchCell*) cell forAccNo:(NSUInteger) accNo { accessory.image = [UIImage imageNamed:@"Connected"]; cell.accessoryView = accessory; - - NSDate* connectedTime = [[MLXMPPManager sharedInstance] connectedTimeFor:[[self.accountList objectAtIndex:accNo] objectForKey:@"account_id"]]; - if(connectedTime) { - cell.detailTextLabel.text = [NSString stringWithFormat:NSLocalizedString(@"Connected since: %@", @""), [self.uptimeFormatter stringFromDate:connectedTime]]; - } } else { accessory.image = [UIImage imageNamed:@"Disconnected"]; cell.accessoryView = accessory; - cell.detailTextLabel.text = NSLocalizedString(@"Connecting...", @""); } } else @@ -116,7 +108,6 @@ -(void) initContactCell:(MLSwitchCell*) cell forAccNo:(NSUInteger) accNo cell.imageView.image = [UIImage systemImageNamed:@"circle"]; accessory.image = nil; cell.accessoryView = accessory; - cell.detailTextLabel.text = NSLocalizedString(@"Account disabled", @""); } } From 5b65ce6e2d6204ebe140280ac3cf34b99049a705 Mon Sep 17 00:00:00 2001 From: Thilo Molitor Date: Sat, 12 Oct 2024 07:44:03 +0200 Subject: [PATCH 08/10] Improve deadlock fix --- Monal/Classes/MLStream.m | 45 ++++++++++++++++++++-------------------- 1 file changed, 23 insertions(+), 22 deletions(-) diff --git a/Monal/Classes/MLStream.m b/Monal/Classes/MLStream.m index 06699e89a..1070be770 100644 --- a/Monal/Classes/MLStream.m +++ b/Monal/Classes/MLStream.m @@ -687,29 +687,30 @@ -(void) generateEvent:(NSStreamEvent) event //don't schedule delegate calls if no runloop was specified if(self.shared_state.runLoop == nil) return; - //schedule the delegate calls in the runloop that was registered - CFRunLoopPerformBlock([self.shared_state.runLoop getCFRunLoop], (__bridge CFStringRef)self.shared_state.runLoopMode, ^{ - //make sure to NOT hold the @synchronized lock when calling the delegate to not introduce deadlocks - BOOL handleEvent = NO; - @synchronized(self.shared_state) { - if(event == NSStreamEventOpenCompleted && self.open_called && self.shared_state.open) - handleEvent = YES; - else if(event == NSStreamEventHasBytesAvailable && self.open_called && self.shared_state.open) - handleEvent = YES; - else if(event == NSStreamEventHasSpaceAvailable && self.open_called && self.shared_state.open) - handleEvent = YES; - else if(event == NSStreamEventErrorOccurred) - handleEvent = YES; - else if(event == NSStreamEventEndEncountered && self.open_called && self.shared_state.open) - handleEvent = YES; - } - if(handleEvent) + //make sure to NOT hold the @synchronized lock when calling the delegate to not introduce deadlocks + BOOL handleEvent = NO; + if(event == NSStreamEventOpenCompleted && self.open_called && self.shared_state.open) + handleEvent = YES; + else if(event == NSStreamEventHasBytesAvailable && self.open_called && self.shared_state.open) + handleEvent = YES; + else if(event == NSStreamEventHasSpaceAvailable && self.open_called && self.shared_state.open) + handleEvent = YES; + else if(event == NSStreamEventErrorOccurred) + handleEvent = YES; + else if(event == NSStreamEventEndEncountered && self.open_called && self.shared_state.open) + handleEvent = YES; + //check if the event should be handled + if(!handleEvent) + DDLogVerbose(@"Ignoring event %ld", (long)event); + else + { + //schedule the delegate calls in the runloop that was registered + CFRunLoopPerformBlock([self.shared_state.runLoop getCFRunLoop], (__bridge CFStringRef)self.shared_state.runLoopMode, ^{ [self->_delegate stream:self handleEvent:event]; - else - DDLogVerbose(@"Ignored event %ld", (long)event); - }); - //trigger wakeup of runloop to execute the block as soon as possible - CFRunLoopWakeUp([self.shared_state.runLoop getCFRunLoop]); + }); + //trigger wakeup of runloop to execute the block as soon as possible + CFRunLoopWakeUp([self.shared_state.runLoop getCFRunLoop]); + } } } From 44f55aaa0afb085925b2cef83ee39ef3b026e7f2 Mon Sep 17 00:00:00 2001 From: lissine Date: Fri, 20 Sep 2024 23:03:51 +0100 Subject: [PATCH 09/10] Allow adding gateway components as contacts --- Monal/Classes/xmpp.m | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/Monal/Classes/xmpp.m b/Monal/Classes/xmpp.m index 684b53cf4..959607af1 100644 --- a/Monal/Classes/xmpp.m +++ b/Monal/Classes/xmpp.m @@ -4471,9 +4471,15 @@ -(AnyPromise*) checkJidType:(NSString*) jid [discoInfo setiqTo:jid]; [discoInfo setDiscoInfoNode]; [self sendIq:discoInfo withResponseHandler:^(XMPPIQ* response) { + NSSet* identities = [NSSet setWithArray:[response find:@"{http://jabber.org/protocol/disco#info}query/identity@category"]]; NSSet* features = [NSSet setWithArray:[response find:@"{http://jabber.org/protocol/disco#info}query/feature@var"]]; - //check if this is a muc or account - if([features containsObject:@"http://jabber.org/protocol/muc"]) + //check if this is an account or a muc + //this test has to come first because a gateway component may have an "account" identity while also supporintg MUC. + //usually this means that there's a bot at the component's address that facilitates registration without adhoc commands. + //the "account" jidType makes it possible to add the component as a contact. + if([identities containsObject:@"account"]) + return resolve(@"account"); + else if([features containsObject:@"http://jabber.org/protocol/muc"]) return resolve(@"muc"); else return resolve(@"account"); From 61d46982df04236d994e718d87d2c0ab16bf3287 Mon Sep 17 00:00:00 2001 From: lissine Date: Wed, 9 Oct 2024 20:23:54 +0100 Subject: [PATCH 10/10] Make the discovery of jid type more reliable for MUCs XEP-0045 requires a room to - have an identity with category='conference' - advertise support for the 'http://jabber.org/protocol/muc' feature Thus it is more reliable to check for both of these things. --- Monal/Classes/xmpp.m | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Monal/Classes/xmpp.m b/Monal/Classes/xmpp.m index 959607af1..fa6e985ce 100644 --- a/Monal/Classes/xmpp.m +++ b/Monal/Classes/xmpp.m @@ -4479,7 +4479,8 @@ -(AnyPromise*) checkJidType:(NSString*) jid //the "account" jidType makes it possible to add the component as a contact. if([identities containsObject:@"account"]) return resolve(@"account"); - else if([features containsObject:@"http://jabber.org/protocol/muc"]) + else if([identities containsObject:@"conference"] + && [features containsObject:@"http://jabber.org/protocol/muc"]) return resolve(@"muc"); else return resolve(@"account");