1356427 - Only check binary files against the Application Reputation whitelist

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, enhancement, P2)

Description

[François Marier [:francois]]

We're checking the Application Reputation whitelist for all download types, not just for binary files. This is not necessary and we could return earlier.

In particular, we could return before checking certificates against the
whitelist:
https://searchfox.org/mozilla-central/rev/d4eaa9c2fa54d553349ac88f0c312155a4c6e89e/toolkit/components/downloads/ApplicationReputation.cpp#855

Comment 1

[BMO Automation]

Per policy at https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Inactive_Bugs. If this bug is not an enhancement request or a bug not present in a supported release of Firefox, then it may be reopened.

Comment 2

[Dimi Lee [:dimi]]

Attachment: Bug 1356427 - P1. Remove redundant code from PendingLookup::LookupNext. r?francois

When mBlocklistCount is set, we will call PendingLookup::OnComplete() to
block this download. The |if(mBlocklistCount)| in PendingLookup::LookupNext
will never be true.

mBlocklistCount member variable is also useless after removing the code.
But I think we can keep it since we have mAllowlistCount and we might also
need it in the future.

Comment 3

[Dimi Lee [:dimi]]

Attachment: Bug 1356427 - P2. Only check binary files against the Application Reputation whitelist, r?francois

We already skip checking whitelist for redirects and referrer in Bug 1434741
So this patch just does the following to remove redundant whitelist
check:
1. Skip checking whitelist for the final URL if it is not a binary
2. Skip parsing certificate for non-binary files

After applying this patch, whitelist will only be used for final URL of
binary file and certificates of binary file

Comment 4

[Dimi Lee [:dimi]]

Attachment: Bug 1356427 - P3. Add a testcase to test download protection will skip checking whitelist for non-binary files. r?francois

Add xpcshell test test_whitelisted_none_binary_source. In this test,
we pass a whitelisted sourceURI and a non-binary file extension.
Check the returned result is NO_LIST to ensure no whitelisted local db
query has been made.

This patch also adds binary filename for testcases require
whitelist query being made.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ced08ec47ace
P1. Remove redundant code from PendingLookup::LookupNext. r=francois
https://hg.mozilla.org/integration/autoland/rev/c3e09876ab13
P2. Only check binary files against the Application Reputation whitelist, r=francois
https://hg.mozilla.org/integration/autoland/rev/6e0c72599e0b
P3. Add a testcase to test download protection will skip checking whitelist for non-binary files. r=francois

Comment 6

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/ced08ec47ace
https://hg.mozilla.org/mozilla-central/rev/c3e09876ab13
https://hg.mozilla.org/mozilla-central/rev/6e0c72599e0b