Closed Bug 1415160 Opened 2 years ago Closed 2 years ago

Enable various sandbox mitigation for the NPAPI plugin sandbox

Categories

(Core :: Security: Process Sandboxing, enhancement, P1)

All
Windows
enhancement

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: jimm, Assigned: handyman)

Details

(Whiteboard: sb+)

Attachments

(2 files, 2 obsolete files)

We have a few mitigations currently available we should experiment with enabling. Links to info on each of these in feature comparison spreadsheet. 

MITIGATION_STRICT_HANDLE_CHECKS
MITIGATION_DLL_SEARCH_ORDER
MITIGATION_HARDEN_TOKEN_IL_POLICY
MITIGATION_EXTENSION_POINT_DISABLE
MITIGATION_DYNAMIC_CODE_DISABLE
MITIGATION_DYNAMIC_CODE_DISABLE_WITH_OPT_OUT
MITIGATION_DYNAMIC_CODE_OPT_OUT_THIS_THREAD
MITIGATION_NONSYSTEM_FONT_DISABLE
MITIGATION_IMAGE_LOAD_NO_REMOTE
MITIGATION_IMAGE_LOAD_NO_LOW_LABEL
MITIGATION_IMAGE_LOAD_PREFER_SYS32
Assignee: nobody → davidp99
Priority: -- → P2
Whiteboard: sb+
Priority: P2 → P1
Attached patch Enable new mitigations (obsolete) — Splinter Review
I still need to experiment more with this in a VM (mainly for the no-remote-image mitigation) and with IME (for extension point mitigation and maybe the no-low-label mitigation).  But so far, these mitigations look good.

-----------------------

A few mitigations that didn't make the cut:

MITIGATION_STRICT_HANDLE_CHECKS:
Previous known cause of bug 1198368 comment 22.  Mitigation was removed in bug 1197943.  The Amazon Instant Video issue no longer exists and the crash-stats in the bug are so old they no longer exist -- the offending call stack is lost.  However, I have found additional failures for this mitigation.  For example, the display goes blank when using the file uploader here: http://www.tinywebgallery.com/en/tfu/web_demo1.php.

MITIGATION_DYNAMIC_CODE_DISABLE:
Fails on a number of sites, including: http://www.cartoonnetwork.com/video/adventuretime/evicted-episode.html

MITIGATION_IMAGE_LOAD_NO_LOW_LABEL
Blocks images with the "low mandatory" integrity level.
MITIGATION_IMAGE_LOAD_NO_REMOTE
Forbid loading images from "remote" devices.
Did not interfere with running from USB.  
With either of these mitigations (MITIGATION_IMAGE_LOAD_NO_LOW_LABEL and MITIGATION_IMAGE_LOAD_NO_REMOTE), it fails on everything when Firefox is run from a network drive.  E.g. bug 1360804 comment 0.

------------------------

Some basic explanations of the mitigations that _did_ get included:

MITIGATION_DLL_SEARCH_ORDER:
Sets the DLL search order to LOAD_LIBRARY_SEARCH_DEFAULT_DIRS

MITIGATION_HARDEN_TOKEN_IL_POLICY
Disallows impersonation of process token by other lower-integrity processes.

MITIGATION_EXTENSION_POINT_DISABLE
Blocks appinit, winsock lsps, global windows hooks (vs thread-targeted hooks) and "legacy" IME

MITIGATION_NONSYSTEM_FONT_DISABLE
GDI can only use system fonts

MITIGATION_IMAGE_LOAD_PREFER_SYS32
Prefers System32 folder over DLL search path, etc when looking for binaries to resolve forward exports in Import Address Table resolution.

--------------------------

Here are some of the places I looked / experiments I tried:

* Testing all pages on Firefox run from USB drive
* Testing all pages on Firefox run from networked drive
* Https test in bug 1360804 comment 0.
* Simple game: http://www.worldofmunchkin.com/game/demo.html
* Video: http://wwwns.akamai.com/hdnetwork/demo/flash/default.html
** This is known to fail in Nightly when exiting fullscreen in local builds (bug 1426731)
* File transfer: http://www.tinywebgallery.com/en/tfu/web_demo1.php
* Video and Fonts (has a font selector): http://www.cartoonnetwork.com/video/adventuretime/crystals-have-power-episode.html
* More video (select KimCartoon Beta as Server and JW Player as Player): http://kimcartoon.me/Cartoon/Justice-League-Action/Episode-45?id=78282
* More fonts (Click "View Running Example" at bottom of page): https://help.adobe.com/en_US/flex/using/WS2db454920e96a9e51e63e3d11c0bf6320a-7ffe.html
* Microphone and Settings window: https://www.testden.com/scripts/toeflibt/mictest.asp
** This demo worked but seems to be busted now on Nightly.  Chrome doesn't even acknowledge the pages' Flash contents.
* Camera: https://www.onlinemictest.com/webcam-test-in-adobe-flash/
* Video and sockets: https://tv.xfinity.com/recordings
** Works modulo bug 1425828

Again, this could still use testing in a VM and with an IME client.
(I should add that MITIGATION_DYNAMIC_CODE_OPT_OUT_THIS_THREAD is not a process mitigation -- it is a flag used with the MITIGATION_DYNAMIC_CODE_DISABLE_WITH_OPT_OUT mitigation but since we don't use it, they weren't included in this investigation.)
Hey Masayuki,

