456957 - nsIContentPolicy cannot examine redirected urls

Status: NEW

(Core :: Networking, defect, P3)

Description

[arno renevier]

Hi,
nsIContentPolicy cannot examine and filter redirected urls.

steps to reproduce:
- install adblock plus http://adblockplus.org/en/installation
- block "*imageshack*" in preferences
- go to: http://www.fdn.fr/~arenevier/mozilla/bugs/evilad_no_redirect.html
- check that evil ad is blocked
- now, go to to http://www.fdn.fr/~arenevier/mozilla/bugs/evilad_redirect.html
- evil ad is shown, even if hosted on imageshack

That behaviour is not the same, but has some similarities with bug 167047

Comment 1

[Anthony Lieuallen]

Confirmed when testing Greasemonkey issue
https://github.com/greasemonkey/greasemonkey/issues/1673

Greasemonkey's current mechanism is to use nsIContentPolicy to detect URLs being loaded that satisfy a pattern (URL ends in .user.js), block the load, then re-start the request so that we can get its contents.  If the matching request is the result of a 302 redirect, it never shows up in shouldLoad nor shouldProcess, so we fail to handle it.

Comment 2

[Devdatta Akhawe [:devd]]

I discussed this with bz today and his suggestion (he can correct me if I am wrong) was to add a NS_CheckContentLoadPolicy (http://mxr.mozilla.org/mozilla-central/source/content/base/public/nsContentPolicyUtils.h#200) call in the OnChannelRedirect code (http://mxr.mozilla.org/mozilla-central/source/caps/src/nsScriptSecurityManager.cpp#2365). 

CSP code can give a hint on what to do http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsCSPService.cpp#207

We do not want nsicontentpolicy to care about internal redirects, so we should filter them out using NS_IsInternalSameURIRedirect (which occur when there is a proxy).

One thing missing is how to handle protocol redirects (e.g., mailto: URIs redirecting to http://mail.google.com/) should nsicontentpolicy be called? If not, how to filter these out ? I will need to breakpoint on this case and manually see whats happening.

This was just a brain dump: I will write a patch soon.

Comment 3

[Devdatta Akhawe [:devd]]

Attachment: nsIContentPolicy cannot examine redirected urls

Comment 4

[Devdatta Akhawe [:devd]]

Comment on attachment 702123 [details] [diff] [review]
nsIContentPolicy cannot examine redirected urls

WIP. I don't know how to get loadtype from the channel. bsmith: can you help?

bz: currently not ignoring fixed protocol handler redirects, but other than that any feedback appreciated.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 702123 [details] [diff] [review]
nsIContentPolicy cannot examine redirected urls

This needs to come after OnRedirectVerifyCallback, no?

Past that, your chances of getting the right contentType are slim.  Might be better to make one up or something (TYPE_REDIRECT or whatnot?).  Failing that we'd have to store types on all channels or something.

Other than that, and random whitespace nits, seems fine.

Comment 6

[Devdatta Akhawe [:devd]]

I am sorry; but it _is_ called after OnRedirectVerifyCallback, right? Did you mean come before?

I think it is critical to get the type, otherwise this won't be that useful. For example, CSP couldn't use this without the type. We should try to replicate as much of the nsicontentpolicy as possible.

I am trying to understand what CSP does: http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsCSPService.cpp#225 Is ChannelPolicy something I can also rely on ?

Comment 7

[Boris Zbarsky [:bzbarsky]]

I meant before, sorry.

Comment 8

[Devdatta Akhawe [:devd]]

ok thanks. I also just looked at NoScript and it seems it keeps its own state and tracks the type for each channel. It then uses the oldChannel to figure out the type. I think it makes sense to just add types on all channels: right now, (correct me if I am wrong) everyone seems to be storing their state using the channel, and solving the same problem again and again.

Comment 9

[Honza Bambas (:mayhemer)]

Comment on attachment 702123 [details] [diff] [review]
nsIContentPolicy cannot examine redirected urls

Review of attachment 702123 [details] [diff] [review]:
-----------------------------------------------------------------

::: caps/src/nsScriptSecurityManager.cpp
@@ +2389,5 @@
>      if (NS_FAILED(rv))
>          return rv;
>  
>      cb->OnRedirectVerifyCallback(NS_OK);
> +    if(!NS_IsInternalSameURIRedirect(oldChannel, newChannel, redirFlags)){

cb->OnRedirectVerifyCallback(NS_OK); must be the last thing to do in this callback.

@@ +2402,5 @@
> +                                                  nullptr,
> +                                                  &shouldLoad);
> +        NS_ENSURE_SUCCESS(rv,rv);
> +        if(NS_CP_REJECTED(shouldLoad)) {
> +            return NS_ERROR_FAILURE;

Isn't there some more specific error for this?  Worth to create one to more easily hunt for redirect failures.

General notes: styling, whitespaces.

Comment 10

[Devdatta Akhawe [:devd]]

Thanks!

> Isn't there some more specific error for this?  Worth to create one to more
> easily hunt for redirect failures.

I copied it from another place in the gecko codebase. This is a contentPolicy failure (or blocking) and not a redirect failure, right? I think we should maintain the same error that we have for other content policy blocks. What do you think?

Also, just a status note: I know how to call nsIContentPolicy, but I feel like this code will have limited value if we don't pass in the contentType to the contentPolicy calls: only things like HSTS, mixedContent etc. will be able to use this. Does anyone have thoughts on how bad an idea it would be to stuff contentType into the channel ?

Comment 11

[Devdatta Akhawe [:devd]]

Attachment: nsIContentPolicy cannot examine redirected urls

Comment 12

[Devdatta Akhawe [:devd]]

Attachment: nsIContentPolicy cannot examine redirected urls

Comment 13

[Devdatta Akhawe [:devd]]

Comment on attachment 703151 [details] [diff] [review]
nsIContentPolicy cannot examine redirected urls

Revised patch with a new loadType and whitespace/styling fixed, as far as I can tell. I am relying on TYPE_REDIRECT now. My thinking is that this can be used by mixedContent and HSTS etc. Another bug can take care of stuffing the loadType into the channel and sending it correctly.

Or is it better to do everything in a single patch?

I am still looking for an explanation of what CSP is doing (see comment 6) in case anyone has the time.

Comment 14

[Honza Bambas (:mayhemer)]

Comment on attachment 703151 [details] [diff] [review]
nsIContentPolicy cannot examine redirected urls

Review of attachment 703151 [details] [diff] [review]:
-----------------------------------------------------------------

I am not a peer for CSP, and since I'm sure this is not the most correct approach (however, the place to call CSP check may be correct) I'll rather delegate to Jonas.

I think that implementers of ShouldLoad will, in case of TYPE_REDIRECT, want to examine the original url, not just the new one.  Also the decision can be different for different types of content policy that we should pass to the callback as well (I can imagine the mixed content blocker for instance).

Based on that I think we need a new method ShouldRedirect(principal, oldURI, newURI, contentPolicyType, ...).  See also bug 805136 comment 1, that may be actually more proper place to fix.

::: caps/src/nsScriptSecurityManager.cpp
@@ +2389,5 @@
>      if (NS_FAILED(rv))
>          return rv;
>  
> +    if (!NS_IsInternalSameURIRedirect(oldChannel, newChannel, redirFlags))
> +    {

'if () {', one line.

::: content/base/public/nsIContentPolicy.idl
@@ +23,5 @@
>   * WARNING: do not block the caller from shouldLoad or shouldProcess (e.g.,
>   * by launching a dialog to prompt the user for something).
>   */
>  
> +[scriptable,uuid(38cc474d-449c-46b0-a5a3-347472fb8b03)]

No need to change iid when adding just a const.

Comment 15

[Devdatta Akhawe [:devd]]

> Based on that I think we need a new method ShouldRedirect(principal, oldURI,
> newURI, contentPolicyType, ...).  See also bug 805136 comment 1, that may be
> actually more proper place to fix.

Hmm.. that might be a nicer fix. What do others think? 

What are the scenarios you are thinking of where the oldURI would be useful?

Comment 16

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

I support the idea of saving the nsContentPolicyType in the channel in an officially-supported way if it helps the redirect case. The content policy really needs to know the nsContentPolicyType when processing the redirect case, since we're still going to allow HTTP -> HTTPS redirects for img and video content, but disallow them for most everything else.

I also prefer the ShouldRedirect approach. I think that calling ShouldLoad is a bad idea because callers of ShouldLoad are likely expecting to be passed the relevant DOM elements as the context.

One question though: nsMixedContentBlocker seems to use the context parameter extensively, but in the redirect case it will never be passed the context. I'm guessing that if nsMixedContentBlocker can properly handle redirects without the context, then it should be able to properly handle the non-redirect cases without the context too. In that case nsMixedContentBlocker::ShouldLoad should be modified to stop using the context, so that its logic can be factored out into a common function that can be called by both ShouldLoad and ShouldRedirect.

Because of the uncertainty of how this would work for nsMixedContentBlocker, IMO we should ensure that the new API is sufficient for nsMixedContentBlocker before we commit this change. However, it would be great to have the patch ready to go before then so nsMixedContentBlocker can use it.

Comment 17

[Devdatta Akhawe [:devd]]

It seems possible to pass in the principal (the current patch already does so), which should tell mixedContentBlocker whether or not to block. But, if the user has over-ridden, is the principal a sufficient argument or does mixedContentBlocker need the document (i.e., the context argument)? I know CSP stuffs all its state into the principal: what does mixedcontentblocker do? Do you know who we can ask?

I agree calling ShouldLoad is a bad idea, and ShouldRedirect is better. It is annoying though, because, if I am not wrong, all consumers of nsIContentPolicy would have to add a new method. It seems like a pretty major and possibly breaking change. I would love to hear bz/jonas's thought on this too.

Comment 18

[Ian Melven :imelven]

As some possibly useful background, there's ideas around a 'better' Content Policy API in this thread : https://groups.google.com/forum/#!topic/mozilla.dev.platform/veLFoy09ydg/discussion

Comment 19

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 703151 [details] [diff] [review]
nsIContentPolicy cannot examine redirected urls

Review of attachment 703151 [details] [diff] [review]:
-----------------------------------------------------------------

I don't think I'm a good person to review this. Maybe try bz? Or jduell?

Comment 20

[Tanvi Vyas[:tanvi]]

> It seems possible to pass in the principal (the current patch already does
> so), which should tell mixedContentBlocker whether or not to block. But, if
> the user has over-ridden, is the principal a sufficient argument or does
> mixedContentBlocker need the document (i.e., the context argument)? I know
> CSP stuffs all its state into the principal: what does mixedcontentblocker
> do? Do you know who we can ask?

nsMixedContentBlocker.cpp does use the context exstensively - http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsMixedContentBlocker.cpp.

One example, is that we use it to get the docshell and the root document, where we store a few flags that tell us whether there is mixed active or mixed display content, whether it has loaded or been blocked, and tell us if the user has overriden blocking.(In reply to Devdatta Akhawe [:devd] from comment #17)

Comment 21

[Jason Duell]

Comment on attachment 703151 [details] [diff] [review]
nsIContentPolicy cannot examine redirected urls

bz has been reviewing this stuff most recently.  If he's swamped, bholley?

Comment 22

[Devdatta Akhawe [:devd]]

Comment on attachment 703151 [details] [diff] [review]
nsIContentPolicy cannot examine redirected urls

I am going to cancel the review for now. I am also leaning towards a separate ShouldRedirect call instead of overloading ShouldLoad; additionally, as Tanvi's comments point out, we need to get context working too (in addition to contentType). 

Does anyone know of a better way than stuffing it all in the channel? Asking for bz's feedback.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 703151 [details] [diff] [review]
nsIContentPolicy cannot examine redirected urls

I feel like I've been involved in this a good bit already; would like another pair of eyes on it.  All I have is the whitespace nit for the space after the '='.

Maybe Honza?

Comment 24

[Boris Zbarsky [:bzbarsky]]

Oh, I see, this got repurposed...

ShouldRedirect would make sense to me, yes.

Comment 25

[Devdatta Akhawe [:devd]]

Do you know if there is a way to get context and contentType other than stuffing it in the channel ?

Comment 26

[Boris Zbarsky [:bzbarsky]]

Not that I can think of.

Comment 27

[Tanvi Vyas[:tanvi]]

Dev, not sure if this will or will not apply for your patch, but for the mixed content blocker I had to do something special for document.open(), which creates a new channel.  You may have to take that case into account as well so you don't lose the contentType and context information.  

My patch for this from bug 822367 - https://bugzilla.mozilla.org/attachment.cgi?id=699512&action=diff#a/content/html/document/src/nsHTMLDocument.cpp_sec2

Comment 28

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Devdatta Akhawe [:devd] from comment #22)
> Additionally, as Tanvi's comments point out, we need to get context working
> too (in addition to contentType). 
> 
> Does anyone know of a better way than stuffing it all in the channel? Asking
> for bz's feedback.

I suggest that we try to rewrite mixed content blocker in such a way that it never relies on the context for the redirect case. It will be very difficult or impossible to pass the context to the mixed content blocker for e10s builds (i.e. B2G). And, also, AFAICT mixed content blocker isn't really sensitive to the context. Looking at nsMixedContentBlocker, it seems like it needs a way to navigate from the channel to the Window/TabParent. isn't there already a way to do that?

Once we navigate to the Window/TabParent, then we can determine what the origin of the root document is through the nsIWebNavigation.currentURI property. And, it seems like it needs to be able to control the UI features (whether to show the doorhanger and what state the doorhanger/address bar should have). Perhaps we should just add those features to nsIWebNavigation and then have the Window/TabParent's nsIWebNavigation implementation forward the calls to its DocShell?

In particular, I am hoping that in B2G, we should eventually (soon?) have the chrome for the browser in the parent process while the tabs live in a child process or child processes. (Not sure if that is still a goal or not though.) In that case, it seems to make sense to me to have a nsIWebNavigation implementation in the parent to handle the state of all the security indicators.

bz, does this seem reasonable?

Comment 29

[Boris Zbarsky [:bzbarsky]]

> Looking at nsMixedContentBlocker, it seems like it needs a way to navigate from the
> channel to the Window/TabParent. isn't there already a way to do that?

From an nsIChannel, absolutely.  See nsILoadContext.

> Then we can determine what the origin of the root document is through the
> nsIWebNavigation.currentURI property

No, you cannot.  Don't ever use that for anything security-related, ever.

However, you can just get the nsIPrincipal directly off the Document if you have one.

> Perhaps we should just add those features to nsIWebNavigation and then have the
> Window/TabParent's nsIWebNavigation implementation forward the calls to its DocShell?

I'm not sure what any of those have to do with docshell or webnavigation.  Those are pure browser front-end features, unrelated to docshells in any way that I can tell...

Comment 30

[Devdatta Akhawe [:devd]]

So, it seems the right thing to do is to stuff contentType in the channel and leave context alone? that sounds good to me too. Or am I misunderstanding bz and bsmith above?

Tanvi: Can you confirm this should be sufficient for mixedcontentblocker? Or is there a place where it really really needs context?

Comment 31

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Boris Zbarsky (:bz) from comment #29)
> > Then we can determine what the origin of the root document is through the
> > nsIWebNavigation.currentURI property
> 
> No, you cannot.  Don't ever use that for anything security-related, ever.
> 
> However, you can just get the nsIPrincipal directly off the Document if you
> have one.

OK, let me ask my question more directly. If we were to have the address bar in the parent process and the web content in a child process for the B2G browser, then we would NOT have access to the document from within the parent process. So, from the parent process, what is the secure way of determining the principle of the root document?

> > Perhaps we should just add those features to nsIWebNavigation and then have the
> > Window/TabParent's nsIWebNavigation implementation forward the calls to its DocShell?
> 
> I'm not sure what any of those have to do with docshell or webnavigation. 
> Those are pure browser front-end features, unrelated to docshells in any way
> that I can tell...

I meant that we could/should add the APIs for controlling the mixed content blocker doorhanger behavior/appearance state to nsIWebNavigation. If not nsIWebNavigation, then is there another place to add them? It is not per-docshell state, it is per-root state.

Comment 32

[Honza Bambas (:mayhemer)]

Also don't forget that an http channel can be shared by an unspecified number of image request proxies that can be happening for different documents.  IMO, untrackable in the parent process at all.

Maybe the mixedcontentblocked doesn't need to handle ShouldRedirect at all.  As I understand, part of the fix for this bug is to call ShouldLoad() for the new redirected channel as well (the complete sequence should IMO be: ShouldLoad(oldChannel, context), ShouldRedirect(oldChannel, newChannel), ShouldLoad(newChannel, context)).  Or not?

Comment 33

[Devdatta Akhawe [:devd]]

My understanding is to only do "ShouldRedirect(.. whatever arguments are needed for things to work..)" 

If we decide to use shouldLoad, we will definitely need to have all the arguments that the nsicontentpolicy requires. This is why I actually like just calling shouldRedirect---this way, we only give the arguments we can give (e.g., not send  context)

Comment 34

[Boris Zbarsky [:bzbarsky]]

> So, from the parent process, what is the secure way of determining the principle of the
> root document?

You can examine the document currently loaded in the docshell, presumably (may need to add ipdl for that), but there's no guarantee that that's the document that the load started with, right?

The only way I can see to solve this problem is to set the originating principal on the channel (something I really wish we always did anyway) and serialize that over to the chrome process when we send the channel over...

> It is not per-docshell state

But each docshell is an nsIWebNavigation.

Stuff that's per-root sounds like it belongs on docshelltreeowner or some such.

Comment 35

[Christoph Kerschbaumer [:ckerschb]]

We are working on Bug "Mixed content blocking activates even if extensions rewrite all resources to HTTPS" [1] where we are storing the contentPolicyType in the channel. When calling the function nsHttpChannel::BeginConnect(), we are calling the former ShouldLoad function to determine whether MCB allows the load or not. If MCB decides to reject the load, we call cancel on the channel.

To get the current context in EvaluateMCB (former shouldLoad), we query the window or the node from the callbacks on the channel; also special case handling for XMLHttpRequests, which needs the context for event handlers and also early returns for Safebrowsing and OCSP, etc.

When a channel gets redirected, wouldn't that approach also work, or am I am I missing something?


[1] https://bugzilla.mozilla.org/show_bug.cgi?id=878890
[2] http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpChannel.cpp#4506

Comment 36

[Boris Zbarsky [:bzbarsky]]

> we query the window or the node from the callbacks on the channel; 

How does that work in an e10s setting?

But yes, in general if you do something during BeginConnect() you should be able to do it during a redirect.

Comment 37

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Boris Zbarsky [:bz] from comment #36)
> > we query the window or the node from the callbacks on the channel; 
> 
> How does that work in an e10s setting?

I am not sure how that would work in e10s, are you suggesting we should also stuff the context into the channel?

Comment 38

[Boris Zbarsky [:bzbarsky]]

I'm just suggesting that whatever setup you create needs to work in e10s, since we're shipping e10s stuff to users.

Comment 39

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I think we should simply start investing in implementing the APIs described at

https://groups.google.com/forum/#!msg/mozilla.dev.platform/veLFoy09ydg/2XcWUXSiVbEJ

I feel like we keep trying to avoid it, but it would solve a lot of problems in one go.

Comment 40

[Firefox Bug Husbandry Bot]

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258

Comment 41

[Firefox Bug Husbandry Bot]

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258

Comment 42

[Mike Kaply [:mkaply]]

This was just reported as a problem with enterprise policy:

Bug 1711870 - policy WebsiteFilter is not applied after redirect

I'm wondering if there's anything I can do to workaround this...

Comment 43

[BugBot [:suhaib / :marco/ :calixte]]

The severity field for this bug is relatively low, S3. However, the bug has 14 votes.
:kershaw, could you consider increasing the bug severity?

For more information, please visit auto_nag documentation.

Comment 44

[BugBot (nomail) [:suhaib / :marco/ :calixte]]

The last needinfo from me was triggered in error by recent activity on the bug. I'm clearing the needinfo since this is a very old bug and I don't know if it's still relevant.