1505482 - Comcast does not allow login in Windows 7

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect, P1)

Description

[David Parks [:handyman]]

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.

Comment 1

[Chris Peterson [:cpeterson]]

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

Comment 2

[David Parks [:handyman]]

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.

Comment 3

[David Parks [:handyman]]

Attachment: Bug 1505482: Allow DLL patcher to resolve some backward short JMPs (r?aklotz!)

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.

Comment 4

[David Parks [:handyman]]

Builds:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=93af251f8ffee741a0b5a3ec9c313590866d5b07

Comment 5

[David Parks [:handyman]]

Attachment: Disable restricting SIDs in Win7 NPAPI sandbox - For Beta Uplift

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 6

[David Parks [:handyman]]

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

Comment 7

[Julien Cristau [:jcristau]]

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

Comment 8

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/releases/mozilla-beta/rev/767930fe66d2

Comment 9

[Tom Kloos]

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.

Comment 10

[David Parks [:handyman]]

Manual checkin of the trunk patch -- Lando permissions have stopped working (again).

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6150c08114a8
Allow DLL patcher to resolve some backward short JMPs (r=aklotz)

Comment 12

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/6150c08114a8

Comment 13

[Ryan VanderMeulen [:RyanVM]]

What do we need to do for Beta65 here, David?

Comment 14

[Tom Kloos]

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

Comment 15

[David Parks [:handyman]]

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 16

[David Parks [:handyman]]

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

Comment 17

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/5e8f96ef4c33

Comment 19

[Tom Kloos]

Confirmed fixed in firefox-65.0b8_20190103150357 win64 under Windows 7.