375435 - URLs requested by plugins should trigger content policies

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Wladimir Palant]

Attachment: Proposed patch

If you install Adblock Plus and go to Yahoo Maps (URL above) you will notice that the browser loads content from many different addresses, e.g. the status bar indicates that content is transferred from xml1.maps.yahoo.com. Yet the list of blockable elements in Adblock Plus (opens on Ctrl+Shift+B) doesn't show anything of it - URLs requested by plugins are not reported to content policies. In this particular case the loophole is used to load ads and to track the user. I think this needs to be fixed, similar to bug 371123.

Attaching a tested patch.

Comment 1

[Johnny Stenback (:jst)]

That patch looks like a patch against the 1.8 branch. We'd want a trunk patch first, and once that's landed on the trunk etc, then we can consider whether this is something we want to fix for the branches as well. The trunk already does content policy checking for plugin instantiation, but I think we'd still want something like this patch for the trunk as well.

Comment 2

[Wladimir Palant]

Johnny, my tree is simply a little bit out of date. But the code in NewPluginURLStream() is still exactly the same, I only got it wrong with the includes - there have been changes in this part. I will update my tree and give you a new patch.

As to policy checking for plugin instantiation - this is not only trunk, we have had it for quite a while already. But none of the URLs loaded by the plugin through the browser get checked, neither on branch nor on trunk.

Comment 3

[Wladimir Palant]

Attachment: Proposed patch (bitrot eliminated)

Comment 4

[Johnny Stenback (:jst)]

Comment on attachment 259857 [details] [diff] [review]
Proposed patch (bitrot eliminated)

r=jst, sorry for thinking your first patch was against the branch.

Comment 5

[Josh Aas]

Comment on attachment 259857 [details] [diff] [review]
Proposed patch (bitrot eliminated)

I can't do sr. I marked r+, just get jst to mark sr. Thanks!

Comment 6

[Christian :Biesinger (don't email me, ping me on IRC)]

Does this code also have the <object>/<embed> node available? that'd seem like a better context.

Comment 7

[Wladimir Palant]

Attachment: Patch v2

Right, we can pass the object node as context here. Changed that, also removed adding include files from the patch (these include files are there already).

Comment 8

[Josh Aas]

Comment on attachment 260602 [details] [diff] [review]
Patch v2

What exactly is the benefit of using the object node as context?

+    PRInt16 shouldLoad = nsIContentPolicy::ACCEPT;

Shouldn't the default be to reject? It looks like the value here shouldn't really come in to play but reject seems safer if we're going to set a value at all.

Comment 9

[Wladimir Palant]

No, the default for content policies is always accept, check the other callers of NS_CheckContentLoadPolicy. The content policy is a blacklist, everything that isn't explicitly forbidden is allowed.

If we use the object node as context the consumers will have more information when deciding whether the request should be blocked. They will know that the URL has been requested by an object and they will know which object it was (the document can contain more than one). Adblock Plus will not use this information but other extensions might.

Comment 10

[Johnny Stenback (:jst)]

Comment on attachment 260602 [details] [diff] [review]
Patch v2

sr=jst

Comment 11

[Josh Aas]

I'll check this in tonight.

Comment 12

[Josh Aas]

checked in on trunk

Comment 13

[Christian Persch (GNOME) (away; not receiving bug mail)]

I'm courious why TYPE_OTHER was chosen instead of TYPE_OBJECT (or maybe a new TYPE_PLUGIN) ? That's rather unspecific, and thus content policies cannot specifically check for plugin requests, at least not without further checks to the passed context data (and even then there could be other situations that send TYPE_OTHER).

Comment 14

[Wladimir Palant]

Because TYPE_OBJECT should be the object itself - e.g. the Flash SWF file that is displayed. Here we have data of unspecified type loaded by the object, something entirely different. So this would be TYPE_OBJECT_SUBREQUEST if anything. Feel free to file a new bug on increasing the number of types to be more specific - I was thinking about this myself.

Comment 15

[Wladimir Palant]

Comment on attachment 260602 [details] [diff] [review]
Patch v2

This should land on the 1.8 branch as well, reasoning is basically the same as in bug 371123 comment 6. This patch closes a loophole in the content policies that allows plugins to load data without going through content policies. The patch is pretty straightforward, no obvious failure points. It is not to be expected that any content policies will have problems handling these calls, similar TYPE_OTHER calls exist in the codebase already.

Comment 16

[Daniel Veditz [:dveditz]]

Comment on attachment 260602 [details] [diff] [review]
Patch v2

approved for 1.8.1.4, a=dveditz for release-drivers

Comment 17

[Christian Persch (GNOME) (away; not receiving bug mail)]

(In reply to comment #14)
> Because TYPE_OBJECT should be the object itself - e.g. the Flash SWF file that
> is displayed. Here we have data of unspecified type loaded by the object,
> something entirely different. So this would be TYPE_OBJECT_SUBREQUEST if
> anything. Feel free to file a new bug on increasing the number of types to be
> more specific - I was thinking about this myself.

That'll fit into bug 375314.

Comment 18

[Josh Aas]

landed on 1.8 branch

Comment 19

[Carsten Book [:Tomcat]]

verified fixed 1.8.1.4 using Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.8.1.4pre) Gecko/2007042805 BonEcho/2.0.0.4pre and the steps to reproduce from comment 0. On maps.yahoo.com i see the followings urls now on the list of blockable items like: http://xml1.maps.yahoo.com/flashAds.xml?adpos=XTL&q=Balko%20OK%2073931&wpx=941&hpx=123&mag=14&r=2806&.intl=us&ei=UTF-8&sessionID=1401712746&.crumb=

Adding verified keyword