1195242 - The whole page should be replaced by a Safe Browsing warning when a frame or sub-resource is blocked

Status: NEW

(Toolkit :: Safe Browsing, defect, P3)

Description

[Pavel Zeldin]

Attachment: halog_jzip.halog

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/44.0.2403.155 Safari/537.36

Steps to reproduce:

Hi guys,

this issue occurs with the latest Firefox version, 40.0.2, on some selected urls with redirected links.
save as link works flawlessly, but with direct mouse click, there's no redirect and no download.
in some cases, there's an empty redirect response.
By now we've found out the following details:
If the user updates or upgrades to FF version 40.0.2 (clean machine) he can successfully download the files from the sites mentioned above in a period of ~ 20 min.
After that period it is no longer possible to download the files.
Restarting PC has no effect.
link example:
http://download.jzip.com/jZipSetup.exe
when downloading from firefox directly, the issue occurs. on any other browser and with save as link, there's no problem.


Actual results:

nothing happened, the get results in a 302, and nothing downloaded.


Expected results:

there should be a redirect to the corresponding file.

Comment 1

[Pavel Zeldin]

Attachment: koyotesoft_empty.halog

Comment 2

[Loic]

Your .exe link returns "Reported Unwanted Software Page!"

Comment 3

[:Gijs (out for now; he/him)]

(In reply to Loic from comment #2)
> Your .exe link returns "Reported Unwanted Software Page!"

Pavel, is this what you mean?

Comment 4

[Pavel Zeldin]

there was no response after the initial redirect, the browser simply dropped the connection, and did not provided any information or notification of the "Unwanted Software Page!".
as on direct access to the file, i think that the reported unwanted page should be shown to the end user, and let him choose if he's interested in continuing or not, just like in any other browser.

Comment 5

[:Gijs (out for now; he/him)]

(In reply to Pavel Zeldin from comment #4)
> there was no response after the initial redirect, the browser simply dropped
> the connection, and did not provided any information or notification of the
> "Unwanted Software Page!".
> as on direct access to the file, i think that the reported unwanted page
> should be shown to the end user, and let him choose if he's interested in
> continuing or not, just like in any other browser.

I also get the unwanted software warning. I don't really understand why you wouldn't, unless you've turned off safebrowsing (in the options/preferences, under "security", check for "block reported attack sites" and "block reported web forgeries".

As it is, that means we can't really reproduce the issue you seem to be seeing. Can you clarify further? Should clicking that link from this bugzilla page be enough to reproduce it? Can you try a clean Firefox profile? ( https://support.mozilla.org/kb/profile-manager-create-and-remove-firefox-profiles )
Safebrowsing should be on by default on any new Firefox profile.

Comment 6

[Pavel Zeldin]

when accessing directly to the file - from here - http://download.jzip.com/jZipSetup.exe, i do get the unwanted application.
however, when i click on this page - http://www.jzip.com/, on the Get Jzip, there is no unwanted app page, nothing happens.

Comment 7

[:Gijs (out for now; he/him)]

I can reproduce this on current beta. Not sure what's going on. Dragana and/or Christoph, can you take a look? Devtools don't even show any response, so it seems like something in networking is getting confused for some reason...

Comment 8

[Dragana Damjanovic [:dragana]]

So I had a look into this.
So from networking part everything is working correctly. Both do the same thing. The problem is the download gets blocked but there is no page to show the error, or something goes wrong there.
If you take the site that is not working and open download in separate tab (right -> Open in new tab) it is working properly.

When you click it tries to download from http://download.jzip.com/jZipSetup.exe but the tab stays on http://www.jzip.com/ and instead of showing error on that tab it thinks there is no tab to show the error... but I do not know this code ... 

So from log I see that the error page is constructed and when it should be shown the log shows:

2015-08-25 09:49:40.351392 UTC - 1577318976[130f1c0]: Shown Window: about:blocked?e=unwantedBlocked&u=http%3A//download.jzip.com/jZipSetup.exe&s=blacklist&c=UTF-8&f=regular&d=The%20site%20at%20download.jzip.com%20has%20been%20reported%20as%20serving%20unwanted%20software%20and%20has%20been%20blocked%20based%20on%20your%20security%20preferences.
2015-08-25 09:49:40.351397 UTC - 1577318976[130f1c0]:  Focused Window: http://www.jzip.com/

....
2015-08-25 09:49:44.010908 UTC - 1577318976[130f1c0]: <<Blur begin>>
2015-08-25 09:49:44.010913 UTC - 1577318976[130f1c0]: ISM: IMEStateManager::OnChangeFocus(aPresContext=0x0, aContent=0x0, aCause=CAUSE_UNKNOWN)
2015-08-25 09:49:44.010917 UTC - 1577318976[130f1c0]: ISM: IMEStateManager::OnChangeFocusInternal(aPresContext=0x0, aContent=0x0 (TabParent=0x0), aAction={ mCause=CAUSE_UNKNOWN, mFocusChange=FOCUS_NOT_CHANGED }), sPresContext=0x1f08e20, sContent=0x1fc0c30, sActiveTabParent=0x0, sActiveIMEContentObserver=0x0, sInstalledMenuKeyboardListener=false
2015-08-25 09:49:44.010922 UTC - 1577318976[130f1c0]: ISM:   IMEStateManager::OnChangeFocusInternal(), no nsPresContext is being activated
2015-08-25 09:49:44.010927 UTC - 1577318976[130f1c0]: Element a has been blurred



The working version write in log:
2015-08-25 09:52:07.223703 UTC - -1061078464[a2f1c0]: Shown Window: about:blocked?e=unwantedBlocked&u=http%3A//download.jzip.com/jZipSetup.ex&s=blacklist&c=UTF-8&f=regular&d=The%20site%20at%20download.jzip.com%20has%20been%20reported%20as%20serving%20unwanted%20software%20and%20has%20been%20blocked%20based%20on%20your%20security%20preferences.
2015-08-25 09:52:07.223709 UTC - -1061078464[a2f1c0]:  Focused Window: about:blocked?e=unwantedBlocked&u=http%3A//download.jzip.com/jZipSetup.ex&s=blacklist&c=UTF-8&f=regular&d=The%20site%20at%20download.jzip.com%20has%20been%20reported%20as%20serving%20unwanted%20software%20and%20has%20been%20blocked%20based%20on%20your%20security%20preferences.
2015-08-25 09:52:07.223831 UTC - -1061078464[a2f1c0]: <<Focus begin>>


I do not know this code to interpret the log.

Comment 9

[:Gijs (out for now; he/him)]

I tried to run mozregression on this but I can't get it to break at all because I don't have the safebrowsing db on the clean profile that mozregression uses. Anyway, I presume this is a regression. Christoph, if you have additional insight here that'd still be really useful.

Pavel, do you know if this worked in earlier releases, and if so in which one(s) ?

I should note that downloading that file sets off Windows Defender on my windows machine. :-\

Comment 10

[Christoph Kerschbaumer [:ckerschb]]

(In reply to :Gijs Kruitbosch from comment #9)
> I tried to run mozregression on this but I can't get it to break at all
> because I don't have the safebrowsing db on the clean profile that
> mozregression uses. Anyway, I presume this is a regression. Christoph, if
> you have additional insight here that'd still be really useful.

It sounds very similar to Bug 1136055 which we recently fixed. I also find that change uplifted to the current release [1], so we must go through a different code path/use a different channel and hence a different loadInfo. Unfortunately I don't have time to look into this problem today, but I can look into it later this week. Most likely it's a very similar issue to Bug 1136055, where mixed content after redirect blocks the request. (Somewhere around here [2]).

My guess is that the principal in the loadInfo is not the systemPrincipal and the loadingNode in the loadInfo is null, hence mixedContentBlocker bails because it can not query the root docshell and blocks the load.


[1] http://mxr.mozilla.org/mozilla-release/source/browser/base/content/nsContextMenu.js#1297
[2] http://mxr.mozilla.org/mozilla-central/source/dom/security/nsMixedContentBlocker.cpp#244
[3] http://mxr.mozilla.org/mozilla-central/source/dom/security/nsMixedContentBlocker.cpp#625

Comment 11

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #10)
> My guess is that the principal in the loadInfo is not the systemPrincipal
> and the loadingNode in the loadInfo is null, hence mixedContentBlocker bails
> because it can not query the root docshell and blocks the load.

See [3].

> [3]
> http://mxr.mozilla.org/mozilla-central/source/dom/security/
> nsMixedContentBlocker.cpp#625

Comment 12

[Loic]

My regression range:
good=2015-04-22
bad=2015-04-23
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=946ac85af8f4&tochange=0b202671c9e2

Comment 13

[Tanvi Vyas[:tanvi]]

Looks like this regression range is where unwanted software support was added.

8b191f5f9687	Francois Marier — Bug 1147212 - Add support for goog-unwanted-shavar. r=gcp,r=matej,r=smaug

Adding Francois.

Comment 14

[:Gijs (out for now; he/him)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #10)
> My guess is that the principal in the loadInfo is not the systemPrincipal
> and the loadingNode in the loadInfo is null,

This doesn't make sense to me, because the loadingNode is surely the linking <a> element or whatever?

> hence mixedContentBlocker bails
> because it can not query the root docshell and blocks the load.

I would have assumed that a network error page was always loadable, mixed content blocking or no. And either way, both source and destination in this case are on http://, not https, so I don't see why this breaks... The only thing special about the setup here seems to be the fact that the load is targeted to an iframe (#downframe). There's also an onclick attribute, but removing that doesn't make a difference. Removing the target attribute from the link to avoid loading this in the iframe makes the page reach the expected error page.

Comment 15

[Christoph Kerschbaumer [:ckerschb]]

(In reply to :Gijs Kruitbosch from comment #14)
> > hence mixedContentBlocker bails
> > because it can not query the root docshell and blocks the load.
> 
> I would have assumed that a network error page was always loadable, mixed
> content blocking or no. And either way, both source and destination in this
> case are on http://, not https, so I don't see why this breaks...

Even though it's not fixing the specific problem here, let me explain my thinking: If the loadingPrincipal is the systemPrincipal, then content policies are bypassed. If the loadingPrincipal is not the systemPrincipal and we do *not* provide a loadingNode in the loadInfo, then mixed content blocker returns false, because it cannot query the toplevel docshell [1]. Please note, in this case it doesn't matter if any involved schemes are http or https. But that's just for clarification.

I spent some time debugging, but as of now, I can only confirm that mixed content blocking is not responsible for the breakage here.

[1] http://mxr.mozilla.org/mozilla-central/source/dom/security/nsMixedContentBlocker.cpp#624

Comment 16

[Christoph Kerschbaumer [:ckerschb]]

I finally got a chance to debug that.

STR:
1) visit: http://www.jzip.com/
2) click [Get Jzip]

Current Behavior: Nothing happens
Expected Behavior: Unwanted App page should be displayed

I was able to verify that indeed unwanted software blocks the load; which is correct, because we get the same behavior when pasting the link directly into the URL bar: http://download.jzip.com/jZipSetup.exe

We are then about to load the following URL:

about:blocked?e=unwantedBlocked&u=http%3A//download.jzip.com/jZipSetup.exe&s=blacklist&c=UTF-8&f=regular&d=The%20site%20at%20download.jzip.com%20has%20been%20reported%20as%20serving%20unwanted%20software%20and%20has%20been%20blocked%20based%20on%20your%20security%20preferences


The problem is that the 'about:blocked' page does *not* get displayed as it should. The stacktrace underneath shows that everything until DoURILoad() seems to be correct. From that point on, I am not sure where the actual error happens.

Francois, did you ever run into such a problem with 'unwanted software'?

Also, the title of this bug is misleading, there is no redirect involved!


#1  0x00007fffe94fab11 in nsDocShell::DoURILoad (this=0x7fffc1b18000, aURI=0x7fffca69a590, aReferrerURI=0x0, aSendReferrer=true, aReferrerPolicy=0, aOwner=0x0, aTypeHint=0x0, 
    aFileName=..., aPostData=0x0, aHeadersData=0x0, aFirstParty=true, aDocShell=0x0, aRequest=0x7fffffff90d0, aIsNewWindowTarget=false, aBypassClassifier=false, 
    aForceAllowCookies=false, aSrcdoc=..., aBaseURI=0x0, aContentPolicyType=29) at /home/ckerschb/moz/mc/docshell/base/nsDocShell.cpp:10404
#2  0x00007fffe94d140c in nsDocShell::InternalLoad (this=0x7fffc1b18000, aURI=0x7fffca69a590, aReferrer=0x0, aReferrerPolicy=0, aOwner=0x0, aFlags=1, aWindowTarget=0x0, 
    aTypeHint=0x0, aFileName=..., aPostData=0x0, aHeadersData=0x0, aLoadType=65537, aSHEntry=0x0, aFirstParty=true, aSrcdoc=..., aSourceDocShell=0x7fffc1b181a8, aBaseURI=0x0, 
    aDocShell=0x0, aRequest=0x0) at /home/ckerschb/moz/mc/docshell/base/nsDocShell.cpp:10201
#3  0x00007fffe94e3af4 in nsDocShell::LoadErrorPage (this=0x7fffc1b18000, aURI=0x7fffd6755f00, aURL=0x0, aErrorPage=0x7fffffffa680 "blocked", 
    aErrorType=0x7fffffffa9d8 u"unwantedBlocked", 
    aDescription=0x7fffc3be3c08 u"The site at download.jzip.com has been reported as serving unwanted software and has been blocked based on your security preferences.", 
    aCSSClass=0x7fffebcb9792 <.L.str254> "blacklist", aFailedChannel=0x7fffc005e058) at /home/ckerschb/moz/mc/docshell/base/nsDocShell.cpp:5185
