Skip to content

Commit

Permalink
[SBv5] Ensure response is valid and successful before returning the t…
Browse files Browse the repository at this point in the history 
…hreat type

Add the following checks before returning the threat type in
safe_browsing_api_handler_bridge:
  * Validate that the returned values are defined in the enum. They
    can be undefined if there is a version mismatch. The response
    status is not checked in case there is a new success value added
    before it is integrated in Chrome.
  * Check that the response is successful based on lookup result and
    response status.
  * Add a note on why threat metadata is currently not returned and
    the requirement to support it.

Bug: 1444511
Change-Id: I4a5640ac525f18a0154ab9a7e94a9240f9ff5fe0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4779056
Reviewed-by: thefrog <thefrog@chromium.org>
Commit-Queue: Xinghui Lu <xinghuilu@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1185022}
  • Loading branch information
Xinghui Lu authored and Chromium LUCI CQ committed Aug 18, 2023
1 parent 65c72d5 commit b769c5f
Show file tree
Hide file tree
Showing 4 changed files with 287 additions and 68 deletions.
82 changes: 42 additions & 40 deletions ...g/chromium/components/safe_browsing/SafeBrowsingApiHandlerBridgeNativeUnitTestHelper.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -99,12 +99,9 @@ public static class MockSafeBrowsingApiHandler implements SafeBrowsingApiHandler

// Mock time it takes for a lookup request to complete.
private static final long DEFAULT_CHECK_DELTA_MS = 10;
private static final int DEFAULT_RESPONSE_STATUS = 0;

// Maps to store preset values, keyed by uri.
private static final Map<String, int[]> sExpectedRequestThreatTypesMap = new HashMap<>();
private static final Map<String, Integer> sExpectedRequestProtocolMap = new HashMap<>();
private static final Map<String, Integer> sResponseThreatTypeMap = new HashMap<>();
private static final Map<String, UrlCheckDoneValues> sPresetValuesMap = new HashMap<>();

