Interceptor in FunctionHook::HookProtectedMode should be persistent

RESOLVED FIXED in Firefox 60

Status

()

P1
normal
RESOLVED FIXED
a year ago
11 months ago

People

(Reporter: aklotz, Assigned: handyman)

Tracking

Trunk
mozilla61
Unspecified
Windows
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox59 unaffected, firefox60 fixed, firefox61 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

a year ago
The interceptor in FunctionHook::HookProtectedMode is allocated on the stack. When destroyed, that interceptor will fix up its trampolines such that the hooks are effectively inert.

That interceptor needs to live beyond the lifetime of HookProtectedMode's scope.
(Reporter)

Comment 1

a year ago
David, could you take a look at this please?
Flags: needinfo?(davidp99)
(Reporter)

Updated

a year ago
status-firefox60: --- → affected
Hey Aaron, what's the priority on this?
Flags: needinfo?(aklotz)
(Reporter)

Comment 3

a year ago
This does break pref-based disabling of Protected Mode in 32-bit flash.

I don't know what the current state of Flash Sandboxing is on 32-bit Windows, but if we intend to override their sandbox by default, this should be high priority. Otherwise it's not too urgent.
Flags: needinfo?(aklotz)
(Assignee)

Comment 4

a year ago
I won't have time this week but should be able to take this later.
Assignee: nobody → davidp99
Flags: needinfo?(davidp99)
Priority: -- → P3
(Assignee)

Updated

a year ago
Priority: P3 → P1
(Assignee)

Comment 5

a year ago
There was already an nsWindowsDllInterceptor intended for this purpose -- this was essentially a cut-and-paste fail.
Attachment #8965044 - Flags: review?(aklotz)
(Reporter)

Updated

a year ago
Attachment #8965044 - Flags: review?(aklotz) → review+
(Assignee)

Updated

a year ago
Keywords: checkin-needed

Comment 6

a year ago
Pushed by btara@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6147dace3d0e
Use static nsWindowsDllInterceptor in plugin's HookProtectedMode r=aklotz
Keywords: checkin-needed

Comment 7

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6147dace3d0e
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox61: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
(Assignee)

Comment 8

a year ago
Comment on attachment 8965044 [details] [diff] [review]
Use static nsWindowsDllInterceptor in plugin's HookProtectedMode

Approval Request Comment
[Feature/Bug causing the regression]:
bug 1382251 refactored the nsWindowsDllInterceptor usage and introduced this regression

[User impact if declined]:
The Adobe Protected Mode sandbox (the 32 bit sandbox for Flash) cannot be disabled in about:addons.

[Is this code covered by automated tests?]:
No

[Has the fix been verified in Nightly?]:
Yes

[Needs manual test from QE? If yes, steps to reproduce]: 
No

[List of other uplifts needed for the feature/fix]:
None

[Is the change risky?]:
No

[Why is the change risky/not risky?]:
This small change restores long-standing behavior to the way it existed before the refactor.

[String changes made/needed]:
None
Attachment #8965044 - Flags: approval-mozilla-beta?
status-firefox59: --- → unaffected
status-firefox-esr52: --- → unaffected
Comment on attachment 8965044 [details] [diff] [review]
Use static nsWindowsDllInterceptor in plugin's HookProtectedMode

flash sandboxing fix, approved for 60.0b12
Attachment #8965044 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Setting qe-verify- based on David's assessment on manual testing needs (Comment 8).
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.