#4  0x00007fffe94e1525 in nsDocShell::DisplayLoadError (this=0x7fffc1b18000, aError=NS_ERROR_UNWANTED_URI, aURI=0x7fffd6755f00, aURL=0x0, aFailedChannel=0x7fffc005e058)
    at /home/ckerschb/moz/mc/docshell/base/nsDocShell.cpp:5058
#5  0x00007fffe94f13f9 in nsDocShell::EndPageLoad (this=0x7fffc1b18000, aProgress=0x7fffc1b18028, aChannel=0x7fffc005e058, aStatus=NS_ERROR_UNWANTED_URI)
    at /home/ckerschb/moz/mc/docshell/base/nsDocShell.cpp:7626
#6  0x00007fffe94ee3be in nsDocShell::OnStateChange (this=0x7fffc1b18000, aProgress=0x7fffc1b18028, aRequest=0x7fffc005e058, aStateFlags=131088, aStatus=NS_ERROR_UNWANTED_URI)
    at /home/ckerschb/moz/mc/docshell/base/nsDocShell.cpp:7158
#7  0x00007fffe94f15a5 in non-virtual thunk to nsDocShell::OnStateChange(nsIWebProgress*, nsIRequest*, unsigned int, nsresult) () at Unified_cpp_docshell_base0.cpp:7165
#8  0x00007fffe615b42c in nsDocLoader::DoFireOnStateChange (this=0x7fffc1b18000, aProgress=0x7fffc1b18028, aRequest=0x7fffc005e058, aStateFlags=@0x7fffffffb6b4: 131088, 
    aStatus=NS_ERROR_UNWANTED_URI) at /home/ckerschb/moz/mc/uriloader/base/nsDocLoader.cpp:1250
