Closed
Bug 1321724
Opened 8 years ago
Closed 7 years ago
[e10s] Local HTML cannot be opened in Firefox 50
Categories
(Core :: Security: Process Sandboxing, defect)
Tracking
()
RESOLVED
FIXED
mozilla54
People
(Reporter: mesmackgod, Assigned: bobowen)
References
Details
(Keywords: multiprocess, regression, Whiteboard: sbwc2)
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
jimm
:
review+
jcristau
:
approval-mozilla-aurora+
jcristau
:
approval-mozilla-beta+
gchang
:
approval-mozilla-release-
|
Details | Diff | Splinter Review |
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•8 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•8 years ago
|
OS: Unspecified → Windows 7
Hardware: Unspecified → x86_64
Comment 2•8 years ago
|
||
[Tracking Requested - why for this release]: Regression
Blocks: e10s
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:
--- → ?
Summary: Local HTML cannot be opened in Firefox 50 → [e10s] Local HTML cannot be opened in Firefox 50
Updated•8 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment 3•8 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•8 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•8 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•8 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/
Comment 7•8 years ago
|
||
(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•7 years ago
|
||
Sounds sandboxing related.
Flags: needinfo?(jmathies) → needinfo?(bobowencode)
Whiteboard: sb?
Updated•7 years ago
|
Component: Untriaged → Security: Process Sandboxing
Product: Firefox → Core
Assignee | ||
Comment 9•7 years ago
|
||
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•7 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)
Assignee | ||
Comment 11•7 years ago
|
||
Thanks, so it looks like the issue might be with Active Directory Users, I'll try and reproduce.
Updated•7 years ago
|
Reporter | ||
Comment 12•7 years ago
|
||
Why wontfix? This was a feature that was working perfectly until this releas?
Comment 13•7 years ago
|
||
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 :)
Assignee | ||
Comment 14•7 years ago
|
||
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•7 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)
Assignee | ||
Comment 16•7 years ago
|
||
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)
Assignee | ||
Comment 17•7 years ago
|
||
(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•7 years ago
|
||
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?
Assignee | ||
Comment 19•7 years ago
|
||
(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•7 years ago
|
||
We use the 14393 Win 10 SDK in automation as well.
Updated•7 years ago
|
Whiteboard: sb? → sbwc2
Comment 21•7 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+
Comment 22•7 years ago
|
||
Too late for 51 at this point, would still welcome this fix for 52 before we ship the ESR.
status-firefox54:
--- → affected
Assignee | ||
Comment 23•7 years ago
|
||
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)
Assignee | ||
Updated•7 years ago
|
Attachment #8827110 -
Attachment is obsolete: true
Attachment #8827110 -
Flags: feedback?(davidp99)
Attachment #8827110 -
Flags: feedback?(aklotz)
Updated•7 years ago
|
Attachment #8834005 -
Flags: review?(jmathies) → review+
Assignee | ||
Comment 24•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c1dea4dbc537487d16d41a19dfd55087dbd0f7a2
Comment 25•7 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•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/0e6bf137521e
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Comment 27•7 years ago
|
||
Let's have QE verify this before moving forward with uplift requests.
Flags: qe-verify+
Keywords: regressionwindow-wanted
Assignee | ||
Comment 28•7 years ago
|
||
(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)
Comment 29•7 years ago
|
||
OK, that'll work I guess. Please go ahead and nominate then :)
Flags: needinfo?(ryanvm)
Assignee | ||
Comment 30•7 years ago
|
||
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?
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Comment 31•7 years ago
|
||
Carrying r=jimm from original changeset: https://hg.mozilla.org/mozilla-central/rev/0e6bf137521e MozReview-Commit-ID: I2JU5FHUODZ
Assignee | ||
Updated•7 years ago
|
Attachment #8834005 -
Attachment is obsolete: true
Attachment #8834005 -
Flags: approval-mozilla-beta?
Attachment #8834005 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 32•7 years ago
|
||
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?
Assignee | ||
Comment 33•7 years ago
|
||
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 34•7 years ago
|
||
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•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/7ce21ea51bc3
Comment 36•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/eb7b524ddced
Comment 37•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-esr52/rev/eb7b524ddced
status-firefox-esr52:
--- → fixed
Assignee | ||
Comment 38•7 years ago
|
||
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?
Comment 39•7 years ago
|
||
The merge is failing with: warning: conflicts while merging security/sandbox/modifications-to-chromium-to-reapply-after-upstream-merge.txt!
Flags: needinfo?(bobowencode)
Comment 40•7 years ago
|
||
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-
Comment 41•7 years ago
|
||
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)
Updated•7 years ago
|
tracking-firefox50:
? → ---
Flags: needinfo?(bobowencode)
Comment 42•7 years ago
|
||
Since there are no reliable STR for this issue, I'm removing the qe-verify+ flag.
Flags: qe-verify+
Assignee | ||
Updated•6 years ago
|
Flags: needinfo?(mesmackgod)
You need to log in
before you can comment on or make changes to this bug.
Description
•