The details of this bug aren't relevant to you -- no need to bother reading them -- but I could use some help checking the stuff in this patch against IME in Flash.  I emailed you about this a while back because I'd done IME stuff with you before but I'm not even sure if this is your thing.  What I'm looking for is a way to check one (probably any) IME client that works with Flash to make sure that its not trashed by this sandbox code.  I'd expect the kind of failures I'm looking for to be major and obvious (like crashes or graphics failures) so I don't think I need to do anything fancy.  Is there a setup you can recommend that  I can use to test this myself?  Like an IME client that works with Flash and that I can use on some basic site?  I'm sure I won't be able to read the language but I'm hoping for a setup simple enough that I can fake it.
Flags: needinfo?(masayuki)
Oh, sorry, I missed to catch your email.

In Japan market, there are 3 important IMEs. One is MS-IME, this is default IME of Windows. You can install this when you add Japanese keyboard layout from language settings of OS.  Next is Google Japanese Input, this is free for anybody. You can download this from https://www.google.co.jp/intl/ja/ime/ (if you see Japanese language page, click "Windows 版をダウンロード". Last one is ATOK. However, this is proprietary software and needs subscription. So, if you create a test build, I can test it for you.

After you install IMEs, you can activate Japanese keyboard layout with Win+Space (Win8+). Then, you can toggle open/close state of IME with Alt+`. Then, when you open Japanese IME, type "ai" and hit spacebar a couple of times. Then, you should see a candidate window which is displayed by IME. The window should have some Kanji characters in the list. E.g., "愛", "逢", "藍", etc. If sandbox kills internal IPC between IME in the application process and outer conversion process, the candidate window must not be shown or must not have above Kanji characters. Then, type Enter to commit the highlighted word in the candidate list.
Flags: needinfo?(masayuki)
I forgot to introduce this link: http://emk.name/test/swftxt.html
You can test various version of Flash's editor in the test page.
Nit: A secure test page is available now: https://emk.name/test/swftxt.html
Comment on attachment 8938481 [details] [diff] [review]
Enable new mitigations

Thanks masayuki and emk -- that was easy.  MS and Google IMEs seem fine.  I'm just going to submit this and see what happens wrt other IMEs.  There is no need for additional testing, although if you see an issue with Flash and your IME in the coming days, you know what to blame.

Bob: I thought I had already had this reviewed but its pretty self explanatory.  See comment 1 for a list of stuff I tried to verify that it is working.

Tests:
 https://treeherder.mozilla.org/#/jobs?repo=try&revision=c555484317a449c26714a7d791256b1818710e30
Attachment #8938481 - Flags: review?(bobowencode)
Comment on attachment 8938481 [details] [diff] [review]
Enable new mitigations

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

Is it possible we could use the other IMAGE_LOAD mitigations when !sRunningFromNetworkDrive?
Perhaps a follow-up.

::: security/sandbox/win/src/sandboxbroker/sandboxBroker.cpp
@@ +766,5 @@
> +    sandbox::MITIGATION_IMAGE_LOAD_PREFER_SYS32;
> +
> +  result = mPolicy->SetProcessMitigations(mitigations);
> +  SANDBOX_ENSURE_SUCCESS(result,
> +                          "Invalid flags for SetProcessMitigations.");

nit: gained an extra space indent here.

@@ +772,5 @@
> +  sandbox::MitigationFlags delayedMitigations =
> +    sandbox::MITIGATION_DLL_SEARCH_ORDER;
> +
> +  result = mPolicy->SetDelayedProcessMitigations(delayedMitigations);
> +  MOZ_RELEASE_ASSERT(sandbox::SBOX_ALL_OK == result,

Can we use SANDBOX_ENSURE_SUCCESS here, please.
Generally we've decided we don't want to crash the browser when failing to start NPAPI, although this shouldn't ever actually fail. :-)
Attachment #8938481 - Flags: review?(bobowencode) → review+
Fixed nits.
Attachment #8938481 - Attachment is obsolete: true
Comment on attachment 8950832 [details] [diff] [review]
Part 1: Enable new mitigations (r=bobowen)

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

::: security/sandbox/win/src/sandboxbroker/sandboxBroker.cpp
@@ +789,5 @@
> +  sandbox::MitigationFlags delayedMitigations =
> +    sandbox::MITIGATION_DLL_SEARCH_ORDER;
> +
> +  result = mPolicy->SetDelayedProcessMitigations(delayedMitigations);
> +  SANDBOX_ENSURE_SUCCESS(sandbox::SBOX_ALL_OK == result,

This should be:
SANDBOX_ENSURE_SUCCESS(result,
Yeah, you caught me in the middle of fixing that (and more)... but treeherder went down so I couldn't get tests.  Now I can.

Tests:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bf4792a7a6308a6532f1a4bc5d1fbf5aca244665
Attachment #8950832 - Attachment is obsolete: true
You reminded me that I had already written and tested this patch.  Everything ran fine with the non-networked drive condition.  Looks like I never posted this when I never requested the review.
Attachment #8951016 - Flags: review?(bobowencode)
Attachment #8951014 - Flags: review+
Attachment #8951016 - Flags: review?(bobowencode) → review+
Keywords: checkin-needed
Pushed by apavel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e6fc425cf9b4
Part 1 - Enable new NPAPI Windows Process Mitigations; r=bobowen
https://hg.mozilla.org/integration/mozilla-inbound/rev/9659c9a29139
Part 2 - Add mitigations to plugin process if not running from network drive r=bobowen
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e6fc425cf9b4
https://hg.mozilla.org/mozilla-central/rev/9659c9a29139
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.