Skip to content

GitLab

T
tangled
Commit 93c657cc authored by ajwong@chromium.org's avatar ajwong@chromium.org
Browse files
Options

Revert 203400 "Add a killswitch for CSD malware IP match and rep..." 

This CL introduced a race on the host_ variable.

> Add a killswitch for CSD malware IP match and report feature. Use a new killswitch whitelist URL which is part of the CSD whitelist.
> 
> BUG=176647
> 
> Review URL: https://chromiumcodereview.appspot.com/14999008

TBR=kewang@google.com

Review URL: https://codereview.chromium.org/16398002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@204087 0039d316-1c4b-4281-b951-d872f2087c98
parent f2fcbd24
Hide whitespace changes
Inline Side-by-side
Showing with 6 additions and 67 deletions
chrome/browser/safe_browsing/client_side_detection_host.cc
View file @ 93c657cc
......@@ -177,8 +177,6 @@ class ClientSideDetectionHost::ShouldClassifyUrlRequest
return;
}
host_->malware_killswitch_on_ = database_manager_->MalwareKillSwitchOn();
BrowserThread::PostTask(
BrowserThread::UI,
FROM_HERE,
......@@ -253,7 +251,6 @@ ClientSideDetectionHost::ClientSideDetectionHost(WebContents* tab)
csd_service_(NULL),
weak_factory_(this),
unsafe_unique_page_id_(-1),
malware_killswitch_on_(false),
malware_report_enabled_(false) {
DCHECK(tab);
// Note: csd_service_ and sb_service will be NULL here in testing.
......@@ -389,16 +386,15 @@ void ClientSideDetectionHost::OnPhishingDetectionDone(
browse_info_.get() &&
verdict->ParseFromString(verdict_str) &&
verdict->IsInitialized()) {
// We do the malware IP matching and request sending if the feature
// is enabled
if (malware_report_enabled_ && !malware_killswitch_on_) {
if (malware_report_enabled_) {
scoped_ptr<ClientMalwareRequest> malware_verdict(
new ClientMalwareRequest);
// Start browser-side malware feature extraction. Once we're done it will
// send the malware client verdict request.
malware_verdict->set_url(verdict->url());
feature_extractor_->ExtractMalwareFeatures(
browse_info_.get(), malware_verdict.get());
browse_info_.get(),
malware_verdict.get());
MalwareFeatureExtractionDone(malware_verdict.Pass());
}
......
chrome/browser/safe_browsing/client_side_detection_host.h
View file @ 93c657cc
......@@ -137,12 +137,8 @@ class ClientSideDetectionHost : public content::WebContentsObserver,
int unsafe_unique_page_id_;
scoped_ptr<SafeBrowsingUIManager::UnsafeResource> unsafe_resource_;
// Whether the malware IP matching feature killswitch is on.
bool malware_killswitch_on_;
// Whether the malware bad ip matching and report feature is enabled.
bool malware_report_enabled_;
DISALLOW_COPY_AND_ASSIGN(ClientSideDetectionHost);
};
......
chrome/browser/safe_browsing/database_manager.cc
View file @ 93c657cc
......@@ -307,14 +307,6 @@ bool SafeBrowsingDatabaseManager::MatchDownloadWhitelistString(
return database_->ContainsDownloadWhitelistedString(str);
}
bool SafeBrowsingDatabaseManager::MalwareKillSwitchOn() {
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
if (!enabled_ || !MakeDatabaseAvailable()) {
return true;
}
return database_->MalwareIPMatchKillSwitchOn();
}
bool SafeBrowsingDatabaseManager::CheckBrowseUrl(const GURL& url,
Client* client) {
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
......
chrome/browser/safe_browsing/database_manager.h
View file @ 93c657cc
......@@ -172,9 +172,6 @@ class SafeBrowsingDatabaseManager
// This method is expected to be called on the IO thread.
virtual bool MatchDownloadWhitelistString(const std::string& str);
// Check if the CSD malware IP matching kill switch is turned on.
virtual bool MalwareKillSwitchOn();
// Called on the IO thread to cancel a pending check if the result is no
// longer needed.
void CancelCheck(Client* client);
......
chrome/browser/safe_browsing/safe_browsing_database.cc
View file @ 93c657cc
......@@ -70,12 +70,6 @@ const size_t kMaxWhitelistSize = 5000;
const char kWhitelistKillSwitchUrl[] =
"sb-ssl.google.com/safebrowsing/csd/killswitch"; // Don't change this!
// If the hash of this exact expression is on a whitelist then the
// malware IP blacklisting feature will be disabled in csd.
// Don't change this!
const char kMalwareIPKillSwitchUrl[] =
"sb-ssl.google.com/safebrowsing/csd/killswitch_malware";
// To save space, the incoming |chunk_id| and |list_id| are combined
// into an |encoded_chunk_id| for storage by shifting the |list_id|
// into the low-order bits. These functions decode that information.
......@@ -1614,12 +1608,3 @@ void SafeBrowsingDatabaseNew::LoadWhitelist(
whitelist->first.swap(new_whitelist);
}
}
bool SafeBrowsingDatabaseNew::MalwareIPMatchKillSwitchOn() {
SBFullHash malware_kill_switch;
crypto::SHA256HashString(kMalwareIPKillSwitchUrl, &malware_kill_switch,
sizeof(malware_kill_switch));
std::vector<SBFullHash> full_hashes;
full_hashes.push_back(malware_kill_switch);
return ContainsWhitelistedHashes(csd_whitelist_, full_hashes);
};
chrome/browser/safe_browsing/safe_browsing_database.h
View file @ 93c657cc
......@@ -171,8 +171,6 @@ class SafeBrowsingDatabase {
const std::vector<SBPrefix>& prefixes,
const std::vector<SBFullHashResult>& full_hits) = 0;
virtual bool MalwareIPMatchKillSwitchOn() = 0;
// The name of the bloom-filter file for the given database file.
// NOTE(shess): OBSOLETE. Present for deleting stale files.
static base::FilePath BloomFilterForFilename(
......@@ -299,9 +297,6 @@ class SafeBrowsingDatabaseNew : public SafeBrowsingDatabase {
const std::vector<SBPrefix>& prefixes,
const std::vector<SBFullHashResult>& full_hits) OVERRIDE;
// Returns the value of malware_kill_switch_;
virtual bool MalwareIPMatchKillSwitchOn() OVERRIDE;
private:
friend class SafeBrowsingDatabaseTest;
FRIEND_TEST_ALL_PREFIXES(SafeBrowsingDatabaseTest, HashCaching);
......@@ -373,7 +368,8 @@ class SafeBrowsingDatabaseNew : public SafeBrowsingDatabase {
// Lock for protecting access to variables that may be used on the
// IO thread. This includes |prefix_set_|, |full_browse_hashes_|,
// |pending_browse_hashes_|, |prefix_miss_cache_|, |csd_whitelist_|.
// |pending_browse_hashes_|, |prefix_miss_cache_|, |csd_whitelist_|,
// and |csd_whitelist_all_urls_|.
base::Lock lookup_lock_;
// Underlying persistent store for chunk data.
......
chrome/browser/safe_browsing/safe_browsing_database_unittest.cc
View file @ 93c657cc
......@@ -1367,19 +1367,6 @@ TEST_F(SafeBrowsingDatabaseTest, Whitelists) {
EXPECT_FALSE(database_->ContainsDownloadWhitelistedUrl(
GURL(std::string("http://www.google.com/"))));
// Test only add the malware IP killswitch
csd_chunks.clear();
chunk.hosts.clear();
InsertAddChunkHostFullHashes(
&chunk, 15, "sb-ssl.google.com/",
"sb-ssl.google.com/safebrowsing/csd/killswitch_malware");
csd_chunks.push_back(chunk);
EXPECT_TRUE(database_->UpdateStarted(&lists));
database_->InsertChunks(safe_browsing_util::kCsdWhiteList, csd_chunks);
database_->UpdateFinished(true);
EXPECT_TRUE(database_->MalwareIPMatchKillSwitchOn());
// Test that the kill-switch works as intended.
csd_chunks.clear();
download_chunks.clear();
......@@ -1388,6 +1375,7 @@ TEST_F(SafeBrowsingDatabaseTest, Whitelists) {
InsertAddChunkHostFullHashes(&chunk, 5, "sb-ssl.google.com/",
"sb-ssl.google.com/safebrowsing/csd/killswitch");
csd_chunks.push_back(chunk);
chunk.hosts.clear();
InsertAddChunkHostFullHashes(&chunk, 5, "sb-ssl.google.com/",
"sb-ssl.google.com/safebrowsing/csd/killswitch");
......@@ -1399,7 +1387,6 @@ TEST_F(SafeBrowsingDatabaseTest, Whitelists) {
download_chunks);
database_->UpdateFinished(true);
EXPECT_TRUE(database_->MalwareIPMatchKillSwitchOn());
EXPECT_TRUE(database_->ContainsCsdWhitelistedUrl(
GURL(std::string("https://") + kGood1Url2 + "/c.html")));
EXPECT_TRUE(database_->ContainsCsdWhitelistedUrl(
......@@ -1427,12 +1414,6 @@ TEST_F(SafeBrowsingDatabaseTest, Whitelists) {
"sb-ssl.google.com/safebrowsing/csd/killswitch");
csd_chunks.push_back(sub_chunk);
sub_chunk.hosts.clear();
InsertSubChunkHostFullHash(
&sub_chunk, 10, 15, "sb-ssl.google.com/",
"sb-ssl.google.com/safebrowsing/csd/killswitch_malware");
csd_chunks.push_back(sub_chunk);
sub_chunk.hosts.clear();
InsertSubChunkHostFullHash(&sub_chunk, 1, 5,
"sb-ssl.google.com/",
......@@ -1445,7 +1426,6 @@ TEST_F(SafeBrowsingDatabaseTest, Whitelists) {
download_chunks);
database_->UpdateFinished(true);
EXPECT_FALSE(database_->MalwareIPMatchKillSwitchOn());
EXPECT_TRUE(database_->ContainsCsdWhitelistedUrl(
GURL(std::string("https://") + kGood1Url2 + "/c.html")));
EXPECT_TRUE(database_->ContainsCsdWhitelistedUrl(
......
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc
View file @ 93c657cc
......@@ -143,9 +143,6 @@ class TestSafeBrowsingDatabase : public SafeBrowsingDatabase {
const std::vector<SBFullHashResult>& full_hits) OVERRIDE {
// Do nothing for the cache.
}
virtual bool MalwareIPMatchKillSwitchOn() OVERRIDE {
return false;
}
// Fill up the database with test URL.
void AddUrl(const GURL& url,
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment