Closed Bug 1146874 Opened 9 years ago Closed 9 years ago

Windows process sandbox should check that the process has launched successfully.

Categories

(Core :: Security: Process Sandboxing, defect)

All
Windows 7
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla41
Tracking Status
firefox39 --- verified
firefox40 --- verified
firefox41 --- verified

People

(Reporter: bobowen, Assigned: bobowen)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

The sandbox broker should check that the process has launched successfully before attempting to resuming the process.
QA Contact: bobowen.code
Assignee: nobody → bobowen.code
QA Contact: bobowen.code
I have a fix for the sandboxBroker.cpp, but it seems that the rest of the content IPC set-up doesn't handle the failure very well and crashes elsewhere.

So, I need to find a fix for that as well.
This used to crash if SpawnTarget() failed because targetInfo isn't initialised and we attempted to close a bogus handle.

I've initialised it even though we're now checking the return value for SpawnTarget.
Attachment #8603357 - Flags: review?(tabraldes)
Once the sandbox broker is fixed EME seems to handle this OK now.
As in, the video fails to play, but Firefox doesn't crash.

All I've done here is add a bit of extra logging and change some of the current logging to better reflect what's going on.

I think we should get this and part 1 uplifted to 38.0.5, 39 and possibly esr38, while we're looking into fixing the child process start-up problems.

One thing I noticed was that it didn't appear to attempt to relaunch the child process when the video was retried, even in a new tab.
Attachment #8603362 - Flags: review?(cpearce)
In non-e10s mode - this at least stops the thumbnail content process from crashing Firefox if it fails to start.

In e10s mode - while Firefox doesn't crash, obviously no pages get rendered.
We'll need a follow-up to handle this scenario better, possibly by triggering the standard child crash behaviour.
The same possibly goes for the GMP case.
Attachment #8603364 - Flags: review?(wmccloskey)
Attachment #8603357 - Flags: review?(tabraldes) → review+
Attachment #8603364 - Flags: review?(wmccloskey) → review+
Attachment #8603362 - Flags: review?(cpearce) → review+
Comment on attachment 8603357 [details] [diff] [review]
Part 1: Check that Windows sandboxed process starts correctly.

