431782 - HTTP redirects can bypass content policies

Status: NEW

(Core :: Security, defect)

Description

[Alex Vincent [:WeirdAl]]

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.

Comment 1

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

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

Comment 2

[Alex Vincent [:WeirdAl]]

Attachment: 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.

Comment 3

[Daniel Veditz [:dveditz]]

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.

Comment 4

[Wladimir Palant]

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

[Giorgio Maone [:ma1]]

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.

Comment 6

[Daniel Veditz [:dveditz]]

(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

Comment 7

[Wladimir Palant]

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.

Comment 8

[Alex Vincent [:WeirdAl]]

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.