Last Comment Bug 340852 - ActiveX plugin has a bad default content policy
: ActiveX plugin has a bad default content policy
Status: RESOLVED FIXED
: fixed1.8.0.7, fixed1.8.1
Product: Core Graveyard
Classification: Graveyard
Component: Embedding: ActiveX Wrapper (show other bugs)
: Trunk
: All All
: -- major (vote)
: ---
Assigned To: Wladimir Palant
: Michael Dunn
Mentors:
: 328830 (view as bug list)
Depends on:
Blocks: abp
  Show dependency treegraph
 
Reported: 2006-06-08 10:45 PDT by Wladimir Palant
Modified: 2012-04-04 01:09 PDT (History)
4 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Proposed patch (1.39 KB, patch)
2006-06-08 10:52 PDT, Wladimir Palant
mrbkap: review+
bzbarsky: superreview+
dveditz: approval1.8.0.7+
darin.moz: approval1.8.1+
Details | Diff | Splinter Review

Description Wladimir Palant 2006-06-08 10:45:07 PDT
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 2006-06-08 10:52:03 PDT
Created attachment 224883 [details] [diff] [review]
Proposed patch
Comment 2 Benjamin Smedberg [:bsmedberg] 2006-06-08 11:01:04 PDT
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) 2006-06-08 11:49:05 PDT
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 [:bz] 2006-06-09 18:25:54 PDT
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 [:bz] 2006-06-09 19:42:41 PDT
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 2006-06-28 09:01:29 PDT
Comment on attachment 224883 [details] [diff] [review]
Proposed patch

Requesting approval for branches.
Comment 7 Darin Fisher 2006-06-28 20:42:39 PDT
Comment on attachment 224883 [details] [diff] [review]
Proposed patch

a=darin on behalf of drivers
Comment 8 :Gavin Sharp [email: gavin@gavinsharp.com] 2006-07-25 16:32:00 PDT
Wladimir, does this patch need to be landed this patch on the 1.8 branch?
Comment 9 Wladimir Palant 2006-08-06 09:51:29 PDT
Yes, it does. I was on vacation, sorry.
Comment 10 Daniel Veditz [:dveditz] 2006-08-15 15:43:10 PDT
Comment on attachment 224883 [details] [diff] [review]
Proposed patch

approved for 1.8.0 branch, a=dveditz for drivers
Comment 11 Benjamin Smedberg [:bsmedberg] 2006-08-16 13:31:19 PDT
Fixed on MOZILLA_1_8_BRANCH and MOZILLA_1_8_0_BRANCH
Comment 12 Wayne Mery (:wsmwk, NI for questions) 2008-07-06 15:43:26 PDT
*** Bug 328830 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.