435081 - Malware protection "Ignore this warning" link does not work if the URL contains a # fragment (anchor)

Status: REOPENED

(Toolkit :: Safe Browsing, defect, P3)

Description

[Lee Houghton]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9pre) Gecko/2008052006 Firefox/3.0pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9pre) Gecko/2008052006 Firefox/3.0pre

If the URL contains a fragment portion, the "Ignore this warning" link merely shows a "Reported Attack Site!" info bar, but does not show the actual page.

Reproducible: Always

Steps to Reproduce:
1. Visit http://www.mozilla.com/firefox/its-an-attack.html
2. Click "Ignore this warning"
3. Visit http://www.mozilla.com/firefox/its-an-attack.html#test
4. Click "Ignore this warning"
Actual Results:  
1. Malware protection page shows
2. Clicking "Ignore this warning" proceeds to the actual page
3. Malware protection page shows
4. Click "Ignore this warning" does not proceed to the actual page

Expected Results:  
1. Malware protection page shows
2. Clicking "Ignore this warning" proceeds to the actual page
3. Malware protection page shows
4. Click "Ignore this warning" proceeds to the actual page

Comment 1

[juan becerra [:juanb]]

Confirmed on Fx3rc1 on Vista, and if you keep clicking on the "ignore this warning" link in step 4 you'll have to click as many times on the X on the "Reported Web Forgery!" info bar that shows on top.

Comment 2

[Johnathan Nightingale [:johnath]]

Confirmed the behaviour described. Not a showstopper for me since blocked malware is not something we're in a real rush to allow access to, and there's an easy (though non-obvious) workaround, but it's a touch surprising to me that it happens.  Nothing in the error console either.

Comment 3

[Johnathan Nightingale [:johnath]]

The front-end code that hands off the clickthrough attempt is here:

http://mxr.mozilla.org/mozilla/source/browser/base/content/browser.js#2325

Comment 4

[Daniel Veditz [:dveditz]]

Flagging for 1.9.0.x so we don't forget about it.

Comment 6

[Samuel Sidler (old account; do not CC)]

Johnath is the lucky winner of this bug!

Comment 7

[Ingemar Nilsson]

A similar bug seems to exist for attack sites that are supposed to be displayed in an iframe. Go to the following page (since it may be an attack site, use NoScript or turn off scripting globally while doing this):

http://www.gondolen.se/default.asp?n1=3&n2=42

The center iframe will display the "Reported attack site" warning. Clicking on the ignore link will not display the page, but will display the malware information bar mentioned above. The bars stack up, so if you click several times, the bar has to be closed several times to actually go away.

Now view the page source, and find the iframe. Copy the URL and paste it into a new tab. Now click on the "ignore this warning" link, and the expected page shows.

With Javascript and other scripting systems turned off, are these "attack sites" really still dangerous? Maybe Firefox could turn off scripts (I use the definition broadly, and thus it includes any locally executed content) on a page reported as a malware site? Or maybe this is more appropriately handled by NoScript, which could allow me to ignore attack site warnings for sites where I do not allow scripts to run. Just some ideas.

Comment 8

[Lee Houghton]

Attachment: Patch

This passes the LOAD_FLAGS_IS_REFRESH to the loadURI method so that instead of just scrolling to the anchor in the error document, it loads the blocked document.

It also fixes the case where a blocked page is contained in an <iframe> by getting the nsIWebNavigation from the dom window containing errorDoc and using that instead of gBrowser.

PS. This is my first patch, so if it happens to be the worst patch ever, sorry :)

Comment 9

[Johnathan Nightingale [:johnath]]

(In reply to comment #8)
> Created an attachment (id=337824) [details]
> Patch
> 
> This passes the LOAD_FLAGS_IS_REFRESH to the loadURI method so that instead of
> just scrolling to the anchor in the error document, it loads the blocked
> document.
> 
> It also fixes the case where a blocked page is contained in an <iframe> by
> getting the nsIWebNavigation from the dom window containing errorDoc and using
> that instead of gBrowser.
> 
> PS. This is my first patch, so if it happens to be the worst patch ever, sorry
> :)

That's actually pretty hot, Lee.  I just sat down with gavin and spent a couple minutes understanding why we didn't catch that ourselves, but it's a great solution.  I assume you've tested this to ensure it fixes the problem?

Way to go.  Really - I know it seems little, but you've made Firefox better.

Comment 10

[Johnathan Nightingale [:johnath]]

Comment on attachment 337824 [details] [diff] [review]
Patch

Couple of drive-by requests:

>+        // Get the nsIWebNavigation from the error document in case it is 
>+        // inside a frame element.
>+        var webNav = errorDoc.defaultView.QueryInterface(Ci.nsIInterfaceRequestor)
>+                                          .getInterface(Ci.nsIWebNavigation);

Can you add "(Bug 442929)" to the comment here?

>+        // Pass LOAD_FLAGS_IS_REFRESH so that it will not simply scroll to an anchor 
>+        // in the document if that's part of the URI.
>+        webNav.loadURI(errorDoc.location.href,
>+                       nsIWebNavigation.LOAD_FLAGS_BYPASS_CLASSIFIER |
>+                       nsIWebNavigation.LOAD_FLAGS_IS_REFRESH, 
>+                       null, null, null);

And here, can you reference bug 435081 in the comment?

Comment 11

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 337824 [details] [diff] [review]
Patch

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js

>+        // Get the nsIWebNavigation from the error document in case it is 
>+        // inside a frame element.
>+        var webNav = errorDoc.defaultView.QueryInterface(Ci.nsIInterfaceRequestor)
>+                                          .getInterface(Ci.nsIWebNavigation);
>+        // Pass LOAD_FLAGS_IS_REFRESH so that it will not simply scroll to an anchor 
>+        // in the document if that's part of the URI.
>+        webNav.loadURI(errorDoc.location.href,
>+                       nsIWebNavigation.LOAD_FLAGS_BYPASS_CLASSIFIER |
>+                       nsIWebNavigation.LOAD_FLAGS_IS_REFRESH, 
>+                       null, null, null);

Thanks, Lee! Now that you're using the right webNav (which fixes bug bug 442929 too!), I think we can simplify this further to just:

webNav.reload(nsIWebNavigation.LOAD_FLAGS_BYPASS_CLASSIFIER);

Want to try that and post another patch?

Comment 12

[Johnathan Nightingale [:johnath]]

So, I confirm Lee's patch works, but gavin's suggested simplification doesn't.  That's no surprise though, since we found that docshell's reload() doesn't check for the BYPASS_CLASSIFIER flag the way that loadURI does.  What's less happy-making is that even adding the check doesn't seem to make things work.

We can get dcamp and/or bz to chime in here - I wonder how deep this rabbit hole goes...

Poor, poor Lee.  :)

Comment 13

[Johnathan Nightingale [:johnath]]

To be clear - the reason why we might not prefer to just take your patch's approach as-is is that the flag you're using is really intended for meta-refreshes, not for this kind of reload.  Maybe we look the other way on it, but I think we'd rather understand what's going on.

Comment 14

[:Gavin Sharp [email: [e-mail]]]

I tried adding:

        PRUint32 flags = INTERNAL_LOAD_FLAGS_NONE;
        if (aReloadFlags & LOAD_FLAGS_BYPASS_CLASSIFIER)
          flags |= INTERNAL_LOAD_FLAGS_BYPASS_CLASSIFIER;

to ::Reload, and passed "flags" to the InternalLoad call, but that doesn't fix it, because it looks like we're not even hitting that case, instead going through one of the LoadHistoryEntry calls. LoadHistoryEntry takes a loadtype, not load flags, so I guess that means we'd need to create a new loadtype that includes LOAD_FLAGS_BYPASS_CLASSIFER? I'm kind of confused about how the various docshell flags/loadtypes interact.

Comment 15

[Lee Houghton]

Mid-air collision! I'll just submit what I was already going to submit:

(In reply to comment #13)
> To be clear - the reason why we might not prefer to just take your patch's
> approach as-is is that the flag you're using is really intended for
> meta-refreshes, not for this kind of reload.  Maybe we look the other way on
> it, but I think we'd rather understand what's going on.
Yes, I understand and I agree. 

(In reply to comment #12)
> So, I confirm Lee's patch works, but gavin's suggested simplification doesn't. 
> That's no surprise though, since we found that docshell's reload() doesn't
> check for the BYPASS_CLASSIFIER flag the way that loadURI does.  What's less
> happy-making is that even adding the check doesn't seem to make things work.
> 
> We can get dcamp and/or bz to chime in here - I wonder how deep this rabbit
> hole goes...
> 
> Poor, poor Lee.  :)

Where were you adding the check? Before it calls InternalLoad?

I think if you add a check there it won't work in this case because when it gets into Reload it goes into LoadHistoryEntry instead, and LoadHistoryEntry also doesn't check for the LOAD_FLAGS_BYPASS_CLASSIFIER flag. So I tried adding a check for it (LoadHistoryEntry doesn't get the actual aReloadFlags, just the "load type", so I checked that):

    PRUint32 loadFlags = INTERNAL_LOAD_FLAGS_NONE;
    if(LOAD_TYPE_HAS_FLAGS(aLoadType, LOAD_FLAGS_BYPASS_CLASSIFIER))
        loadFlags |= INTERNAL_LOAD_FLAGS_BYPASS_CLASSIFIER;

(and then passing that to InternalLoad)

But it didn't work (gah!) because MAKE_LOAD_TYPE/LOAD_TYPE_HAS_FLAGS expects the flags parameter to fit in 16 bits, and LOAD_FLAGS_BYPASS_CLASSIFIER is 0x10000 which doesn't. The LOAD_FLAGS_BYPASS_CLASSIFIER bit gets bit shifted out. I don't know what to do about that. :(

(Note: there's also a few flags like nsIWebNavigation::LOAD_FLAGS_MASK (0xFFFF) and LOAD_FLAGS_ERROR_PAGE (0x8000, "This should be bigger than all flags on nsIWebNavigation" in nsDocShellLoadTypes.h which seem oblivious to the existence of LOAD_FLAGS_BYPASS_CLASSIFIER, but I don't think they're immediately responsible for anything)

Comment 16

[:Gavin Sharp [email: [e-mail]]]

Yeah, BYPASS_CLASSIFIER is in EXTRA_LOAD_FLAGS, and doesn't have an associated loadType. Perhaps that can be changed without too much trouble? I think you'd need to ask bz or dcamp or biesi.

Comment 17

[Boris Zbarsky [:bzbarsky]]

OK.  So today I ran into the bustage that comment 15 is all about and landed a patch that fixes some of it and documents the rest for now (and in particular documents that you can't pass the classifier flag to reload()).

We could just make reload() handle the flag (and EXTRA_LOAD_FLAGS in general) by masking it off early enough in reload(), like loadURI() does, and adding load flag arguments to LoadHistoryEntry and copying some code from the nsIURI version of LoadURI into LoadHistoryEntry and Reload() (or better yet factoring that code out into a helper method).  I think that should do the right thing, since the history entry is for the page that we really tried to load, not for the error page (or at least hitting reload does the right thing).

I'd probably prefer that to using LOAD_FLAGS_IS_REFRESH, which is currently unused as the comments say...

Comment 20

[Roman R.]

Still happens in 43.0a1 (2015-09-11)

http://seriesparaassistironline.org/2015/08/assistir-online-continuum-s04e01-4x01-legendado.html#more-31055

Comment 21

[François Marier [:francois]]

Here's a better test case for it (hard-coded in the internal list): http://itisatrap.org/firefox/its-a-trap.html#foo

Comment 22

[BMO Automation]

Per policy at https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Inactive_Bugs. If this bug is not an enhancement request or a bug not present in a supported release of Firefox, then it may be reopened.

Comment 23

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

The severity field for this bug is relatively low, S4. However, the bug has 3 duplicates.
:dimi, could you consider increasing the bug severity?

For more information, please visit auto_nag documentation.

Comment 24

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