From 0380d81c3d2488bf0df9cfc1877005e1cdaa07bc Mon Sep 17 00:00:00 2001 From: Yee Cheng Chin Date: Tue, 15 Aug 2023 18:08:10 -0700 Subject: [PATCH] Harden NSConnection security in handling third-party connections Currently, MacVim uses Distributed Objects / NSConnection as the IPC mechanism. The child Vim process connects to the parent MacVim process using NSConnection and registers itself. A security issue with this is that NSConnection is global, and any process can connect to the app, and MacVim isn't too hardened against this issue. Note that one reason why we do need the ability for the MacVim app to accept random connections is to support the `:gui` command from a random Vim process, and to supported listing server names. One issue is that while the app protocol (MMAppProtocol) is only a few functions, we were exposing the entire app, which exposes functions like `executeInLoginShell`, which could be invoked by the caller, which is quite unsafe as it could be invoked by any third-party app. Fix this issue by using `NSProtocolChecker` to make sure we only expose the APIs that we want to expose. Each Vim controller now also gets a randomized ID instead of an incremental one. Currently the API for sending messages from Vim to MacVim is public, meaning any app can send message to MacVim. Using a randomized ID makes it more difficult for an attacker to guess the ID (which used to always start at 1) and injects random commands to MacVim pretending to be the Vim process. Also, make sure if MacVim failed to register the NSConnection on launch, just display an error and terminate. This usually happens if multiple MacVim instances are opened, but also if an attacking app is trying to register a connection first using the same name. This way the user would know something is wrong instead of MacVim being opened by not able to do anything as it didn't register the connection. In the near future, the IPC mechanism will be switched to XPC, which is the preferred way by Apple as Distributed Objects has been deprecated for a long time. It will have proper security to only accept processes within the same app to message each other. It will be done in #1157. --- src/MacVim/MMAppController.m | 36 +++++++++++++++------ src/MacVim/MMBackend.h | 2 +- src/MacVim/MMVimController.h | 4 +-- src/MacVim/MMVimController.m | 31 ++++++++++-------- src/MacVim/MacVim.h | 4 +-- src/MacVim/MacVim.xcodeproj/project.pbxproj | 4 +++ 6 files changed, 53 insertions(+), 28 deletions(-) diff --git a/src/MacVim/MMAppController.m b/src/MacVim/MMAppController.m index b12b5637a8..571efb5253 100644 --- a/src/MacVim/MMAppController.m +++ b/src/MacVim/MMAppController.m @@ -304,7 +304,9 @@ fsEventCallback(ConstFSEventStreamRef streamRef, // unlikely to fix it, we graciously give them the default connection.) connection = [[NSConnection alloc] initWithReceivePort:[NSPort port] sendPort:nil]; - [connection setRootObject:self]; + NSProtocolChecker *rootObject = [NSProtocolChecker protocolCheckerWithTarget:self + protocol:@protocol(MMAppProtocol)]; + [connection setRootObject:rootObject]; [connection setRequestTimeout:MMRequestTimeout]; [connection setReplyTimeout:MMReplyTimeout]; @@ -315,6 +317,20 @@ fsEventCallback(ConstFSEventStreamRef streamRef, if (![connection registerName:name]) { ASLogCrit(@"Failed to register connection with name '%@'", name); [connection release]; connection = nil; + + NSAlert *alert = [[NSAlert alloc] init]; + [alert addButtonWithTitle:NSLocalizedString(@"OK", + @"Dialog button")]; + [alert setMessageText:NSLocalizedString(@"MacVim cannot be opened", + @"MacVim cannot be opened, title")]; + [alert setInformativeText:[NSString stringWithFormat:NSLocalizedString( + @"MacVim could not set up its connection. It's likely you already have MacVim opened elsewhere.", + @"MacVim already opened, text")]]; + [alert setAlertStyle:NSAlertStyleCritical]; + [alert runModal]; + [alert release]; + + [[NSApplication sharedApplication] terminate:nil]; } // Register help search handler to support search Vim docs via the Help menu @@ -859,7 +875,7 @@ fsEventCallback(ConstFSEventStreamRef streamRef, - (void)removeVimController:(id)controller { - ASLogDebug(@"Remove Vim controller pid=%d id=%d (processingFlag=%d)", + ASLogDebug(@"Remove Vim controller pid=%d id=%lu (processingFlag=%d)", [controller pid], [controller vimControllerId], processingFlag); NSUInteger idx = [vimControllers indexOfObject:controller]; @@ -1540,7 +1556,7 @@ fsEventCallback(ConstFSEventStreamRef streamRef, return nil; } -- (unsigned)connectBackend:(byref in id )proxy pid:(int)pid +- (unsigned long)connectBackend:(byref in id )proxy pid:(int)pid { ASLogDebug(@"pid=%d", pid); @@ -1570,21 +1586,21 @@ fsEventCallback(ConstFSEventStreamRef streamRef, } - (oneway void)processInput:(in bycopy NSArray *)queue - forIdentifier:(unsigned)identifier + forIdentifier:(unsigned long)identifier { // NOTE: Input is not handled immediately since this is a distributed // object call and as such can arrive at unpredictable times. Instead, // queue the input and process it when the run loop is updated. if (!(queue && identifier)) { - ASLogWarn(@"Bad input for identifier=%d", identifier); + ASLogWarn(@"Bad input for identifier=%lu", identifier); return; } - ASLogDebug(@"QUEUE for identifier=%d: <<< %@>>>", identifier, + ASLogDebug(@"QUEUE for identifier=%lu: <<< %@>>>", identifier, debugStringForMessageQueue(queue)); - NSNumber *key = [NSNumber numberWithUnsignedInt:identifier]; + NSNumber *key = [NSNumber numberWithUnsignedLong:identifier]; NSArray *q = [inputQueues objectForKey:key]; if (q) { q = [q arrayByAddingObjectsFromArray:queue]; @@ -2715,7 +2731,7 @@ fsEventCallback(ConstFSEventStreamRef streamRef, NSEnumerator *e = [queues keyEnumerator]; NSNumber *key; while ((key = [e nextObject])) { - unsigned ukey = [key unsignedIntValue]; + unsigned long ukey = [key unsignedLongValue]; int i = 0, count = [vimControllers count]; for (i = 0; i < count; ++i) { MMVimController *vc = [vimControllers objectAtIndex:i]; @@ -2737,7 +2753,7 @@ fsEventCallback(ConstFSEventStreamRef streamRef, } if (i == count) { - ASLogWarn(@"No Vim controller for identifier=%d", ukey); + ASLogWarn(@"No Vim controller for identifier=%lu", ukey); } } @@ -2758,7 +2774,7 @@ fsEventCallback(ConstFSEventStreamRef streamRef, - (void)addVimController:(MMVimController *)vc { - ASLogDebug(@"Add Vim controller pid=%d id=%d", + ASLogDebug(@"Add Vim controller pid=%d id=%lu", [vc pid], [vc vimControllerId]); int pid = [vc pid]; diff --git a/src/MacVim/MMBackend.h b/src/MacVim/MMBackend.h index f0141da736..6a730a0f3f 100644 --- a/src/MacVim/MMBackend.h +++ b/src/MacVim/MMBackend.h @@ -21,7 +21,7 @@ NSConnection *connection; NSConnection *vimServerConnection; id appProxy; - unsigned identifier; + unsigned long identifier; NSDictionary *colorDict; NSDictionary *sysColorDict; NSDictionary *actionDict; diff --git a/src/MacVim/MMVimController.h b/src/MacVim/MMVimController.h index 6ab169adc8..0d99780019 100644 --- a/src/MacVim/MMVimController.h +++ b/src/MacVim/MMVimController.h @@ -34,7 +34,7 @@ #endif > { - unsigned identifier; + unsigned long identifier; BOOL isInitialized; MMWindowController *windowController; id backendProxy; @@ -58,7 +58,7 @@ - (id)initWithBackend:(id)backend pid:(int)processIdentifier; - (void)uninitialize; -- (unsigned)vimControllerId; +- (unsigned long)vimControllerId; - (id)backendProxy; - (int)pid; - (void)setServerName:(NSString *)name; diff --git a/src/MacVim/MMVimController.m b/src/MacVim/MMVimController.m index 6b9e06eb99..b4b0675c59 100644 --- a/src/MacVim/MMVimController.m +++ b/src/MacVim/MMVimController.m @@ -56,8 +56,6 @@ static NSTimeInterval MMBackendProxyRequestTimeout = 0; // Timeout used for setDialogReturn:. static NSTimeInterval MMSetDialogReturnTimeout = 1.0; -static unsigned identifierCounter = 1; - static BOOL isUnsafeMessage(int msgid); @@ -168,8 +166,15 @@ static BOOL isUnsafeMessage(int msgid); if (!(self = [super init])) return nil; - // TODO: Come up with a better way of creating an identifier. - identifier = identifierCounter++; + // Use a random identifier. Currently, MMBackend connects using a public + // NSConnection, which has security implications. Using random identifiers + // make it much harder for third-party attacker to spoof. + int secSuccess = SecRandomCopyBytes(kSecRandomDefault, sizeof(identifier), &identifier); + if (secSuccess != errSecSuccess) { + // Don't know what concrete reasons secure random would fail, but just + // as a failsafe, use a less secure option. + identifier = ((unsigned long)arc4random()) << 32 | (unsigned long)arc4random(); + } windowController = [[MMWindowController alloc] initWithVimController:self]; @@ -257,7 +262,7 @@ static BOOL isUnsafeMessage(int msgid); isInitialized = NO; } -- (unsigned)vimControllerId +- (unsigned long)vimControllerId { return identifier; } @@ -436,7 +441,7 @@ static BOOL isUnsafeMessage(int msgid); [backendProxy processInput:msgid data:data]; } @catch (NSException *ex) { - ASLogDebug(@"processInput:data: failed: pid=%d id=%d msg=%s reason=%@", + ASLogDebug(@"processInput:data: failed: pid=%d id=%lu msg=%s reason=%@", pid, identifier, MMVimMsgIDStrings[msgid], ex); } } @@ -468,7 +473,7 @@ static BOOL isUnsafeMessage(int msgid); } @catch (NSException *ex) { sendOk = NO; - ASLogDebug(@"processInput:data: failed: pid=%d id=%d msg=%s reason=%@", + ASLogDebug(@"processInput:data: failed: pid=%d id=%lu msg=%s reason=%@", pid, identifier, MMVimMsgIDStrings[msgid], ex); } @finally { @@ -500,7 +505,7 @@ static BOOL isUnsafeMessage(int msgid); ASLogDebug(@"eval(%@)=%@", expr, eval); } @catch (NSException *ex) { - ASLogDebug(@"evaluateExpression: failed: pid=%d id=%d reason=%@", + ASLogDebug(@"evaluateExpression: failed: pid=%d id=%lu reason=%@", pid, identifier, ex); } @@ -517,7 +522,7 @@ static BOOL isUnsafeMessage(int msgid); errorString:errstr]; ASLogDebug(@"eval(%@)=%@", expr, eval); } @catch (NSException *ex) { - ASLogDebug(@"evaluateExpressionCocoa: failed: pid=%d id=%d reason=%@", + ASLogDebug(@"evaluateExpressionCocoa: failed: pid=%d id=%lu reason=%@", pid, identifier, ex); *errstr = [ex reason]; } @@ -556,7 +561,7 @@ static BOOL isUnsafeMessage(int msgid); [windowController processInputQueueDidFinish]; } @catch (NSException *ex) { - ASLogDebug(@"Exception: pid=%d id=%d reason=%@", pid, identifier, ex); + ASLogDebug(@"Exception: pid=%d id=%lu reason=%@", pid, identifier, ex); } } @@ -1275,7 +1280,7 @@ static BOOL isUnsafeMessage(int msgid); noteNewRecentFilePath:path]; } @catch (NSException *ex) { - ASLogDebug(@"Exception: pid=%d id=%d reason=%@", pid, identifier, ex); + ASLogDebug(@"Exception: pid=%d id=%lu reason=%@", pid, identifier, ex); } @finally { [conn setRequestTimeout:oldTimeout]; @@ -1308,7 +1313,7 @@ static BOOL isUnsafeMessage(int msgid); [backendProxy setDialogReturn:ret]; } @catch (NSException *ex) { - ASLogDebug(@"setDialogReturn: failed: pid=%d id=%d reason=%@", + ASLogDebug(@"setDialogReturn: failed: pid=%d id=%lu reason=%@", pid, identifier, ex); } } @@ -2089,7 +2094,7 @@ static BOOL isUnsafeMessage(int msgid); - (void)scheduleClose { - ASLogDebug(@"pid=%d id=%d", pid, identifier); + ASLogDebug(@"pid=%d id=%lu", pid, identifier); // NOTE! This message can arrive at pretty much anytime, e.g. while // the run loop is the 'event tracking' mode. This means that Cocoa may diff --git a/src/MacVim/MacVim.h b/src/MacVim/MacVim.h index 5e6af3b81e..1f4328b6a7 100644 --- a/src/MacVim/MacVim.h +++ b/src/MacVim/MacVim.h @@ -197,9 +197,9 @@ typedef NSString* NSAttributedStringKey; // connectBackend:pid: and processInput:forIdentifier:). // @protocol MMAppProtocol -- (unsigned)connectBackend:(byref in id )proxy pid:(int)pid; +- (unsigned long)connectBackend:(byref in id )proxy pid:(int)pid; - (oneway void)processInput:(in bycopy NSArray *)queue - forIdentifier:(unsigned)identifier; + forIdentifier:(unsigned long)identifier; - (NSArray *)serverList; @end diff --git a/src/MacVim/MacVim.xcodeproj/project.pbxproj b/src/MacVim/MacVim.xcodeproj/project.pbxproj index 834b551a86..80dce37960 100644 --- a/src/MacVim/MacVim.xcodeproj/project.pbxproj +++ b/src/MacVim/MacVim.xcodeproj/project.pbxproj @@ -74,6 +74,7 @@ 909894382A56EB1E007B84A3 /* WhatsNew.xib in Resources */ = {isa = PBXBuildFile; fileRef = 909894362A56EB1E007B84A3 /* WhatsNew.xib */; }; 9098943C2A56ECF6007B84A3 /* MMWhatsNewController.m in Sources */ = {isa = PBXBuildFile; fileRef = 9098943B2A56ECF6007B84A3 /* MMWhatsNewController.m */; }; 90A33BEA28D563DF003A2E2F /* MMSparkle2Delegate.m in Sources */ = {isa = PBXBuildFile; fileRef = 90A33BE928D563DF003A2E2F /* MMSparkle2Delegate.m */; }; + 90AF83AB2A8C37F70046DA2E /* Security.framework in Frameworks */ = {isa = PBXBuildFile; fileRef = 90AF83A92A8C37F70046DA2E /* Security.framework */; }; 90B9877D2A579F9500FC95D6 /* WebKit.framework in Frameworks */ = {isa = PBXBuildFile; fileRef = 90B9877B2A579F9500FC95D6 /* WebKit.framework */; settings = {ATTRIBUTES = (Weak, ); }; }; /* End PBXBuildFile section */ @@ -423,6 +424,7 @@ 90AF83B62AA15C660046DA2E /* nv_cmds.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; name = nv_cmds.h; path = ../nv_cmds.h; sourceTree = ""; }; 90AF83B72AA15C660046DA2E /* vim9cmds.c */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.c; name = vim9cmds.c; path = ../vim9cmds.c; sourceTree = ""; }; 90AF83B82AA15C660046DA2E /* termdefs.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; name = termdefs.h; path = ../termdefs.h; sourceTree = ""; }; + 90AF83A92A8C37F70046DA2E /* Security.framework */ = {isa = PBXFileReference; lastKnownFileType = wrapper.framework; name = Security.framework; path = System/Library/Frameworks/Security.framework; sourceTree = SDKROOT; }; 90B9877B2A579F9500FC95D6 /* WebKit.framework */ = {isa = PBXFileReference; lastKnownFileType = wrapper.framework; name = WebKit.framework; path = System/Library/Frameworks/WebKit.framework; sourceTree = SDKROOT; }; 90F84F1E2521F2270000268B /* ko */ = {isa = PBXFileReference; lastKnownFileType = text.plist.strings; name = ko; path = ko.lproj/MainMenu.strings; sourceTree = ""; }; 90F84F232521F6480000268B /* ca */ = {isa = PBXFileReference; lastKnownFileType = text.plist.strings; name = ca; path = ca.lproj/MainMenu.strings; sourceTree = ""; }; @@ -462,6 +464,7 @@ isa = PBXFrameworksBuildPhase; buildActionMask = 2147483647; files = ( + 90AF83AB2A8C37F70046DA2E /* Security.framework in Frameworks */, 90B9877D2A579F9500FC95D6 /* WebKit.framework in Frameworks */, 1DFE25A50C527BC4003000F7 /* PSMTabBarControl.framework in Frameworks */, 8D11072F0486CEB800E47090 /* Cocoa.framework in Frameworks */, @@ -650,6 +653,7 @@ 29B97323FDCFA39411CA2CEA /* Frameworks */ = { isa = PBXGroup; children = ( + 90AF83A92A8C37F70046DA2E /* Security.framework */, 90B9877B2A579F9500FC95D6 /* WebKit.framework */, 52A364721C4A5789005757EC /* Sparkle.framework */, 1D8B5A52104AF9FF002E59D5 /* Carbon.framework */,