HTTP redirects can bypass content policies

NEW
Unassigned

Status

()

Core
Security
10 years ago
6 years ago

People

(Reporter: WeirdAl, Unassigned)

Tracking

(Blocks: 1 bug, {privacy})

Trunk
privacy
Points:
---
Bug Flags:
blocking1.9 -
wanted1.8.1.x +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 obsolete attachment)

(Reporter)

Description

10 years ago
At Skyfire, we use a content policy to implement a "blacklist" of websites.  On the 1.8 branch (and probably trunk, per code inspection), HTTP 301 and 302 redirects can bypass the content policies and load a URL we would normally block.

I have an internal testcase for this, but I'm not sure how I'd automate it for mochitesting.
Flags: blocking1.9?
Flags: blocking1.8.1.15?
Not a regression, not a blocker.  Content policy code has been a source of painful bug interaction recently so IMO we shouldn't take this even in the presence of a patch, pre-3.0.  1.9.0.x if we get good test coverage and increased confidence about CP interactions.

Seems like a mochichrome can add a CP entry, load a httpd.js URL that redirects, and see if the CP was called for the redirected-to page (pass) or the URL loads (fail).
Flags: blocking1.9? → blocking1.9-
(Reporter)

Comment 2

10 years ago
Created attachment 319039 [details] [diff] [review]
partial patch (1.8 branch)

I don't want to ask for reviews on this, because right now it uses nsIContentPolicy::TYPE_OTHER without asking if there's a better way to do so.  Any suggestions would be welcome.
We'll look at a reviewed, tested patch, but not a blocker. If the trunk guys are worried about messing in this area it makes me a little worried too.
Flags: blocking1.8.1.15? → wanted1.8.1.x+
I don't think this is anything for 1.8.1, and I share Shaver's doubts about 1.9. The fact that redirects don't go through content policies is well-known but I didn't raise this issue because I didn't see any good solution - and neither did I see a need to act. So far, to my knowledge nobody tried to fool Adblock Plus with redirects, and blocking the redirector isn't exactly hard anyway.

The patch as given is pretty useless IMO because it looses all context information. mListenerContext is an object of unknown type, its meaning is only known to the channel listener so that it should definitely not be passed as context here. We don't have the proper content type, we cannot find back to the loading document (not to mention the DOM node) and we don't have a principal. Even the referrer is unknown if sending the referrer is prohibited for some reason. The state of the DOM node doing the request is not updated properly either of course.

At that point the content policy is better off registering an observer for "http-on-modify-request" and trying to match redirects with previous content policy calls. At least here the original URL being loaded is known.

Note that nsXBLService has a class implementing nsIChannelEventSink, for same-origin checking (trunk only). This is a more viable placement for a content policy call since it lets you keep context, but then we would need all other content policy callers to implement nsIChannelEventSink as well. Not impossible but I think we need a very good reason if we do that.

Comment 5

10 years ago
I second comment 4.

NoScript works around this problem by implementing nsIChannelEventSink and "remembering" some context from the previous content policy call. 
Other implementors may want to do something similar as a short/mid-term work around, until a low-risk solution can be accepted.
(In reply to comment #4)
> So far, to my knowledge nobody tried to fool Adblock Plus with redirects,
> and blocking the redirector isn't exactly hard anyway.

I don't know if it's because of AdBlock or not, but someone's up to something if they're going so far to redirect favicons: http://www.webappsec.org/lists/websecurity/archive/2008-05/msg00073.html

Keywords: privacy
Right - Adblock Plus generally doesn't block favicons because they are displayed in chrome context (Adblock is a different story, it has a hardcoded exception for "favicon.ico", urgh). But that's a different issue.
(Reporter)

Comment 8

10 years ago
Comment on attachment 319039 [details] [diff] [review]
partial patch (1.8 branch)

This patch isn't going to work anymore - not only did it bitrot, but nsIContentPolicy.h isn't available if tier_gecko hasn't gone through export yet.
Attachment #319039 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.