Bug 1434741 - Only check final download URL against the application reputation whitelist. r?gcp
MozReview-Commit-ID: QCaStgteko
--- a/toolkit/components/reputationservice/ApplicationReputation.cpp
+++ b/toolkit/components/reputationservice/ApplicationReputation.cpp
@@ -83,16 +83,18 @@ using safe_browsing::ClientDownloadReque
#define PREF_BLOCK_POTENTIALLY_UNWANTED "browser.safebrowsing.downloads.remote.block_potentially_unwanted"
#define PREF_BLOCK_UNCOMMON "browser.safebrowsing.downloads.remote.block_uncommon"
// MOZ_LOG=ApplicationReputation:5
mozilla::LazyLogModule ApplicationReputationService::prlog("ApplicationReputation");
#define LOG(args) MOZ_LOG(ApplicationReputationService::prlog, mozilla::LogLevel::Debug, args)
#define LOG_ENABLED() MOZ_LOG_TEST(ApplicationReputationService::prlog, mozilla::LogLevel::Debug)
+enum class LookupType { AllowlistOnly, BlocklistOnly, BothLists };
+
class PendingDBLookup;
// A single use class private to ApplicationReputationService encapsulating an
// nsIApplicationReputationQuery and an nsIApplicationReputationCallback. Once
// created by ApplicationReputationService, it is guaranteed to call mCallback.
// This class is private to ApplicationReputationService.
class PendingLookup final : public nsIStreamListener,
public nsITimerCallback,
@@ -138,18 +140,20 @@ private:
nsCOMPtr<nsIApplicationReputationQuery> mQuery;
// The callback with which to report the verdict.
nsCOMPtr<nsIApplicationReputationCallback> mCallback;
// An array of strings created from certificate information used to whitelist
// the downloaded file.
nsTArray<nsCString> mAllowlistSpecs;
- // The source URI of the download, the referrer and possibly any redirects.
+ // The source URI of the download (i.e. final URI after any redirects).
nsTArray<nsCString> mAnylistSpecs;
+ // The referrer and possibly any redirects.
+ nsTArray<nsCString> mBlocklistSpecs;
// When we started this query
TimeStamp mStartTime;
// The channel used to talk to the remote lookup server
nsCOMPtr<nsIChannel> mChannel;
// Timer to abort this lookup if it takes too long
@@ -214,17 +218,17 @@ private:
// for each (cert, issuer) pair in each chain of certificates that is
// associated with the binary.
nsresult GenerateWhitelistStrings();
// Parse the XPCOM certificate lists and stick them into the protocol buffer
// version.
nsresult ParseCertificates(nsIArray* aSigArray);
- // Adds the redirects to mAnylistSpecs to be looked up.
+ // Adds the redirects to mBlocklistSpecs to be looked up.
nsresult AddRedirects(nsIArray* aRedirects);
// Helper function to ensure that we call PendingLookup::LookupNext or
// PendingLookup::OnComplete.
nsresult DoLookupInternal();
// Looks up all the URIs that may be responsible for allowlisting or
// blocklisting the downloaded file. These URIs may include whitelist strings
@@ -249,58 +253,58 @@ public:
NS_DECL_NSIURLCLASSIFIERCALLBACK
// Constructor and destructor
explicit PendingDBLookup(PendingLookup* aPendingLookup);
// Look up the given URI in the safebrowsing DBs, optionally on both the allow
// list and the blocklist. If there is a match, call
// PendingLookup::OnComplete. Otherwise, call PendingLookup::LookupNext.
- nsresult LookupSpec(const nsACString& aSpec, bool aAllowlistOnly);
+ nsresult LookupSpec(const nsACString& aSpec, const LookupType& aLookupType);
private:
~PendingDBLookup();
// The download appeared on the allowlist, blocklist, or no list (and thus
// could trigger a remote query.
enum LIST_TYPES {
ALLOW_LIST = 0,
BLOCK_LIST = 1,
NO_LIST = 2,
};
nsCString mSpec;
- bool mAllowlistOnly;
+ LookupType mLookupType;
RefPtr<PendingLookup> mPendingLookup;
nsresult LookupSpecInternal(const nsACString& aSpec);
};
NS_IMPL_ISUPPORTS(PendingDBLookup,
nsIUrlClassifierCallback)
PendingDBLookup::PendingDBLookup(PendingLookup* aPendingLookup) :
- mAllowlistOnly(false),
+ mLookupType(LookupType::BothLists),
mPendingLookup(aPendingLookup)
{
LOG(("Created pending DB lookup [this = %p]", this));
}
PendingDBLookup::~PendingDBLookup()
{
LOG(("Destroying pending DB lookup [this = %p]", this));
mPendingLookup = nullptr;
}
nsresult
PendingDBLookup::LookupSpec(const nsACString& aSpec,
- bool aAllowlistOnly)
+ const LookupType& aLookupType)
{
LOG(("Checking principal %s [this=%p]", aSpec.Data(), this));
mSpec = aSpec;
- mAllowlistOnly = aAllowlistOnly;
+ mLookupType = aLookupType;
nsresult rv = LookupSpecInternal(aSpec);
if (NS_FAILED(rv)) {
nsAutoCString errorName;
mozilla::GetErrorName(rv, errorName);
LOG(("Error in LookupSpecInternal() [rv = %s, this = %p]",
errorName.get(), this));
return mPendingLookup->LookupNext(); // ignore this lookup and move to next
}
@@ -331,57 +335,59 @@ PendingDBLookup::LookupSpecInternal(cons
LOG(("Checking DB service for principal %s [this = %p]", mSpec.get(), this));
nsCOMPtr<nsIUrlClassifierDBService> dbService =
do_GetService(NS_URLCLASSIFIERDBSERVICE_CONTRACTID, &rv);
NS_ENSURE_SUCCESS(rv, rv);
nsAutoCString tables;
nsAutoCString allowlist;
Preferences::GetCString(PREF_DOWNLOAD_ALLOW_TABLE, allowlist);
- if (!allowlist.IsEmpty()) {
+ if ((mLookupType != LookupType::BlocklistOnly) && !allowlist.IsEmpty()) {
tables.Append(allowlist);
}
nsAutoCString blocklist;
Preferences::GetCString(PREF_DOWNLOAD_BLOCK_TABLE, blocklist);
- if (!mAllowlistOnly && !blocklist.IsEmpty()) {
- tables.Append(',');
+ if ((mLookupType != LookupType::AllowlistOnly) && !blocklist.IsEmpty()) {
+ if (!tables.IsEmpty()) {
+ tables.Append(',');
+ }
tables.Append(blocklist);
}
return dbService->Lookup(principal, tables, this);
}
NS_IMETHODIMP
PendingDBLookup::HandleEvent(const nsACString& tables)
{
// HandleEvent is guaranteed to call either:
// 1) PendingLookup::OnComplete if the URL matches the blocklist, or
// 2) PendingLookup::LookupNext if the URL does not match the blocklist.
// Blocklisting trumps allowlisting.
nsAutoCString blockList;
Preferences::GetCString(PREF_DOWNLOAD_BLOCK_TABLE, blockList);
- if (!mAllowlistOnly && FindInReadable(blockList, tables)) {
+ if ((mLookupType != LookupType::AllowlistOnly) && FindInReadable(blockList, tables)) {
mPendingLookup->mBlocklistCount++;
Accumulate(mozilla::Telemetry::APPLICATION_REPUTATION_LOCAL, BLOCK_LIST);
LOG(("Found principal %s on blocklist [this = %p]", mSpec.get(), this));
return mPendingLookup->OnComplete(true, NS_OK,
nsIApplicationReputationService::VERDICT_DANGEROUS);
}
nsAutoCString allowList;
Preferences::GetCString(PREF_DOWNLOAD_ALLOW_TABLE, allowList);
- if (FindInReadable(allowList, tables)) {
+ if ((mLookupType != LookupType::BlocklistOnly) && FindInReadable(allowList, tables)) {
mPendingLookup->mAllowlistCount++;
Accumulate(mozilla::Telemetry::APPLICATION_REPUTATION_LOCAL, ALLOW_LIST);
LOG(("Found principal %s on allowlist [this = %p]", mSpec.get(), this));
// Don't call onComplete, since blocklisting trumps allowlisting
- } else {
- LOG(("Didn't find principal %s on any list [this = %p]", mSpec.get(),
- this));
- Accumulate(mozilla::Telemetry::APPLICATION_REPUTATION_LOCAL, NO_LIST);
+ return mPendingLookup->LookupNext();
}
+
+ LOG(("Didn't find principal %s on any list [this = %p]", mSpec.get(), this));
+ Accumulate(mozilla::Telemetry::APPLICATION_REPUTATION_LOCAL, NO_LIST);
return mPendingLookup->LookupNext();
}
NS_IMPL_ISUPPORTS(PendingLookup,
nsIStreamListener,
nsIRequestObserver,
nsIObserver,
nsISupportsWeakReference)
@@ -768,47 +774,60 @@ PendingLookup::GetDownloadType(const nsA
}
nsresult
PendingLookup::LookupNext()
{
// We must call LookupNext or SendRemoteQuery upon return.
// Look up all of the URLs that could allow or block this download.
// Blocklist first.
+
+ // If any of mAnylistSpecs or mBlocklistSpecs matched the blocklist,
+ // go ahead and block.
if (mBlocklistCount > 0) {
return OnComplete(true, NS_OK,
nsIApplicationReputationService::VERDICT_DANGEROUS);
}
+
int index = mAnylistSpecs.Length() - 1;
nsCString spec;
if (index >= 0) {
- // Check the source URI, referrer and redirect chain.
+ // Check the source URI only.
spec = mAnylistSpecs[index];
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) {
- return OnComplete(true, NS_OK,
- nsIApplicationReputationService::VERDICT_DANGEROUS);
+
+ index = mBlocklistSpecs.Length() - 1;
+ if (index >= 0) {
+ // Check the referrer and redirect chain.
+ spec = mBlocklistSpecs[index];
+ mBlocklistSpecs.RemoveElementAt(index);
+ RefPtr<PendingDBLookup> lookup(new PendingDBLookup(this));
+ return lookup->LookupSpec(spec, LookupType::BlocklistOnly);
}
- // If any of mAnylistSpecs matched the allowlist, go ahead and pass.
+
+ // Now that we've looked up all of the URIs against the blocklist,
+ // if any of mAnylistSpecs or mAllowlistSpecs matched the allowlist,
+ // go ahead and pass.
if (mAllowlistCount > 0) {
return OnComplete(false, NS_OK);
}
+
// Only binary signatures remain.
index = mAllowlistSpecs.Length() - 1;
if (index >= 0) {
spec = mAllowlistSpecs[index];
LOG(("PendingLookup::LookupNext: checking %s on allowlist", spec.get()));
mAllowlistSpecs.RemoveElementAt(index);
RefPtr<PendingDBLookup> lookup(new PendingDBLookup(this));
- return lookup->LookupSpec(spec, true);
+ return lookup->LookupSpec(spec, LookupType::AllowlistOnly);
}
+
// There are no more URIs to check against local list. If the file is
// not eligible for remote lookup, bail.
if (!IsBinaryFile()) {
LOG(("Not eligible for remote lookups [this=%p]", this));
return OnComplete(false, NS_OK);
}
nsresult rv = SendRemoteQuery();
if (NS_FAILED(rv)) {
@@ -981,17 +1000,17 @@ PendingLookup::AddRedirects(nsIArray* aR
rv = principal->GetURI(getter_AddRefs(uri));
NS_ENSURE_SUCCESS(rv, rv);
// Add the spec to our list of local lookups. The most recent redirect is
// the last element.
nsCString spec;
rv = GetStrippedSpec(uri, spec);
NS_ENSURE_SUCCESS(rv, rv);
- mAnylistSpecs.AppendElement(spec);
+ mBlocklistSpecs.AppendElement(spec);
LOG(("ApplicationReputation: Appending redirect %s\n", spec.get()));
// Store the redirect information in the remote request.
ClientDownloadRequest_Resource* resource = mRequest.add_resources();
resource->set_url(spec.get());
resource->set_type(ClientDownloadRequest::DOWNLOAD_REDIRECT);
rv = iter->HasMoreElements(&hasMoreRedirects);
@@ -1127,17 +1146,17 @@ PendingLookup::DoLookupInternal()
resource->set_type(ClientDownloadRequest::DOWNLOAD_URL);
nsCOMPtr<nsIURI> referrer = nullptr;
rv = mQuery->GetReferrerURI(getter_AddRefs(referrer));
if (referrer) {
nsCString referrerSpec;
rv = GetStrippedSpec(referrer, referrerSpec);
NS_ENSURE_SUCCESS(rv, rv);
- mAnylistSpecs.AppendElement(referrerSpec);
+ mBlocklistSpecs.AppendElement(referrerSpec);
resource->set_referrer(referrerSpec.get());
}
nsCOMPtr<nsIArray> redirects;
rv = mQuery->GetRedirects(getter_AddRefs(redirects));
if (redirects) {
AddRedirects(redirects);
} else {
LOG(("ApplicationReputation: Got no redirects [this=%p]", this));
--- a/toolkit/components/reputationservice/test/unit/test_app_rep.js
+++ b/toolkit/components/reputationservice/test/unit/test_app_rep.js
@@ -264,16 +264,17 @@ add_test(function test_local_blacklist()
});
add_test(function test_referer_blacklist() {
Services.prefs.setCharPref(appRepURLPref,
"http://localhost:4444/download");
let counts = get_telemetry_counts();
let listCounts = counts.listCounts;
listCounts[BLOCK_LIST]++;
+ listCounts[NO_LIST]++;
gAppRep.queryReputation({
sourceURI: exampleURI,
referrerURI: blocklistedURI,
fileSize: 12,
}, function onComplete(aShouldBlock, aStatus) {
Assert.equal(Cr.NS_OK, aStatus);
Assert.ok(aShouldBlock);
check_telemetry(counts.shouldBlock + 1, listCounts);
@@ -282,16 +283,17 @@ add_test(function test_referer_blacklist
});
add_test(function test_blocklist_trumps_allowlist() {
Services.prefs.setCharPref(appRepURLPref,
"http://localhost:4444/download");
let counts = get_telemetry_counts();
let listCounts = counts.listCounts;
listCounts[BLOCK_LIST]++;
+ listCounts[ALLOW_LIST]++;
gAppRep.queryReputation({
sourceURI: whitelistedURI,
referrerURI: blocklistedURI,
fileSize: 12,
}, function onComplete(aShouldBlock, aStatus) {
Assert.equal(Cr.NS_OK, aStatus);
Assert.ok(aShouldBlock);
check_telemetry(counts.shouldBlock + 1, listCounts);
@@ -301,32 +303,35 @@ add_test(function test_blocklist_trumps_
add_test(function test_redirect_on_blocklist() {
Services.prefs.setCharPref(appRepURLPref,
"http://localhost:4444/download");
let counts = get_telemetry_counts();
let listCounts = counts.listCounts;
listCounts[BLOCK_LIST]++;
listCounts[ALLOW_LIST]++;
+ listCounts[NO_LIST]++;
let secman = Services.scriptSecurityManager;
let badRedirects = Cc["@mozilla.org/array;1"]
.createInstance(Ci.nsIMutableArray);
let redirect1 = {
QueryInterface: XPCOMUtils.generateQI([Ci.nsIRedirectHistoryEntry]),
principal: secman.createCodebasePrincipal(exampleURI, {}),
};
badRedirects.appendElement(redirect1);
let redirect2 = {
QueryInterface: XPCOMUtils.generateQI([Ci.nsIRedirectHistoryEntry]),
principal: secman.createCodebasePrincipal(blocklistedURI, {}),
};
badRedirects.appendElement(redirect2);
+ // Add a whitelisted URI that will not be looked up against the
+ // whitelist (i.e. it will match NO_LIST).
let redirect3 = {
QueryInterface: XPCOMUtils.generateQI([Ci.nsIRedirectHistoryEntry]),
principal: secman.createCodebasePrincipal(whitelistedURI, {}),
};
badRedirects.appendElement(redirect3);
gAppRep.queryReputation({
sourceURI: whitelistedURI,
@@ -335,8 +340,79 @@ add_test(function test_redirect_on_block
fileSize: 12,
}, function onComplete(aShouldBlock, aStatus) {
Assert.equal(Cr.NS_OK, aStatus);
Assert.ok(aShouldBlock);
check_telemetry(counts.shouldBlock + 1, listCounts);
run_next_test();
});
});
+
+add_test(function test_whitelisted_source() {
+ Services.prefs.setCharPref(appRepURLPref,
+ "http://localhost:4444/download");
+ let counts = get_telemetry_counts();
+ let listCounts = counts.listCounts;
+ listCounts[ALLOW_LIST]++;
+ gAppRep.queryReputation({
+ sourceURI: whitelistedURI,
+ fileSize: 12,
+ }, function onComplete(aShouldBlock, aStatus) {
+ Assert.equal(Cr.NS_OK, aStatus);
+ Assert.ok(!aShouldBlock);
+ check_telemetry(counts.shouldBlock, listCounts);
+ run_next_test();
+ });
+});
+
+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;
+ gAppRep.queryReputation({
+ sourceURI: exampleURI,
+ referrerURI: whitelistedURI,
+ fileSize: 12,
+ }, function onComplete(aShouldBlock, aStatus) {
+ Assert.equal(Cr.NS_OK, aStatus);
+ Assert.ok(!aShouldBlock);
+ check_telemetry(counts.shouldBlock, listCounts);
+ run_next_test();
+ });
+});
+
+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;
+ let secman = Services.scriptSecurityManager;
+ let okayRedirects = Cc["@mozilla.org/array;1"]
+ .createInstance(Ci.nsIMutableArray);
+
+ let redirect1 = {
+ QueryInterface: XPCOMUtils.generateQI([Ci.nsIRedirectHistoryEntry]),
+ principal: secman.createCodebasePrincipal(exampleURI, {}),
+ };
+ okayRedirects.appendElement(redirect1);
+
+ // Add a whitelisted URI that will not be looked up against the
+ // whitelist (i.e. it will match NO_LIST).
+ let redirect2 = {
+ QueryInterface: XPCOMUtils.generateQI([Ci.nsIRedirectHistoryEntry]),
+ principal: secman.createCodebasePrincipal(whitelistedURI, {}),
+ };
+ okayRedirects.appendElement(redirect2);
+
+ gAppRep.queryReputation({
+ sourceURI: exampleURI,
+ redirects: okayRedirects,
+ fileSize: 12,
+ }, function onComplete(aShouldBlock, aStatus) {
+ Assert.equal(Cr.NS_OK, aStatus);
+ Assert.ok(!aShouldBlock);
+ check_telemetry(counts.shouldBlock, listCounts);
+ run_next_test();
+ });
+});