Bootstrap Sandbox: Ensure swap_integer messages are read-only.

sandbox/mac/launchd_interception_server.cc

Index: sandbox/mac/launchd_interception_server.cc
diff --git a/sandbox/mac/launchd_interception_server.cc b/sandbox/mac/launchd_interception_server.cc
index 73e0d6720b862cdfffb0cd8028dc410e81fd5977..6c20181124eb9aa99aef06b243a3e696fb348d4e 100644
--- a/sandbox/mac/launchd_interception_server.cc
+++ b/sandbox/mac/launchd_interception_server.cc
@@ -231,10 +231,16 @@ void LaunchdInterceptionServer::HandleLookUp(mach_msg_header_t* request,
 void LaunchdInterceptionServer::HandleSwapInteger(mach_msg_header_t* request,
                                                   mach_msg_header_t* reply,
                                                   pid_t sender_pid) {
-  // TODO(rsesek): Crack the message and ensure that the swap is only being
-  // used to get the value of a VPROC key, and do not allow setting it.
-  VLOG(2) << "Forwarding vproc swap message #" << request->msgh_id;
-  ForwardMessage(request, reply);
+  // Only allow getting information out of launchd. Do not allow setting
+  // values. Two commonly observed values that are retrieved are
+  // VPROC_GSK_MGR_PID and VPROC_GSK_TRANSACTIONS_ENABLED.

[Mark Mentovai, 2014/05/15 22:36:56]

Just out of curiosity, what framework is requesting these?

[Robert Sesek, 2014/05/15 22:54:17]

I don't know why CFNotificationCenter needs to know launchd's pid, and the
sudden termination stuff is done using transactions/assertions.

MGR_PID:

* thread #1: tid = 0x190466, 0x92adcf7a libsystem_kernel.dylib`mach_msg_trap +
10, queue = 'com.apple.main-thread, stop reason = signal SIGSTOP
    frame #0: 0x92adcf7a libsystem_kernel.dylib`mach_msg_trap + 10
    frame #1: 0x92adc16c libsystem_kernel.dylib`mach_msg + 68
    frame #2: 0x9476bff6
liblaunch.dylib`___lldb_unnamed_function16$$liblaunch.dylib + 137
    frame #3: 0x94768666 liblaunch.dylib`vproc_swap_integer + 123
    frame #4: 0x94f3617d CoreFoundation`__CFXNotificationCenterSetupConnection +
109
    frame #5: 0x94f35f63 CoreFoundation`__CFXNotificationCenterCreate + 355
    frame #6: 0x94f35df4
CoreFoundation`__CFNotificationCenterGetDistributedCenter_block_invoke + 36
    frame #7: 0x9a6fe329 libdispatch.dylib`dispatch_once_f + 150
    frame #8: 0x9a6ff1cd libdispatch.dylib`dispatch_once + 31
    frame #9: 0x94f69abf CoreFoundation`CFNotificationCenterGetDistributedCenter
+ 95
    frame #10: 0x94f699de CoreFoundation`__71+[CFPrefsSource
withSourceForIdentifier:user:byHost:container:perform:]_block_invoke + 30
    frame #11: 0x9a6fe329 libdispatch.dylib`dispatch_once_f + 150
    frame #12: 0x9a6ff1cd libdispatch.dylib`dispatch_once + 31
    frame #13: 0x94f697e2 CoreFoundation`+[CFPrefsSource
withSourceForIdentifier:user:byHost:container:perform:] + 546
    frame #14: 0x94f695b6 CoreFoundation`-[CFPrefsSearchListSource
addSourceForIdentifier:user:byHost:] + 134
    frame #15: 0x94f63dcc CoreFoundation`+[CFPrefsSearchListSource
withSearchListForIdentifier:perform:] + 364
    frame #16: 0x94f63c26 CoreFoundation`CFPreferencesCopyAppValue + 166
    frame #17: 0x99909e57 Foundation`-[NSUserDefaults(NSUserDefaults) init] +
348
    frame #18: 0x99909552 Foundation`+[NSUserDefaults(NSUserDefaults)
standardUserDefaults] + 93
    frame #19: 0x9b36a0e3 AppKit`+[NSApplication initialize] + 104
    frame #20: 0x98b5ceb6 libobjc.A.dylib`_class_initialize + 526
    frame #21: 0x98b5ccdb libobjc.A.dylib`_class_initialize + 51
    frame #22: 0x98b62de1 libobjc.A.dylib`lookUpImpOrForward + 109
    frame #23: 0x98b5cca3 libobjc.A.dylib`_class_lookupMethodAndLoadCache3 + 55

TRANSACTIONS_ENABLED:

* thread #1: tid = 0x190466, 0x92adcf7a libsystem_kernel.dylib`mach_msg_trap +
10, queue = 'com.apple.main-thread, stop reason = signal SIGSTOP
    frame #0: 0x92adcf7a libsystem_kernel.dylib`mach_msg_trap + 10
    frame #1: 0x92adc230 libsystem_kernel.dylib`mach_msg + 264
    frame #2: 0x9476bff6
liblaunch.dylib`___lldb_unnamed_function16$$liblaunch.dylib + 137
    frame #3: 0x94768666 liblaunch.dylib`vproc_swap_integer + 123
    frame #4: 0x9476e17f
liblaunch.dylib`___lldb_unnamed_function38$$liblaunch.dylib + 82
    frame #5: 0x9a6fe329 libdispatch.dylib`dispatch_once_f + 150
    frame #6: 0x94768775 liblaunch.dylib`_vproc_transaction_begin + 133
    frame #7: 0x94f5d30a CoreFoundation`_CFSuddenTerminationDisable + 122
    frame #8: 0x94f3893f CoreFoundation`_CFXNotificationRegisterObserver + 1279
    frame #9: 0x94f69b98 CoreFoundation`CFNotificationCenterAddObserver + 200
    frame #10: 0x94f69a14 CoreFoundation`__71+[CFPrefsSource
withSourceForIdentifier:user:byHost:container:perform:]_block_invoke + 84
    frame #11: 0x9a6fe329 libdispatch.dylib`dispatch_once_f + 150
    frame #12: 0x9a6ff1cd libdispatch.dylib`dispatch_once + 31
    frame #13: 0x94f697e2 CoreFoundation`+[CFPrefsSource
withSourceForIdentifier:user:byHost:container:perform:] + 546
    frame #14: 0x94f695b6 CoreFoundation`-[CFPrefsSearchListSource
addSourceForIdentifier:user:byHost:] + 134
    frame #15: 0x94f63dcc CoreFoundation`+[CFPrefsSearchListSource
withSearchListForIdentifier:perform:] + 364
    frame #16: 0x94f63c26 CoreFoundation`CFPreferencesCopyAppValue + 166
    frame #17: 0x99909e57 Foundation`-[NSUserDefaults(NSUserDefaults) init] +
348
    frame #18: 0x99909552 Foundation`+[NSUserDefaults(NSUserDefaults)
standardUserDefaults] + 93
    frame #19: 0x9b36a0e3 AppKit`+[NSApplication initialize] + 104
    frame #20: 0x98b5ceb6 libobjc.A.dylib`_class_initialize + 526
    frame #21: 0x98b5ccdb libobjc.A.dylib`_class_initialize + 51
    frame #22: 0x98b62de1 libobjc.A.dylib`lookUpImpOrForward + 109
    frame #23: 0x98b5cca3 libobjc.A.dylib`_class_lookupMethodAndLoadCache3 + 55

+  if (compat_shim_.swap_integer_is_get_only(request)) {
+    VLOG(2) << "Forwarding vproc swap_integer message.";
+    ForwardMessage(request, reply);

[Mark Mentovai, 2014/05/15 22:36:56]

FYI:

Some things that launchd’s job_mig_swap_integer does even for “get” look a
little shady. VPROC_GSK_GLOBAL_LOG_MASK and VPROC_GSK_GLOBAL_UMASK both set
process-wide state momentarily before dropping back to the original value. This
process-wide state can affect things launchd is doing on other threads.

[Robert Sesek, 2014/05/15 22:54:17]

Right, but since this doesn't deal in any attacker-supplied data, it doesn't
concern me all that much.

+  } else {
+    VLOG(2) << "Rejecting non-read-only swap_integer message.";
+    RejectMessage(request, reply, BOOTSTRAP_NOT_PRIVILEGED);
+  }
 }
 
 void LaunchdInterceptionServer::SendReply(mach_msg_header_t* reply) {