Closed Bug 1344453 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: Security: Process Sandboxing, defect)

51 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox52 --- wontfix
firefox53 + verified
firefox54 + fixed
firefox55 --- verified

People

(Reporter: michal256, Assigned: bobowen)

References

Details

(Whiteboard: sbwc2)

Attachments

(2 files, 1 obsolete file)

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
Component: Untriaged → Security: Process Sandboxing
Product: Firefox → Core
Duplicate of this bug: 1344781
Whiteboard: sbwc2
Assignee: nobody → bobowencode
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
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.
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.
Attachment #8851595 - Flags: review?(jmathies)
Attachment #8851595 - Flags: review?(jmathies) → review+
Attachment #8851596 - Flags: review?(jmathies) → review+
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.
Attachment #8851711 - Flags: review?(jmathies)
Attachment #8851595 - Attachment is obsolete: true
Attachment #8851711 - Flags: review?(jmathies) → review+
Pushed by bobowencode@gmail.com:
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 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.
Attachment #8851711 - Flags: approval-mozilla-beta?
Attachment #8851711 - Flags: approval-mozilla-aurora?
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.
Attachment #8851596 - Flags: approval-mozilla-beta?
Attachment #8851596 - Flags: approval-mozilla-aurora?
Blocks: 1336703
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!
Flagging for manual verification, since this wants to go in late beta.
Flags: qe-verify+
Flags: needinfo?(brindusa.tot)
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.
Attachment #8851711 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8851596 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Flags: needinfo?(brindusa.tot)
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.
Attachment #8851596 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8851711 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Reproduced the initial issue using Fx 51.0.1, verified as fixed on Firefox 53 beta 9 using Windows 10.
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.