968002 - What happens if a content process sends AddNewProcess?

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

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.

Comment 1

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: bug968002-restrict-addnew-hg0.diff

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.

Comment 2

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

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.

Comment 3

[Cervantes Yu [:cyu] [:cervantes]]

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()?

Comment 4

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

(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 5

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: bug968002-restrict-addnew-hg1.diff

Comment 6

[Cervantes Yu [:cyu] [:cervantes]]

Comment on attachment 8385753 [details] [diff] [review]
bug968002-restrict-addnew-hg1.diff

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

Looks good to me.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f5d49762e064

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f5d49762e064

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Please request approval-mozilla-b2g28 on this patch for uplift to v1.3.

Comment 10

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

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.

Comment 11

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

(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.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/5ed82e82a72d