Skip to content

Commit 399b43e

Browse files
ychinychin
authored
Merge pull request from GHSA-9jgj-jfwg-99fv
Harden NSConnection security in handling third-party connections
2 parents 3927f58 + 0380d81 commit 399b43e

File tree

6 files changed

+53
-28
lines changed

6 files changed

+53
-28
lines changed

‎src/MacVim/MMAppController.m‎

Lines changed: 26 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -304,7 +304,9 @@ - (id)init
304304
// unlikely to fix it, we graciously give them the default connection.)
305305
connection = [[NSConnection alloc] initWithReceivePort:[NSPort port]
306306
sendPort:nil];
307-
[connection setRootObject:self];
307+
NSProtocolChecker *rootObject = [NSProtocolChecker protocolCheckerWithTarget:self
308+
protocol:@protocol(MMAppProtocol)];
309+
[connection setRootObject:rootObject];
308310
[connection setRequestTimeout:MMRequestTimeout];
309311
[connection setReplyTimeout:MMReplyTimeout];
310312

@@ -315,6 +317,20 @@ - (id)init
315317
if (![connection registerName:name]) {
316318
ASLogCrit(@"Failed to register connection with name '%@'", name);
317319
[connection release]; connection = nil;
320+
321+
NSAlert *alert = [[NSAlert alloc] init];
322+
[alert addButtonWithTitle:NSLocalizedString(@"OK",
323+
@"Dialog button")];
324+
[alert setMessageText:NSLocalizedString(@"MacVim cannot be opened",
325+
@"MacVim cannot be opened, title")];
326+
[alert setInformativeText:[NSString stringWithFormat:NSLocalizedString(
327+
@"MacVim could not set up its connection. It's likely you already have MacVim opened elsewhere.",
328+
@"MacVim already opened, text")]];
329+
[alert setAlertStyle:NSAlertStyleCritical];
330+
[alert runModal];
331+
[alert release];
332+
333+
[[NSApplication sharedApplication] terminate:nil];
318334
}
319335

320336
// Register help search handler to support search Vim docs via the Help menu
@@ -859,7 +875,7 @@ - (NSMenuItem *)appMenuItemTemplate
859875

