Comcast does not allow login in Windows 7

RESOLVED FIXED in Firefox 64

Status

()

defect
P1
normal
RESOLVED FIXED
7 months ago
5 months ago

People

(Reporter: handyman, Assigned: handyman)

Tracking

unspecified
mozilla66
Unspecified
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr60 unaffected, firefox63 unaffected, firefox64 fixed, firefox65 fixed, firefox66 fixed)

Details

Attachments

(2 attachments)

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
Blocks: 1426733
Flags: needinfo?(davidp99)
Assignee

Comment 2

7 months ago
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.
Flags: needinfo?(davidp99)
Assignee

Comment 3

7 months ago
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.
Assignee

Comment 5

7 months ago
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
Attachment #9025815 - Flags: review?(jmathies)
Assignee

Updated

7 months ago
Attachment #9025815 - Flags: review?(jmathies)
Assignee

Updated

7 months ago
Attachment #9025815 - Flags: review?(jmathies)

Updated

7 months ago
Attachment #9025815 - Flags: review?(jmathies) → review+
Assignee

Comment 6

7 months ago
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+

Comment 9

7 months ago
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).
Keywords: checkin-needed

Comment 11

6 months ago
Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6150c08114a8
Allow DLL patcher to resolve some backward short JMPs (r=aklotz)
Keywords: checkin-needed

Comment 12

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6150c08114a8
Status: NEW → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
What do we need to do for Beta65 here, David?
Flags: needinfo?(davidp99)

Comment 14

6 months ago
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.
Flags: needinfo?(davidp99)
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+

Comment 19

5 months ago
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.