Interceptor in FunctionHook::HookProtectedMode should be persistent

RESOLVED FIXED in Firefox 60

Status

()

defect
P1
normal
RESOLVED FIXED
a year ago
a year 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
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)
I won't have time this week but should be able to take this later.
Assignee: nobody → davidp99
Flags: needinfo?(davidp99)
Priority: -- → P3
Priority: P3 → P1
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+
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
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
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?
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.