Closed Bug 1321724 Opened 3 years ago Closed 3 years ago

[e10s] Local HTML cannot be opened in Firefox 50

Categories

(Core :: Security: Process Sandboxing, defect, major)

50 Branch
x86_64
Windows 7
defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox-esr45 --- unaffected
firefox50 --- wontfix
firefox51 --- wontfix
firefox52 --- fixed
firefox-esr52 --- fixed
firefox53 --- fixed
firefox54 --- fixed

People

(Reporter: mesmackgod, Assigned: bobowen)

References

Details

(Keywords: multiprocess, regression, Whiteboard: sbwc2)

Attachments

(1 file, 2 obsolete files)

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.
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?
OS: Unspecified → Windows 7
Hardware: Unspecified → x86_64
[Tracking Requested - why for this release]: Regression
Blocks: e10s
Severity: normal → major
Has Regression Range: --- → no
Has STR: --- → yes
Summary: Local HTML cannot be opened in Firefox 50 → [e10s] Local HTML cannot be opened in Firefox 50
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
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.
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.
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)
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)
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.
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)
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)
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.
Whiteboard: sb? → sbwc2
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.
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)
Attachment #8834005 - Flags: review?(jmathies) → review+
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
https://hg.mozilla.org/mozilla-central/rev/0e6bf137521e
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Let's have QE verify this before moving forward with uplift requests.
Flags: qe-verify+
(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: → 1337413
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+
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)
Flags: needinfo?(bobowencode)
Since there are no reliable STR for this issue, I'm removing the qe-verify+ flag.
Flags: qe-verify+
Flags: needinfo?(mesmackgod)
You need to log in before you can comment on or make changes to this bug.