#9  0x00007fffe615aef1 in nsDocLoader::doStopDocumentLoad (this=0x7fffc1b18000, request=0x7fffc005e058, aStatus=NS_ERROR_UNWANTED_URI)
    at /home/ckerschb/moz/mc/uriloader/base/nsDocLoader.cpp:831
#10 0x00007fffe6159b4b in nsDocLoader::DocLoaderIsEmpty (this=0x7fffc1b18000, aFlushLayout=true) at /home/ckerschb/moz/mc/uriloader/base/nsDocLoader.cpp:721
#11 0x00007fffe615a933 in nsDocLoader::OnStopRequest (this=0x7fffc1b18000, aRequest=0x7fffc005e058, aCtxt=0x0, aStatus=NS_ERROR_UNWANTED_URI)
    at /home/ckerschb/moz/mc/uriloader/base/nsDocLoader.cpp:605
#12 0x00007fffe615ad3d in non-virtual thunk to nsDocLoader::OnStopRequest(nsIRequest*, nsISupports*, nsresult) () at Unified_cpp_uriloader_base0.cpp:609
#13 0x00007fffe50805b8 in nsLoadGroup::RemoveRequest (this=0x7fffc3612b00, request=0x7fffc005e058, ctxt=0x0, aStatus=NS_ERROR_UNWANTED_URI)
    at /home/ckerschb/moz/mc/netwerk/base/nsLoadGroup.cpp:643
#14 0x00007fffe5352a53 in mozilla::net::nsHttpChannel::OnStopRequest (this=0x7fffc005e000, request=0x7fffca99f800, ctxt=0x0, status=NS_ERROR_UNWANTED_URI)
    at /home/ckerschb/moz/mc/netwerk/protocol/http/nsHttpChannel.cpp:5941
#15 0x00007fffe5352f3d in non-virtual thunk to mozilla::net::nsHttpChannel::OnStopRequest(nsIRequest*, nsISupports*, nsresult) () at Unified_cpp_protocol_http1.cpp:5949
#16 0x00007fffe507d24c in nsInputStreamPump::OnStateStop (this=0x7fffca99f800) at /home/ckerschb/moz/mc/netwerk/base/nsInputStreamPump.cpp:717
#17 0x00007fffe507c222 in nsInputStreamPump::OnInputStreamReady (this=0x7fffca99f800, stream=0x7fffc3ffb000) at /home/ckerschb/moz/mc/netwerk/base/nsInputStreamPump.cpp:436
#18 0x00007fffe507d36f in non-virtual thunk to nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*) () at Unified_cpp_netwerk_base2.cpp:498
#19 0x00007fffe4eec970 in nsInputStreamReadyEvent::Run (this=0x7fffcad9b540) at /home/ckerschb/moz/mc/xpcom/io/nsStreamUtils.cpp:91
#20 0x00007fffe4f1a283 in nsThread::ProcessNextEvent (this=0x7ffff6b7ba00, aMayWait=false, aResult=0x7fffffffc0ee) at /home/ckerschb/moz/mc/xpcom/threads/nsThread.cpp:864
#21 0x00007fffe4f907b7 in NS_ProcessNextEvent (aThread=0x7ffff6b7ba00, aMayWait=false) at /home/ckerschb/moz/mc/xpcom/glue/nsThreadUtils.cpp:277
#22 0x00007fffe55a2adb in mozilla::ipc::MessagePump::Run (this=0x7fffe1e7d9c0, aDelegate=0x7ffff6b6a540) at /home/ckerschb/moz/mc/ipc/glue/MessagePump.cpp:95
#23 0x00007fffe5528335 in MessageLoop::RunInternal (this=0x7ffff6b6a540) at /home/ckerschb/moz/mc/ipc/chromium/src/base/message_loop.cc:234
#24 0x00007fffe5528265 in MessageLoop::RunHandler (this=0x7ffff6b6a540) at /home/ckerschb/moz/mc/ipc/chromium/src/base/message_loop.cc:227
#25 0x00007fffe552823d in MessageLoop::Run (this=0x7ffff6b6a540) at /home/ckerschb/moz/mc/ipc/chromium/src/base/message_loop.cc:201
#26 0x00007fffe89a0113 in nsBaseAppShell::Run (this=0x7fffd9715040) at /home/ckerschb/moz/mc/widget/nsBaseAppShell.cpp:156
#27 0x00007fffe9a0e9f7 in nsAppStartup::Run (this=0x7fffd9719100) at /home/ckerschb/moz/mc/toolkit/components/startup/nsAppStartup.cpp:281
#28 0x00007fffe9ae4085 in XREMain::XRE_mainRun (this=0x7fffffffc9b8) at /home/ckerschb/moz/mc/toolkit/xre/nsAppRunner.cpp:4292
#29 0x00007fffe9ae49df in XREMain::XRE_main (this=0x7fffffffc9b8, argc=1, argv=0x7fffffffdfa8, aAppData=0x7fffffffcc60) at /home/ckerschb/moz/mc/toolkit/xre/nsAppRunner.cpp:4389
#30 0x00007fffe9ae523f in XRE_main (argc=1, argv=0x7fffffffdfa8, aAppData=0x7fffffffcc60, aFlags=0) at /home/ckerschb/moz/mc/toolkit/xre/nsAppRunner.cpp:4478
#31 0x0000000000405a20 in do_main (argc=1, argv=0x7fffffffdfa8, xreDirectory=0x7ffff6b4c900) at /home/ckerschb/moz/mc/browser/app/nsBrowserApp.cpp:212
#32 0x000000000040515c in main (argc=1, argv=0x7fffffffdfa8) at /home/ckerschb/moz/mc/browser/app/nsBrowserApp.cpp:399

