Wladimir Palant Assignee
	Description • 18 years ago

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.

	Johnny Stenback (:jst)
	Comment 1 • 18 years ago

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.

	Wladimir Palant Assignee
	Comment 2 • 18 years ago

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.

	Johnny Stenback (:jst)
	Comment 4 • 18 years ago

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

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

	Josh Aas
	Comment 5 • 18 years ago

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!

	Christian :Biesinger (don't email me, ping me on IRC)
	Comment 6 • 18 years ago

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

	Wladimir Palant Assignee
	Comment 7 • 18 years ago

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

	Josh Aas
	Comment 8 • 18 years ago

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.

	Wladimir Palant Assignee
	Comment 9 • 18 years ago

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.

	Johnny Stenback (:jst)
	Comment 10 • 18 years ago

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

sr=jst

	Josh Aas
	Comment 11 • 18 years ago

I'll check this in tonight.

	Josh Aas
	Comment 12 • 18 years ago

checked in on trunk

	Christian Persch (GNOME) (away; not receiving bug mail)
	Comment 13 • 18 years ago

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

	Wladimir Palant Assignee
	Comment 14 • 18 years ago

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.

	Wladimir Palant Assignee
	Comment 15 • 18 years ago

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.

	Daniel Veditz [:dveditz]
	Comment 16 • 18 years ago

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

approved for 1.8.1.4, a=dveditz for release-drivers

	Christian Persch (GNOME) (away; not receiving bug mail)
	Comment 17 • 18 years ago

(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.

	Josh Aas
	Comment 18 • 18 years ago

landed on 1.8 branch

	Carsten Book [:Tomcat]
	Comment 19 • 18 years ago

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