Closed Bug 1434741 Opened 5 years ago Closed 4 years ago

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

Categories

(Toolkit :: Safe Browsing, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox58 --- wontfix
firefox59 --- wontfix
firefox60 --- fixed

People

(Reporter: francois, Assigned: francois)

References

(Blocks 1 open bug)

Details

(Keywords: sec-low, Whiteboard: [adv-main60-])

Attachments

(1 file)

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.
Priority: P2 → P1
Keywords: sec-low
Similarly, if the referrer is whitelisted, we will skip the remote lookup.
Group: toolkit-core-security
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 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 on attachment 8949236 [details]
Bug 1434741 - Only check final download URL against the application reputation whitelist.

https://reviewboard.mozilla.org/r/218606/#review224730
Attachment #8949236 - Flags: review?(gpascutto) → review+
Pushed by fmarier@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b579fb6511ba
Only check final download URL against the application reputation whitelist. r=gcp
https://hg.mozilla.org/mozilla-central/rev/b579fb6511ba
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Whiteboard: [adv-main60+]
Whiteboard: [adv-main60+] → [adv-main60-]
You need to log in before you can comment on or make changes to this bug.