340852 - ActiveX plugin has a bad default content policy

Status: RESOLVED FIXED

(Core Graveyard :: Embedding: ActiveX Wrapper, defect)

Description

[Wladimir Palant]

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.

Comment 1

[Wladimir Palant]

Attachment: Proposed patch

Comment 2

[Benjamin Smedberg]

Comment on attachment 224883 [details] [diff] [review]
Proposed patch

blake, in johnny's absence is this something you can look at?

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

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.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 224883 [details] [diff] [review]
Proposed patch

Looks good.  Let me know if you need this checked in, ok?

Comment 5

[Boris Zbarsky [:bzbarsky]]

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

Comment 6

[Wladimir Palant]

Comment on attachment 224883 [details] [diff] [review]
Proposed patch

Requesting approval for branches.

Comment 7

[Darin Fisher]

Comment on attachment 224883 [details] [diff] [review]
Proposed patch

a=darin on behalf of drivers

Comment 8

[:Gavin Sharp [email: [e-mail]]]

Wladimir, does this patch need to be landed this patch on the 1.8 branch?

Comment 9

[Wladimir Palant]

Yes, it does. I was on vacation, sorry.

Comment 10

[Daniel Veditz [:dveditz]]

Comment on attachment 224883 [details] [diff] [review]
Proposed patch

approved for 1.8.0 branch, a=dveditz for drivers

Comment 11

[Benjamin Smedberg]

Fixed on MOZILLA_1_8_BRANCH and MOZILLA_1_8_0_BRANCH