860876
- (void)removeVimController:(id)controller
861877
{
862-
ASLogDebug(@"Remove Vim controller pid=%d id=%d (processingFlag=%d)",
878+
ASLogDebug(@"Remove Vim controller pid=%d id=%lu (processingFlag=%d)",
863879
[controller pid], [controller vimControllerId], processingFlag);
864880

865881
NSUInteger idx = [vimControllers indexOfObject:controller];
@@ -1540,7 +1556,7 @@ - (MMVimController *)keyVimController
15401556
return nil;
15411557
}
15421558

1543-
- (unsigned)connectBackend:(byref in id <MMBackendProtocol>)proxy pid:(int)pid
1559+
- (unsigned long)connectBackend:(byref in id <MMBackendProtocol>)proxy pid:(int)pid
15441560
{
15451561
ASLogDebug(@"pid=%d", pid);
15461562

@@ -1570,21 +1586,21 @@ - (unsigned)connectBackend:(byref in id <MMBackendProtocol>)proxy pid:(int)pid
15701586
}
15711587

15721588
- (oneway void)processInput:(in bycopy NSArray *)queue
1573-
forIdentifier:(unsigned)identifier
1589+
forIdentifier:(unsigned long)identifier
15741590
{
15751591
// NOTE: Input is not handled immediately since this is a distributed
15761592
// object call and as such can arrive at unpredictable times. Instead,
15771593
// queue the input and process it when the run loop is updated.
15781594

15791595
if (!(queue && identifier)) {
1580-
ASLogWarn(@"Bad input for identifier=%d", identifier);
1596+
ASLogWarn(@"Bad input for identifier=%lu", identifier);
15811597
return;
15821598
}
15831599

1584-
ASLogDebug(@"QUEUE for identifier=%d: <<< %@>>>", identifier,
1600+
ASLogDebug(@"QUEUE for identifier=%lu: <<< %@>>>", identifier,
15851601
debugStringForMessageQueue(queue));
15861602

1587-
NSNumber *key = [NSNumber numberWithUnsignedInt:identifier];
1603+
NSNumber *key = [NSNumber numberWithUnsignedLong:identifier];
15881604
NSArray *q = [inputQueues objectForKey:key];
15891605
if (q) {
15901606
q = [q arrayByAddingObjectsFromArray:queue];
@@ -2715,7 +2731,7 @@ - (void)processInputQueues:(id)sender
27152731
NSEnumerator *e = [queues keyEnumerator];
27162732
NSNumber *key;
27172733
while ((key = [e nextObject])) {
2718-
unsigned ukey = [key unsignedIntValue];
2734+
unsigned long ukey = [key unsignedLongValue];
27192735
int i = 0, count = [vimControllers count];
27202736
for (i = 0; i < count; ++i) {
27212737
MMVimController *vc = [vimControllers objectAtIndex:i];
@@ -2737,7 +2753,7 @@ - (void)processInputQueues:(id)sender
27372753
}
27382754

27392755
if (i == count) {
2740-
ASLogWarn(@"No Vim controller for identifier=%d", ukey);
2756+
ASLogWarn(@"No Vim controller for identifier=%lu", ukey);
27412757
}
27422758
}
27432759

@@ -2758,7 +2774,7 @@ - (void)processInputQueues:(id)sender
27582774

27592775
- (void)addVimController:(MMVimController *)vc
27602776
{
2761-
ASLogDebug(@"Add Vim controller pid=%d id=%d",
2777+
ASLogDebug(@"Add Vim controller pid=%d id=%lu",
27622778
[vc pid], [vc vimControllerId]);
27632779

27642780
int pid = [vc pid];

‎src/MacVim/MMBackend.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@
2121
NSConnection *connection;
2222
NSConnection *vimServerConnection;
2323
id appProxy;
24-
unsigned identifier;
24+
unsigned long identifier;
2525
NSDictionary *colorDict;
2626
NSDictionary *sysColorDict;
2727
NSDictionary *actionDict;

‎src/MacVim/MMVimController.h‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@
3434
#endif
3535
>
3636
{
37-
unsigned identifier;
37+
unsigned long identifier;
3838
BOOL isInitialized;
3939
MMWindowController *windowController;
4040
id backendProxy;
@@ -58,7 +58,7 @@
5858

5959
- (id)initWithBackend:(id)backend pid:(int)processIdentifier;
6060
- (void)uninitialize;
61-
- (unsigned)vimControllerId;
61+
- (unsigned long)vimControllerId;
6262
- (id)backendProxy;
6363
- (int)pid;
6464
- (void)setServerName:(NSString *)name;

‎src/MacVim/MMVimController.m‎

Lines changed: 18 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -56,8 +56,6 @@
5656
// Timeout used for setDialogReturn:.
5757
static NSTimeInterval MMSetDialogReturnTimeout = 1.0;
5858

59-
static unsigned identifierCounter = 1;
60-
6159
static BOOL isUnsafeMessage(int msgid);
6260

6361

@@ -168,8 +166,15 @@ - (id)initWithBackend:(id)backend pid:(int)processIdentifier
168166
if (!(self = [super init]))
169167
return nil;
170168

171-
// TODO: Come up with a better way of creating an identifier.
172-
identifier = identifierCounter++;
169+
// Use a random identifier. Currently, MMBackend connects using a public
170+
// NSConnection, which has security implications. Using random identifiers
171+
// make it much harder for third-party attacker to spoof.
172+
int secSuccess = SecRandomCopyBytes(kSecRandomDefault, sizeof(identifier), &identifier);
173+
if (secSuccess != errSecSuccess) {
174+
// Don't know what concrete reasons secure random would fail, but just
175+
// as a failsafe, use a less secure option.
176+
identifier = ((unsigned long)arc4random()) << 32 | (unsigned long)arc4random();
177+
}
173178

174179
windowController =
175180
[[MMWindowController alloc] initWithVimController:self];
@@ -257,7 +262,7 @@ - (void)uninitialize
257262
isInitialized = NO;
258263
}
259264

260-
- (unsigned)vimControllerId
265+
- (unsigned long)vimControllerId
261266
{
262267
return identifier;
263268
}
@@ -436,7 +441,7 @@ - (void)sendMessage:(int)msgid data:(NSData *)data
436441
[backendProxy processInput:msgid data:data];
437442
}
438443
@catch (NSException *ex) {
439-
ASLogDebug(@"processInput:data: failed: pid=%d id=%d msg=%s reason=%@",
444+
ASLogDebug(@"processInput:data: failed: pid=%d id=%lu msg=%s reason=%@",
440445
pid, identifier, MMVimMsgIDStrings[msgid], ex);
441446
}
442447
}
@@ -468,7 +473,7 @@ - (BOOL)sendMessageNow:(int)msgid data:(NSData *)data
468473
}
469474
@catch (NSException *ex) {
470475
sendOk = NO;
471-
ASLogDebug(@"processInput:data: failed: pid=%d id=%d msg=%s reason=%@",
476+
ASLogDebug(@"processInput:data: failed: pid=%d id=%lu msg=%s reason=%@",
472477
pid, identifier, MMVimMsgIDStrings[msgid], ex);
473478
}
474479
@finally {
@@ -500,7 +505,7 @@ - (NSString *)evaluateVimExpression:(NSString *)expr
500505
ASLogDebug(@"eval(%@)=%@", expr, eval);
501506
}
502507
@catch (NSException *ex) {
503-
ASLogDebug(@"evaluateExpression: failed: pid=%d id=%d reason=%@",
508+
ASLogDebug(@"evaluateExpression: failed: pid=%d id=%lu reason=%@",
504509
pid, identifier, ex);
505510
}
506511

@@ -517,7 +522,7 @@ - (id)evaluateVimExpressionCocoa:(NSString *)expr
517522
errorString:errstr];
518523
ASLogDebug(@"eval(%@)=%@", expr, eval);
519524
} @catch (NSException *ex) {
520-
ASLogDebug(@"evaluateExpressionCocoa: failed: pid=%d id=%d reason=%@",
525+
ASLogDebug(@"evaluateExpressionCocoa: failed: pid=%d id=%lu reason=%@",
521526
pid, identifier, ex);
522527
*errstr = [ex reason];
523528
}
@@ -556,7 +561,7 @@ - (void)processInputQueue:(NSArray *)queue
556561
[windowController processInputQueueDidFinish];
557562
}
558563
@catch (NSException *ex) {
559-
ASLogDebug(@"Exception: pid=%d id=%d reason=%@", pid, identifier, ex);
564+
ASLogDebug(@"Exception: pid=%d id=%lu reason=%@", pid, identifier, ex);
560565
}
561566
}
562567

@@ -1275,7 +1280,7 @@ - (void)savePanelDidEnd:(NSSavePanel *)panel code:(int)code
12751280
noteNewRecentFilePath:path];
12761281
}
12771282
@catch (NSException *ex) {
1278-
ASLogDebug(@"Exception: pid=%d id=%d reason=%@", pid, identifier, ex);
1283+
ASLogDebug(@"Exception: pid=%d id=%lu reason=%@", pid, identifier, ex);
12791284
}
12801285
@finally {
12811286
[conn setRequestTimeout:oldTimeout];
@@ -1308,7 +1313,7 @@ - (void)alertDidEnd:(MMAlert *)alert code:(int)code context:(void *)context
13081313
[backendProxy setDialogReturn:ret];
13091314
}
13101315
@catch (NSException *ex) {
1311-
ASLogDebug(@"setDialogReturn: failed: pid=%d id=%d reason=%@",
1316+
ASLogDebug(@"setDialogReturn: failed: pid=%d id=%lu reason=%@",
13121317
pid, identifier, ex);
13131318
}
13141319
}
@@ -2089,7 +2094,7 @@ - (void)connectionDidDie:(NSNotification *)notification
20892094

20902095
- (void)scheduleClose
20912096
{
2092-
ASLogDebug(@"pid=%d id=%d", pid, identifier);
2097+
ASLogDebug(@"pid=%d id=%lu", pid, identifier);
20932098

20942099
// NOTE! This message can arrive at pretty much anytime, e.g. while
20952100
// the run loop is the 'event tracking' mode. This means that Cocoa may

‎src/MacVim/MacVim.h‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -197,9 +197,9 @@ typedef NSString* NSAttributedStringKey;
197197
// connectBackend:pid: and processInput:forIdentifier:).
198198
//
199199
@protocol MMAppProtocol
200-
- (unsigned)connectBackend:(byref in id <MMBackendProtocol>)proxy pid:(int)pid;
200+
- (unsigned long)connectBackend:(byref in id <MMBackendProtocol>)proxy pid:(int)pid;
201201
- (oneway void)processInput:(in bycopy NSArray *)queue
202-
forIdentifier:(unsigned)identifier;
202+
forIdentifier:(unsigned long)identifier;
203203
- (NSArray *)serverList;
204204
@end
205205

‎src/MacVim/MacVim.xcodeproj/project.pbxproj‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,7 @@
7474
909894382A56EB1E007B84A3 /* WhatsNew.xib in Resources */ = {isa = PBXBuildFile; fileRef = 909894362A56EB1E007B84A3 /* WhatsNew.xib */; };
7575
9098943C2A56ECF6007B84A3 /* MMWhatsNewController.m in Sources */ = {isa = PBXBuildFile; fileRef = 9098943B2A56ECF6007B84A3 /* MMWhatsNewController.m */; };
7676
90A33BEA28D563DF003A2E2F /* MMSparkle2Delegate.m in Sources */ = {isa = PBXBuildFile; fileRef = 90A33BE928D563DF003A2E2F /* MMSparkle2Delegate.m */; };
77+
90AF83AB2A8C37F70046DA2E /* Security.framework in Frameworks */ = {isa = PBXBuildFile; fileRef = 90AF83A92A8C37F70046DA2E /* Security.framework */; };
7778
90B9877D2A579F9500FC95D6 /* WebKit.framework in Frameworks */ = {isa = PBXBuildFile; fileRef = 90B9877B2A579F9500FC95D6 /* WebKit.framework */; settings = {ATTRIBUTES = (Weak, ); }; };
7879
/* End PBXBuildFile section */
7980

@@ -423,6 +424,7 @@
423424
90AF83B62AA15C660046DA2E /* nv_cmds.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; name = nv_cmds.h; path = ../nv_cmds.h; sourceTree = "<group>"; };
424425
90AF83B72AA15C660046DA2E /* vim9cmds.c */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.c; name = vim9cmds.c; path = ../vim9cmds.c; sourceTree = "<group>"; };
425426
90AF83B82AA15C660046DA2E /* termdefs.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; name = termdefs.h; path = ../termdefs.h; sourceTree = "<group>"; };
427+
90AF83A92A8C37F70046DA2E /* Security.framework */ = {isa = PBXFileReference; lastKnownFileType = wrapper.framework; name = Security.framework; path = System/Library/Frameworks/Security.framework; sourceTree = SDKROOT; };
426428
90B9877B2A579F9500FC95D6 /* WebKit.framework */ = {isa = PBXFileReference; lastKnownFileType = wrapper.framework; name = WebKit.framework; path = System/Library/Frameworks/WebKit.framework; sourceTree = SDKROOT; };
427429
90F84F1E2521F2270000268B /* ko */ = {isa = PBXFileReference; lastKnownFileType = text.plist.strings; name = ko; path = ko.lproj/MainMenu.strings; sourceTree = "<group>"; };
428430
90F84F232521F6480000268B /* ca */ = {isa = PBXFileReference; lastKnownFileType = text.plist.strings; name = ca; path = ca.lproj/MainMenu.strings; sourceTree = "<group>"; };
@@ -462,6 +464,7 @@
462464
isa = PBXFrameworksBuildPhase;
463465
buildActionMask = 2147483647;
464466
files = (
467+
90AF83AB2A8C37F70046DA2E /* Security.framework in Frameworks */,
465468
90B9877D2A579F9500FC95D6 /* WebKit.framework in Frameworks */,
466469
1DFE25A50C527BC4003000F7 /* PSMTabBarControl.framework in Frameworks */,
467470
8D11072F0486CEB800E47090 /* Cocoa.framework in Frameworks */,
@@ -650,6 +653,7 @@
650653
29B97323FDCFA39411CA2CEA /* Frameworks */ = {
651654
isa = PBXGroup;
652655
children = (
656+
90AF83A92A8C37F70046DA2E /* Security.framework */,
653657
90B9877B2A579F9500FC95D6 /* WebKit.framework */,
654658
52A364721C4A5789005757EC /* Sparkle.framework */,
655659
1D8B5A52104AF9FF002E59D5 /* Carbon.framework */,

0 commit comments

Comments
 (0)