Closed Bug 1147212 Opened 9 years ago Closed 9 years ago

Support goog-unwanted-shavar for detecting potentially unwanted software

Categories

(Core :: DOM: Security, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla40
Tracking Status
firefox40 --- verified
relnote-firefox --- 40+

People

(Reporter: mmc, Assigned: francois)

References

(Depends on 1 open bug)

Details

Attachments

(4 files, 5 obsolete files)

Google just announced support for goog-unwanted-shavar, which they said should be treated similar to goog-malware-shavar and goog-phish-shavar and show an interstitial when a URL matches. We should support this in Firefox.
I guess we need a new list category for this? Or we need to re-architect the code displaying the warning can check the actual list name and make the call what page to show there?
(In reply to Gian-Carlo Pascutto [:gcp] from comment #3)
> I guess we need a new list category for this? Or we need to re-architect the
> code displaying the warning can check the actual list name and make the call
> what page to show there?

We need a new error code. Somewhere deep in the docshell code there is a mapping from NS_ERROR_PHISHING_URI -> about:phishing, etc.
>We need a new error code. Somewhere deep in the docshell code there is a mapping from 
>NS_ERROR_PHISHING_URI -> about:phishing, etc.

Do you think it's most sensible to just extend this, or should we think about just pushing list names through all of those layers and let the code that has to pick out the error page make the mapping between list names and what error to show?

I mean, currently we have "malware", "phishing" tables with associated warnings, "tracking" with tables but no warnings, so soon we'll have "unwanted" too. From bug 1107372 it's already turning out that at the moment you show the error pages you need the info about what the list was too.
Assignee: nobody → mmc
Status: NEW → ASSIGNED
Comment on attachment 8584806 [details] [diff] [review]
Add support for goog-unwanted-shavar

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

Here's a sort of working patch, missing tests and with placeholder copy. The annoying thing about the docshell code is that the string bundles defined in the .properties files are basically unused -- however, at least some of them must exist, otherwise we error out of LoadErrorPage. I'm not sure yet what the minimal set of required stringbundles is. The actual part that controls how the interstitials work is blockedSite.xhtml and phishing-afterload-warning-message.dtd.
(In reply to Gian-Carlo Pascutto [:gcp] from comment #5)
> >We need a new error code. Somewhere deep in the docshell code there is a mapping from 
> >NS_ERROR_PHISHING_URI -> about:phishing, etc.
> 
> Do you think it's most sensible to just extend this, or should we think
> about just pushing list names through all of those layers and let the code
> that has to pick out the error page make the mapping between list names and
> what error to show?
> 
> I mean, currently we have "malware", "phishing" tables with associated

> warnings, "tracking" with tables but no warnings, 

I think this particular one is OK -- it handles warnings differently through the shield dropdown and never should show an interstitial.

> so soon we'll have
> "unwanted" too. From bug 1107372 it's already turning out that at the moment
> you show the error pages you need the info about what the list was too.

I'm not sure what to do about that, other than defer the reporting problem til later.
Comment on attachment 8584806 [details] [diff] [review]
Add support for goog-unwanted-shavar

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

::: browser/app/profile/firefox.js
@@ +1017,5 @@
>  // only useful for suppressing remote lookups for signed binaries which we can
>  // only verify on Windows (Bug 974579).
>  pref("urlclassifier.downloadAllowTable", "goog-downloadwhite-digest256");
>  #endif
> +pref("urlclassifier.malwareTable", "goog-malware-shavar,test-malware-simple,goog-unwanted-shavar");

nit: group the goog entries

::: browser/base/content/blockedSite.xhtml
@@ +89,5 @@
>         * Initialize custom strings and functionality for blocked malware case
>         */
>        function initPage_malware()
>        {
> +        // Remove phishing and unwanted strings

Ugh. "It seemed clever at the time".

::: browser/base/content/content.js
@@ +731,5 @@
>  
>    onAboutBlocked: function (targetElement, ownerDoc) {
>      sendAsyncMessage("Browser:SiteBlockedError", {
>        location: ownerDoc.location.href,
> +      // TODO: Not sure about this one

https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#2846

It controls the report URL, so you need a tristate?

::: browser/locales/en-US/chrome/browser/safebrowsing/phishing-afterload-warning-message.dtd
@@ +13,5 @@
>  <!ENTITY safeb.blocked.malwarePage.longDesc "<p>Attack pages try to install programs that steal private information, use your computer to attack others, or damage your system.</p><p>Some attack pages intentionally distribute harmful software, but many are compromised without the knowledge or permission of their owners.</p>">
>  
> +<!ENTITY safeb.blocked.unwantedPage.title "Reported Unwanted Software Page!">
> +<!-- Localization note (safeb.blocked.malware.shortDesc) - Please don't translate the contents of the <span id="unwanted_sitename"/> tag.  It will be replaced at runtime with a domain name (e.g. www.badsite.com) -->
> +<!ENTITY safeb.blocked.unwantedPage.shortDesc "This web page at <span id='unwanted_sitename'/> has been reported as an attack page and has been blocked based on your security preferences.">

"Attack page" sounds a bit more like malware. The "unwanted software site" you used elsewhere fits better.

::: browser/locales/en-US/chrome/overrides/netError.dtd
@@ +166,5 @@
>  
> +<!ENTITY unwantedBlocked.title "Suspected Unwanted Software Site!">
> +<!ENTITY unwantedBlocked.longDesc "
> +<p>Attack sites try to install unwanted software programs that damage your system.</p>
> +<p>Website owners who believe their site has been reported as an attack site in error may <a href='http://www.stopbadware.org/home/reviewinfo' >request a review</a>.</p>

Is that URL correct? Do we know the correct one?

::: docshell/base/nsDocShell.cpp
@@ +4897,5 @@
>  nsDocShell::DisplayLoadError(nsresult aError, nsIURI* aURI,
>                               const char16_t* aURL,
>                               nsIChannel* aFailedChannel)
>  {
> +  PR_LOG(gDocShellLog, PR_LOG_DEBUG, ("nsDocShell[%p]::DisplayLoadError"));

nit: remove this

@@ +5087,5 @@
>        error.AssignLiteral("malwareBlocked");
>        bucketId = IsFrame() ? nsISecurityUITelemetry::WARNING_MALWARE_PAGE_FRAME
>                             : nsISecurityUITelemetry::WARNING_MALWARE_PAGE_TOP;
> +    } else {
> +      // TODO: Add telemetry

File bug or add to this bug.

@@ +5088,5 @@
>        bucketId = IsFrame() ? nsISecurityUITelemetry::WARNING_MALWARE_PAGE_FRAME
>                             : nsISecurityUITelemetry::WARNING_MALWARE_PAGE_TOP;
> +    } else {
> +      // TODO: Add telemetry
> +      PR_LOG(gDocShellLog, PR_LOG_DEBUG, ("nsDocShell[%p]::SBWarning unwantedBlocked", this));

nit: remove the logging
Attachment #8584806 - Flags: feedback+
Comment on attachment 8584806 [details] [diff] [review]
Add support for goog-unwanted-shavar

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

::: browser/base/content/content.js
@@ +731,5 @@
>  
>    onAboutBlocked: function (targetElement, ownerDoc) {
>      sendAsyncMessage("Browser:SiteBlockedError", {
>        location: ownerDoc.location.href,
> +      // TODO: Not sure about this one

Wow, I never saw that diagnostic page before: https://safebrowsing.google.com/safebrowsing/diagnostic?client=Firefox&hl=en-US&site=http://itisatrap.org/firefox/its-an-attack.html

The phishing button goes to the SUMO page which looks way better: https://support.mozilla.org/en-US/kb/how-does-phishing-and-malware-protection-work?as=u&utm_source=inproduct

I think we should just make this go to the SUMO page for all 3 cases.
That's actually what I was talking about here:
https://bugzilla.mozilla.org/show_bug.cgi?id=1107372#c4
(In reply to Gian-Carlo Pascutto [:gcp] from comment #12)
> That's actually what I was talking about here:
> https://bugzilla.mozilla.org/show_bug.cgi?id=1107372#c4

Sorry, I am confused. How do you get to browser.safebrowsing.malware.reportURL from chrome? Is it hidden in a menu item somewhere? I also thought that the snippet from comment 11 points to the "Why is this page blocked" button visible in https://bug1147212.bugzilla.mozilla.org/attachment.cgi?id=8584807, which is something else entirely.
Blocks: 1149867
Depends on: 1150334
Assignee: mmc → francois
Depends on: 1151279
Depends on: 1153085
Attached patch TestsSplinter Review
I've added the unwanted list to all of the relevant tests we have already.
Attachment #8593719 - Flags: review?(gpascutto)
gcp, this should address most of your comments from comment 10 and it fixes all of the TODOs left by Monica.

I have also simplified the code around the "why blocked" button. Now it always takes users to SUMO since it seems much more user friendly than the weird Google diagnostic page. I've filed bug 1153085 to get the new list added to SUMO.
Attachment #8584806 - Attachment is obsolete: true
Attachment #8593725 - Flags: review?(gpascutto)
Matej, would you be willing to review the copy on this patch? It's the Firefox side of the test page you reviewed on bug 1151279.

Here's the main user-visible error page (similar to what you see what you go
to http://itisatrap.org/firefox/its-an-attack.html):

> Reported Unwanted Software Site!
> 
> This web page at <span id='unwanted_sitename'/> has been reported to contain
> unwanted software and has been blocked based on your security preferences.
> 
> Unwanted software pages try to install software that can be deceptive and
> affect your system in unexpected ways.

and this one I couldn't actually find in Firefox, but we have similar pages
for malware/phishing:

> Suspected Unwanted Software Site!
> 
> The site at %S has been reported as serving unwanted software and has been
> blocked based on your security preferences.
> 
> Unwanted software pages try to install software that can be deceptive and
> affect your system in unexpected ways.

Note: it would be ideal if these two were the same, but I wanted to match
the existing malware/phishing messages, which for historical reasons I
presume, use "Reported" in the first case and "Suspected" in the second, as
well as "This web page at" in the first case and "The site at" in the
second.

If you think one version is better, I can file a follow-up bug to harmonize
these two sets of pages.
Attachment #8584807 - Attachment is obsolete: true
Attachment #8593728 - Flags: review?(matej)
Attachment #8593719 - Flags: review?(gpascutto) → review+
Comment on attachment 8593725 [details] [diff] [review]
Add support for goog-unwanted-shavar

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

::: security/manager/boot/public/nsISecurityUITelemetry.idl
@@ +115,5 @@
>  
> +const uint32_t WARNING_UNWANTED_PAGE_TOP = 92;
> +const uint32_t WARNING_UNWANTED_PAGE_TOP_WHY_BLOCKED = 93;
> +const uint32_t WARNING_UNWANTED_PAGE_TOP_GET_ME_OUT_OF_HERE = 94;
> +const uint32_t WARNING_UNWANTED_PAGE_TOP_IGNORE_WARNING = 95;

Is your stuff the only thing not sorted in this file?

If so, maybe move it to the appropriate place and leave a comment here.

@@ +155,1 @@
>  // We only have buckets up to 100.

We should probably nudge whoever owns this file.

::: toolkit/components/url-classifier/SafeBrowsing.jsm
@@ +200,5 @@
>      // Add test entries to the DB.
>      // XXX bug 779008 - this could be done by DB itself?
>      const phishURL   = "itisatrap.org/firefox/its-a-trap.html";
>      const malwareURL = "itisatrap.org/firefox/its-an-attack.html";
> +    const unwantedURL = "itisatrap.org/firefox/unwanted.html";

nit: inconsistent indentation
Attachment #8593725 - Flags: review?(gpascutto) → review+
Matej, I found where the "Suspected Unwanted Software Site!" error page is used: Firefox for Android.
Matej, also relevant is the suggested warning language from Google:

> Warning — The site ahead may contain harmful programs.
> 
> Attackers might attempt to trick you into installing programs that harm your
> browsing experience (for example, by changing your homepage or showing extra
> ads on sites you visit). You can learn more about unwanted software at
> https://www.google.com/about/company/unwanted-software-policy.html. 

Source: https://developers.google.com/safe-browsing/developers_guide_v3#UserWarnings
Fixed two nits from gcp, carrying his r+.
Attachment #8593725 - Attachment is obsolete: true
Attachment #8594617 - Flags: review+
Re-adding a line that was deleted by mistake in an earlier patch. Carrying r+ from gcp.
Attachment #8594617 - Attachment is obsolete: true
Attachment #8594622 - Flags: review+
Keeler, are you ok with my using up almost all of the remaining buckets in nsISecurityUITelemetry.idl?
Flags: needinfo?(dkeeler)
Comment on attachment 8594622 [details] [diff] [review]
Add support for goog-unwanted-shavar

Olli, are you able to do a review of the tiny docshell/dom changes contained in this patch?
Attachment #8594622 - Flags: review?(bugs)
Attachment #8593728 - Flags: review?(matej) → review+
I think we're going to be looking at redesigning these pages, so let's look at the interstitials as well when we do that. For now this looks good to me. Thanks.
(In reply to François Marier [:francois] from comment #22)
> Keeler, are you ok with my using up almost all of the remaining buckets in
> nsISecurityUITelemetry.idl?

Works for me.
Flags: needinfo?(dkeeler)
Attachment #8594622 - Flags: review?(bugs) → review+
Hey gcp, can I please ask you to take another quick look at the patch. I've added some missing android UI. The diff from what you last looked at is here:

https://bitbucket.org/fmarier/mozilla-central-mq-1147212/commits/dd0a3d073f803016f78d355501999284018ee7ad
Attachment #8594622 - Attachment is obsolete: true
Attachment #8595222 - Flags: review?(gpascutto)
Attachment #8595222 - Flags: review?(gpascutto) → review+
https://hg.mozilla.org/mozilla-central/rev/8b191f5f9687
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
http://www.itisatrap.org/firefox/its-a-trap.html
http://www.itisatrap.org/firefox/its-an-attack.html

Do we have a similar page for this system? Localizers will need it for testing, but I guess also automated testing will need it.
Keywords: feature
Keywords: relnote
QA Contact: mwobensmith
Release Note Request (optional, but appreciated)
[Why is this notable]: It automatically protects our users against some deceptive software.
[Suggested wording]: Protection from potentially unwanted software
[Links (documentation, blog post, etc)]: https://www.google.com/about/company/unwanted-software-policy.html and http://googleonlinesecurity.blogspot.co.nz/2015/03/even-more-unwanted-software-protection.html

This got added to both Desktop and Android.
relnote-firefox: --- → ?
Keywords: feature, relnote
Verified on Nightly Fx40 Mac/Win/Linux 2015-05-05.
Status: RESOLVED → VERIFIED
I presume this patch deprecated
https://dxr.mozilla.org/mozilla-central/source/browser/app/profile/firefox.js#980

Aside from the above, it's not even clear to me which of the 6 above report*URL we still use.
For what it's worth, the patch on bug 1109475 also removes the unused reporting URLs.
François, do we have some other URL besides a Google URL? no blog post or documentation?
Flags: needinfo?(francois)
A blog post is scheduled to be published on Tuesday 11 August, 7am Pacific at this URL:

  https://blog.mozilla.org/security/2015/08/11/expanded-malware-protection-in-firefox/

This is what it will contain:

  https://etherpad.mozilla.org/JKMjgRpo9O

Let me know if you'd like me to change the time/date when it will be published.
Flags: needinfo?(francois)
Depends on: 1195242
You need to log in before you can comment on or make changes to this bug.