[e10s] Local HTML cannot be opened in Firefox 50

RESOLVED FIXED in Firefox 52

Status

()

Core
Security: Process Sandboxing
--
major
RESOLVED FIXED
2 years ago
4 months ago

People

(Reporter: mesmackgod, Assigned: bobowen)

Tracking

({multiprocess, regression})

50 Branch
mozilla54
x86_64
Windows 7
multiprocess, regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr45 unaffected, firefox50 wontfix, firefox51 wontfix, firefox52 fixed, firefox-esr52 fixed, firefox53 fixed, firefox54 fixed)

Details

(Whiteboard: sbwc2)

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

2 years ago
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:50.0) Gecko/20100101 Firefox/50.0
Build ID: 20161129173726

Steps to reproduce:

Attempted to open file:///C:/Users/<MyUID>/Downloads/time.html
Which i have written locally, as an amusing homepage. The contents of the file are irrelevant to the actual issue.

I can open the file in notepad, and edit it without UAC prompting. I am actually the owner of the file.



Actual results:

Firefox reports the file is inaccessible.


Expected results:

Firefox renders my atrocious blinky homepage.
(Reporter)

Comment 1

2 years ago
I found https://support.mozilla.org/en-US/questions/1147139 while looking for what happened between 49.0.2 (which was able to render my html) and 50, which claimed it could not read the file.
As suggested in that article, I attempted to surf down the file:///C:/ path, and was only able to get to C:/users before it failed. I then set browser.tabs.remote.autostart.2 to false, and restarted the browser. I have my horrible blinky homepage back. The article notes this as a issue with e10s?
(Reporter)

Updated

2 years ago
OS: Unspecified → Windows 7
Hardware: Unspecified → x86_64
[Tracking Requested - why for this release]: Regression
Blocks: 516752
Severity: normal → major
Has Regression Range: --- → no
Has STR: --- → yes
status-firefox50: --- → affected
status-firefox51: --- → ?
status-firefox52: --- → ?
status-firefox53: --- → ?
status-firefox-esr45: --- → unaffected
tracking-firefox50: --- → ?
Keywords: multiprocess, regression, regressionwindow-wanted
Summary: Local HTML cannot be opened in Firefox 50 → [e10s] Local HTML cannot be opened in Firefox 50

Comment 3

2 years ago
I tried to provide with the regression window wanted but I couldn't reproduce this issue, tested on Windows 7 32-bit and on Windows 10 64-bit using Firefox 50 Release nor on Firefox Nightly 50, also with e10s enable and disable.

If you still have the issue please create a new profile, you have the steps here:https://support.mozilla.org/en-US/kb/profile-manager-create-and-remove-firefox-profiles?redirectlocale=en-US&redirectslug=Managing-profiles#w_starting-the-profile-manager
(Reporter)

Comment 4

2 years ago
Updating the profile did nothing.
I uninstalled 50.0.2, and reinstalled 49.0.2. Path was accessible.
I reinstalled 50.0.2 via upgrade. Path is inaccessible.

Is there any way to enable extra debugging to determine exactly what permission it is looking for and failing to acquire in e10s? As stated previously, this is only an issue with browser.tabs.remote.autostart.2 set to true.
(Reporter)

Comment 5

2 years ago
Running Firefox 50 as Administrator still prevents it from rendering anything in that path. I have no idea what permission it could be attempting to read that the Administrator couldn't read there. As stated previously, that's my user folder, and I can browse and open everything in it without UAC prompts.

Comment 6

2 years ago
Considering the issue is reproducible on your end could you please try to narrow down the regression window for this particular issue?
http://mozilla.github.io/mozregression/
(In reply to mesmackgod from comment #4)
> Updating the profile did nothing.
> I uninstalled 50.0.2, and reinstalled 49.0.2. Path was accessible.
> I reinstalled 50.0.2 via upgrade. Path is inaccessible.
> 
> Is there any way to enable extra debugging to determine exactly what
> permission it is looking for and failing to acquire in e10s? As stated
> previously, this is only an issue with browser.tabs.remote.autostart.2 set
> to true.

Jim, can you help with this?
Flags: needinfo?(jmathies)

Comment 8

2 years ago
Sounds sandboxing related.
Flags: needinfo?(jmathies) → needinfo?(bobowencode)
Whiteboard: sb?
Component: Untriaged → Security: Process Sandboxing
Product: Firefox → Core
Thanks for taking the time to report this.

Could you start a command prompt, run the following and paste the results for each please (you can replace your user name in the results, if you're concerned about revealing that):

icacls C:\Users\<MyUID>\Downloads\time.html
icacls C:\Users\<MyUID>\Downloads
icacls C:\Users\<MyUID>
icacls C:\Users


Also, could you test with the following pref set and e10s enabled (just to confirm it is the sandbox):
  security.sandbox.content.level=0
Flags: needinfo?(bobowencode) → needinfo?(mesmackgod)
(Reporter)

Comment 10

2 years ago
Win 7 Pro x64
FF 50.1.0
browser.tabs.remote.autostart.2=true: File is inaccessible.
security.sandbox.content.level=0: File is accessible.

C:\Users\<MyUID>>icacls C:\Users\<MyUID>\Downloads\time.html
C:\Users\<MyUID>\Downloads\time.html NT AUTHORITY\SYSTEM:(I)(F)
                                    <AD Domain>\<MyUID>:(I)(F)
                                    BUILTIN\Administrators (built-in):(I)(F)

Successfully processed 1 files; Failed processing 0 files

C:\Users\<MyUID>>icacls C:\Users\<MyUID>\Downloads
C:\Users\<MyUID>\Downloads NT AUTHORITY\SYSTEM:(I)(OI)(CI)(F)
                          <AD Domain>\<MyUID>:(I)(OI)(CI)(F)
                          BUILTIN\Administrators (built-in):(I)(OI)(CI)(F)

Successfully processed 1 files; Failed processing 0 files

C:\Users\<MyUID>>icacls C:\Users\<MyUID>
C:\Users\<MyUID> NT AUTHORITY\SYSTEM:(OI)(CI)(F)
                <AD Domain>\<MyUID>:(OI)(CI)(F)
                BUILTIN\Administrators (built-in):(OI)(CI)(F)

Successfully processed 1 files; Failed processing 0 files

C:\Users\<MyUID>>icacls C:\Users\
C:\Users\ NT AUTHORITY\SYSTEM:(OI)(CI)(F)
          BUILTIN\Administrators (built-in):(OI)(CI)(F)
          BUILTIN\Users:(RX)
          BUILTIN\Users:(OI)(CI)(IO)(GR,GE)
          Everyone:(RX)
          Everyone:(OI)(CI)(IO)(GR,GE)

Successfully processed 1 files; Failed processing 0 files

C:\Users\<MyUID>>icacls C:\
C:\ BUILTIN\Administrators (built-in):(F)
    BUILTIN\Administrators (built-in):(OI)(CI)(IO)(F)
    NT AUTHORITY\SYSTEM:(F)
    NT AUTHORITY\SYSTEM:(OI)(CI)(IO)(F)
    BUILTIN\Users:(OI)(CI)(RX)
    NT AUTHORITY\Authenticated Users:(OI)(CI)(IO)(M)
    NT AUTHORITY\Authenticated Users:(AD)
    Mandatory Label\High Mandatory Level:(OI)(NP)(IO)(NW)

Successfully processed 1 files; Failed processing 0 files

C:\Users\<MyUID>>

When browsing via Firefox, with e10s on, and sandbox 1, I can get directory contents only as deep as file:///C:/Users/
Navigate any deeper, and I get "Access to the file was denied."

Not that it is probative, but downloading files to C:\Users\<MyUID>\Downloads\ from within FF works, even with e10s on, and sandbox 1
Flags: needinfo?(mesmackgod)
Thanks, so it looks like the issue might be with Active Directory Users, I'll try and reproduce.
status-firefox50: affected → wontfix
status-firefox51: ? → affected
status-firefox52: ? → affected
status-firefox53: ? → affected
(Reporter)

Comment 12

2 years ago
Why wontfix? This was a feature that was working perfectly until this releas?
Firefox 50 is already released and we're not going to ship an out-of-band to fix this issue. Note that future versions haven't been marked wontfix :)
Hi, would you mind testing the two Nightly Try builds linked below please (you might want to do a custom install if you already have Nightly).
I've not managed to reproduce, but I'm hoping that they both fix this problem.

If you run them with something like the following, then you can create a new profile for testing, to prevent any problems with your normal profile:

 firefox.exe -no-remote -P 


https://archive.mozilla.org/pub/firefox/try-builds/bobowencode@gmail.com-10b87d69fce9ddb6d683be4696eaf2705333dcc1/try-win32/firefox-53.0a1.en-US.win32.installer.exe

https://archive.mozilla.org/pub/firefox/try-builds/bobowencode@gmail.com-a0670168ea3c088771f5d311e9f7050f350dd89f/try-win32/firefox-53.0a1.en-US.win32.installer.exe
Assignee: nobody → bobowencode
Status: NEW → ASSIGNED
Flags: needinfo?(mesmackgod)
(Reporter)

Comment 15

2 years ago
Win 7 Pro x64
Nightly 53.0a1 (2017-01-06) (32-bit) (10b87d69fce9ddb6d683be4696eaf2705333dcc1)
browser.tabs.remote.autostart.2=true(default)
security.sandbox.content.level=1 (default)
File is accessible. 
However, a possibly unrelated bug:
file:///C:/Users/<MyUID>/Downloads, and every path back up to c:/ displays what should be an index of files but all the files are missing names and links until you mouse over, at which point you get a statusbar indicator that there is a link there, but still no name or link, only icons.

Nightly 53.0a1 (2017-01-06) (32-bit) (a0670168ea3c088771f5d311e9f7050f350dd89f)
browser.tabs.remote.autostart.2=true(default)
security.sandbox.content.level=1 (default)
File is accessible. 
Similar rendering issue with directory contents. It knows there are files, but it doesn't show them correctly.
As I told Bob, I am sorry it took me so long to complete this test. Life, ya know?
Flags: needinfo?(mesmackgod)
Created attachment 8827110 [details] [diff] [review]
Change USER_NON_ADMIN access token level from whitelist to blacklist containing Admin SIDs

This adds the ability to create a blacklist based on the existing code for the whitelist.

The list of Admin SIDs was just crudely compiled from winnt.h, are there any other important SIDs that people feel should be excluded?

Or alternatively, should we just go back to using USER_UNPROTECTED which uses the same access token as the parent?
In most set-ups, this will probably only affect people who are local Admins and have UAC turned off, because the built-in Administrators SID is normally already deny only otherwise.

While this currently affects the normal content process, soon this will just be used for the file content process.

MozReview-Commit-ID: 9cx2R6kMUwa
Attachment #8827110 - Flags: feedback?(jmathies)
Attachment #8827110 - Flags: feedback?(davidp99)
Attachment #8827110 - Flags: feedback?(aklotz)
(In reply to mesmackgod from comment #15)

Thanks again for testing these.

> File is accessible. 
> However, a possibly unrelated bug:
> file:///C:/Users/<MyUID>/Downloads, and every path back up to c:/ displays
> what should be an index of files but all the files are missing names and
> links until you mouse over, at which point you get a statusbar indicator
> that there is a link there, but still no name or link, only icons.

Just a note to say this was filed as bug 1331195, as it was still happening when sandbox was disabled, so appears to be a separate issue.
FYI, I'm getting compiler complaints about a few of the SIDs:

 0:16.32 c:/mozilla-src/mozilla-unified/security/sandbox/chromium/sandbox/win/src/restricted_token_utils.cc(58): error C2065: 'WinAccountEnterpriseKeyAdminsSid': undeclared identifier
 0:16.32 c:/mozilla-src/mozilla-unified/security/sandbox/chromium/sandbox/win/src/restricted_token_utils.cc(59): error C2065: 'WinAccountKeyAdminsSid': undeclared identifier
 0:16.32 c:/mozilla-src/mozilla-unified/security/sandbox/chromium/sandbox/win/src/restricted_token_utils.cc(63): error C2065: 'WinBuiltinStorageReplicaAdminsSid': undeclared identifier

I truly don't know how to access the SID list.  The SIDs that I could find descriptions for:
https://msdn.microsoft.com/en-us/library/windows/desktop/aa379650(v=vs.85).aspx
I'm not clear on how we determine which SIDs are relevant and which are not (and, I'm guessing, plenty are redundant).  There are so many.  FYI, if I just search for "Admin" in the SID list in the header file, I get your list (minus the 3 SIDs my winnt.h doesn't have).  Coincidence probably.

I'd also be concerned that a blacklist becomes out of date when a new problematic SID is introduced but the whitelist is ok as long as SID internal definitions don't change.

Do you know what the SID was that is causing this bug?  Would it fix the whitelist case if we set it up as we currently do, test it (open a file or whatever) and use USER_UNPROTECTED (or something else) in that case?
(In reply to David Parks (dparks) [:handyman] from comment #18)
> FYI, I'm getting compiler complaints about a few of the SIDs:
> 
>  0:16.32
> c:/mozilla-src/mozilla-unified/security/sandbox/chromium/sandbox/win/src/
> restricted_token_utils.cc(58): error C2065:
> 'WinAccountEnterpriseKeyAdminsSid': undeclared identifier
>  0:16.32
> c:/mozilla-src/mozilla-unified/security/sandbox/chromium/sandbox/win/src/
> restricted_token_utils.cc(59): error C2065: 'WinAccountKeyAdminsSid':
> undeclared identifier
>  0:16.32
> c:/mozilla-src/mozilla-unified/security/sandbox/chromium/sandbox/win/src/
> restricted_token_utils.cc(63): error C2065:
> 'WinBuiltinStorageReplicaAdminsSid': undeclared identifier

Mine is from version 10.0.14393.0, not sure what build servers use, but maybe I should remove them if they're going to cause problems in local builds.

> I'd also be concerned that a blacklist becomes out of date when a new
> problematic SID is introduced but the whitelist is ok as long as SID
> internal definitions don't change.

Yeah, this is a concern, but given the alternative of nothing I suppose it's OK.
I think in most of the cases just the following are the important ones.

WinBuiltinAdministratorsSid
WinLocalAccountAndAdministratorSid

These are the two that are set to Deny only if you are a local admin with UAC turned on (at least as far as I can tell on Win7 and Win10).
So, maybe I should just stick to those.

> Do you know what the SID was that is causing this bug?  Would it fix the
> whitelist case if we set it up as we currently do, test it (open a file or
> whatever) and use USER_UNPROTECTED (or something else) in that case?

I don't and in the general case it could be anything, including custom groups I suppose.
We'd have to try and test a known file, which might pass, but that wouldn't guarantee you could open everything you could before.
We use the 14393 Win 10 SDK in automation as well.

Updated

2 years ago
Whiteboard: sb? → sbwc2

Comment 21

2 years ago
Comment on attachment 8827110 [details] [diff] [review]
Change USER_NON_ADMIN access token level from whitelist to blacklist containing Admin SIDs

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

Sorry this took so long. I'd echo david's concern about future changes to WELL_KNOWN_SID_TYPE, but it sounds like that shouldn't be a major issue. I wonder if we could put in some sort of compile check on the size of that enum so that if it changes, we bail on the build?

::: security/sandbox/chromium/sandbox/win/src/restricted_token_utils.cc
@@ +60,5 @@
> +      deny_only_sids.push_back(WinAccountPolicyAdminsSid);
> +      deny_only_sids.push_back(WinBuiltinAdministratorsSid);
> +      deny_only_sids.push_back(WinBuiltinHyperVAdminsSid);
> +      deny_only_sids.push_back(WinBuiltinStorageReplicaAdminsSid);
> +      deny_only_sids.push_back(WinLocalAccountAndAdministratorSid);

lets make sure these are added in the same order as WELL_KNOWN_SID_TYPE. this list is currently out of sync with my sdk, any reason for that?
Attachment #8827110 - Flags: feedback?(jmathies) → feedback+
Too late for 51 at this point, would still welcome this fix for 52 before we ship the ESR.
status-firefox51: affected → wontfix
status-firefox54: --- → affected
Created attachment 8834005 [details] [diff] [review]
Change USER_NON_ADMIN access token level from whitelist to blacklist containing Admin SIDs

I've reduced this down to just then Admin SIDs from the Win8 SDK winnt.h.
I've also put them in the order of the file now.

I don't think it is worth adding some sort of static assert on the size of the enum as this might be different for valid build set-ups.

MozReview-Commit-ID: 9cx2R6kMUwa
Attachment #8834005 - Flags: review?(jmathies)
Attachment #8827110 - Attachment is obsolete: true
Attachment #8827110 - Flags: feedback?(davidp99)
Attachment #8827110 - Flags: feedback?(aklotz)

Updated

2 years ago
Attachment #8834005 - Flags: review?(jmathies) → review+

Comment 25

2 years ago
Pushed by bobowencode@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e6bf137521e
Change USER_NON_ADMIN access token level from whitelist to blacklist containing Admin SIDs. r=jimm

Comment 26

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0e6bf137521e
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox54: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Let's have QE verify this before moving forward with uplift requests.
Flags: qe-verify+
Keywords: regressionwindow-wanted
(In reply to Ryan VanderMeulen [:RyanVM] from comment #27)
> Let's have QE verify this before moving forward with uplift requests.

Unfortunately, we don't have STR for this issue.
Although the original reporter tested a try build with a very similar change (comment 15).
Flags: needinfo?(ryanvm)
OK, that'll work I guess. Please go ahead and nominate then :)
Flags: needinfo?(ryanvm)
Comment on attachment 8834005 [details] [diff] [review]
Change USER_NON_ADMIN access token level from whitelist to blacklist containing Admin SIDs

*Sheriff Uplift Note*
The changes to the following file will fail to apply on Beta, but this does not matter as the file only contains notes for development when taking an update from chromium code:
security/sandbox/modifications-to-chromium-to-reapply-after-upstream-merge.txt

Approval Request Comment
[Feature/Bug causing the regression]:
Windows Content Process Sandbox

[User impact if declined]:
Users with these particular access token set-ups will not be able to load file:// URL pages that rely on it.

[Is this code covered by automated tests?]:
From a regression point of view yes, because all the e10s tests on Windows run with the sandbox enabled.

[Has the fix been verified in Nightly?]:
Original poster verified a try build, which had essentially the same patch in it.

[Needs manual test from QE? If yes, steps to reproduce]: 
No, as we do not have STR.

[List of other uplifts needed for the feature/fix]:
None.

[Is the change risky?]:
Very slightly.

[Why is the change risky/not risky?]:
I've only said very slightly, because this change is to the sandboxing code. The actual change is straight forward and the policy setting it affects is only used by the content process sandbox.

[String changes made/needed]:
None.
Attachment #8834005 - Flags: approval-mozilla-beta?
Attachment #8834005 - Flags: approval-mozilla-aurora?
See Also: → bug 1337413
Created attachment 8836086 [details] [diff] [review]
Part 6: Re-apply - Change USER_NON_ADMIN access token level from whitelist to blacklist containing Admin SIDs

Carrying r=jimm from original changeset:
https://hg.mozilla.org/mozilla-central/rev/0e6bf137521e

MozReview-Commit-ID: I2JU5FHUODZ
Attachment #8834005 - Attachment is obsolete: true
Attachment #8834005 - Flags: approval-mozilla-beta?
Attachment #8834005 - Flags: approval-mozilla-aurora?
Comment on attachment 8834005 [details] [diff] [review]
Change USER_NON_ADMIN access token level from whitelist to blacklist containing Admin SIDs

(bzexport overwrote this by accident)

*Sheriff Uplift Note*
The changes to the following file will fail to apply on Beta, but this does not matter as the file only contains notes for development when taking an update from chromium code:
security/sandbox/modifications-to-chromium-to-reapply-after-upstream-merge.txt

Approval Request Comment
[Feature/Bug causing the regression]:
Windows Content Process Sandbox

[User impact if declined]:
Users with these particular access token set-ups will not be able to load file:// URL pages that rely on it.

[Is this code covered by automated tests?]:
From a regression point of view yes, because all the e10s tests on Windows run with the sandbox enabled.

[Has the fix been verified in Nightly?]:
Original poster verified a try build, which had essentially the same patch in it.

[Needs manual test from QE? If yes, steps to reproduce]: 
No, as we do not have STR.

[List of other uplifts needed for the feature/fix]:
None.

[Is the change risky?]:
Very slightly.

[Why is the change risky/not risky?]:
I've only said very slightly, because this change is to the sandboxing code. The actual change is straight forward and the policy setting it affects is only used by the content process sandbox.

[String changes made/needed]:
None.
Attachment #8834005 - Attachment is obsolete: false
Attachment #8834005 - Flags: approval-mozilla-beta?
Attachment #8834005 - Flags: approval-mozilla-aurora?
Comment on attachment 8836086 [details] [diff] [review]
Part 6: Re-apply - Change USER_NON_ADMIN access token level from whitelist to blacklist containing Admin SIDs

Uploaded incorrectly by bzexport.
Attachment #8836086 - Attachment is obsolete: true
Comment on attachment 8834005 [details] [diff] [review]
Change USER_NON_ADMIN access token level from whitelist to blacklist containing Admin SIDs

windows sandbox fix for aurora53 and beta52
Attachment #8834005 - Flags: approval-mozilla-beta?
Attachment #8834005 - Flags: approval-mozilla-beta+
Attachment #8834005 - Flags: approval-mozilla-aurora?
Attachment #8834005 - Flags: approval-mozilla-aurora+

Comment 35

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/7ce21ea51bc3
status-firefox53: affected → fixed

Comment 36

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/eb7b524ddced
status-firefox52: affected → fixed

Comment 37

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-esr52/rev/eb7b524ddced
status-firefox-esr52: --- → fixed
Blocks: 1337413
Comment on attachment 8834005 [details] [diff] [review]
Change USER_NON_ADMIN access token level from whitelist to blacklist containing Admin SIDs

In case we have to create a release for some other reason, we might want to consider uplifting this. 
It also fixes bug 1337413 (and possibly other reported printing bugs), where printing is being blocked by the sandbox for some people.

See comment 32 for uplift notes.

*Sheriff Uplift Note*
Again the changes to the following file will fail to apply on Release, but this does not matter as the file only contains notes for development when taking an update from chromium code:
security/sandbox/modifications-to-chromium-to-reapply-after-upstream-merge.txt
Attachment #8834005 - Flags: approval-mozilla-release?
The merge is failing with:
warning: conflicts while merging security/sandbox/modifications-to-chromium-to-reapply-after-upstream-merge.txt!
Flags: needinfo?(bobowencode)
Comment on attachment 8834005 [details] [diff] [review]
Change USER_NON_ADMIN access token level from whitelist to blacklist containing Admin SIDs

We don't have then plan to have dot release for 51. Rel51-.
Attachment #8834005 - Flags: approval-mozilla-release? → approval-mozilla-release-
Hello mesmackgod,

Could you please verify this fix also on Firefox 52.0b9 - https://archive.mozilla.org/pub/firefox/candidates/52.0b9-candidates/build2/ , since we don't have STR in order to confirm the fix.
Flags: needinfo?(mesmackgod)
tracking-firefox50: ? → ---
Flags: needinfo?(bobowencode)
Since there are no reliable STR for this issue, I'm removing the qe-verify+ flag.
Flags: qe-verify+
(Assignee)

Updated

4 months ago
Flags: needinfo?(mesmackgod)
You need to log in before you can comment on or make changes to this bug.