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)
Core
DOM: Security
Tracking
()
VERIFIED
FIXED
mozilla40
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.
Reporter | ||
Comment 1•9 years ago
|
||
http://googleonlinesecurity.blogspot.com/2015/03/even-more-unwanted-software-protection.html
Assignee | ||
Comment 2•9 years ago
|
||
Google's Unwanted Software Policy: https://www.google.com/about/company/unwanted-software-policy.html
Comment 3•9 years ago
|
||
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?
Reporter | ||
Comment 4•9 years ago
|
||
(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.
Comment 5•9 years ago
|
||
>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.
Reporter | ||
Comment 6•9 years ago
|
||
Reporter | ||
Updated•9 years ago
|
Assignee: nobody → mmc
Status: NEW → ASSIGNED
Reporter | ||
Comment 7•9 years ago
|
||
This is the output of https://bugzilla.mozilla.org/attachment.cgi?id=8584806.
Reporter | ||
Comment 8•9 years ago
|
||
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.
Reporter | ||
Comment 9•9 years ago
|
||
(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 10•9 years ago
|
||
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+
Reporter | ||
Comment 11•9 years ago
|
||
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.
Comment 12•9 years ago
|
||
That's actually what I was talking about here: https://bugzilla.mozilla.org/show_bug.cgi?id=1107372#c4
Reporter | ||
Comment 13•9 years ago
|
||
(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.
Assignee | ||
Updated•9 years ago
|
Assignee: mmc → francois
Assignee | ||
Comment 14•9 years ago
|
||
I've added the unwanted list to all of the relevant tests we have already.
Attachment #8593719 -
Flags: review?(gpascutto)
Assignee | ||
Comment 15•9 years ago
|
||
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)
Assignee | ||
Comment 16•9 years ago
|
||
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)
Updated•9 years ago
|
Attachment #8593719 -
Flags: review?(gpascutto) → review+
Comment 17•9 years ago
|
||
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+
Assignee | ||
Comment 18•9 years ago
|
||
Matej, I found where the "Suspected Unwanted Software Site!" error page is used: Firefox for Android.
Assignee | ||
Comment 19•9 years ago
|
||
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
Assignee | ||
Comment 20•9 years ago
|
||
Fixed two nits from gcp, carrying his r+.
Attachment #8593725 -
Attachment is obsolete: true
Attachment #8594617 -
Flags: review+
Assignee | ||
Comment 21•9 years ago
|
||
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+
Assignee | ||
Comment 22•9 years ago
|
||
Keeler, are you ok with my using up almost all of the remaining buckets in nsISecurityUITelemetry.idl?
Flags: needinfo?(dkeeler)
Assignee | ||
Comment 23•9 years ago
|
||
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)
Assignee | ||
Comment 24•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e54c7a9169ab
OS: Mac OS X → All
Hardware: x86 → All
Updated•9 years ago
|
Attachment #8593728 -
Flags: review?(matej) → review+
Comment 25•9 years ago
|
||
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)
Updated•9 years ago
|
Attachment #8594622 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 27•9 years ago
|
||
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)
Updated•9 years ago
|
Attachment #8595222 -
Flags: review?(gpascutto) → review+
https://hg.mozilla.org/mozilla-central/rev/8b191f5f9687
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Comment 30•9 years ago
|
||
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.
Assignee | ||
Comment 31•9 years ago
|
||
Francesco: we do, it's at http://www.itisatrap.org/firefox/unwanted.html
Assignee | ||
Updated•9 years ago
|
QA Contact: mwobensmith
Assignee | ||
Comment 32•9 years ago
|
||
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:
--- → ?
Comment 33•9 years ago
|
||
Verified on Nightly Fx40 Mac/Win/Linux 2015-05-05.
Status: RESOLVED → VERIFIED
Comment 34•9 years ago
|
||
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.
Assignee | ||
Comment 35•9 years ago
|
||
For what it's worth, the patch on bug 1109475 also removes the unused reporting URLs.
Updated•9 years ago
|
Comment 36•9 years ago
|
||
François, do we have some other URL besides a Google URL? no blog post or documentation?
Flags: needinfo?(francois)
Assignee | ||
Comment 37•9 years ago
|
||
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)
You need to log in
before you can comment on or make changes to this bug.
Description
•