1465911 - Disable CPOWs outside of test mode

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Tom S [:evilpie]]

Nothing outside except tests should require CPOWs to work nowadays. CPOWs represent a huge IPC attack surface, which we should disable for normal builds.

Comment 1

[Tom S [:evilpie]]

We can't actually disable CPOWs completely, because we still use them as tokens to pass around from child->parent->child in the context menu. But what we can do is disable all the methods. That should drastically reduce the IPC surface. JavaScriptParent::allowMessage already has code that does this. I wonder if we could just rip of this out and replace it with xpc::IsInAutomation(). However it does seem nice to even disable some of this stuff in test mode when possible, so we should probably just keep it.

Comment 2

[Tom S [:evilpie]]

Attachment: Disable CPOWs outside of test mode

Comment 3

[Tom S [:evilpie]]

This code seems total convoluted to me. I feel like we should just always disallow "unsafe" CPOW usage and even remove the pref for that. Also maybe we can warn about "safe" CPOW usage, but this might cause some log spam, I don't know.

Comment 4

[Tom S [:evilpie]]

I think we also need to add something to WrapperAnswer::Recv* to prevent child->parent CPOW IPC calls.

Comment 5

[Tom S [:evilpie]]

For the WrapperAnswer case, should I use 
1. CrashIfNotInAutomation(), this would crash the parent even in release
2. if (!InAutomation()) return false, kills the child process
3. if (!InAutomation()) *rs = ReturnStatus(SomethingBad()), could be handled gracefully by throwing an exception in the child.

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Tom Schuster [:evilpie] from comment #5)
> For the WrapperAnswer case, should I use 
> 1. CrashIfNotInAutomation(), this would crash the parent even in release
> 2. if (!InAutomation()) return false, kills the child process
> 3. if (!InAutomation()) *rs = ReturnStatus(SomethingBad()), could be handled

My preference would be 2: I don't think it's worth killing the parent over this, but we should definitely be killing the child so the dev that would have introduced this use fixes it sooner rather than later.

Comment 7

[Tom S [:evilpie]]

Attachment: Disable CPOWs also when receiving message from the child. r?

Comment 8

[Tom S [:evilpie]]

Comment on attachment 8986769 [details] [diff] [review]
Disable CPOWs also when receiving message from the child. r?

Because this change is not test at all, I plan on landing this after the merge.

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Thanks for taking care of this!

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cfc300ef2cf9
Disable CPOWs outside of test mode. r=mrbkap
https://hg.mozilla.org/integration/mozilla-inbound/rev/c24b5f6944bb
Disable CPOWs also when receiving message from the child. r=mrbkap

Comment 11

[Kris Maglione [:kmag]]

\o/

Comment 12

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/cfc300ef2cf9
https://hg.mozilla.org/mozilla-central/rev/c24b5f6944bb