1434741 - Download protection checks are skipped if anything in the redirect chain is whitelisted

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect, P1)

Description

[François Marier [:francois]]

Dimi found that in Firefox, we will skip the download protection check if any of the URLs in the redirect chain are on the whitelist:

https://searchfox.org/mozilla-central/rev/f41017f5cd7b5f3835d282fbe011d2899574fc2b/toolkit/components/reputationservice/ApplicationReputation.cpp#774-777

According to Dimi, this is different from Chrome which only looks at the final URL for whitelisting purposes.

This means that the following redirection:

    whitelisted.com -> blacklisted.com

will be blocked in both browsers, thanks to bug 1017228, but this one:

    whitelisted.com -> unsure.com

will be whitelisted on Firefox but will trigger a remote lookup on Chrome.

Our behaviour should match Chrome's and we should only look at the final URL when deciding whether or not to skip the check.

Comment 1

[François Marier [:francois]]

Similarly, if the referrer is whitelisted, we will skip the remote lookup.

Comment 2

[François Marier [:francois]]

Attachment: Bug 1434741 - Only check final download URL against the application reputation whitelist.

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

Comment 3

[François Marier [:francois]]

Comment on attachment 8949236 [details]
Bug 1434741 - Only check final download URL against the application reputation whitelist.

https://reviewboard.mozilla.org/r/218606/#review224414

::: toolkit/components/reputationservice/ApplicationReputation.cpp:800
(Diff revision 1)
>      mAnylistSpecs.RemoveElementAt(index);
>      RefPtr<PendingDBLookup> lookup(new PendingDBLookup(this));
> -    return lookup->LookupSpec(spec, false);
> +    return lookup->LookupSpec(spec, LookupType::BothLists);
>    }
> -  // If any of mAnylistSpecs matched the blocklist, go ahead and block.
> -  if (mBlocklistCount > 0) {
> +
> +  index = mBlocklistSpecs.Length() - 1;

This second `if (mBlocklistCount > 0)` was redundant in the original code. There's no way to get down here without triggering the same check at the top of the function.

::: toolkit/components/reputationservice/test/unit/test_app_rep.js:272
(Diff revision 1)
>    Services.prefs.setCharPref(appRepURLPref,
>                               "http://localhost:4444/download");
>    let counts = get_telemetry_counts();
>    let listCounts = counts.listCounts;
>    listCounts[BLOCK_LIST]++;
> +  listCounts[NO_LIST]++;

We're now checking the `sourceURI` before the `referrerURI`  so we're expecting to see the results from both.

::: toolkit/components/reputationservice/test/unit/test_app_rep.js:291
(Diff revision 1)
>    Services.prefs.setCharPref(appRepURLPref,
>                               "http://localhost:4444/download");
>    let counts = get_telemetry_counts();
>    let listCounts = counts.listCounts;
>    listCounts[BLOCK_LIST]++;
> +  listCounts[ALLOW_LIST]++;

Same comment as before. The order that we check things in has changed.

::: toolkit/components/reputationservice/test/unit/test_app_rep.js:371
(Diff revision 1)
> +add_test(function test_whitelisted_referrer() {
> +  Services.prefs.setCharPref(appRepURLPref,
> +                             "http://localhost:4444/download");
> +  let counts = get_telemetry_counts();
> +  let listCounts = counts.listCounts;
> +  listCounts[NO_LIST] += 2;

Checking here that the `ALLOW_LIST` count has not been bumped because we're no longer checking the referrer against the whitelist.

::: toolkit/components/reputationservice/test/unit/test_app_rep.js:389
(Diff revision 1)
> +add_test(function test_whitelisted_redirect() {
> +  Services.prefs.setCharPref(appRepURLPref,
> +                             "http://localhost:4444/download");
> +  let counts = get_telemetry_counts();
> +  let listCounts = counts.listCounts;
> +  listCounts[NO_LIST] += 3;

Similarly, we're no longer checking the redirect chain against the whitelist.

Comment 4

[Code Review Bot [:reviewbot]]

Comment on attachment 8949236 [details]
Bug 1434741 - Only check final download URL against the application reputation whitelist.

https://reviewboard.mozilla.org/r/218606/#review224420


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/components/reputationservice/test/unit/test_app_rep.js:410
(Diff revision 1)
> +  };
> +  redirects.appendElement(redirect2);
> +
> +  gAppRep.queryReputation({
> +    sourceURI: exampleURI,
> +    redirects: redirects,

Error: Expected property shorthand. [eslint: object-shorthand]

Comment 5

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8949236 [details]
Bug 1434741 - Only check final download URL against the application reputation whitelist.

https://reviewboard.mozilla.org/r/218606/#review224730

Comment 6

[François Marier [:francois]]

Comment on attachment 8949236 [details]
Bug 1434741 - Only check final download URL against the application reputation whitelist.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/218606/diff/1-2/

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b579fb6511ba
Only check final download URL against the application reputation whitelist. r=gcp

Comment 8

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/b579fb6511ba