Bootstrap Sandbox: Ensure swap_integer messages are read-only.

Bootstrap Sandbox: Ensure swap_integer messages are read-only.

BUG=367863
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=270899

[Robert Sesek, 2014-05-15 22:21:06]

Even though this message struct stayed the same, I decided to put it into
os_compat so that all that is encapsulated in a single file.

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

LGTM

https://codereview.chromium.org/284153005/diff/20001/sandbox/mac/launchd_inte...
File sandbox/mac/launchd_interception_server.cc (right):

https://codereview.chromium.org/284153005/diff/20001/sandbox/mac/launchd_inte...
sandbox/mac/launchd_interception_server.cc:236: // VPROC_GSK_MGR_PID and
VPROC_GSK_TRANSACTIONS_ENABLED.
Just out of curiosity, what framework is requesting these?

https://codereview.chromium.org/284153005/diff/20001/sandbox/mac/launchd_inte...
sandbox/mac/launchd_interception_server.cc:239: ForwardMessage(request, reply);
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.

https://codereview.chromium.org/284153005/diff/20001/sandbox/mac/os_compatibi...
File sandbox/mac/os_compatibility.cc (right):

https://codereview.chromium.org/284153005/diff/20001/sandbox/mac/os_compatibi...
sandbox/mac/os_compatibility.cc:97: return request->inkey == 0 && request->inval
== 0 && request->outkey != 0;
OK, although request->inval doesn’t matter if request->inkey is 0.

https://codereview.chromium.org/284153005/diff/20001/sandbox/mac/os_compatibi...
File sandbox/mac/os_compatibility.h (right):

https://codereview.chromium.org/284153005/diff/20001/sandbox/mac/os_compatibi...
sandbox/mac/os_compatibility.h:43: // swapping two kes.
Spelling nit: keys.

I don’t actually see in the current launchd source what’s getting swapped.

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

https://codereview.chromium.org/284153005/diff/20001/sandbox/mac/launchd_inte...
File sandbox/mac/launchd_interception_server.cc (right):

https://codereview.chromium.org/284153005/diff/20001/sandbox/mac/launchd_inte...
sandbox/mac/launchd_interception_server.cc:236: // VPROC_GSK_MGR_PID and
VPROC_GSK_TRANSACTIONS_ENABLED.
On 2014/05/15 22:36:56, Mark Mentovai wrote:
> Just out of curiosity, what framework is requesting these?

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

https://codereview.chromium.org/284153005/diff/20001/sandbox/mac/launchd_inte...
sandbox/mac/launchd_interception_server.cc:239: ForwardMessage(request, reply);
On 2014/05/15 22:36:56, Mark Mentovai wrote:
> 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.

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

https://codereview.chromium.org/284153005/diff/20001/sandbox/mac/os_compatibi...
File sandbox/mac/os_compatibility.cc (right):

https://codereview.chromium.org/284153005/diff/20001/sandbox/mac/os_compatibi...
sandbox/mac/os_compatibility.cc:97: return request->inkey == 0 && request->inval
== 0 && request->outkey != 0;
On 2014/05/15 22:36:56, Mark Mentovai wrote:
> OK, although request->inval doesn’t matter if request->inkey is 0.

Yes, I was being pedantic in case they accidentally don't check the right thing.

https://codereview.chromium.org/284153005/diff/20001/sandbox/mac/os_compatibi...
File sandbox/mac/os_compatibility.h (right):

https://codereview.chromium.org/284153005/diff/20001/sandbox/mac/os_compatibi...
sandbox/mac/os_compatibility.h:43: // swapping two kes.
On 2014/05/15 22:36:56, Mark Mentovai wrote:
> Spelling nit: keys.

Done.

> I don’t actually see in the current launchd source what’s getting swapped.

Right, "swapping" seems to just mean get-and-set in a single operation.

[Robert Sesek, 2014-05-15 22:54:33]

The CQ bit was checked by rsesek@chromium.org

[commit-bot: I haz the power, 2014-05-15 22:54:48]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rsesek@chromium.org/284153005/40001

[commit-bot: I haz the power, 2014-05-16 02:05:45]

Change committed as 270899