@Override
public void setObserver(Observer observer) {
Expand All @@ -114,42 +111,53 @@ public void setObserver(Observer observer) {
@Override
public void startUriLookup(
final long callbackId, String uri, int[] threatTypes, int protocol) {
Assert.assertTrue(sExpectedRequestThreatTypesMap.containsKey(uri));
int[] expectedThreatTypes = sExpectedRequestThreatTypesMap.get(uri);
Assert.assertTrue(sPresetValuesMap.containsKey(uri));
UrlCheckDoneValues presetValues = sPresetValuesMap.get(uri);
int[] expectedThreatTypes = presetValues.mExpectedThreatTypes;
Assert.assertNotNull(expectedThreatTypes);
// The order of threatTypes doesn't matter.
Arrays.sort(expectedThreatTypes);
Arrays.sort(threatTypes);
Assert.assertArrayEquals(threatTypes, expectedThreatTypes);
Assert.assertTrue(sExpectedRequestProtocolMap.containsKey(uri));
Assert.assertEquals(Integer.valueOf(protocol), sExpectedRequestProtocolMap.get(uri));

Assert.assertTrue(sResponseThreatTypeMap.containsKey(uri));
int[] emptyThreatAttributes = new int[0];
mObserver.onUrlCheckDone(callbackId, LookupResult.SUCCESS,
sResponseThreatTypeMap.get(uri), emptyThreatAttributes, DEFAULT_RESPONSE_STATUS,
DEFAULT_CHECK_DELTA_MS);
}
Assert.assertEquals(protocol, presetValues.mExpectedProtocol);

public static void tearDown() {
sExpectedRequestThreatTypesMap.clear();
sExpectedRequestProtocolMap.clear();
sResponseThreatTypeMap.clear();
mObserver.onUrlCheckDone(callbackId, presetValues.mReturnedLookupResult,
presetValues.mReturnedThreatType, presetValues.mReturnedThreatAttributes,
presetValues.mReturnedResponseStatus, DEFAULT_CHECK_DELTA_MS);
}

public static void setExpectedThreatTypes(String uri, int[] threatTypes) {
Assert.assertFalse(sExpectedRequestThreatTypesMap.containsKey(uri));
sExpectedRequestThreatTypesMap.put(uri, threatTypes);
public static void tearDown() {
sPresetValuesMap.clear();
}

public static void setExpectedProtocol(String uri, int protocol) {
Assert.assertFalse(sExpectedRequestProtocolMap.containsKey(uri));
sExpectedRequestProtocolMap.put(uri, protocol);
public static void setUrlCheckDoneValues(String uri, int[] expectedThreatTypes,
int expectedProtocol, int returnedLookupResult, int returnedThreatType,
int[] returnedThreatAttributes, int returnedResponseStatus) {
Assert.assertFalse(sPresetValuesMap.containsKey(uri));
sPresetValuesMap.put(uri,
new UrlCheckDoneValues(expectedThreatTypes, expectedProtocol,
returnedLookupResult, returnedThreatType, returnedThreatAttributes,
returnedResponseStatus));
}

public static void setResponseThreatType(String uri, int threatType) {
Assert.assertFalse(sResponseThreatTypeMap.containsKey(uri));
sResponseThreatTypeMap.put(uri, threatType);
private static class UrlCheckDoneValues {
public final int[] mExpectedThreatTypes;
public final int mExpectedProtocol;
public final int mReturnedLookupResult;
public final int mReturnedThreatType;
public final int[] mReturnedThreatAttributes;
public final int mReturnedResponseStatus;

private UrlCheckDoneValues(int[] expectedThreatTypes, int expectedProtocol,
int returnedLookupResult, int returnedThreatType,
int[] returnedThreatAttributes, int returnedResponseStatus) {
mExpectedThreatTypes = expectedThreatTypes;
mExpectedProtocol = expectedProtocol;
mReturnedLookupResult = returnedLookupResult;
mReturnedThreatType = returnedThreatType;
mReturnedThreatAttributes = returnedThreatAttributes;
mReturnedResponseStatus = returnedResponseStatus;
}
}
}

Expand Down Expand Up @@ -188,17 +196,11 @@ static void setSafetyNetApiHandlerResult(int result) {
}

@CalledByNative
static void setExpectedSafeBrowsingApiHandlerThreatTypes(String uri, int[] threatTypes) {
MockSafeBrowsingApiHandler.setExpectedThreatTypes(uri, threatTypes);
}

@CalledByNative
static void setExpectedSafeBrowsingApiHandlerProtocol(String uri, int protocol) {
MockSafeBrowsingApiHandler.setExpectedProtocol(uri, protocol);
}

@CalledByNative
static void setSafeBrowsingApiHandlerThreatType(String uri, int threatType) {
MockSafeBrowsingApiHandler.setResponseThreatType(uri, threatType);
static void setSafeBrowsingApiHandlerResponse(String uri, int[] expectedThreatTypes,
int expectedProtocol, int returnedLookupResult, int returnedThreatType,
int[] returnedThreatAttributes, int returnedResponseStatus) {
MockSafeBrowsingApiHandler.setUrlCheckDoneValues(uri, expectedThreatTypes, expectedProtocol,
returnedLookupResult, returnedThreatType, returnedThreatAttributes,
returnedResponseStatus);
}
}
110 changes: 105 additions & 5 deletions components/safe_browsing/android/safe_browsing_api_handler_bridge.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,94 @@ void ReportUmaResult(UmaRemoteCallResult result) {
UmaRemoteCallResult::MAX_VALUE);
}

// Validate the values returned from SafeBrowsing API are defined in enum. The
// response can be out of range if there is version mismatch between Chrome and
// the GMSCore APK, or the enums between c++ and java are not aligned.
bool IsResponseFromJavaValid(SafeBrowsingApiLookupResult lookup_result,
SafeBrowsingJavaThreatType threat_type,
const std::vector<int>& threat_attributes) {
bool is_lookup_result_recognized = false;
switch (lookup_result) {
case SafeBrowsingApiLookupResult::SUCCESS:
case SafeBrowsingApiLookupResult::FAILURE:
is_lookup_result_recognized = true;
break;
}
if (!is_lookup_result_recognized) {
return false;
}

bool is_threat_type_recognized = false;
switch (threat_type) {
case SafeBrowsingJavaThreatType::NO_THREAT:
case SafeBrowsingJavaThreatType::UNWANTED_SOFTWARE:
case SafeBrowsingJavaThreatType::POTENTIALLY_HARMFUL_APPLICATION:
case SafeBrowsingJavaThreatType::SOCIAL_ENGINEERING:
case SafeBrowsingJavaThreatType::SUBRESOURCE_FILTER:
case SafeBrowsingJavaThreatType::BILLING:
is_threat_type_recognized = true;
break;
}
if (!is_threat_type_recognized) {
return false;
}

for (int threat_attribute : threat_attributes) {
SafeBrowsingJavaThreatAttribute threat_attribute_enum =
static_cast<SafeBrowsingJavaThreatAttribute>(threat_attribute);
bool is_threat_attribute_recognized = false;
switch (threat_attribute_enum) {
case SafeBrowsingJavaThreatAttribute::CANARY:
case SafeBrowsingJavaThreatAttribute::FRAME_ONLY:
is_threat_attribute_recognized = true;
break;
}
if (!is_threat_attribute_recognized) {
return false;
}
}

// Not checking response_status here. This is to avoid the
// API adding a new success response_status while we haven't integrated the
// new value yet. In this case, we still want to return the threat_type.
// TODO(crbug.com/1444511): Add a histogram to track unrecognized status.
return true;
}

bool IsLookupSuccessful(SafeBrowsingApiLookupResult lookup_result,
SafeBrowsingJavaResponseStatus response_status) {
bool is_lookup_result_success = false;
switch (lookup_result) {
case SafeBrowsingApiLookupResult::SUCCESS:
is_lookup_result_success = true;
break;
case SafeBrowsingApiLookupResult::FAILURE:
break;
}
if (!is_lookup_result_success) {
return false;
}

// Note that we check explicit failure statuses instead of success statuses.
// This is to avoid the API adding a new success response_status while we
// haven't integrated the new value yet. The impact of a missing failure
// status is smaller since the API is expected to return a safe threat type in
// a failure anyway.
bool is_response_status_success = true;
switch (response_status) {
case SafeBrowsingJavaResponseStatus::SUCCESS_WITH_LOCAL_BLOCKLIST:
case SafeBrowsingJavaResponseStatus::SUCCESS_WITH_REAL_TIME:
case SafeBrowsingJavaResponseStatus::SUCCESS_FALLBACK_REAL_TIME_TIMEOUT:
case SafeBrowsingJavaResponseStatus::SUCCESS_FALLBACK_REAL_TIME_THROTTLED:
break;
case SafeBrowsingJavaResponseStatus::FAILURE_NETWORK_UNAVAILABLE:
case SafeBrowsingJavaResponseStatus::FAILURE_BLOCK_LIST_UNAVAILABLE:
is_response_status_success = false;
break;
}
return is_response_status_success;
}

// Convert a SBThreatType to a Java SafetyNet API threat type. We only support
// a few.
SafetyNetJavaThreatType SBThreatTypeToSafetyNetJavaThreatType(
Expand Down Expand Up @@ -314,8 +402,23 @@ void OnUrlCheckDoneOnSBThreadBySafeBrowsingApi(
std::move((pending_callbacks)[callback_id]);
pending_callbacks.erase(callback_id);

// TODO(crbug.com/1444511): Consume other fields before returning the
// threat_type.
if (!IsResponseFromJavaValid(lookup_result, threat_type, threat_attributes)) {
std::move(*callback).Run(SB_THREAT_TYPE_SAFE, ThreatMetadata());
return;
}

if (!IsLookupSuccessful(lookup_result, response_status)) {
std::move(*callback).Run(SB_THREAT_TYPE_SAFE, ThreatMetadata());
return;
}

// The API currently doesn't have required threat types
// (ABUSIVE_EXPERIENCE_VIOLATION, BETTER_ADS_VIOLATION) to work with threat
// attributes, so threat attributes are currently disabled. It should not
// affect browse URL checks (mainframe and subresource URLs). However, this
// must be changed before it is used for subresource filter checks.
// Similarly, threat attributes must be consumed if we decide to use malware
// landing info on Android.
std::move(*callback).Run(SafeBrowsingJavaToSBThreatType(threat_type),
ThreatMetadata());
}
Expand Down Expand Up @@ -346,9 +449,6 @@ void JNI_SafeBrowsingApiBridge_OnUrlCheckDoneBySafeBrowsingApi(
? content::GetUIThreadTaskRunner({})
: content::GetIOThreadTaskRunner({});

// TODO(crbug.com/1444511): Add a check that j_threat_type,
// j_threat_attributes and j_response_status are all defined values (in case
// that there is a mismatch between Clank and SafeBrowsing API).
SafeBrowsingApiLookupResult lookup_result =
static_cast<SafeBrowsingApiLookupResult>(j_lookup_result);
SafeBrowsingJavaThreatType threat_type =
Expand Down

0 comments on commit b769c5f

Please sign in to comment.