Android malware comes with the POTENTIALLY_HARMFUL_APPLICATION threat type instead of MALWARE_THREAT: https://searchfox.org/mozilla-central/rev/ae94cfb36d804727dfb38f2750bfcaab4621df6e/toolkit/components/url-classifier/chromium/safebrowsing.proto#266 We should create a new list for it (goog-pha-proto), add it to provider.google4.lists and then create a new warning page for it: https://developers.google.com/safe-browsing/v4/usage-limits#suggested--warning-language

I am thinking if we should add new error code for this threat or just reuse NS_ERROR_MALWARE. The challenge for later approach is the different user facing error message. For example: "Warning — The site ahead may contain malware. Attackers might attempt to install dangerous apps on your device that steal or delete your information (for example, photos, passwords, messages, and credit cards)." Adding new error code seems better but it requires us to add new references for the new code in many places. (e.g. docshell, httpchannelChild, ...) http://searchfox.org/mozilla-central/search?q=NS_ERROR_MALWARE&case=false&regexp=false&path=

(In reply to Henry Chang [:hchang] from comment #1) > I am thinking if we should add new error code for this threat or > just reuse NS_ERROR_MALWARE. The challenge for later approach is > the different user facing error message. Given that we are required to use a different error message for this, I think a new NS_ERROR_HARMFUL is our best option. Maybe test-harmful-simple and goog-harmful-proto as the list names. The approved wording of the new error page can be found in bug 1358536: - Desktop: https://cl.ly/0X0T0D1U270w - Android: https://cl.ly/401M3R1o2g25 We'll also need to create a new test page on https://github.com/mozilla/itisatrap.

Comment on attachment 8890737 [details] Bug 1375277 - New safebrowsing threat type "POTENTIALLY_HARMFUL_APPLICATION" introduced by v4. https://reviewboard.mozilla.org/r/161934/#review168462 In order to be able to test this easily, we should do the following before landing: - add a hardcoded URL to `test-harmful-simple`: `itisatrap.org/firefox/harmful.html` - create a landing page on https://github.com/mozilla/itisatrap ::: docshell/base/nsDocShell.cpp:5154 (Diff revision 3) > bucketId = IsFrame() ? nsISecurityUITelemetry::WARNING_UNWANTED_PAGE_FRAME > : nsISecurityUITelemetry::WARNING_UNWANTED_PAGE_TOP; > + } else if (NS_ERROR_HARMFUL_URI == aError) { > + sendTelemetry = true; > + error = "harmfulBlocked"; > + bucketId = IsFrame() ? nsISecurityUITelemetry::WARNING_MALWARE_PAGE_FRAME We should use a distinct telemetry code here to avoid grouping together two unrelated results. That said, we've used all of the buckets in that probe. Maybe we should bite the bullet and move the Safe Browsing UI codes out of this probe and into a new `URLCLASSIFIER_UI` probe. ::: docshell/base/nsDocShell.cpp:11054 (Diff revision 3) > // a NullPrincipal. In other cases, e.g. typing a data: URL into > // the URL-Bar, the triggeringPrincipal is a SystemPrincipal; > // we don't want to block those loads. Only exception, loads coming > // from an external applicaton (e.g. Thunderbird) don't load > // using a codeBasePrincipal, but we want to block those loads. > - if (isDataURI && (aLoadFromExternal || > + if (isDataURI && (aLoadFromExternal || nit: unnecessary whitespace change ::: mobile/android/chrome/content/browser.js:4902 (Diff revision 3) > } else if (errorDoc.documentURI.includes("e=unwantedBlocked")) { > sendTelemetry = true; > bucketName = "WARNING_UNWANTED_PAGE_"; > + } else if (errorDoc.documentURI.includes("e=harmfulBlocked")) { > + sendTelemetry = true; > + bucketName = "WARNING_MALWARE_PAGE_"; This should also use a distinct bucket name. ::: toolkit/components/url-classifier/nsUrlClassifierUtils.cpp:242 (Diff revision 3) > { "goog-badbinurl-proto", MALICIOUS_BINARY}, // 7 > { "goog-downloadwhite-proto", CSD_DOWNLOAD_WHITELIST}, // 9 > > // For testing purpose. > { "test-phish-proto", SOCIAL_ENGINEERING_PUBLIC}, // 2 > + { "test-harmful-proto", POTENTIALLY_HARMFUL_APPLICATION}, // 4 Is that used anywhere? You seem to be using `test-harmful-simple` instead. ::: xpcom/base/ErrorList.py:256 (Diff revision 3) > # For example, see nsIRequestObserver::onStopRequest. > > # The async request completed successfully. > errors["NS_BINDING_SUCCEEDED"] = errors["NS_OK"] > > - # The async request failed for some unknown reason. > + # The async request failed for some unknown reason. nit: unnecessary whitespace change ::: xpcom/base/ErrorList.py:317 (Diff revision 3) > # The connection does not exist. XXX unused - consider removing. > errors["NS_ERROR_NOT_CONNECTED"] = FAILURE(12) > # The connection attempt failed, for example, because no server was > # listening at specified host:port. > errors["NS_ERROR_CONNECTION_REFUSED"] = FAILURE(13) > - # The connection was lost due to a timeout error. > + # The connection was lost due to a timeout error. nit: unnecessary whitespace change ::: xpcom/base/ErrorList.py:342 (Diff revision 3) > # XXX really need to better rationalize these error codes. are consumers of > # necko really expected to know how to discern the meaning of these?? > # This request is not resumable, but it was tried to resume it, or to > # request resume-specific data. > errors["NS_ERROR_NOT_RESUMABLE"] = FAILURE(25) > - # The request failed as a result of a detected redirection loop. > + # The request failed as a result of a detected redirection loop. nit: unnecessary whitespace change

Comment on attachment 8891273 [details] Bug 1375277 - Test case for new error code NS_ERROR_HARMFUL_URI. https://reviewboard.mozilla.org/r/162484/#review168470

Comment on attachment 8890737 [details] Bug 1375277 - New safebrowsing threat type "POTENTIALLY_HARMFUL_APPLICATION" introduced by v4. https://reviewboard.mozilla.org/r/161934/#review168462 Regarding "add a hardcoded URL to test-harmful-simple: itisatrap.org/firefox/harmful.html" Do you mean adding to addMozEntries()? BTW, I wonder the actual wording for the warning page. I asked photon people about the new warning page target milestone and it's said to not at 57. So, I am not adopting the new UI (at least not in this patch) and use the current template so that the wording is not 100% applicable. Do you think what I am current using is fine or we should request for some wording else? Thanks! > We should use a distinct telemetry code here to avoid grouping together two unrelated results. > > That said, we've used all of the buckets in that probe. > > Maybe we should bite the bullet and move the Safe Browsing UI codes out of this probe and into a new `URLCLASSIFIER_UI` probe. Oops I forgot to mention this. Yes, all the bucket is run out so I use 'maleware' instead for testing purpose. I should have mentioned this. > Is that used anywhere? > > You seem to be using `test-harmful-simple` instead. Nope. I thought I would use it in the test case but then realize I don't need to test v4 with this specific threat type at all (at least for now)

Comment on attachment 8890737 [details] Bug 1375277 - New safebrowsing threat type "POTENTIALLY_HARMFUL_APPLICATION" introduced by v4. https://reviewboard.mozilla.org/r/161934/#review168462 Regarding "add a hardcoded URL to test-harmful-simple: itisatrap.org/firefox/harmful.html" Do you mean adding to addMozEntries()? BTW, I wonder the actual wording for the warning page. I asked photon people about the new warning page target milestone and it's said to not at 57. So, I am not adopting the new UI (at least not in this patch) and use the current template so that the wording is not 100% applicable. Do you think what I am current using is fine or we should request for some wording else? Thanks! > We should use a distinct telemetry code here to avoid grouping together two unrelated results. > > That said, we've used all of the buckets in that probe. > > Maybe we should bite the bullet and move the Safe Browsing UI codes out of this probe and into a new `URLCLASSIFIER_UI` probe. Oops I forgot to mention this. Yes, all the bucket is run out so I use 'maleware' instead for testing purpose. I should have mentioned this. > Is that used anywhere? > > You seem to be using `test-harmful-simple` instead. Nope. I thought I would use it in the test case but then realize I don't need to test v4 with this specific threat type at all (at least for now)

BTW, what should https://itisatrap.org/firefox/harmful.html look like? How about the same content as https://itisatrap.org/firefox/its-an-attack.html ? Thanks :)

(In reply to Henry Chang [:hchang] from comment #10) > Regarding "add a hardcoded URL to test-harmful-simple: > itisatrap.org/firefox/harmful.html" > > Do you mean adding to addMozEntries()? That's right. > BTW, I wonder the actual wording for the warning page. I asked photon people > about the new > warning page target milestone and it's said to not at 57. So, I am not > adopting the new UI (at least > not in this patch) and use the current template so that the wording is not > 100% applicable. > Do you think what I am current using is fine or we should request for some > wording else? Yes, I think the wording you put together is fine. It will be updated later anyways. (In reply to Henry Chang [:hchang] from comment #11) > BTW, what should https://itisatrap.org/firefox/harmful.html look like? > > How about the same content as > https://itisatrap.org/firefox/its-an-attack.html ? I think it would be easier to start with this template: https://itisatrap.org/firefox/unwanted.html Here's what I would suggest: "Potentially Harmful Application I’m a naughty Web page trying to install dangerous apps! OK, not really, but some sites can offer apps that steal or delete your information (for example, photos, passwords, messages, and credit cards). Firefox provides built-in <a href="http://www.mozilla.org/firefox/phishing-protection/">Phishing and Malware Protection</a> to help you avoid potentially harmful applications. If you are using Firefox for Android 60 or later, you should have been warned to stay away from this page."

Comment on attachment 8892821 [details] [diff] [review] Landing page for 'harmful' threat Review of attachment 8892821 [details] [diff] [review]: ----------------------------------------------------------------- Reviewed on GitHub.

Comment on attachment 8890737 [details] Bug 1375277 - New safebrowsing threat type "POTENTIALLY_HARMFUL_APPLICATION" introduced by v4. https://reviewboard.mozilla.org/r/161934/#review169444 r+ with comments ::: commit-message-e8400:1 (Diff revisions 3 - 4) > -Bug 1375277 - New safebrowsing threat type "POTENTIALLY_HARMFUL_APPLICATION" introdued by v4. > +Bug 1375277 - Part 1. New safebrowsing threat type "POTENTIALLY_HARMFUL_APPLICATION" introdued by v4. typo: "introduced" nit: given that the other parts fairly self-contained, you could drop the "Part 1" from the description ::: docshell/base/nsDocShell.cpp (Diff revisions 3 - 4) > } else if (NS_ERROR_UNWANTED_URI == aError) { > sendTelemetry = true; > error = "unwantedBlocked"; > bucketId = IsFrame() ? nsISecurityUITelemetry::WARNING_UNWANTED_PAGE_FRAME > : nsISecurityUITelemetry::WARNING_UNWANTED_PAGE_TOP; > - } else if (NS_ERROR_HARMFUL_URI == aError) { I think we still need to set the `error` variable in this patch, even though the telemetry will come later. ::: browser/locales/en-US/chrome/overrides/appstrings.properties:34 (Diff revision 4) > #LOCALIZATION NOTE (externalProtocolUnknown): The following string is shown if the application name can't be determined > externalProtocolUnknown=<Unknown> > externalProtocolChkMsg=Remember my choice for all links of this type. > externalProtocolLaunchBtn=Launch application > malwareBlocked=The site at %S has been reported as an attack site and has been blocked based on your security preferences. > +harmfulBlocked=The site at %S has been reported as a harmful site and has been blocked based on your security preferences. Maybe we need the word "potentially" in here. harmfulBlocked=The site at %S has been reported as a potentially harmful site and has been blocked based on your security preferences. ::: mobile/locales/en-US/overrides/appstrings.properties:34 (Diff revision 4) > #LOCALIZATION NOTE (externalProtocolUnknown): The following string is shown if the application name can't be determined > externalProtocolUnknown=<Unknown> > externalProtocolChkMsg=Remember my choice for all links of this type. > externalProtocolLaunchBtn=Launch application > malwareBlocked=The site at %S has been reported as an attack site and has been blocked based on your security preferences. > +harmfulBlocked=The site at %S has been reported as a harmful site and has been blocked based on your security preferences. "potentially harmful"

Comment on attachment 8892743 [details] Bug 1375277 - Move Safe Browsing UI events to a separate telemetry probe. https://reviewboard.mozilla.org/r/163730/#review169456 ::: commit-message-91abd:1 (Diff revision 1) > +Bug 1375277 - Part 2. Move safebrowsing related telemetry to a separate probe. I would suggest "Move Safe Browsing UI events to a separate telemetry probe." ::: commit-message-91abd:3 (Diff revision 1) > +Bug 1375277 - Part 2. Move safebrowsing related telemetry to a separate probe. > + > +All safebrowsing-related UI telemetry used to be under SECURITY_UI probe That paragraph is probably not worth including in the commit log. The motivation is not super important given it's already in the diff as a comment. ::: browser/base/content/browser.js:3154 (Diff revision 1) > + } else if (reason === "harmful") { > + sendTelemetry = true; > + bucketName = "WARNING_HARMFUL_PAGE_"; > } > - let secHistogram = Services.telemetry.getHistogramById("SECURITY_UI"); > + let secHistogram = Services.telemetry.getHistogramById("URLCLASSIFIER_UI"); > let nsISecTel = Ci.nsISecurityUITelemetry; Shouldn't that be `IUrlClassifierUITelemetry`? ::: mobile/android/chrome/content/browser.js:4890 (Diff revision 1) > } else if (errorDoc.documentURI.startsWith("about:blocked")) { > // The event came from a button on a malware/phishing block page > // First check whether it's malware, phishing or unwanted, so that we > // can use the right strings/links > let bucketName = ""; > + let probe = "URLCLASSIFIER_UI"; That should be a `const` instead of a variable. ::: security/manager/ssl/nsISecurityUITelemetry.idl:148 (Diff revision 1) > -const uint32_t WARNING_UNWANTED_PAGE_FRAME = 96; > -const uint32_t WARNING_UNWANTED_PAGE_FRAME_WHY_BLOCKED = 97; > -const uint32_t WARNING_UNWANTED_PAGE_FRAME_GET_ME_OUT_OF_HERE = 98; > -const uint32_t WARNING_UNWANTED_PAGE_FRAME_IGNORE_WARNING = 99; > - > -// This uses up buckets till 99 (including) > +// const uint32_t WARNING_UNWANTED_PAGE_FRAME = 96; > +// const uint32_t WARNING_UNWANTED_PAGE_FRAME_WHY_BLOCKED = 97; > +// const uint32_t WARNING_UNWANTED_PAGE_FRAME_GET_ME_OUT_OF_HERE = 98; > +// const uint32_t WARNING_UNWANTED_PAGE_FRAME_IGNORE_WARNING = 99; > + > +// All the buckets are used so the safebrowsing-related buckets are nit: "were moved to" ::: security/manager/ssl/nsISecurityUITelemetry.idl:149 (Diff revision 1) > -const uint32_t WARNING_UNWANTED_PAGE_FRAME_WHY_BLOCKED = 97; > -const uint32_t WARNING_UNWANTED_PAGE_FRAME_GET_ME_OUT_OF_HERE = 98; > -const uint32_t WARNING_UNWANTED_PAGE_FRAME_IGNORE_WARNING = 99; > - > -// This uses up buckets till 99 (including) > -// We only have buckets up to 100. > +// const uint32_t WARNING_UNWANTED_PAGE_FRAME_WHY_BLOCKED = 97; > +// const uint32_t WARNING_UNWANTED_PAGE_FRAME_GET_ME_OUT_OF_HERE = 98; > +// const uint32_t WARNING_UNWANTED_PAGE_FRAME_IGNORE_WARNING = 99; > + > +// All the buckets are used so the safebrowsing-related buckets are > +// moved to nsIUrlClassifierUITelemetry under URLCLASSIFIER_UI probe. I would remove the reference to the filename and instead just say "moved under the URLCLASSIFIER_UI" probe. ::: toolkit/components/telemetry/Histograms.json:13625 (Diff revision 1) > "n_buckets": 100, > "bug_numbers": [1274919], > "description": "Measure the time how long the cursor is hovering before opening the unselcted tab. Only record the data if someone requests for sending unselected tab hover msg.", > "releaseChannelCollection": "opt-out" > + }, > + "URLCLASSIFIER_UI": { Now that I think of it, `URLCLASSIFIER_UI_EVENTS` would be more descriptive. Sorry, I should have thought of that before. ::: toolkit/components/telemetry/Histograms.json:13627 (Diff revision 1) > "description": "Measure the time how long the cursor is hovering before opening the unselcted tab. Only record the data if someone requests for sending unselected tab hover msg.", > "releaseChannelCollection": "opt-out" > + }, > + "URLCLASSIFIER_UI": { > + "record_in_processes": ["main", "content"], > + "alert_emails": ["seceng-telemetry@mozilla.com", "fxprivacyandsecurity@mozilla.com"], You can remove `fxprivacyandsecurity`. Add `francois@mozila.com` too since we now require the email of a person too. ::: toolkit/components/telemetry/Histograms.json:13631 (Diff revision 1) > + "record_in_processes": ["main", "content"], > + "alert_emails": ["seceng-telemetry@mozilla.com", "fxprivacyandsecurity@mozilla.com"], > + "bug_numbers": [1375277], > + "expires_in_version": "never", > + "kind": "enumerated", > + "n_values": 100, I don't think we'll ever need that many. Let's make it double the number of buckets we currently use (i.e. 64). ::: toolkit/components/telemetry/Histograms.json:13632 (Diff revision 1) > + "alert_emails": ["seceng-telemetry@mozilla.com", "fxprivacyandsecurity@mozilla.com"], > + "bug_numbers": [1375277], > + "expires_in_version": "never", > + "kind": "enumerated", > + "n_values": 100, > + "description": "URL-CLassifier-related (aka Safe Browsing) UI events. See nsIUrlClassifierUITelemetry.idl for the specific values." "URL Classifier-related" ::: toolkit/components/telemetry/histogram-whitelists.json:1613 (Diff revision 1) > "UPDATE_STATUS_ERROR_CODE_UNKNOWN_STARTUP", > "UPDATE_STATUS_ERROR_CODE_COMPLETE_STAGE", > "UPDATE_STATUS_ERROR_CODE_PARTIAL_STAGE", > "UPDATE_STATUS_ERROR_CODE_UNKNOWN_STAGE", > "SECURITY_UI", > + "URLCLASSIFIER_UI", We shouldn't need this if we reduce the number of buckets. ::: toolkit/components/url-classifier/nsIUrlClassifierUITelemetry.idl:1 (Diff revision 1) > +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- This file should be called `IUrlClassifierUITelemetry.idl` since we're not supposed to add any more `ns*` files.

Comment on attachment 8892743 [details] Bug 1375277 - Move Safe Browsing UI events to a separate telemetry probe. https://reviewboard.mozilla.org/r/163730/#review170034 Looks good to me, r+ and datareview+.

Pushed by hchang@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/fa94b9e702ec New safebrowsing threat type "POTENTIALLY_HARMFUL_APPLICATION" introduced by v4. r=francois https://hg.mozilla.org/integration/autoland/rev/17248bdb72c9 Move Safe Browsing UI events to a separate telemetry probe. r=francois https://hg.mozilla.org/integration/autoland/rev/89ed2b795d66 Test case for new error code NS_ERROR_HARMFUL_URI. r=francois

https://hg.mozilla.org/mozilla-central/rev/fa94b9e702ec https://hg.mozilla.org/mozilla-central/rev/17248bdb72c9 https://hg.mozilla.org/mozilla-central/rev/89ed2b795d66

Comment on attachment 8890737 [details] Bug 1375277 - New safebrowsing threat type "POTENTIALLY_HARMFUL_APPLICATION" introduced by v4. https://reviewboard.mozilla.org/r/161934/#review170880 ::: browser/locales/en-US/chrome/browser/safebrowsing/phishing-afterload-warning-message.dtd:37 (Diff revision 6) > <!ENTITY safeb.blocked.phishingPage.shortDesc2 "This web page at <span id='phishing_sitename'/> has been reported as a deceptive site and has been blocked based on your security preferences."> > <!ENTITY safeb.blocked.phishingPage.longDesc2 "<p>Deceptive sites are designed to trick you into doing something dangerous, like installing software, or revealing your personal information, like passwords, phone numbers or credit cards.</p><p>Entering any information on this web page may result in identity theft or other fraud.</p>"> > > +<!ENTITY safeb.blocked.harmfulPage.title "The site ahead may contain malware"> > +<!-- Localization note (safeb.blocked.harmfulPage.shortDesc) - Please don't translate the contents of the <span id="harmful_sitename"/> tag. It will be replaced at runtime with a domain name (e.g. www.badsite.com) --> > +<!ENTITY safeb.blocked.harmfulPage.shortDesc "Firefox blocked this page because it might try to install dangerous apps that steal or delete your information (for example, photos, passwords, messages and credit cards)."> Is there a reason to hard-code Firefox in this string? That's normally an error

To clarify: Firefox should normally be &brandShortName;, unless there's a specific reason or technical limitation. In case this needs fixing, https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_content_best_practices#Changing_existing_strings

(In reply to Wes Kocher (:KWierso) from comment #29) > https://hg.mozilla.org/mozilla-central/rev/fa94b9e702ec 1) No newline at end of file (phishing-afterload-warning-message.dtd) 2) I always wondered why/how a site could steal "credit cards" rather than "credit card data/info/details" (there’s 5 other occurrences in the tree for similar strings, but still) 3) The "site ahead" is probably a little new to localizers, but should be clear enough (not vital, but perhaps something to think about when choosing words)