Closed Bug 1394335 Opened 7 years ago Closed 7 years ago

goog-unwanted-shavar is unexpectedly added to malware list in V4 only

Categories

(Toolkit :: Safe Browsing, defect, P1)

Product:
Toolkit
Component:
Safe Browsing
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 + verified
firefox57 --- verified

People

(Reporter: tnguyen, Assigned: tnguyen)

References

Details

(Whiteboard: #sbv4-m9)

Attachments

(2 files)

Bug 1394335 - Add correct unwanted list when users enable uncommon and unwanted software from preference page
59 bytes, text/x-review-board-request
francois
: review+
Details
Beta release patch - Add correct unwanted list when users enable uncommon and unwanted software from preference page
9.75 KB, patch
gchang
: approval-mozilla-beta+
Details | Diff | Splinter Review 
http://searchfox.org/mozilla-central/rev/18c16ebf818abb86805ce08a6e537e4cd826f044/browser/components/preferences/in-content/security.js#205
When users disable and enable unwanted list in V4 only, then goog-unwanted-shavar is added to the malware list. That may trigger V2 update
Blocks: 1366920
status-firefox56: --- → ?
status-firefox57: --- → affected
Priority: -- → P1
Whiteboard: #sbv4-m9
tracking-firefox56: --- → ?
Hmm, the patch only works for explicit google list only,  I think we have to move unwanted list to a separated preference. In that way, we can add/remove correctly all unwanted lists of all providers (if there's any)
Assignee: nobody → tnguyen
Status: NEW → ASSIGNED
Tracking since we are planning rollout for this feature for 56.
tracking-firefox56: ? → +
Comment on attachment 8901743 [details]
Bug 1394335 - Add correct unwanted list when users enable uncommon and unwanted software from preference page

https://reviewboard.mozilla.org/r/173192/#review178784

I think we should add a little more logic here in order to support both V2 and V4. See comments below.

::: browser/components/preferences/in-content-new/privacy.js:1105
(Diff revision 1)
>        blockUnwantedPref.value = blockUncommonUnwanted.checked;
>        blockUncommonPref.value = blockUncommonUnwanted.checked;
>  
>        let malware = malwareTable.value
>          .split(",")
> -        .filter(x => x !== "goog-unwanted-shavar" && x !== "test-unwanted-simple");
> +        .filter(x => x !== "goog-unwanted-proto" && x !== "test-unwanted-simple");

Let's filter out both "goog-unwanted-proto" and "goog-unwanted-shavar"

::: browser/components/preferences/in-content-new/privacy.js:1108
(Diff revision 1)
>        let malware = malwareTable.value
>          .split(",")
> -        .filter(x => x !== "goog-unwanted-shavar" && x !== "test-unwanted-simple");
> +        .filter(x => x !== "goog-unwanted-proto" && x !== "test-unwanted-simple");
>  
>        if (blockUncommonUnwanted.checked) {
> -        malware.push("goog-unwanted-shavar");
> +        malware.push("goog-unwanted-proto");

Here we should look for "goog-malware-shavar" in the array and if it's there, then we add "goog-unwanted-shavar".

If it's not there, we assume we're on V4 and then add "goog-unwanted-proto" instead.
Attachment #8901743 - Flags: review-
Thanks Francois,
I updated the patch, assuming Google is the only provider which provides -unwanted- list.
Comment on attachment 8901743 [details]
Bug 1394335 - Add correct unwanted list when users enable uncommon and unwanted software from preference page

https://reviewboard.mozilla.org/r/173192/#review178868
Attachment #8901743 - Flags: review?(francois) → review+
Updated the patch because I think we may have to update a test for the new logic.
Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b0e40a6c2f50b84f259e77189c5cdbd4229b1472
Comment on attachment 8901743 [details]
Bug 1394335 - Add correct unwanted list when users enable uncommon and unwanted software from preference page

https://reviewboard.mozilla.org/r/173192/#review179312

Let's be more explicit about pref setting in the test so that it's less fragile.

::: browser/components/preferences/in-content-new/tests/browser_security-2.js:68
(Diff revision 4)
>  add_task(async function() {
> -  async function checkPrefSwitch(val1, val2) {
> +  async function checkPrefSwitch(val1, val2, isV2) {
>      Services.prefs.setBoolPref("browser.safebrowsing.downloads.remote.block_potentially_unwanted", val1);
>      Services.prefs.setBoolPref("browser.safebrowsing.downloads.remote.block_uncommon", val2);
> +    let malwareTable = Services.prefs.getCharPref("urlclassifier.malwareTable");
> +    if (isV2 && malwareTable.indexOf("goog-malware-shavar") ==-1) {

This is a bit hacky because it's doing the minimum to make the logic of the checkbox work.

We may as well overwrite `urlclassifier.malwareTable` completely:

    malwareTable = "goog-malware-" + isV2 ? "shavar" : "proto"
    if val1 && val2:
        malwareTable += ",goog-unwanted-" ? isV2 : "shavar" : "proto"
    malwareTable += ",test-malware-simple"
    if val1 && val2:
        malwareTable += ",test-unwanted-simple"

so that it's set to a value that is closer to the real one.

::: browser/components/preferences/in-content/tests/browser_security.js:107
(Diff revision 4)
>  add_task(async function() {
> -  async function checkPrefSwitch(val1, val2) {
> +  async function checkPrefSwitch(val1, val2, isV2) {
>      Services.prefs.setBoolPref("browser.safebrowsing.downloads.remote.block_potentially_unwanted", val1);
>      Services.prefs.setBoolPref("browser.safebrowsing.downloads.remote.block_uncommon", val2);
> +    let malwareTable = Services.prefs.getCharPref("urlclassifier.malwareTable");
> +    if (isV2 && malwareTable.indexOf("goog-malware-shavar") ==-1) {

ditto
Attachment #8901743 - Flags: review+ → review-
Sure, thanks for your review
Comment on attachment 8901743 [details]
Bug 1394335 - Add correct unwanted list when users enable uncommon and unwanted software from preference page

https://reviewboard.mozilla.org/r/173192/#review179350

Thanks Thomas!

::: browser/components/preferences/in-content-new/tests/browser_security-2.js:62
(Diff revisions 4 - 5)
>  
>    await checkPrefSwitch(true);
>    await checkPrefSwitch(false);
>  });
>  
> +requestLongerTimeout(2);

Is that in seconds? Should we make it 5 seconds instead to make sure it doesn't turn into an intermittent when run on slow build machines?

::: browser/components/preferences/in-content/tests/browser_security.js:100
(Diff revision 5)
>  
>    await checkPrefSwitch(true);
>    await checkPrefSwitch(false);
>  });
>  
> +requestLongerTimeout(2);

ditto
Attachment #8901743 - Flags: review?(francois) → review+
That means 2*45s
Keywords: checkin-needed
Comment on attachment 8902605 [details] [diff] [review]
Beta release patch - Add correct unwanted list when users enable uncommon and unwanted software from preference page

Beta release patch, but need to wait for the patch landing in m-c first
Attachment #8902605 - Attachment description: Add correct unwanted list when users enable uncommon and unwanted software from preference page → Beta release patch - Add correct unwanted list when users enable uncommon and unwanted software from preference page
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/c80c4a770752
Add correct unwanted list when users enable uncommon and unwanted software from preference page r=francois
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/3c2a787cc5a2
Add correct unwanted list when users enable uncommon and unwanted software from preference page r=francois
Comment on attachment 8902605 [details] [diff] [review]
Beta release patch - Add correct unwanted list when users enable uncommon and unwanted software from preference page

Approval Request Comment
[Feature/Bug causing the regression]: Enable V4 in FF 56 bug 1366920
[User impact if declined]: V2 update in google list is still getting around
[Is this code covered by automated tests?]: N
[Has the fix been verified in Nightly?]: Not yet
[Needs manual test from QE? If yes, steps to reproduce]: 
Start with an official build of Firefox (e.g. Nightly).

1. Go to about:preferences#privacy
2. Make sure all the check boxes in Security->Phishing Protection are shown
3. Uncheck/check the check boxes "Warn you about unwanted and uncommon software"
4. Go to about:config and search urlclassifier.malwaretable
5. Make sure in V4 only, only goog-unwanted-proto is added/removed

Repeat from step 3 several times

[List of other uplifts needed for the feature/fix]: No
[Is the change risky?]: No
[Why is the change risky/not risky?]: Only flip correct preference 
[String changes made/needed]: No
Attachment #8902605 - Flags: approval-mozilla-beta?
https://hg.mozilla.org/mozilla-central/rev/3c2a787cc5a2
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Andrei, can your team verify the fix ? Thanks!
Flags: qe-verify+
Flags: needinfo?(andrei.vaida)
Small correction to the testing steps:

(In reply to Thomas Nguyen[:tnguyen] ni plz from comment #21)
> 5. Make sure in V4 only, only goog-unwanted-proto is added/removed

goog-unwanted-proto AND test-unwanted-simple will be added/removed as the "Warn you about unwanted and uncommon software" checkbox is toggled.
Timea, maybe you can help check this as well since you are executing the sign-off test this week? Thanks!
Flags: needinfo?(timea.zsoldos)
I have reproduced this issue using Firefox  57.0a1 (2017.08.28) on Win 8.1 x64.
I can confirm this issue is fixed, I verified using Firefox 57.0a1 (2017.09.05) Win 8.1 x64, Mac OS X 10.11 and Ubuntu 14.04 x64.
status-firefox57: fixed → verified
Flags: needinfo?(timea.zsoldos)
(thanks Timea for the timely help!)

Hi Liz:

would you kindly approve the uplift? Thanks.

(In reply to Timea Zsoldos from comment #26)
> I have reproduced this issue using Firefox  57.0a1 (2017.08.28) on Win 8.1
> x64.
> I can confirm this issue is fixed, I verified using Firefox 57.0a1
> (2017.09.05) Win 8.1 x64, Mac OS X 10.11 and Ubuntu 14.04 x64.
Flags: needinfo?(lhenry)
Comment on attachment 8902605 [details] [diff] [review]
Beta release patch - Add correct unwanted list when users enable uncommon and unwanted software from preference page

Fix the issue related to Safe Browsing and was verified. Beta56+.
Flags: needinfo?(lhenry)
Attachment #8902605 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
I can confirm this issue is fixed, I verified using Firefox 56.0b11 Win 8.1 x64, Mac OS X 10.10 and Ubuntu 14.04 x64.
Status: RESOLVED → VERIFIED
status-firefox56: fixed → verified
Flags: qe-verify+
Flags: needinfo?(andrei.vaida)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: