Closed Bug 968002 Opened 6 years ago Closed 6 years ago

What happens if a content process sends AddNewProcess?

Categories

(Core :: IPC, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30
blocking-b2g 1.3+
Tracking Status
firefox28 --- wontfix
firefox29 --- wontfix
firefox30 --- fixed
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed

People

(Reporter: jld, Assigned: jld)

References

Details

Attachments

(1 file, 1 obsolete file)

As I understand it, the AddNewProcess message of the PContent protocol is used by the Nuwa process after it forks a content process, to make the parent create a new ContentParent and connect it to the cloned protocols.

But we seem to not be checking that the message is sent by Nuwa.  I don't know what happens if a content process sends it, but there might be security implications.  I think there isn't a legitimate reason for that to happen, but I don't know this code very well.

It should be simple to check for IsNuwaProcess() in RecvAddNewProcess, if that's the case.
The patch is simple, and doesn't affect normal operation.

I don't have a test case that tries to send AddNewProcess from an untrusted child, and I still don't know what the potential security exposure is if this isn't fixed… but I don't see why that needs to block the fix.
Assignee: nobody → jld
Status: NEW → ASSIGNED
Attachment #8384838 - Flags: review?(bent.mozilla)
We're using Nuwa on 1.3T, so we'd want this patch there (but not 1.3, as I understand it).   The patch should be very low-risk, because nothing that isn't the Nuwa process calls SendAddNewProcess.
blocking-b2g: --- → 1.3T?
Nice catch! It's bad that a compromised content process can potentially crash the chrome process by sending the AddNewProcess() message with an empty file descriptor set. We should also add this check to ContentParent::OnNuwaReady() to check if Nuwa-related operations really come from the Nuwa process. Jed, are you going to update the patch to also check ContentParent::OnNuwaReady()?
(In reply to Cervantes Yu from comment #3)
> We should also add this check to ContentParent::OnNuwaReady() to
> check if Nuwa-related operations really come from the Nuwa process.

Good point; I'll update the patch.
Attachment #8384838 - Flags: review?(bent.mozilla)
Attachment #8384838 - Attachment is obsolete: true
Attachment #8385753 - Flags: review?(bent.mozilla)
Attachment #8385753 - Flags: feedback?(cyu)
Comment on attachment 8385753 [details] [diff] [review]
bug968002-restrict-addnew-hg1.diff

Review of attachment 8385753 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me.
Attachment #8385753 - Flags: feedback?(cyu) → feedback+
Attachment #8385753 - Flags: review?(bent.mozilla) → review+
blocking-b2g: 1.3T? → 1.3+
https://hg.mozilla.org/mozilla-central/rev/f5d49762e064
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Please request approval-mozilla-b2g28 on this patch for uplift to v1.3.
Flags: needinfo?(jld)
Comment on attachment 8385753 [details] [diff] [review]
bug968002-restrict-addnew-hg1.diff

NOTE: This flag is now for security issues only. Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): https://bugzil.la/nuwa
User impact if declined: If combined with a sec-critical bug, would allow a compromised content process to crash the entire phone, and privilege escalation hasn't been ruled out yet.
Testing completed: https://tbpl.mozilla.org/?tree=Try&rev=9abee2c0e334
Risk to taking this patch (and alternatives if risky): None; our code will never send these messages from a non-Nuwa process
String or UUID changes made by this patch: None.
Attachment #8385753 - Flags: approval-mozilla-b2g28?
Flags: needinfo?(jld)
(In reply to Jed Davis [:jld] from comment #10)
> Testing completed: https://tbpl.mozilla.org/?tree=Try&rev=9abee2c0e334

Ignore the Hf and V failures; they're not supported on Gecko 28 as far as I can tell, and I didn't think to exclude them from the try run.
Attachment #8385753 - Flags: approval-mozilla-b2g28? → approval-mozilla-b2g28+
You need to log in before you can comment on or make changes to this bug.