Skip to content

Commit

Permalink
Revert of Sample 1% of whitelisted downloads to still send download p…
Browse files Browse the repository at this point in the history 
…ings, if: (1) user opted in extended rep… (patchset #1 id:1 of https://codereview.chromium.org/1920293002/ )

Reason for revert:
merge conflict unresolved.

Original issue's description:
> Sample 1% of whitelisted downloads to still send download pings, if: (1) user opted in extended reporting (2) user not in incognito mode.
>
> CSD server side change will make sure always return "SAFE" for these sampled pings.
>
> This cl is patched from 1866033003 (reverted), with changes to address buildbot memory warnings on linux Valgrind Test and nparker's comments on previous CL.
>
> BUG=553674
>
> Committed: https://crrev.com/4722e9b36cfbca7493d3e4f7e2242ac6b8f30a04
> Cr-Commit-Position: refs/heads/master@{#387968}
>
> patch from issue 1866033003 at patchset 370001 (http://crrev.com/1866033003#ps370001)
>
> Review URL: https://codereview.chromium.org/1898013002
>
> Cr-Commit-Position: refs/heads/master@{#388325}
> (cherry picked from commit 75389f5)
>
> Committed: https://chromium.googlesource.com/chromium/src/+/575419fe258c678c589e179c998581992886f89a

TBR=
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=553674

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

Cr-Commit-Position: refs/branch-heads/2661@{#633}
Cr-Branched-From: ef6f6ae-refs/heads/master@{#378081}
  • Loading branch information
jialiul authored and Commit bot committed Apr 26, 2016
1 parent 0dbc142 commit 2d9c77f
Show file tree
Hide file tree
Showing 4 changed files with 508 additions and 457 deletions.
75 changes: 22 additions & 53 deletions chrome/browser/safe_browsing/download_protection_service.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
#include "base/metrics/field_trial.h"
#include "base/metrics/histogram.h"
#include "base/metrics/sparse_histogram.h"
#include "base/rand_util.h"
#include "base/sequenced_task_runner_helpers.h"
#include "base/stl_util.h"
#include "base/strings/string_number_conversions.h"
Expand Down Expand Up @@ -70,8 +69,6 @@ using content::BrowserThread;

namespace {
static const int64_t kDownloadRequestTimeoutMs = 7000;
// We sample 1% of whitelisted downloads to still send out download pings.
static const double kWhitelistDownloadSampleRate = 0.01;
} // namespace

namespace safe_browsing {
Expand Down Expand Up @@ -295,10 +292,6 @@ class DownloadProtectionService::CheckClientDownloadRequest
finished_(false),
type_(ClientDownloadRequest::WIN_EXECUTABLE),
start_time_(base::TimeTicks::Now()),
skipped_url_whitelist_(false),
skipped_certificate_whitelist_(false),
is_extended_reporting_(false),
is_incognito_(false),
weakptr_factory_(this) {
DCHECK_CURRENTLY_ON(BrowserThread::UI);
item_->AddObserver(this);
Expand All @@ -308,14 +301,6 @@ class DownloadProtectionService::CheckClientDownloadRequest
DVLOG(2) << "Starting SafeBrowsing download check for: "
<< item_->DebugString(true);
DCHECK_CURRENTLY_ON(BrowserThread::UI);
if (item_->GetBrowserContext()) {
Profile* profile =
Profile::FromBrowserContext(item_->GetBrowserContext());
is_extended_reporting_ = profile &&
profile->GetPrefs()->GetBoolean(
prefs::kSafeBrowsingExtendedReportingEnabled);
is_incognito_ = item_->GetBrowserContext()->IsOffTheRecord();
}
// TODO(noelutz): implement some cache to make sure we don't issue the same
// request over and over again if a user downloads the same binary multiple
// times.
Expand Down Expand Up @@ -727,13 +712,6 @@ class DownloadProtectionService::CheckClientDownloadRequest
WHITELIST_TYPE_MAX);
}

virtual bool ShouldSampleWhitelistedDownload() {
// We currently sample 1% whitelisted downloads from users who opted
// in extended reporting and are not in incognito mode.
return service_ && is_extended_reporting_ && !is_incognito_ &&
base::RandDouble() < service_->whitelist_sample_rate();
}

void CheckWhitelists() {
DCHECK_CURRENTLY_ON(BrowserThread::IO);

Expand All @@ -747,33 +725,23 @@ class DownloadProtectionService::CheckClientDownloadRequest
if (url.is_valid() && database_manager_->MatchDownloadWhitelistUrl(url)) {
DVLOG(2) << url << " is on the download whitelist.";
RecordCountOfWhitelistedDownload(URL_WHITELIST);
if (ShouldSampleWhitelistedDownload()) {
skipped_url_whitelist_ = true;
} else {
// TODO(grt): Continue processing without uploading so that
// ClientDownloadRequest callbacks can be run even for this type of safe
// download.
PostFinishTask(SAFE, REASON_WHITELISTED_URL);
return;
}
// TODO(grt): Continue processing without uploading so that
// ClientDownloadRequest callbacks can be run even for this type of safe
// download.
PostFinishTask(SAFE, REASON_WHITELISTED_URL);
return;
}

bool should_sample_for_certificate = ShouldSampleWhitelistedDownload();
if (signature_info_.trusted()) {
for (int i = 0; i < signature_info_.certificate_chain_size(); ++i) {
if (CertificateChainIsWhitelisted(
signature_info_.certificate_chain(i))) {
RecordCountOfWhitelistedDownload(SIGNATURE_WHITELIST);
if (should_sample_for_certificate) {
skipped_certificate_whitelist_ = true;
break;
} else {
// TODO(grt): Continue processing without uploading so that
// ClientDownloadRequest callbacks can be run even for this type of
// safe download.
PostFinishTask(SAFE, REASON_TRUSTED_EXECUTABLE);
return;
}
// TODO(grt): Continue processing without uploading so that
// ClientDownloadRequest callbacks can be run even for this type of
// safe download.
PostFinishTask(SAFE, REASON_TRUSTED_EXECUTABLE);
return;
}
}
}
Expand Down Expand Up @@ -854,9 +822,17 @@ class DownloadProtectionService::CheckClientDownloadRequest
// before sending it.
if (!service_)
return;
bool is_extended_reporting = false;
if (item_->GetBrowserContext()) {
Profile* profile =
Profile::FromBrowserContext(item_->GetBrowserContext());
is_extended_reporting = profile &&
profile->GetPrefs()->GetBoolean(
prefs::kSafeBrowsingExtendedReportingEnabled);
}

ClientDownloadRequest request;
if (is_extended_reporting_) {
if (is_extended_reporting) {
request.mutable_population()->set_user_population(
ChromeUserPopulation::EXTENDED_REPORTING);
} else {
Expand All @@ -866,8 +842,6 @@ class DownloadProtectionService::CheckClientDownloadRequest
request.set_url(SanitizeUrl(item_->GetUrlChain().back()));
request.mutable_digests()->set_sha256(item_->GetHash());
request.set_length(item_->GetReceivedBytes());
request.set_skipped_url_whitelist(true);
request.set_skipped_certificate_whitelist(true);
for (size_t i = 0; i < item_->GetUrlChain().size(); ++i) {
ClientDownloadRequest::Resource* resource = request.add_resources();
resource->set_url(SanitizeUrl(item_->GetUrlChain()[i]));
Expand Down Expand Up @@ -1086,10 +1060,6 @@ class DownloadProtectionService::CheckClientDownloadRequest
base::TimeTicks start_time_; // Used for stats.
base::TimeTicks timeout_start_time_;
base::TimeTicks request_start_time_;
bool skipped_url_whitelist_;
bool skipped_certificate_whitelist_;
bool is_extended_reporting_;
bool is_incognito_;
base::WeakPtrFactory<CheckClientDownloadRequest> weakptr_factory_;

DISALLOW_COPY_AND_ASSIGN(CheckClientDownloadRequest);
Expand All @@ -1102,10 +1072,9 @@ DownloadProtectionService::DownloadProtectionService(
enabled_(false),
binary_feature_extractor_(new BinaryFeatureExtractor()),
download_request_timeout_ms_(kDownloadRequestTimeoutMs),
feedback_service_(
new DownloadFeedbackService(request_context_getter_.get(),
BrowserThread::GetBlockingPool())),
whitelist_sample_rate_(kWhitelistDownloadSampleRate) {
feedback_service_(new DownloadFeedbackService(
request_context_getter, BrowserThread::GetBlockingPool())) {

if (sb_service) {
ui_manager_ = sb_service->ui_manager();
database_manager_ = sb_service->database_manager();
Expand Down
11 changes: 1 addition & 10 deletions chrome/browser/safe_browsing/download_protection_service.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -140,10 +140,6 @@ class DownloadProtectionService {
ClientDownloadRequestSubscription RegisterClientDownloadRequestCallback(
const ClientDownloadRequestCallback& callback);

double whitelist_sample_rate() const {
return whitelist_sample_rate_;
}

protected:
// Enum to keep track why a particular download verdict was chosen.
// This is used to keep some stats around.
Expand Down Expand Up @@ -180,9 +176,7 @@ class DownloadProtectionService {
friend class DownloadProtectionServiceTest;

FRIEND_TEST_ALL_PREFIXES(DownloadProtectionServiceTest,
CheckClientDownloadWhitelistedUrlWithoutSampling);
FRIEND_TEST_ALL_PREFIXES(DownloadProtectionServiceTest,
CheckClientDownloadWhitelistedUrlWithSampling);
CheckClientDownloadWhitelistedUrl);
FRIEND_TEST_ALL_PREFIXES(DownloadProtectionServiceTest,
CheckClientDownloadValidateRequest);
FRIEND_TEST_ALL_PREFIXES(DownloadProtectionServiceTest,
Expand Down Expand Up @@ -255,9 +249,6 @@ class DownloadProtectionService {
// Normally empty.
std::set<std::string> manual_blacklist_hashes_;

// Rate of whitelisted downloads we sample to send out download ping.
double whitelist_sample_rate_;

DISALLOW_COPY_AND_ASSIGN(DownloadProtectionService);
};
} // namespace safe_browsing
Expand Down
Loading

0 comments on commit 2d9c77f

Please sign in to comment.