641098 - nsIContentPolicy::shouldLoad() called with incorrect origin argument for meta refresh after link click

Status: NEW

(Core :: DOM: Navigation, defect)

Description

[Justin Samuel]

User-Agent:       Mozilla/5.0 (X11; Linux x86_64; rv:2.0b13pre) Gecko/20110311 Firefox/4.0b13pre
Build Identifier: Firefox 4.0 rc1

Setup: three documents---A, B, and C---with different URLs where A has a link to B and B has a meta refresh tag that loads C.

Clicking A's link to B results in the correct origin and destination arguments in the call to shouldLoad (origin is A's URL, destination is B's). However, when the meta refresh in B occurs, shouldLoad is not called with origin B and destination C but rather with origin A and destination C. That is, the origin appears to be the previous document's URL (the one with the link the user followed), not the current document's URL that contains the meta refresh.

Reproducible: Always




This bug causes problems for the RequestPolicy extension.

When I have time, I'll try to narrow down when this issue began.

This bug seems similar to bug 593174.

Comment 1

[Boris Zbarsky [:bzbarsky]]

This has probably been happening forever.  The relevant code and comments are in nsDocShell::ForceRefreshURI:

        /* for redirects we mimic HTTP, which passes the
         *  original referrer
         */
        nsCOMPtr<nsIURI> internalReferrer;
        GetReferringURI(getter_AddRefs(internalReferrer));
        if (internalReferrer) {
            loadInfo->SetReferrer(internalReferrer);
        }

(that's getting the referrer that was used for page B and setting it on the loadInfo for the meta refresh) followed by this code in nsDocShell::InternalLoad:

    // XXXbz would be nice to know the loading principal here... but we don't
    nsCOMPtr<nsIPrincipal> loadingPrincipal;
    if (aReferrer) {
        nsCOMPtr<nsIScriptSecurityManager> secMan =
            do_GetService(NS_SCRIPTSECURITYMANAGER_CONTRACTID, &rv);
        NS_ENSURE_SUCCESS(rv, rv);

        rv = secMan->GetCodebasePrincipal(aReferrer,
                                          getter_AddRefs(loadingPrincipal));
    }

Comment 2

[Boris Zbarsky [:bzbarsky]]

We should just fix that "xxx" bit; at least use aOwner in cases when it's non-null...

Comment 3

[Markus Milleder]

I want to argue for treating the origin argument different from the referrer.
(Disclosure: I've originally reported this behavior as a RequestPolicy bug, see below)

A click expresses trust that the link target will know how to display the linked-to content.
By the time of a redirect, that trust is already transferred to the redirecting site, and any decision on whether the redirect itself is trustworthy should depend on my trust in the redirecting site.

The resolution of this bug may have an impact on content security policy, exactly in the case that the CSP allows accessing some external content (say, an external image) that get moved and replaced with a redirect. I believe I have seen cases where a popular image got moved to amazonaws.com.

RequestPolicy: https://www.requestpolicy.com/dev/ticket/210
Testcase (Firefox + Requestpolicy, all default settings)
> Google for FreeMind
> Click the first result - http://freemind.sourceforge.net/
> The page says "You'll be redirected", but nothing happens.
> Page source fragment: <meta http-equiv="refresh" content="0; 
URL=http://freemind.sourceforge.net/wiki/index.php/Main_Page">

The discussion here shows why RP blocks the redirect: There is no explicit trust for Google to access Sourceforge, and the implicit trust from the click has run out by arriving at the page with the redirect.
The Freemind front page would be implicitly trusted to redirect to the wiki page on the same domain, but the origin is still set to Google.

Comment 4

[Justin Samuel]

Boris, thanks for tracking down the relevant code. Fixing this will/should be a good task for me if nobody gets there first, but I probably won't have time to look at it until summer.

Markus, I believe you're saying that we should be on the lookout for whether this bug allows a CSP bypass. I haven't seen a case where this bug does affect CSP. For example, if a CSP policy allows a page to have images with source foo.com and foo.com redirects those images to bar.com, then CSP correctly blocks the loading of the image from bar.com (it doesn't follow the redirect). I don't think this bug will affect CSP for two reasons:

1) Redirects don't go through nsIContentPolicy::shouldLoad, so CSP wouldn't run into this bug in the normal case of redirected content.

2) From what I understand, this bug is only affecting a document that is refreshing to a different URI than the current document. I don't think there are any CSP directives that directly relate to document redirection.

3) I don't think CSP ever looks at the origin argument to shouldLoad, so it couldn't be affected by this bug. (That was based on a very quick look just now.)

Let me know if I misunderstood. If you do find a CSP bypass with this bug, I'd assume that would raise the priority of this bug and we could talk Brandon into fixing it rather than waiting on me getting around to taking a crack at it. (CC'ing Brandon, anyways, in case there's some impact on CSP that he could see.)

Comment 5

[Markus Milleder]

(In reply to comment #4)

I don't think this allows a CSP bypass.

My comment about CSP was rather to keep an eye on what CSP does in a similar situation:
> For example, if a CSP policy allows a page to have images with source
> foo.com and foo.com redirects those images to bar.com, then CSP correctly
> blocks the loading of the image from bar.com (it doesn't follow the redirect).

Given this description, another possible solution to the RequestPolicy problem would be to keep shouldLoad() as is and to show the "This page has asked to redirect..." bar when the refresh fires.

As an unrelated (possible) problem, the described CSP behavior will break the case I mentioned in comment #3 where a small hoster moves a popular large image to AWS to handle a load spike without touching the pages containing the image.
I'll pay attention if I see this again and submit I new bug if I do. (I haven't seen it on the pages in my permanent exception list :-(

Comment 6

[Boris Zbarsky [:bzbarsky]]

The patch in bug 767134 made us start using aOwner for the loadingPrincipal.

The problem is that meta refresh doesn't actually pass in an owner, afaict.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Whoever fixes this needs to be careful to not reintroduce 475636, though.