Closed Bug 968002 Opened 6 years ago Closed 6 years ago
What happens if a content process sends Add
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
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.
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+
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.
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?
(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.