1344453 - [BUG] [Windows] [e10s] Unable to open attachments from Windows Mail trusted store app in Firefox

Status: RESOLVED FIXED

(Core :: Security: Process Sandboxing, defect)

Description

[michal256]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:51.0) Gecko/20100101 Firefox/51.0
Build ID: 20170125094131

Steps to reproduce:

Since the implementation of e10s with Windows sandbox (content process running on low integrity level) I have noticed issues opening attachments from Windows Mail store app (pre-installed with Win10). This issue is not present when using firefox with e10s disabled.

Scenario:
Set up Windows 10 Mail app to receive e-mails. Set up Firefox as your default web browser. Make sure e10s is enabled.
Receive an e-mail with *.htm or *.html file attached.
Try opening it directly from Mail app into your default browser (Firefox)


Actual results:

Firefox displays Access to the file was denied error page.


Expected results:

Attached html file is displayed in Firefox

Comment 2

[Liz Henry (:lizzard) (relman/hg->git project)]

This seems likely to be a common scenario. 

Tracking for 53. Time is a little tight if you want to get a patch into 53 so please ask for uplift (and testing/verification) if you land a patch.

Comment 3

[Bob Owen (:bobowen) (PTO until 17th Oct)]

Attachment: Part 1: Allow a special all paths rule in the Windows process sandbox when using semantics FILES_ALLOW_READONLY

This also changes the read only related tests in filesystem_interception.cc to checking for NT_SUCCESS instead of just not STATUS_ACCESS_DENIED.
This is what the registry_interception.cc tests do and in some cases we can get a status of 0xC0000201 (STATUS_NETWORK_OPEN_RESTRICTION), which gets returned and we never ask the broker.

Comment 4

[Bob Owen (:bobowen) (PTO until 17th Oct)]

Attachment: Part 2: Add FILES_ALLOW_READONLY rule to all paths when Windows child process should have full read access

Comment 5

[Bob Owen (:bobowen) (PTO until 17th Oct)]

e10s try push with those patches:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2113f47112108e2b91e37c2f0b4dfee852b8b70d

Comment 6

[Bob Owen (:bobowen) (PTO until 17th Oct)]

Attachment: Part 1: Allow a special all paths rule in the Windows process sandbox when using semantics FILES_ALLOW_READONLY

This also changes the read only related status checks in filesystem_interception.cc to include STATUS_NETWORK_OPEN_RESTRICTION (0xC0000201), which gets returned in some cases and fails because we never ask the broker.

Comment 7

[Bob Owen (:bobowen) (PTO until 17th Oct)]

New try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4355ea9816f807a306423dc2d513bebbc33e2808

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1755a454e2de
Part 1: Allow a special all paths rule in the Windows process sandbox when using semantics FILES_ALLOW_READONLY. r=jimm
https://hg.mozilla.org/integration/mozilla-inbound/rev/1ab46092ef65
Part 2: Add FILES_ALLOW_READONLY rule to all paths when Windows child process should have full read access. r=jimm

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1755a454e2de
https://hg.mozilla.org/mozilla-central/rev/1ab46092ef65

Comment 10

[Bob Owen (:bobowen) (PTO until 17th Oct)]

Comment on attachment 8851711 [details] [diff] [review]
Part 1: Allow a special all paths rule in the Windows process sandbox when using semantics FILES_ALLOW_READONLY

Approval Request Comment
[Feature/Bug causing the regression]:
Windows low integrity content process sandbox.

[User impact if declined]:
Users will not be able to view html page attachment from the Windows 10 Mail app.
This change also fixes bug 1336703.

[Is this code covered by automated tests?]:
The sandbox itself is on when the tests run.

[Has the fix been verified in Nightly?]:
Verified on local build.

[Needs manual test from QE? If yes, steps to reproduce]: 
It would be good to get QE for this. STR are in the Description.

[List of other uplifts needed for the feature/fix]:
Other patch from this bug.

[Is the change risky?]:
No.

[Why is the change risky/not risky?]:
Small change that adds a sandbox rule to allow read access to all file paths when the content process should have read access anyway, but doesn't in some edge cases.

[String changes made/needed]:
None.

Comment 11

[Bob Owen (:bobowen) (PTO until 17th Oct)]

Comment on attachment 8851596 [details] [diff] [review]
Part 2: Add FILES_ALLOW_READONLY rule to all paths when Windows child process should have full read access

Approval Request Comment
[Feature/Bug causing the regression]:
[User impact if declined]:
[Is this code covered by automated tests?]:
[Has the fix been verified in Nightly?]:
[Needs manual test from QE? If yes, steps to reproduce]: 
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]:
[Why is the change risky/not risky?]:
[String changes made/needed]:

Approval Request Comment
[Feature/Bug causing the regression]:
[User impact if declined]:
[Is this code covered by automated tests?]:
[Has the fix been verified in Nightly?]:
[Needs manual test from QE? If yes, steps to reproduce]: 
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]:
[Why is the change risky/not risky?]:
[String changes made/needed]:

See comment 10.

Comment 12

[Bob Owen (:bobowen) (PTO until 17th Oct)]

Sheriff uplift note:

The file security/sandbox/modifications-to-chromium-to-reapply-after-upstream-merge.txt will fail to merge, but this can be safely ignored because it is just notes for upstream updates and not part of the build.

I also notice that I forgot to remove the uplift comment template in comment 11, doh!

Comment 13

[Julien Cristau [:jcristau]]

Flagging for manual verification, since this wants to go in late beta.

Comment 14

[Julien Cristau [:jcristau]]

Comment on attachment 8851711 [details] [diff] [review]
Part 1: Allow a special all paths rule in the Windows process sandbox when using semantics FILES_ALLOW_READONLY

fix a sandboxing issue in aurora54.

I'll let Liz make the call for beta.

Comment 15

[roxleitan]

Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:55.0) Gecko/20100101 Firefox/55.0
Build ID: 20170330030213

Verified fixed using the latest Nightly 55.0a1(Build ID:20170330030213) on Windows 10 x64.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/15291af800d6
https://hg.mozilla.org/releases/mozilla-aurora/rev/412e3fae6b61

Comment 17

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8851596 [details] [diff] [review]
Part 2: Add FILES_ALLOW_READONLY rule to all paths when Windows child process should have full read access

Seems important to fix this for 53, let's get it onto beta 9.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/cfb4a743d657
https://hg.mozilla.org/releases/mozilla-beta/rev/85c59b19c241

Comment 19

[Bogdan Maris, Desktop Test Engineering]

Reproduced the initial issue using Fx 51.0.1, verified as fixed on Firefox 53 beta 9 using Windows 10.