47 bytes, text/x-phabricator-request
|Details | Review|
3.07 KB, patch
|Details | Diff | Splinter Review|
Bug 1488439 fixed an issue with the new plugin sandbox where Comcast wouldn't allow logins due to a sandbox-caused change in the behavior of GetFileAttributesW. The fix was to use the DLL interceptor to intercept the calls and give Flash the information it expects. It turns out Windows 7 has a different sequence of machine instructions for the preamble of GetFileAttributesW that makes the DLL interceptor fail. This will be fixed by making ResolveRedirectedAddress navigate a near jump that targets a far jump.
David, does this login bug also affect Firefox 63 or 64 Beta? Restricting SIDs bug 1426733 landed in 64. But mozilla-release appears to have "dom.ipc.plugins.sandbox-level.flash" pref set to 3: https://hg.mozilla.org/releases/mozilla-release/file/tip/browser/app/profile/firefox.js#l995
status-firefox63: --- → ?
status-firefox64: --- → ?
status-firefox65: --- → affected
This bug is in 64 beta. It is not in 63. We never tied this issue to a sandbox level but we turn on the NPAPI sandbox behavior that caused this issue in bug 1426733. We jumped on this bug because the hope is to uplift this to beta ASAP. We'd like to get this out soon and this stuff seems to get no usage in nightly anyway. STR (same as bug 1488439): * Go to tv.xfinity.com * Enter valid credentials * When prompted, allow flash Expected: * The page shows a spinner and says "Adding Device", and a few seconds later the account home page (a poster of Thanos) comes up. Actual: * The page very briefly flashes a spinner, then returns to the "Allow Flash" page, only some layout is screwy. it stays here forever.
In Windows 7 x64, GetFileAttributesW begins with a short, backwards jump that can't safely be converted by the interceptor. Additionally, the function doesn't have enough NOP space after the JMP for the trampoline. However, the target of the short JMP is a long JMP, followed by plenty of NOP space. This patch moves the trampoline location from the first JMP to the second.
status-firefox63: ? → unaffected
status-firefox-esr60: --- → unaffected
This patch disables the restricting SIDs in the Win7 plugin sandbox, fixing this bug. I'm recommending this for beta uplift since the actual fix (in the patch for trunk) could potentially lead to some surprises if something else doesn't follow the normal MS DLL trampoline behavior. That one should ride the trains. Builds: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a2a53c10a0cea47e679203984d4ce541262cee6e
Comment on attachment 9025815 [details] [diff] [review] Disable restricting SIDs in Win7 NPAPI sandbox - For Beta Uplift [Beta/Release Uplift Approval Request] Feature/Bug causing the regression: Bug 1426733 User impact if declined: Some Flash plugin instances will behave incorrectly, causing e.g. Comcast video not to play Is this code covered by automated tests?: No Has the fix been verified in Nightly?: No Needs manual test from QE?: No If yes, steps to reproduce: List of other uplifts needed: None Risk to taking this patch: Low Why is the change risky/not risky? (and alternatives if risky): This change does two things: 1) It disables a feature of the plugin sandbox that strictly hardens it and 2) it avoids attempting to patch a Windows function that circumvents some problems introduced by the hardened sandbox. Both of these things just increase sandbox permissiveness. Also, #1 is what is already done in the current release (63) String changes made/needed: none
Attachment #9025815 - Flags: approval-mozilla-beta?
Comment on attachment 9025815 [details] [diff] [review] Disable restricting SIDs in Win7 NPAPI sandbox - For Beta Uplift fix a flash issue on win7, approved for 64.0b12
Attachment #9025815 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
status-firefox64: affected → fixed
Confirmed fixed and video can now be played (bug 1425828) starting with firefox-64.0b12_20181122182000 on 64-bit Windows 7. Also, as expected, confirmed to still be broken with firefox-65.0a1.en-US.win64_20181122220059 on 64-bit Win7.
Manual checkin of the trunk patch -- Lando permissions have stopped working (again).
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/6150c08114a8 Allow DLL patcher to resolve some backward short JMPs (r=aklotz)
Status: NEW → RESOLVED
Last Resolved: 3 months ago
status-firefox66: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
What do we need to do for Beta65 here, David?
Confirmed fixed (login and play video now working) in firefox-66.0a1.en-US.win64_20181226215140 on Windows 7. NOT fixed in firefox-65.0b6_20181220174318 on Windows 7 (latest available?).
Yes, this got stale and will need beta uplift. I was going to let it gel for a few days but there isn't really a need since it's probably not getting much exposure in trunk. So the patch "Allow DLL patcher to resolve some backward short JMPs" needs to be uplifted to beta 65. The patch that was uplifted to 64 is not needed here.
Comment on attachment 9023474 [details] Bug 1505482: Allow DLL patcher to resolve some backward short JMPs (r?aklotz!) [Beta/Release Uplift Approval Request] Feature/Bug causing the regression: Bug 1426733 User impact if declined: Some Flash plugin instances will behave incorrectly, causing e.g. Comcast video not to play Is this code covered by automated tests?: No Has the fix been verified in Nightly?: Yes Needs manual test from QE?: No If yes, steps to reproduce: List of other uplifts needed: None Risk to taking this patch: Low Why is the change risky/not risky? (and alternatives if risky): The patch only changes the DLL interceptor behavior in a circumstance where it would otherwise be guaranteed to fail. And failure, in either case, means the function is not intercepted (leading to the behavior in this bug) String changes made/needed: N/A
Attachment #9023474 - Flags: approval-mozilla-beta?
Comment on attachment 9023474 [details] Bug 1505482: Allow DLL patcher to resolve some backward short JMPs (r?aklotz!) [Triage Comment] Fixes video playback issues for some Windows 7 Flash users. Approved for 65.0b8.
Attachment #9023474 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
status-firefox65: affected → fixed
Confirmed fixed in firefox-65.0b8_20190103150357 win64 under Windows 7.
You need to log in before you can comment on or make changes to this bug.