ActiveX plugin has a bad default content policy

RESOLVED FIXED

Status

Core Graveyard
Embedding: ActiveX Wrapper
--
major
RESOLVED FIXED
11 years ago
5 years ago

People

(Reporter: Wladimir Palant, Assigned: Wladimir Palant)

Tracking

(Blocks: 1 bug, {fixed1.8.0.7, fixed1.8.1})

Trunk
fixed1.8.0.7, fixed1.8.1

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

11 years ago
This issue has been known for quite a while, see for example the forum post here: http://forums.mozillazine.org/viewtopic.php?p=1845847#1845847

The problem is that the security policy check in the ActiveX plugin is flawed. It depends on a JS context and if it fails to get one it will allow just any ActiveX to execute. The offending code (WillHandleCLSID() in LegacyPlugin.cpp):

    PRBool isSafe = PR_FALSE;
    PRBool classExists = PR_FALSE;
    JSContext * cx = GetPluginsContext(pData);
    if (cx)
    {
        nsCOMPtr<nsIURI> uri;
        MozAxPlugin::GetCurrentLocation(pData->pPluginInstance, getter_AddRefs(uri));
        MozAxAutoPushJSContext autoContext(cx, uri);
        dispSupport->IsClassSafeToHost(cx, cid, PR_TRUE, &classExists, &isSafe);
    }
    if (classExists && !isSafe)
        return FALSE;
    return TRUE;

That's pretty bad and should be changed to the opposite - if we can't get a JS context we won't allow any ActiveX, just to be on the safe side. Like this:

    JSContext * cx = GetPluginsContext(pData);
    if (!cx)
        return FALSE;

    PRBool isSafe = PR_FALSE;
    PRBool classExists = PR_FALSE;
    nsCOMPtr<nsIURI> uri;
    MozAxPlugin::GetCurrentLocation(pData->pPluginInstance, getter_AddRefs(uri));
    MozAxAutoPushJSContext autoContext(cx, uri);
    dispSupport->IsClassSafeToHost(cx, cid, PR_TRUE, &classExists, &isSafe);
    if (classExists && !isSafe)
        return FALSE;
    return TRUE;

Unless the dependency on the JS context can be eliminated entirely of course.

What is triggering this issue in case of Adblock? The problem occurs when an EMBED is wrapped inside an OBJECT. EMBED gets processed first and Adblock tries to attach an "object tab" on it. This "object tab" happens to be a DIV element that is inserted inside EMBED's parent container - causing the OBJECT to be instantiated prematurely.

Adblock Plus 0.7.0.2 (to be released later today) works around this problem now, it still needs fixing however.
(Assignee)

Comment 1

11 years ago
Created attachment 224883 [details] [diff] [review]
Proposed patch
Comment on attachment 224883 [details] [diff] [review]
Proposed patch

blake, in johnny's absence is this something you can look at?
Attachment #224883 - Flags: review?(mrbkap)
Comment on attachment 224883 [details] [diff] [review]
Proposed patch

I don't know that I count as a peer in this code, but both the explanation and the patch here seem reasonable to me.
Attachment #224883 - Flags: review?(mrbkap) → review+
Assignee: adamlock → trev
(Assignee)

Updated

11 years ago
Attachment #224883 - Flags: superreview?(bzbarsky)
Comment on attachment 224883 [details] [diff] [review]
Proposed patch

Looks good.  Let me know if you need this checked in, ok?
Attachment #224883 - Flags: superreview?(bzbarsky) → superreview+
Patch checked in.  Request branch approvals in a week or so if there are no regressions?

Also, I think you got lucky with IRC -- you were 2 lines away from being out of my scrollback.  Asking for checkins is a good e-mail use case.  ;)
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
(Assignee)

Comment 6

11 years ago
Comment on attachment 224883 [details] [diff] [review]
Proposed patch

Requesting approval for branches.
Attachment #224883 - Flags: approval1.8.1?
Attachment #224883 - Flags: approval1.8.0.6?

Comment 7

11 years ago
Comment on attachment 224883 [details] [diff] [review]
Proposed patch

a=darin on behalf of drivers
Attachment #224883 - Flags: approval1.8.1? → approval1.8.1+
Wladimir, does this patch need to be landed this patch on the 1.8 branch?
(Assignee)

Comment 9

11 years ago
Yes, it does. I was on vacation, sorry.
Comment on attachment 224883 [details] [diff] [review]
Proposed patch

approved for 1.8.0 branch, a=dveditz for drivers
Attachment #224883 - Flags: approval1.8.0.7? → approval1.8.0.7+
Whiteboard: [checkin needed (1.8 branch)]
Fixed on MOZILLA_1_8_BRANCH and MOZILLA_1_8_0_BRANCH
Keywords: fixed1.8.0.7, fixed1.8.1
Whiteboard: [checkin needed (1.8 branch)]
Duplicate of this bug: 328830
(Assignee)

Updated

9 years ago
Blocks: 467520
Component: Embedding: ActiveX Wrapper → Embedding: ActiveX Wrapper
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.