Closed
Bug 375435
Opened 18 years ago
Closed 18 years ago
URLs requested by plugins should trigger content policies
Categories
(Core Graveyard :: Plug-ins, defect)
Core Graveyard
Plug-ins
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: ecfbugzilla, Assigned: ecfbugzilla)
References
()
Details
(Keywords: verified1.8.1.4)
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
jaas
:
review+
jst
:
superreview+
dveditz
:
approval1.8.1.4+
|
Details | Diff | Splinter Review |
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.
Attachment #259704 -
Flags: review?(jst)
Assignee | ||
Updated•18 years ago
|
Status: NEW → ASSIGNED
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.
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.
Assignee | ||
Comment 3•18 years ago
|
||
Attachment #259704 -
Attachment is obsolete: true
Attachment #259857 -
Flags: review?(jst)
Attachment #259704 -
Flags: review?(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.
Attachment #259857 -
Flags: review?(jst) → review+
Assignee | ||
Updated•18 years ago
|
Attachment #259857 -
Flags: superreview?(jonas)
Assignee | ||
Updated•18 years ago
|
Attachment #259857 -
Flags: superreview?(jonas) → superreview?(joshmoz)
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!
Attachment #259857 -
Flags: superreview?(joshmoz) → review+
Comment 6•18 years ago
|
||
Does this code also have the <object>/<embed> node available? that'd seem like a better context.
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).
Attachment #259857 -
Attachment is obsolete: true
Attachment #260602 -
Flags: superreview?(jst)
Attachment #260602 -
Flags: review?(joshmoz)
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.
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.
Comment 10•18 years ago
|
||
Comment on attachment 260602 [details] [diff] [review] Patch v2 sr=jst
Attachment #260602 -
Flags: superreview?(jst) → superreview+
Attachment #260602 -
Flags: review?(joshmoz) → review+
Assignee | ||
Updated•18 years ago
|
Whiteboard: [checkin needed]
Comment 12•18 years ago
|
||
checked in on trunk
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Whiteboard: [checkin needed]
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).
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.
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.
Attachment #260602 -
Flags: approval1.8.1.4?
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
Attachment #260602 -
Flags: approval1.8.1.4? → approval1.8.1.4+
Assignee | ||
Updated•18 years ago
|
Whiteboard: [checkin needed - 1.8 branch]
(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.
Assignee | ||
Updated•18 years ago
|
Keywords: fixed1.8.1.4
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
Keywords: fixed1.8.1.4 → verified1.8.1.4
Updated•2 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•