Approval Request Comment
[Feature/regressing bug #]:
Since the sandbox broker landed, so bug 925571.
Wasn't used in release until bug 985252 for the GMP sandbox.

[User impact if declined]:
Any failure in the start-up / initialisation of a sandboxed child process will probably cause Firefox to crash.
This should only be for GMP processes outside of Nightly.
To be clear this bug doesn't address any potential sandboxed child start-up issues, it just tries to prevent them from totally crashing Firefox.

[Describe test coverage new/current, TreeHerder]:
GMP (for openh264 and EME) has mochitests.
Have done fairly simple manual tests with results as described in comment 3.
I caused the child process to fail by simply renaming plugin-container.exe.

[Risks and why]: 
low to medium: fairly small return value checking and logging changes, but it might cause other unforeseen issues as the child process has failed to start.
However, it won't crash Firefox.

[String/UUID change made/needed]:
None
Attachment #8603357 - Flags: approval-mozilla-beta?
Attachment #8603357 - Flags: approval-mozilla-aurora?
Comment on attachment 8603362 [details] [diff] [review]
Part 2: Add new nspr logging in GMPParent when process fails to launch.

See comment 8.

To be clear, I don't think it is worth uplifting Part 3, as no Windows content processes should be sandboxed outside of Nightly.
Attachment #8603362 - Flags: approval-mozilla-beta?
Attachment #8603362 - Flags: approval-mozilla-aurora?
Comment on attachment 8603357 [details] [diff] [review]
Part 1: Check that Windows sandboxed process starts correctly.

Approved for uplift to aurora and let's see how plugin stability is affected. We have a short beta cycle for 39, but could still uplift this for early beta next week.
Attachment #8603357 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Bob, I know you say unforseen problems, but do you think this might break by only breaking plugin content, like a video, or failing to render a whole page? What do you think we can do here for manual testing or verification for QE to try to break this?
Flags: qe-verify+
Flags: needinfo?(bobowen.code)
Comment on attachment 8603362 [details] [diff] [review]
Part 2: Add new nspr logging in GMPParent when process fails to launch.

Approved for uplift to aurora
Attachment #8603362 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(In reply to Liz Henry (:lizzard) from comment #11)
> Bob, I know you say unforseen problems, but do you think this might break by
> only breaking plugin content, like a video, or failing to render a whole
> page? What do you think we can do here for manual testing or verification
> for QE to try to break this?

It should only break the playing of the GMP content and only in a situation where we would have crashed completely before this fix.

The easiest way to test this is by temporarily renaming the plugin-container.exe file.
It lives in the same directory as firefox.exe (something like "C:\Program Files (x86)\Firefox Developer Edition\").
(Obviously, you can't use this trick with e10s turned on in Nightly. :-) )
This will cause the child process start-up to fail.
Before this fix Firefox will crash, afterwards just the video will fail.

I don't think that the fact that the child has failed to start is likely to cause further problems (other than for GMP).
However, Chris might be in a better situation to answer that.
(It's already the weekend in NZ, so he may not get back to us before Monday.)
Flags: needinfo?(bobowen.code) → needinfo?(cpearce)
Oh, one other thing.

If the thumbnail content process happens to kick-in (I think this can only happen on a new tab page) while you've got plugin-container.exe renamed, then that will cause Firefox to crash.

I don't think that happens very often though.
(In reply to Bob Owen (:bobowen) from comment #13)
> (In reply to Liz Henry (:lizzard) from comment #11)
> > Bob, I know you say unforseen problems, but do you think this might break by
> > only breaking plugin content, like a video, or failing to render a whole
> > page?

If this causes a regression, it should only break the stuff in <video>s feed by EME or OpenH264 plugins, I doubt it could possibly break much else. I think this is fairly low risk, and that we should uplift it to 39.
Flags: needinfo?(cpearce)
Blocks: 1165944
Blocks: 1165945
Comment on attachment 8603362 [details] [diff] [review]
Part 2: Add new nspr logging in GMPParent when process fails to launch.

Parts 1 and 2 approved for uplift to beta (39).
Attachment #8603362 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8603357 [details] [diff] [review]
Part 1: Check that Windows sandboxed process starts correctly.

Approved for uplift to beta (39) to help improve crash issues.
Attachment #8603357 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
I've tried reproducing this bug by modifying plugin-container.exe for FF 39 Aurora Build Id: 20150511004005 and tried playing h264 videos on Flash, for example http://www.flashvideofactory.com/test/demofullscreen345.html . 
But I never managed to make Firefox crash, the video is just not playing. Did I missed something? is there a more reliable way to reproduce the bug?
Flags: needinfo?(bobowen.code)
(In reply to Catalin Varga [QA][:VarCat] from comment #20)
> I've tried reproducing this bug by modifying plugin-container.exe for FF 39
> Aurora Build Id: 20150511004005 and tried playing h264 videos on Flash, for
> example http://www.flashvideofactory.com/test/demofullscreen345.html . 
> But I never managed to make Firefox crash, the video is just not playing.
> Did I missed something? is there a more reliable way to reproduce the bug?

Hi, we don't use our sandbox for Flash at the moment, so this bug didn't affect Flash.
(Although it's good to know that it doesn't crash Firefox when tested in this way.)

You need to start a Gecko Media Plugin (GMP) process, which for openh264 you can do via:

http://mozilla.github.io/webrtc-landing/pc_test.html
Ticking the "Require H.264 video" checkbox.

For EME I think you should be able to use:

http://people.mozilla.org/~cpearce/mse-clearkey/
Flags: needinfo?(bobowen.code)
Thanks a lot for the quick help.
I verified the bug using the following environment:

FF 39.0b1
Build Id: 20150520170903
Os: Win 7 x64, Win 8.1 x86
Verified fixed on latest Aurora 41.0a2 (buildID: 20150729004002) and Firefox 40 Beta 8 (buildID:  	20150727174134).
Status: RESOLVED → VERIFIED
Blocks: 1275430
You need to log in before you can comment on or make changes to this bug.