Comment 17

[:Gijs (out for now; he/him)]

Gah, I should have spotted this sooner.

This works as expected. The error page is simply loaded into the iframe (which makes sense - that's where the link points), and the iframe is invisible.

It's really not clear what else we would be expected to do here - loading the error page in the parent frame will break other usecases, even if it might seem to be the "right" thing here.

I'll leave the needinfo on francois to decide if we need to do something else here, but on the whole I'm tempted to suggest invalid/wontfix.

Comment 18

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Gijs Kruitbosch (PTO until Sep 6) from comment #17)
> This works as expected. The error page is simply loaded into the iframe
> (which makes sense - that's where the link points), and the iframe is
> invisible.

Ah, now it all makes sense to me. Thanks Gijs.

> I'll leave the needinfo on francois to decide if we need to do something
> else here, but on the whole I'm tempted to suggest invalid/wontfix.

In that case I also suggest invalid/wontfix.

Comment 19

[François Marier [:francois]]

I think it would be great if we had a better console log message (than the current "NS_ERROR_NOT_AVAILABLE:"), but I think it is indeed working as intended. So perhaps it's not entirely invalid/wontfix.

Is this "loading a download in an invisible iframe" pattern common?

Comment 20

[:Gijs (out for now; he/him)]

(In reply to François Marier [:francois] from comment #19)
> I think it would be great if we had a better console log message (than the
> current "NS_ERROR_NOT_AVAILABLE:"), but I think it is indeed working as
> intended. So perhaps it's not entirely invalid/wontfix.
> 
> Is this "loading a download in an invisible iframe" pattern common?

It's definitely not unheard of, yes. For instance, iTunes does something similar, and then redirects the toplevel/parent page to a different "success, you downloaded iTunes!" page (which has caused issues for us in the past because the downloading iframe goes away).

The thing that worries me here is that the "unwanted software" page is big and red and scary - and it's big and red and scary for a reason. Anything else that doesn't require the iframe/window, like (say) an infobar, wouldn't be nearly as scary.

We could consider making that page break out of its iframe when displayed... François, does that sound sane?

Comment 21

[François Marier [:francois]]

Attachment: Screenshot of the error page in Chrome

Alternatively, we could do like Chrome and navigate the top-level page to the error page.

Comment 22

[François Marier [:francois]]

(In reply to Gijs Kruitbosch (PTO until Sep 6) from comment #20)
> We could consider making that page break out of its iframe when displayed...
> François, does that sound sane?

Sounds good to me because it tells users what's going on instead of silently blocking the download and just looking like Firefox is broken.

Comment 23

[:Gijs (out for now; he/him)]

I just realized we actually have telemetry for how much stuff happens in frames and how much in toplevel loads:

https://telemetry.mozilla.org/new-pipeline/dist.html#!cumulative=0&end_date=2015-09-29&keys=__none__!__none__!__none__&max_channel_version=release%252F41&measure=SECURITY_UI&min_channel_version=null&product=Firefox&sanitize=1&sort_keys=submissions&start_date=2015-09-17&table=0&trim=1&use_submission_date=0

This shows that about 1/3 of the malwareBlocked pages are shown in iframes, and more like 2/5 of the unwantedBlocked pages.

It seems fair to want to fix this so that people are notified about these attempts and there's an impact (and therefore impetus to fix, either by contacting Google to get off the list, or the ad network to fix its ads, or...) to the sites to actually do something about it (rather than the framing of stuff to try to evade user notice where possible).

François, do you have cycles to work on this?

Comment 24

[François Marier [:francois]]

So the consensus is that we want to block the whole page (with the big red interstitial warning page) whenever a page contains an iframe on the unwanted list?

Comment 25

[:Gijs (out for now; he/him)]

(In reply to François Marier [:francois] from comment #24)
> So the consensus is that we want to block the whole page (with the big red
> interstitial warning page) whenever a page contains an iframe on the
> unwanted list?

I haven't asked anyone else - who did the UX for the original implementation? It'd be good to check with them.

That said, considering Chrome already does this, and considering this bugreport + telemetry, it seems like it:
1) solves a real problem we have today (see this bugreport);
2) shouldn't violate assumptions that sites have been making about how this is supposed to work;
3) may actively help protect users by the negative incentive of having this error page appear across the entire thing (instead of just in a frame that is as likely as not to be invisible anyway).

So it does seem like it'd be sensible. Anyway, let's doublecheck with UX before proceeding, I guess. Can you forward this appropriately?

Comment 26

[François Marier [:francois]]

(In reply to :Gijs Kruitbosch from comment #25)
> I haven't asked anyone else - who did the UX for the original
> implementation? It'd be good to check with them.

I'm not sure who that would be (and not entirely sure there ever was a UX person involved), but I'll CC Bram who is the UX person behind the new TLS error pages.
 
> So it does seem like it'd be sensible. Anyway, let's doublecheck with UX
> before proceeding, I guess. Can you forward this appropriately?

I agree, the Chrome behavior is better than what we've got, so we may as well copy it as a first step.

Comment 27

[Bram Pitoyo [:bram]]

I’ve adapted the security error pages to handle warnings for attack sites and sites carrying unwanted software.

http://brampitoyo.github.io/fx-untrusted-connection/attack.xhtml
http://brampitoyo.github.io/fx-untrusted-connection/unwanted.xhtml

In each case, you’ll see that I’ve titled the page “Harmful Site” and wrote a strong warning that says “This site is trying to infect your computer”. Under the Advanced link, I’ve linked to the Google Safe Browsing diagnostic page so technical users can see it if they’re interested.

Lastly, there’s of course an unsafe link to go ahead and load the site in question.

Google Chrome uses an “x” padlock icon on a striking red background, and Safari uses a red text. How does this design fare in comparison? Thoughts?

Comment 28

[:Gijs (out for now; he/him)]

(In reply to Bram Pitoyo [:bram] from comment #27)
> I’ve adapted the security error pages to handle warnings for attack sites
> and sites carrying unwanted software.
> 
> http://brampitoyo.github.io/fx-untrusted-connection/attack.xhtml
> http://brampitoyo.github.io/fx-untrusted-connection/unwanted.xhtml
> 
> In each case, you’ll see that I’ve titled the page “Harmful Site” and wrote
> a strong warning that says “This site is trying to infect your computer”.
> Under the Advanced link, I’ve linked to the Google Safe Browsing diagnostic
> page so technical users can see it if they’re interested.
> 
> Lastly, there’s of course an unsafe link to go ahead and load the site in
> question.
> 
> Google Chrome uses an “x” padlock icon on a striking red background, and
> Safari uses a red text. How does this design fare in comparison? Thoughts?

The problem is not to do with the styling of the page, it's to do with the page not showing up at all if it's inside a frame that is not visible to the user. The question is whether we should replace the toplevel page with the warning in that case.

Comment 29

[François Marier [:francois]]

(In reply to Bram Pitoyo [:bram] from comment #27)
> I’ve adapted the security error pages to handle warnings for attack sites
> and sites carrying unwanted software.

I moved these mocks to bug 1214859 since it's separate from fixing this one.

Comment 30

[Bram Pitoyo [:bram]]

(In reply to :Gijs Kruitbosch from comment #28)
> […] The question is whether we should replace the toplevel page with the
> warning in that case.

Sorry for misunderstanding the bug. I thought it covered the design and copy of the warning page.

My answer would be “yes”. Our warning page should replace the top level page, so malicious sites can’t be sneaky and hide our warning inside an invisible frame.

Comment 31

[:Gijs (out for now; he/him)]

Attachment: MozReview Request: Bug 1195242 - force malware and phishing block warnings to break out of frames at the docshell level, r?bz

Review commit: https://reviewboard.mozilla.org/r/31603/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/31603/

Comment 32

[:Gijs (out for now; he/him)]

(In reply to :Gijs Kruitbosch from comment #31)
> Created attachment 8709910 [details]
> MozReview Request: Bug 1195242 - force malware and phishing block warnings
> to break out of frames at the docshell level, r?bz
> 
> Review commit: https://reviewboard.mozilla.org/r/31603/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/31603/

FWIW, I am a little bit... uneasy... about this, in that if your site gets XSS'd you can use the blocking mechanism to defeat e.g. iframe sandboxing. That said, hopefully the page is scary enough that people *don't* just click through (do we have data on that?), and it would likewise be bad if we did allow the page to be loaded in the iframe if it were sandboxed or somehow (CSP?) prevented from breaking out of the iframe at the content level (which is why I elected not to just make this a bit of JS on the page itself).

Boris, if you want to delegate this review, I totally understand, but I wanted to at least get your thoughts on that issue, and it seems you, smaug and jst (maybe?) are the only active docshell peers at this point...

Comment 33

[Boris Zbarsky [:bzbarsky]]

> That said, hopefully the page is scary enough that people *don't* just click through 

Can we just disable the option of clicking through if we forced the load into toplevel like that?

If not, I'm really not terribly happy with this.  How does Chrome handle this in the sandboxed case, out of curiosity?

Comment 34

[Cody Crews]

You should see bug 1239729 bz.  I use click jacking there, but I'm fairly sure I could have done some nasty things even without having about:blocked in an iframe(was a slight tweak at one point).

I really just don't want to see a solution that relies on about:blocked taking over the whole tab, or an at best not quite right tweak of how chrome handles this.  Whatever you guys decide on I'll put it on my list to get to quickly.

Both of these scenarios make me thirsty in terms of new ways to possibly attack them :-)

Comment 35

[:Gijs (out for now; he/him)]

(In reply to Boris Zbarsky [:bz] from comment #33)
> > That said, hopefully the page is scary enough that people *don't* just click through 
> 
> Can we just disable the option of clicking through if we forced the load
> into toplevel like that?

I'm not sure how much use that is considering we keep the URL - hit enter in the URL bar and it'll re-prompt without the 'ignore' thing being disabled...

> If not, I'm really not terribly happy with this.  How does Chrome handle
> this in the sandboxed case, out of curiosity?

Not sure off-hand, will check.

Comment 36

[Boris Zbarsky [:bzbarsky]]

> hit enter in the URL bar and it'll re-prompt without the 'ignore' thing being disabled

Um... changing the URL of a page the user actually loaded to the URL of a known attack page seems _really_ fishy.  :(

Comment 37

[:Gijs (out for now; he/him)]

(In reply to Boris Zbarsky [:bz] from comment #36)
> > hit enter in the URL bar and it'll re-prompt without the 'ignore' thing being disabled
> 
> Um... changing the URL of a page the user actually loaded to the URL of a
> known attack page seems _really_ fishy.  :(

Maybe we should just deny the load when not top-level and not display a page at all. (I'll still check Chrome in a bit, in a mtg...)

Comment 38

[:Gijs (out for now; he/him)]

(In reply to Boris Zbarsky [:bz] from comment #33)
> If not, I'm really not terribly happy with this.  How does Chrome handle
> this in the sandboxed case, out of curiosity?

It loads the error page at the top-level (even when the iframe has a sandbox="" attribute), but keeps the URL in the URL field the same as it was.

We could probably fix the URL somehow... I'm actually not sure how it ends up there right now, maybe frontend distills it from the error page's URL...

(In reply to Cody Crews from comment #34)
> You should see bug 1239729 bz.  I use click jacking there, but I'm fairly
> sure I could have done some nasty things even without having about:blocked
> in an iframe(was a slight tweak at one point).

FWIW, I'm aware that we should still add mitigations to about:blocked similar to what we have done for some of our dialogs/panels, but that is in principle orthogonal to what we do here.

Comment 39

[:Gijs (out for now; he/him)]

Comment on attachment 8709910 [details]
MozReview Request: Bug 1195242 - force malware and phishing block warnings to break out of frames at the docshell level, r?bz

Whatever we do decide, this does not seem like the right thing.

Comment 43

[François Marier [:francois]]

The commit identified in comment 13 increased the likelihood that the problem will show up because we expanded the list of sites we show warnings for. It's not a regression since this has always been the behavior when encouraging unsafe things in iframes.

Comment 45

[Bogdan Maris, Desktop Test Engineering]

Still affected in Fx 64.0b8.

Comment 46

[ovidiu boca[:Ovidiu]]

I can still reproduce this on Mac OS X 10.13, Windows 10 and Ubuntu 16.04 with FF Nightly 68.0a1(2019-04-02), FF Beta 67 and Release 66.

Comment 48

[Daniel Veditz [:dveditz]]

https://testsafebrowsing.appspot.com/ links to handy testcases for various situations including this one, possibly from the Chrome or SafeBrowsing team.

Comment 49

[Johann Hofmann [:johannh]]

I think we should WONTFIX this bug in favor of disallowing adding an exception for iframe-based SafeBrowsing pages instead, here's why:

• The spoofing case highlighted in bug 1546695 already works with other necessarily embeddable about: pages, such as about:certerror, so we can't fix it.
• I don't think any user, even experienced ones, can make a trust decision for malicious.com when well-trusted.com embeds it based on the limited information they have on the SafeBrowsing page. It's just not possible to correctly assess the security situation here, I can hardly imagine a scenario where I would want the outcome of this to be that the user whitelists malicious.com based on seeing a warning about well-trusted.com.
• Isn't this a DOS vector? Any frame with script-executing capabilities (which I guess is most of them) on your site can trivially escape their boundary and block your page by navigating to a known blocked domain.

This breakage really seems like an edge case that we could live with. We could consider spawning the red notification bar with a text saying something like "Malicious code was detected and blocked on this page. Proceed with caution".

Let me know what you think. Dan and Gijs, maybe you have an opinion on this?

Comment 50

[:Gijs (out for now; he/him)]

(In reply to Johann Hofmann [:johannh] from comment #49)

I think we should WONTFIX this bug in favor of disallowing adding an exception for iframe-based SafeBrowsing pages instead, here's why:

• The spoofing case highlighted in bug 1546695 already works with other necessarily embeddable about: pages, such as about:certerror, so we can't fix it.

We disallow exceptions in framed about:cert/neterror, right? Per bug 442929 (duped to here), do we also disallow exceptions for framed safebrowsing pages already, or not?

I think it's conceivable that about:blocked/about:cert/neterror have different phishing characteristics, ie there may be a "point" to phishing one but not the others. If, as a user, I see a "we blocked this dangerous page" warning that has some UI on it that suggests I click it, and afterwards I get another block page, I might be inclined to unblock. I'm less convinced that'd work with certificate or network error pages (and also, I'm not sure I see a point in doing it - it implies you have a reachable site with valid TLS credentials but need to go out of your way to get someone to add an exception for some other site with invalid credentials?).

In other words, even if we can't do this for neterror/certerror, I'm not convinced that that means we shouldn't do it for about:blocked.

• I don't think any user, even experienced ones, can make a trust decision for malicious.com when well-trusted.com embeds it based on the limited information they have on the SafeBrowsing page. It's just not possible to correctly assess the security situation here, I can hardly imagine a scenario where I would want the outcome of this to be that the user whitelists malicious.com based on seeing a warning about well-trusted.com.

I'm happy to disallow adding an exception, but that doesn't fix this bug as filed, or the duped spoofing bug.

• Isn't this a DOS vector? Any frame with script-executing capabilities (which I guess is most of them) on your site can trivially escape their boundary and block your page by navigating to a known blocked domain.

That doesn't seem like a strong motivation not to do this to me; iframe sandboxing can prevent navigation and should be used by sites that embed content they don't trust, irrespective of this bug. If the embedding site is directly embedding a blocked domain, showing the warning across the entire page seems better than having a potentially half-broken site (e.g. because postMessage etc. aren't doing what the parent page expects) without clear info for the user.

This breakage really seems like an edge case that we could live with.

I mean, we've lived with it for (at least) 4 years now so clearly yes, but IMHO this is not ideal.

We could consider spawning the red notification bar with a text saying something like "Malicious code was detected and blocked on this page. Proceed with caution".

And do what with the page? Still allow the parent to be embedding it, potentially adding confusing/spoofing UI to it?

Comment 51

[Daniel Veditz [:dveditz]]

(In reply to Johann Hofmann [:johannh] from comment #49)

I think we should WONTFIX this bug in favor of disallowing adding an exception for iframe-based SafeBrowsing pages instead

If we're showing User Interface (error pages, in this case) in a frame we most certainly should be disallowing any actions taken from them. Way too easy to clickjack. If the user wants to make an exception they can use the context menu to show the frame as a new tab and add the exception from there. The user has no way of safely knowing to whom they're granting the exception when it's in a frame.

That aspect of this should go without question, but was this bug ever really about "should there be exceptions in the frame"? It was about not showing the user the page tried to do something malicious because the malicious stuff was running in a tiny frame.

I can hardly imagine a scenario where I would want the outcome of this to be that the user whitelists malicious.com based on seeing a warning about well-trusted.com.

Agreed.

• Isn't this a DOS vector? Any frame with script-executing capabilities (which I guess is most of them) on your site can trivially escape their boundary and block your page by navigating to a known blocked domain.

You don't need script: you can use a meta-refresh or even just a 30x from the server when the browser requests the frame document.

There are a lot of permutations here. Is it a phishing block? Hard to phish if you can't see the content so blocking just in the framed content should be sufficient. What if it's malicious content? If it's "social engineering" you need to see it, and again blanking the frame should work. But if it's some kind of malware attempt, or a download which iirc was the original case in this bug, what then? We blocked it, does the user need to know? The framing site's author?

We could consider spawning the red notification bar with a text saying something like "Malicious code was detected and blocked on this page. Proceed with caution".

That might be a good compromise. Notification of badness is really what I'm interested in and that would do it, without blocking the content of the innocent (presumably) framing site.

Comment 52

[Daniel Veditz [:dveditz]]

(In reply to Johann Hofmann [:johannh] from comment #49)

• I don't think any user, even experienced ones, can make a trust decision for malicious.com when well-trusted.com embeds it based on the limited information they have on the SafeBrowsing page.

NOTE: That is NOT the original scenario here. The top-level site itself had links to malicious downloads. The only reason frames come into the picture is because links targeting hidden frames is one common way to do download links without the user navigating away from your page. Putting the "malicious download" warning in an invisible frame is exactly the wrong thing. Silent failure of an expected download might also encourage the user to do unwise things, like right-click to copy the URL and download it in another tool (if they open it in another Firefox tab we get the chance to show the warning, of course).

In any of these scenarios we don't actually know the top-level site is fine, all we know is it's not on the SafeBrowsing list. Could be reputable-but-hacked, could be reputable-but-adframe-hacked, could be a typosquatting domain where the user thinks it's reputable but it's a not-yet-blocked scam (e.g. searchfox.COM was recently offering Firefox-branded fake-flash and other malware).

I also worry that the SafeBrowsing team will make decisions about which domains to put on the list based on Chrome behavior. If there are a bunch of evil sites all pushing the same common malware for Chrome they can just list the common bit and all the sites will be blocked -- no need (for them) to bloat the list with each variant that pops up. We've seen (anecdata) examples of this in the past where the containing page was obviously bogus but not on the SafeBrowsing list, but of course we don't know exactly why the containing page wasn't added. Maybe it was just too new? testcases hosted at https://testsafebrowsing.appspot.com/ show some of the Chrome/Firefox differences.

Comment 53

[Alin Ilea, Desktop Test Engineering]

Hi,

Then issue is reproducible in latest Nightly version 70.0a1 (2019-07-16) , on Windows 8.

Comment 54

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

Change the status for beta to have the same as nightly and release.
For more information, please visit auto_nag documentation.

Comment 55

[:Gijs (out for now; he/him)]

The work in bug 1591474 to make this infrastructure work under fission will help with making per-frame decisions.

Comment 56

[Marcela (Please NI request to Brindusa Tot)]

I was able to reproduce this issue on latest Nightly version 74.0a1 (2020-02-05) on Windows 10

Comment 57

[Marcela (Please NI request to Brindusa Tot)]

This issue is reproducible on latest Nightly version 76.0a1 on Ubuntu 18. Change flags accordingly.

Comment 58

[Timea Cernea [:tbabos][inactive]]

Updating affected flags for FX78 and Fx77.

Comment 59

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

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

For more information, please visit auto_nag documentation.

Comment 60

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