Commit 3ac9d1b5 authored by Dominique Fauteux-Chapleau's avatar Dominique Fauteux-Chapleau Committed by Commit Bot

Make non-SUCCESS deep scan uploads fail-open

The issue was caused by DlpTriggeredRulesOK returning false for any
non-SUCCESS status. This likely slipped our attention for a while
because the function's name doesn't suggest it reads the entire verdict.
This CL renames it to DlpVerdictAllowsDataUse so its purpose is clearer
and fixes tests so fail-open is accounted for.

Bug: 1085241
Change-Id: I7a81a3635da4fffbf2b888253083aa9a438fb81f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2222551Reviewed-by: default avatarRoger Tawa <rogerta@chromium.org>
Reviewed-by: default avatarScott Violet <sky@chromium.org>
Commit-Queue: Dominique Fauteux-Chapleau <domfc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#773806}
parent 645b46d6
...@@ -101,15 +101,14 @@ void StringSourceRequest::GetRequestData(DataCallback callback) { ...@@ -101,15 +101,14 @@ void StringSourceRequest::GetRequestData(DataCallback callback) {
std::move(callback).Run(result_, data_); std::move(callback).Run(result_, data_);
} }
bool DlpTriggeredRulesOK( bool DlpVerdictAllowsDataUse(
const ::safe_browsing::DlpDeepScanningVerdict& verdict) { const ::safe_browsing::DlpDeepScanningVerdict& verdict) {
// No status returns true since this function is called even when the server // No status or non-SUCCESS statuses return true since the intended behaviour
// doesn't return a DLP scan verdict. // is to be fail-open.
if (!verdict.has_status()) if (!verdict.has_status() ||
verdict.status() != DlpDeepScanningVerdict::SUCCESS) {
return true; return true;
}
if (verdict.status() != DlpDeepScanningVerdict::SUCCESS)
return false;
for (int i = 0; i < verdict.triggered_rules_size(); ++i) { for (int i = 0; i < verdict.triggered_rules_size(); ++i) {
if (verdict.triggered_rules(i).action() == if (verdict.triggered_rules(i).action() ==
...@@ -427,7 +426,7 @@ void DeepScanningDialogDelegate::StringRequestCallback( ...@@ -427,7 +426,7 @@ void DeepScanningDialogDelegate::StringRequestCallback(
text_request_complete_ = true; text_request_complete_ = true;
bool text_complies = ResultShouldAllowDataUse(result, data_.settings) && bool text_complies = ResultShouldAllowDataUse(result, data_.settings) &&
DlpTriggeredRulesOK(response.dlp_scan_verdict()); DlpVerdictAllowsDataUse(response.dlp_scan_verdict());
std::fill(result_.text_results.begin(), result_.text_results.end(), std::fill(result_.text_results.begin(), result_.text_results.end(),
text_complies); text_complies);
...@@ -458,7 +457,7 @@ void DeepScanningDialogDelegate::CompleteFileRequestCallback( ...@@ -458,7 +457,7 @@ void DeepScanningDialogDelegate::CompleteFileRequestCallback(
extensions::SafeBrowsingPrivateEventRouter::kTriggerFileUpload, extensions::SafeBrowsingPrivateEventRouter::kTriggerFileUpload,
file_info_[index].size, result, response); file_info_[index].size, result, response);
bool dlp_ok = DlpTriggeredRulesOK(response.dlp_scan_verdict()); bool dlp_ok = DlpVerdictAllowsDataUse(response.dlp_scan_verdict());
bool malware_ok = true; bool malware_ok = true;
if (response.has_malware_scan_verdict()) { if (response.has_malware_scan_verdict()) {
malware_ok = response.malware_scan_verdict().verdict() == malware_ok = response.malware_scan_verdict().verdict() ==
......
...@@ -1341,7 +1341,7 @@ TEST_P(DeepScanningDialogDelegateAuditOnlyTest, StringFileDataPartialSuccess) { ...@@ -1341,7 +1341,7 @@ TEST_P(DeepScanningDialogDelegateAuditOnlyTest, StringFileDataPartialSuccess) {
EXPECT_TRUE(result.paths_results[0]); EXPECT_TRUE(result.paths_results[0]);
EXPECT_FALSE(result.paths_results[1]); EXPECT_FALSE(result.paths_results[1]);
EXPECT_FALSE(result.paths_results[2]); EXPECT_FALSE(result.paths_results[2]);
EXPECT_FALSE(result.paths_results[3]); EXPECT_TRUE(result.paths_results[3]);
EXPECT_FALSE(result.paths_results[4]); EXPECT_FALSE(result.paths_results[4]);
*called = true; *called = true;
}, },
......
...@@ -58,10 +58,10 @@ class ChromeWebContentsViewDelegateHandleOnPerformDrop : public testing::Test { ...@@ -58,10 +58,10 @@ class ChromeWebContentsViewDelegateHandleOnPerformDrop : public testing::Test {
auto callback = base::BindLambdaForTesting( auto callback = base::BindLambdaForTesting(
[this, scan_succeeds](const base::FilePath&) { [this, scan_succeeds](const base::FilePath&) {
current_requests_count_++; current_requests_count_++;
return scan_succeeds ? FakeDelegate::SuccessfulResponse() return scan_succeeds
: FakeDelegate::DlpResponse( ? FakeDelegate::SuccessfulResponse()
Verdict::FAILURE, std::string(), : FakeDelegate::DlpResponse(Verdict::SUCCESS, "block_rule",
Verdict::TriggeredRule::REPORT_ONLY); Verdict::TriggeredRule::BLOCK);
}); });
auto is_encrypted_callback = auto is_encrypted_callback =
base::BindRepeating([](const base::FilePath&) { return false; }); base::BindRepeating([](const base::FilePath&) { return false; });
......
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