Skip to content

Commit

Permalink
Enable enterprise features for download protection
Browse files Browse the repository at this point in the history
1. Skip checking for enterprise whitelisted downloads
If enterprise admin has configured "SafeBrowsingWhitelistDomains" policy,
and the any URL in this download's |GetUrlChain()| matches any of these
whitelisted domains, download protection will not check the URL(s) and
content of this download.

2. When user opens a dangerous download, trigger |OnDangerousDownloadOpened|
private extension API calls. (This private extension API call is only used by
enterprise reporting extension.)

Bug: 811467, 811454
Change-Id: I7e716a6a10900bc894ad721368edb9c82802efd4
TBR: benjhayden@chromium.org
Reviewed-on: https://chromium-review.googlesource.com/1054832
Commit-Queue: Jialiu Lin <jialiul@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Nathan Parker <nparker@chromium.org>
Reviewed-by: Asanka Herath <asanka@chromium.org>
Reviewed-by: David Trainor <dtrainor@chromium.org>
Reviewed-by: Varun Khaneja <vakh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#559131}
  • Loading branch information
Jialiu Lin authored and Commit Bot committed May 16, 2018
1 parent d916d49 commit 93850e0
Show file tree
Hide file tree
Showing 25 changed files with 315 additions and 33 deletions.
7 changes: 7 additions & 0 deletions chrome/browser/download/chrome_download_manager_delegate.cc
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,7 @@ base::FilePath GetPlatformDownloadPath(Profile* profile,
// * DANGEROUS_URL, if the URL is a known malware site.
// * MAYBE_DANGEROUS_CONTENT, if the content will be scanned for
// malware. I.e. |is_content_check_supported| is true.
// * WHITELISTED_BY_POLICY, if the download matches enterprise whitelist.
// * NOT_DANGEROUS.
void CheckDownloadUrlDone(
const DownloadTargetDeterminerDelegate::CheckDownloadUrlCallback& callback,
Expand All @@ -180,6 +181,9 @@ void CheckDownloadUrlDone(
danger_type = download::DOWNLOAD_DANGER_TYPE_MAYBE_DANGEROUS_CONTENT;
else
danger_type = download::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS;
} else if (result ==
safe_browsing::DownloadCheckResult::WHITELISTED_BY_POLICY) {
danger_type = download::DOWNLOAD_DANGER_TYPE_WHITELISTED_BY_POLICY;
} else {
// If the URL is malicious, we'll use that as the danger type. The results
// of the content check, if one is performed, will be ignored.
Expand Down Expand Up @@ -1018,6 +1022,9 @@ void ChromeDownloadManagerDelegate::CheckClientDownloadDone(
case safe_browsing::DownloadCheckResult::POTENTIALLY_UNWANTED:
danger_type = download::DOWNLOAD_DANGER_TYPE_POTENTIALLY_UNWANTED;
break;
case safe_browsing::DownloadCheckResult::WHITELISTED_BY_POLICY:
danger_type = download::DOWNLOAD_DANGER_TYPE_WHITELISTED_BY_POLICY;
break;
}
DCHECK_NE(danger_type,
download::DOWNLOAD_DANGER_TYPE_MAYBE_DANGEROUS_CONTENT);
Expand Down
1 change: 1 addition & 0 deletions chrome/browser/download/download_danger_prompt.cc
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ const char* GetDangerTypeString(
case download::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS:
case download::DOWNLOAD_DANGER_TYPE_MAYBE_DANGEROUS_CONTENT:
case download::DOWNLOAD_DANGER_TYPE_USER_VALIDATED:
case download::DOWNLOAD_DANGER_TYPE_WHITELISTED_BY_POLICY:
case download::DOWNLOAD_DANGER_TYPE_MAX:
break;
}
Expand Down
3 changes: 3 additions & 0 deletions chrome/browser/download/download_item_model.cc
Original file line number Diff line number Diff line change
Expand Up @@ -443,6 +443,7 @@ base::string16 DownloadItemModel::GetWarningText(const gfx::FontList& font_list,
case download::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS:
case download::DOWNLOAD_DANGER_TYPE_MAYBE_DANGEROUS_CONTENT:
case download::DOWNLOAD_DANGER_TYPE_USER_VALIDATED:
case download::DOWNLOAD_DANGER_TYPE_WHITELISTED_BY_POLICY:
case download::DOWNLOAD_DANGER_TYPE_MAX: {
break;
}
Expand Down Expand Up @@ -497,6 +498,7 @@ bool DownloadItemModel::MightBeMalicious() const {
case download::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS:
case download::DOWNLOAD_DANGER_TYPE_MAYBE_DANGEROUS_CONTENT:
case download::DOWNLOAD_DANGER_TYPE_USER_VALIDATED:
case download::DOWNLOAD_DANGER_TYPE_WHITELISTED_BY_POLICY:
case download::DOWNLOAD_DANGER_TYPE_MAX:
// We shouldn't get any of these due to the IsDangerous() test above.
NOTREACHED();
Expand All @@ -523,6 +525,7 @@ bool DownloadItemModel::IsMalicious() const {
case download::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS:
case download::DOWNLOAD_DANGER_TYPE_MAYBE_DANGEROUS_CONTENT:
case download::DOWNLOAD_DANGER_TYPE_USER_VALIDATED:
case download::DOWNLOAD_DANGER_TYPE_WHITELISTED_BY_POLICY:
case download::DOWNLOAD_DANGER_TYPE_MAX:
case download::DOWNLOAD_DANGER_TYPE_DANGEROUS_FILE:
// We shouldn't get any of these due to the MightBeMalicious() test above.
Expand Down
4 changes: 3 additions & 1 deletion chrome/browser/download/download_target_determiner.cc
Original file line number Diff line number Diff line change
Expand Up @@ -713,8 +713,10 @@ DownloadTargetDeterminer::Result
// Checking if there are prior visits to the referrer is only necessary if the
// danger level of the download depends on the file type.
if (danger_type_ != download::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS &&
danger_type_ != download::DOWNLOAD_DANGER_TYPE_MAYBE_DANGEROUS_CONTENT)
danger_type_ != download::DOWNLOAD_DANGER_TYPE_MAYBE_DANGEROUS_CONTENT &&
danger_type_ != download::DOWNLOAD_DANGER_TYPE_WHITELISTED_BY_POLICY) {
return CONTINUE;
}

// First determine the danger level assuming that the user doesn't have any
// prior visits to the referrer recoreded in history. The resulting danger
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -691,6 +691,7 @@ base::string16 DownloadItemNotification::GetWarningStatusString() const {
case download::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS:
case download::DOWNLOAD_DANGER_TYPE_MAYBE_DANGEROUS_CONTENT:
case download::DOWNLOAD_DANGER_TYPE_USER_VALIDATED:
case download::DOWNLOAD_DANGER_TYPE_WHITELISTED_BY_POLICY:
case download::DOWNLOAD_DANGER_TYPE_MAX: {
break;
}
Expand Down
16 changes: 6 additions & 10 deletions chrome/browser/extensions/api/downloads/downloads_api.cc
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,7 @@ const char kDangerKey[] = "danger";
const char kDangerSafe[] = "safe";
const char kDangerUncommon[] = "uncommon";
const char kDangerUnwanted[] = "unwanted";
const char kDangerWhitelistedByPolicy[] = "whitelistedByPolicy";
const char kDangerUrl[] = "url";
const char kEndTimeKey[] = "endTime";
const char kEndedAfterKey[] = "endedAfter";
Expand Down Expand Up @@ -183,16 +184,11 @@ const char kFinalUrlRegexKey[] = "finalUrlRegex";
// Note: Any change to the danger type strings, should be accompanied by a
// corresponding change to downloads.json.
const char* const kDangerStrings[] = {
kDangerSafe,
kDangerFile,
kDangerUrl,
kDangerContent,
kDangerSafe,
kDangerUncommon,
kDangerAccepted,
kDangerHost,
kDangerUnwanted
};
kDangerSafe, kDangerFile,
kDangerUrl, kDangerContent,
kDangerSafe, kDangerUncommon,
kDangerAccepted, kDangerHost,
kDangerUnwanted, kDangerWhitelistedByPolicy};
static_assert(arraysize(kDangerStrings) == download::DOWNLOAD_DANGER_TYPE_MAX,
"kDangerStrings should have DOWNLOAD_DANGER_TYPE_MAX elements");

Expand Down
1 change: 1 addition & 0 deletions chrome/browser/file_select_helper.cc
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@ bool IsDownloadAllowedBySafeBrowsing(
// failed safe browsing ping.
case Result::UNKNOWN:
case Result::SAFE:
case Result::WHITELISTED_BY_POLICY:
return true;

case Result::DANGEROUS:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -88,9 +88,11 @@ void DownloadFeedbackService::MaybeStorePingsForDownload(
download::DownloadItem* download,
const std::string& ping,
const std::string& response) {
// We never upload SAFE files.
if (result == DownloadCheckResult::SAFE)
// We never upload SAFE or WHITELISTED_BY_POLICY files.
if (result == DownloadCheckResult::SAFE ||
result == DownloadCheckResult::WHITELISTED_BY_POLICY) {
return;
}

UMA_HISTOGRAM_BOOLEAN("SBDownloadFeedback.UploadRequestedByServer",
upload_requested);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,8 @@ TEST_F(DownloadFeedbackServiceTest, MaybeStorePingsForDownload) {
// SAFE will never upload
EXPECT_FALSE(
WillStorePings(DownloadCheckResult::SAFE, upload_requested, ok_size));
EXPECT_FALSE(WillStorePings(DownloadCheckResult::WHITELISTED_BY_POLICY,
upload_requested, ok_size));
// Others will upload if requested.
EXPECT_EQ(upload_requested, WillStorePings(DownloadCheckResult::UNKNOWN,
upload_requested, ok_size));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,16 +10,20 @@
#include "base/task_scheduler/post_task.h"
#include "base/task_scheduler/task_traits.h"
#include "chrome/browser/browser_process.h"
#include "chrome/browser/extensions/api/safe_browsing_private/safe_browsing_private_event_router.h"
#include "chrome/browser/extensions/api/safe_browsing_private/safe_browsing_private_event_router_factory.h"
#include "chrome/browser/profiles/profile.h"
#include "chrome/browser/safe_browsing/download_protection/check_client_download_request.h"
#include "chrome/browser/safe_browsing/download_protection/download_feedback_service.h"
#include "chrome/browser/safe_browsing/download_protection/download_url_sb_client.h"
#include "chrome/browser/safe_browsing/download_protection/ppapi_download_request.h"
#include "chrome/browser/sessions/session_tab_helper.h"
#include "chrome/browser/signin/signin_manager_factory.h"
#include "chrome/common/safe_browsing/binary_feature_extractor.h"
#include "chrome/common/url_constants.h"
#include "components/google/core/browser/google_util.h"
#include "components/safe_browsing/common/safebrowsing_switches.h"
#include "components/signin/core/browser/signin_manager.h"
#include "content/public/browser/browser_context.h"
#include "content/public/browser/browser_thread.h"
#include "content/public/browser/download_item_utils.h"
Expand Down Expand Up @@ -56,6 +60,19 @@ void AddEventUrlToReferrerChain(const download::DownloadItem& item,
event_url_entry->add_server_redirect_chain()->set_url(url.spec());
}

bool MatchesEnterpriseWhitelist(const Profile* profile,
const std::vector<GURL>& url_chain) {
if (!profile)
return false;

const PrefService* prefs = profile->GetPrefs();
for (const GURL& url : url_chain) {
if (IsURLWhitelistedByPolicy(url, *prefs))
return true;
}
return false;
}

} // namespace

const void* const DownloadProtectionService::kDownloadPingTokenKey =
Expand Down Expand Up @@ -128,6 +145,11 @@ bool DownloadProtectionService::IsHashManuallyBlacklisted(
void DownloadProtectionService::CheckClientDownload(
download::DownloadItem* item,
const CheckDownloadCallback& callback) {
if (item->GetDangerType() ==
download::DOWNLOAD_DANGER_TYPE_WHITELISTED_BY_POLICY) {
callback.Run(DownloadCheckResult::WHITELISTED_BY_POLICY);
return;
}
scoped_refptr<CheckClientDownloadRequest> request(
new CheckClientDownloadRequest(item, callback, this, database_manager_,
binary_feature_extractor_.get()));
Expand All @@ -139,6 +161,18 @@ void DownloadProtectionService::CheckDownloadUrl(
download::DownloadItem* item,
const CheckDownloadCallback& callback) {
DCHECK(!item->GetUrlChain().empty());
const content::WebContents* web_contents =
content::DownloadItemUtils::GetWebContents(item);
// |web_contents| can be null in tests.
// Checks if this download is whitelisted by enterprise policy.
if (web_contents &&
MatchesEnterpriseWhitelist(
Profile::FromBrowserContext(web_contents->GetBrowserContext()),
item->GetUrlChain())) {
callback.Run(DownloadCheckResult::WHITELISTED_BY_POLICY);
return;
}

scoped_refptr<DownloadUrlSBClient> client(new DownloadUrlSBClient(
item, this, callback, ui_manager_, database_manager_));
// The client will release itself once it is done.
Expand Down Expand Up @@ -170,6 +204,11 @@ void DownloadProtectionService::CheckPPAPIDownloadRequest(
const CheckDownloadCallback& callback) {
DVLOG(1) << __func__ << " url:" << requestor_url
<< " default_file_path:" << default_file_path.value();
if (MatchesEnterpriseWhitelist(profile,
{requestor_url, initiating_frame_url})) {
callback.Run(DownloadCheckResult::WHITELISTED_BY_POLICY);
return;
}
std::unique_ptr<PPAPIDownloadRequest> request(new PPAPIDownloadRequest(
requestor_url, initiating_frame_url, web_contents, default_file_path,
alternate_extensions, profile, callback, this, database_manager_));
Expand Down Expand Up @@ -265,11 +304,16 @@ void DownloadProtectionService::MaybeSendDangerousDownloadOpenedReport(
std::string token = GetDownloadPingToken(item);
content::BrowserContext* browser_context =
content::DownloadItemUtils::GetBrowserContext(item);
// When users are in incognito mode, no report will be sent and no
// |onDangerousDownloadOpened| extension API will be called.
if (browser_context->IsOffTheRecord())
return;

Profile* profile = Profile::FromBrowserContext(browser_context);
OnDangerousDownloadOpened(item, profile);
if (sb_service_ &&
!token.empty() && // Only dangerous downloads have token stored.
!browser_context->IsOffTheRecord() && profile &&
IsExtendedReportingEnabled(*profile->GetPrefs())) {
profile && IsExtendedReportingEnabled(*profile->GetPrefs())) {
safe_browsing::ClientSafeBrowsingReportRequest report;
report.set_url(item->GetURL().spec());
report.set_type(safe_browsing::ClientSafeBrowsingReportRequest::
Expand Down Expand Up @@ -445,4 +489,19 @@ void DownloadProtectionService::AddReferrerChainToPPAPIClientDownloadRequest(
SafeBrowsingNavigationObserverManager::ATTRIBUTION_FAILURE_TYPE_MAX);
}

void DownloadProtectionService::OnDangerousDownloadOpened(
const download::DownloadItem* item,
Profile* profile) {
const SigninManagerBase* signin_manager =
SigninManagerFactory::GetForProfileIfExists(profile);
std::string username =
signin_manager ? signin_manager->GetAuthenticatedAccountInfo().email
: std::string();

extensions::SafeBrowsingPrivateEventRouterFactory::GetForProfile(profile)
->OnDangerousDownloadOpened(item->GetURL(),
item->GetTargetFilePath().AsUTF8Unsafe(),
item->GetHash(), username);
}

} // namespace safe_browsing
Original file line number Diff line number Diff line change
Expand Up @@ -247,6 +247,9 @@ class DownloadProtectionService {
bool has_user_gesture,
ClientDownloadRequest* out_request);

void OnDangerousDownloadOpened(const download::DownloadItem* item,
Profile* profile);

SafeBrowsingService* sb_service_;
// These pointers may be NULL if SafeBrowsing is disabled.
scoped_refptr<SafeBrowsingUIManager> ui_manager_;
Expand Down
Loading

0 comments on commit 93850e0

Please sign in to comment.