1321724 - [e10s] Local HTML cannot be opened in Firefox 50

Status: RESOLVED FIXED

(Core :: Security: Process Sandboxing, defect)

Description

[mesmackgod]

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.

Comment 1

[mesmackgod]

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?

Comment 2

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

[Tracking Requested - why for this release]: Regression

Comment 3

[Hani Yacoub, Desktop QA]

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

Comment 4

[mesmackgod]

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.

Comment 5

[mesmackgod]

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

[Hani Yacoub, Desktop QA]

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/

Comment 7

[Andrew Overholt [:overholt]]

(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?

Comment 8

[Jim Mathies [:jimm]]

Sounds sandboxing related.

Comment 9

[Bob Owen (:bobowen)]

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

Comment 10

[mesmackgod]

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

Comment 11

[Bob Owen (:bobowen)]

Thanks, so it looks like the issue might be with Active Directory Users, I'll try and reproduce.

Comment 12

[mesmackgod]

Why wontfix? This was a feature that was working perfectly until this releas?

Comment 13

[Ryan VanderMeulen [:RyanVM]]

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 :)

Comment 14

[Bob Owen (:bobowen)]

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/[e-mail]-10b87d69fce9ddb6d683be4696eaf2705333dcc1/try-win32/firefox-53.0a1.en-US.win32.installer.exe

https://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-a0670168ea3c088771f5d311e9f7050f350dd89f/try-win32/firefox-53.0a1.en-US.win32.installer.exe

Comment 15

[mesmackgod]

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?

Comment 16

[Bob Owen (:bobowen)]

Attachment: 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

Comment 17

[Bob Owen (:bobowen)]

(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.

Comment 18

[David Parks [:handyman]]

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?

Comment 19

[Bob Owen (:bobowen)]

(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.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

We use the 14393 Win 10 SDK in automation as well.

Comment 21

[Jim Mathies [:jimm]]

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?

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Too late for 51 at this point, would still welcome this fix for 52 before we ship the ESR.

Comment 23

[Bob Owen (:bobowen)]

Attachment: 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

Comment 24

[Bob Owen (:bobowen)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c1dea4dbc537487d16d41a19dfd55087dbd0f7a2

Comment 25

[Pulsebot]

Pushed by [e-mail]:
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

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

https://hg.mozilla.org/mozilla-central/rev/0e6bf137521e

Comment 27

[Ryan VanderMeulen [:RyanVM]]

Let's have QE verify this before moving forward with uplift requests.

Comment 28

[Bob Owen (:bobowen)]

(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).

Comment 29

[Ryan VanderMeulen [:RyanVM]]

OK, that'll work I guess. Please go ahead and nominate then :)

Comment 30

[Bob Owen (:bobowen)]

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.

Comment 31

[Bob Owen (:bobowen)]

Attachment: 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

Comment 32

[Bob Owen (:bobowen)]

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.

Comment 33

[Bob Owen (:bobowen)]

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.

Comment 34

[Julien Cristau [:jcristau]]

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

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7ce21ea51bc3

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/eb7b524ddced

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/eb7b524ddced

Comment 38

[Bob Owen (:bobowen)]

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

Comment 39

[Sylvestre Ledru [:Sylvestre]]

The merge is failing with:
warning: conflicts while merging security/sandbox/modifications-to-chromium-to-reapply-after-upstream-merge.txt!

Comment 40

[Gerry Chang [:gchang]]

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-.

Comment 41

[Mihai Boldan, Desktop QA [:mboldan]]

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.

Comment 42

[Mihai Boldan, Desktop QA [:mboldan]]

Since there are no reliable STR for this issue, I'm removing the qe-verify+ flag.