From be1cfa9e915c90e7a3e311d149f44e0e67609c5e Mon Sep 17 00:00:00 2001 From: AlexeyBarabash Date: Wed, 16 Oct 2024 23:17:24 +0300 Subject: [PATCH 1/5] [Android] Select the highest priority threat from SafetyNetClient.lookupUri response fixes brave/brave-browser#41581 --- .../BraveSafetyNetThreatsPrioritiesTest.java | 64 +++++++++++++++++++ .../BraveSafeBrowsingApiHandler.java | 29 +++------ .../safe_browsing/BraveSafeBrowsingUtils.java | 32 ++++++++++ test/BUILD.gn | 2 + 4 files changed, 108 insertions(+), 19 deletions(-) create mode 100644 android/javatests/org/chromium/chrome/browser/BraveSafetyNetThreatsPrioritiesTest.java diff --git a/android/javatests/org/chromium/chrome/browser/BraveSafetyNetThreatsPrioritiesTest.java b/android/javatests/org/chromium/chrome/browser/BraveSafetyNetThreatsPrioritiesTest.java new file mode 100644 index 000000000000..0858f931f46e --- /dev/null +++ b/android/javatests/org/chromium/chrome/browser/BraveSafetyNetThreatsPrioritiesTest.java @@ -0,0 +1,64 @@ +/* Copyright (c) 2023 The Brave Authors. All rights reserved. + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this file, + * You can obtain one at https://mozilla.org/MPL/2.0/. */ + +package org.chromium.chrome.browser; + +import androidx.test.filters.SmallTest; + +import org.junit.Assert; +import org.junit.Test; +import org.junit.runner.RunWith; + +import org.chromium.base.test.BaseJUnit4ClassRunner; +import org.chromium.base.test.util.Batch; +import org.chromium.components.safe_browsing.BraveSafeBrowsingUtils; +import org.chromium.components.safe_browsing.BraveSafeBrowsingUtils.SafetyNetJavaThreatType; + +import java.util.ArrayList; +import java.util.Arrays; + +@Batch(Batch.PER_CLASS) +@RunWith(BaseJUnit4ClassRunner.class) +public class BraveSafetyNetThreatsPrioritiesTest { + + @Test + @SmallTest + public void testPriorities() throws Exception { + // Fail safe option for empty input, consider it is safe + Assert.assertEquals( + BraveSafeBrowsingUtils.getMostPriorityThreat(new ArrayList<>()), + SafetyNetJavaThreatType.MAX_VALUE); + + // Single input must return the same value + Assert.assertEquals( + BraveSafeBrowsingUtils.getMostPriorityThreat( + Arrays.asList(new Integer[] {SafetyNetJavaThreatType.SOCIAL_ENGINEERING})), + SafetyNetJavaThreatType.SOCIAL_ENGINEERING); + + // Several threats as input - return the most priority one + Assert.assertEquals( + BraveSafeBrowsingUtils.getMostPriorityThreat( + Arrays.asList( + new Integer[] { + SafetyNetJavaThreatType.UNWANTED_SOFTWARE, + SafetyNetJavaThreatType.SUBRESOURCE_FILTER, + SafetyNetJavaThreatType.SOCIAL_ENGINEERING, + SafetyNetJavaThreatType.BILLING, + SafetyNetJavaThreatType.POTENTIALLY_HARMFUL_APPLICATION, + })), + SafetyNetJavaThreatType.SOCIAL_ENGINEERING); + + Assert.assertEquals( + BraveSafeBrowsingUtils.getMostPriorityThreat( + Arrays.asList( + new Integer[] { + SafetyNetJavaThreatType.SUBRESOURCE_FILTER, + SafetyNetJavaThreatType.CSD_ALLOWLIST, + SafetyNetJavaThreatType.POTENTIALLY_HARMFUL_APPLICATION, + SafetyNetJavaThreatType.UNWANTED_SOFTWARE, + })), + SafetyNetJavaThreatType.POTENTIALLY_HARMFUL_APPLICATION); + } +} diff --git a/components/safe_browsing/android/java/src/org/chromium/components/safe_browsing/BraveSafeBrowsingApiHandler.java b/components/safe_browsing/android/java/src/org/chromium/components/safe_browsing/BraveSafeBrowsingApiHandler.java index d341dcd24213..c60a6afb88dd 100644 --- a/components/safe_browsing/android/java/src/org/chromium/components/safe_browsing/BraveSafeBrowsingApiHandler.java +++ b/components/safe_browsing/android/java/src/org/chromium/components/safe_browsing/BraveSafeBrowsingApiHandler.java @@ -11,8 +11,8 @@ import com.google.android.gms.common.api.ApiException; import com.google.android.gms.common.api.CommonStatusCodes; +import com.google.android.gms.safetynet.SafeBrowsingThreat; import com.google.android.gms.safetynet.SafetyNet; -import com.google.android.gms.safetynet.SafetyNetApi.SafeBrowsingResponse; import com.google.android.gms.safetynet.SafetyNetStatusCodes; import org.chromium.base.ContextUtils; @@ -21,6 +21,9 @@ import org.chromium.components.safe_browsing.BraveSafeBrowsingUtils.SafeBrowsingJavaThreatType; import org.chromium.components.safe_browsing.SafeBrowsingApiHandler.LookupResult; +import java.util.List; +import java.util.stream.Collectors; + /** * Brave implementation of SafeBrowsingApiHandler for Safe Browsing Under the bonnet it still uses * SafetyNet. @@ -120,18 +123,18 @@ public void startUriLookup(long callbackId, String uri, int[] threatTypes, int p DEFAULT_CHECK_DELTA); return; } else { - warnWhenMoreThanOneThreat(sbResponse); + List threats = + sbResponse.getDetectedThreats().stream() + .map(SafeBrowsingThreat::getThreatType) + .collect(Collectors.toList()); - // Response only with the first code mObserver.onUrlCheckDone( callbackId, LookupResult.SUCCESS, BraveSafeBrowsingUtils .safetyNetToSafeBrowsingJavaThreatType( - sbResponse - .getDetectedThreats() - .get(0) - .getThreatType()), + BraveSafeBrowsingUtils + .getMostPriorityThreat(threats)), THREAT_ATTRIBUTES_STUB, SafeBrowsingJavaResponseStatus.SUCCESS_WITH_REAL_TIME, DEFAULT_CHECK_DELTA); @@ -201,18 +204,6 @@ public void startUriLookup(long callbackId, String uri, int[] threatTypes, int p }); } - private void warnWhenMoreThanOneThreat(SafeBrowsingResponse sbResponse) { - if (sbResponse.getDetectedThreats().size() != 1) { - Log.d(TAG, "Unexpected threats count: " + sbResponse.getDetectedThreats().size()); - String threats = ""; - for (int i = 0; i < sbResponse.getDetectedThreats().size(); i++) { - threats += sbResponse.getDetectedThreats().get(i).getThreatType(); - threats += " "; - } - Log.d(TAG, "Threats: [" + threats + "]"); - } - } - public void initSafeBrowsing() { SafetyNet.getClient(ContextUtils.getApplicationContext()).initSafeBrowsing(); mInitialized = true; diff --git a/components/safe_browsing/android/java/src/org/chromium/components/safe_browsing/BraveSafeBrowsingUtils.java b/components/safe_browsing/android/java/src/org/chromium/components/safe_browsing/BraveSafeBrowsingUtils.java index e33357d54182..8b50ecb5049c 100644 --- a/components/safe_browsing/android/java/src/org/chromium/components/safe_browsing/BraveSafeBrowsingUtils.java +++ b/components/safe_browsing/android/java/src/org/chromium/components/safe_browsing/BraveSafeBrowsingUtils.java @@ -6,6 +6,7 @@ package org.chromium.components.safe_browsing; import androidx.annotation.IntDef; +import androidx.annotation.VisibleForTesting; import org.chromium.base.Log; @@ -139,4 +140,35 @@ public static int safetyNetToSafeBrowsingJavaThreatType(int safetyNetThreatType) return SafeBrowsingJavaThreatType.NO_THREAT; } } + + // Priorities from most high to low + private static final int SAFETY_NET_THREAT_PRIORITIES[] = { + SafetyNetJavaThreatType.SOCIAL_ENGINEERING, + SafetyNetJavaThreatType.BILLING, + SafetyNetJavaThreatType.POTENTIALLY_HARMFUL_APPLICATION, + SafetyNetJavaThreatType.UNWANTED_SOFTWARE, + SafetyNetJavaThreatType.SUBRESOURCE_FILTER, + SafetyNetJavaThreatType.CSD_ALLOWLIST, + SafetyNetJavaThreatType.MAX_VALUE + }; + + @VisibleForTesting + public static int getMostPriorityThreat(List threats) { + if (threats.size() == 0) { + return SafetyNetJavaThreatType.MAX_VALUE; + } + + if (threats.size() == 1) { + return threats.get(0); + } + + for (int i = 0; i < SAFETY_NET_THREAT_PRIORITIES.length; ++i) { + if (threats.contains(SAFETY_NET_THREAT_PRIORITIES[i])) { + return SAFETY_NET_THREAT_PRIORITIES[i]; + } + } + + assert false : "Unexpected threat"; + return SafetyNetJavaThreatType.MAX_VALUE; + } } diff --git a/test/BUILD.gn b/test/BUILD.gn index 91f98cd195e6..8003c3575389 100644 --- a/test/BUILD.gn +++ b/test/BUILD.gn @@ -1264,6 +1264,7 @@ if (is_android) { } sources = [ + "//brave/android/javatests/org/chromium/chrome/browser/BraveSafetyNetThreatsPrioritiesTest.java", "//brave/android/javatests/org/chromium/chrome/browser/BraveSwipeRefreshHandlerTest.java", "//brave/android/javatests/org/chromium/chrome/browser/BytecodeTest.java", "//brave/android/javatests/org/chromium/chrome/browser/brave_wallet/BraveWalletUtilsTest.java", @@ -1279,6 +1280,7 @@ if (is_android) { "//base:base_java_test_support", "//base:base_shared_preferences_java", "//brave/components/brave_wallet/common:mojom_java", + "//brave/components/safe_browsing/android:brave_safe_browsing_java", "//cc:cc_java", "//chrome/android/features/keyboard_accessory/public:public_java", "//chrome/browser/android/browserservices/intents:java", From ee143bb4a36c08d9b578d40ccd911015b56fc814 Mon Sep 17 00:00:00 2001 From: AlexeyBarabash Date: Fri, 25 Oct 2024 00:34:07 +0300 Subject: [PATCH 2/5] Update components/safe_browsing/android/java/src/org/chromium/components/safe_browsing/BraveSafeBrowsingApiHandler.java Co-authored-by: Francois Marier --- .../components/safe_browsing/BraveSafeBrowsingApiHandler.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/components/safe_browsing/android/java/src/org/chromium/components/safe_browsing/BraveSafeBrowsingApiHandler.java b/components/safe_browsing/android/java/src/org/chromium/components/safe_browsing/BraveSafeBrowsingApiHandler.java index c60a6afb88dd..25827ee0e5c4 100644 --- a/components/safe_browsing/android/java/src/org/chromium/components/safe_browsing/BraveSafeBrowsingApiHandler.java +++ b/components/safe_browsing/android/java/src/org/chromium/components/safe_browsing/BraveSafeBrowsingApiHandler.java @@ -134,7 +134,7 @@ public void startUriLookup(long callbackId, String uri, int[] threatTypes, int p BraveSafeBrowsingUtils .safetyNetToSafeBrowsingJavaThreatType( BraveSafeBrowsingUtils - .getMostPriorityThreat(threats)), + .getHighestPriorityThreat(threats)), THREAT_ATTRIBUTES_STUB, SafeBrowsingJavaResponseStatus.SUCCESS_WITH_REAL_TIME, DEFAULT_CHECK_DELTA); From 0c47f7232bb66e2a01ceaa3254dac4159df103dd Mon Sep 17 00:00:00 2001 From: AlexeyBarabash Date: Fri, 25 Oct 2024 00:34:14 +0300 Subject: [PATCH 3/5] Update components/safe_browsing/android/java/src/org/chromium/components/safe_browsing/BraveSafeBrowsingUtils.java Co-authored-by: Francois Marier --- .../components/safe_browsing/BraveSafeBrowsingUtils.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/components/safe_browsing/android/java/src/org/chromium/components/safe_browsing/BraveSafeBrowsingUtils.java b/components/safe_browsing/android/java/src/org/chromium/components/safe_browsing/BraveSafeBrowsingUtils.java index 8b50ecb5049c..c09dba78d247 100644 --- a/components/safe_browsing/android/java/src/org/chromium/components/safe_browsing/BraveSafeBrowsingUtils.java +++ b/components/safe_browsing/android/java/src/org/chromium/components/safe_browsing/BraveSafeBrowsingUtils.java @@ -153,7 +153,7 @@ public static int safetyNetToSafeBrowsingJavaThreatType(int safetyNetThreatType) }; @VisibleForTesting - public static int getMostPriorityThreat(List threats) { + public static int getHighestPriorityThreat(List threats) { if (threats.size() == 0) { return SafetyNetJavaThreatType.MAX_VALUE; } From 9a4777422724afdb48373b87f19aac898ede5a32 Mon Sep 17 00:00:00 2001 From: AlexeyBarabash Date: Fri, 25 Oct 2024 01:03:55 +0300 Subject: [PATCH 4/5] Changed priorities, fixed test - codereview notice --- .../browser/BraveSafetyNetThreatsPrioritiesTest.java | 8 ++++---- .../components/safe_browsing/BraveSafeBrowsingUtils.java | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/android/javatests/org/chromium/chrome/browser/BraveSafetyNetThreatsPrioritiesTest.java b/android/javatests/org/chromium/chrome/browser/BraveSafetyNetThreatsPrioritiesTest.java index 0858f931f46e..d2e2490f3a9c 100644 --- a/android/javatests/org/chromium/chrome/browser/BraveSafetyNetThreatsPrioritiesTest.java +++ b/android/javatests/org/chromium/chrome/browser/BraveSafetyNetThreatsPrioritiesTest.java @@ -28,18 +28,18 @@ public class BraveSafetyNetThreatsPrioritiesTest { public void testPriorities() throws Exception { // Fail safe option for empty input, consider it is safe Assert.assertEquals( - BraveSafeBrowsingUtils.getMostPriorityThreat(new ArrayList<>()), + BraveSafeBrowsingUtils.getHighestPriorityThreat(new ArrayList<>()), SafetyNetJavaThreatType.MAX_VALUE); // Single input must return the same value Assert.assertEquals( - BraveSafeBrowsingUtils.getMostPriorityThreat( + BraveSafeBrowsingUtils.getHighestPriorityThreat( Arrays.asList(new Integer[] {SafetyNetJavaThreatType.SOCIAL_ENGINEERING})), SafetyNetJavaThreatType.SOCIAL_ENGINEERING); // Several threats as input - return the most priority one Assert.assertEquals( - BraveSafeBrowsingUtils.getMostPriorityThreat( + BraveSafeBrowsingUtils.getHighestPriorityThreat( Arrays.asList( new Integer[] { SafetyNetJavaThreatType.UNWANTED_SOFTWARE, @@ -51,7 +51,7 @@ public void testPriorities() throws Exception { SafetyNetJavaThreatType.SOCIAL_ENGINEERING); Assert.assertEquals( - BraveSafeBrowsingUtils.getMostPriorityThreat( + BraveSafeBrowsingUtils.getHighestPriorityThreat( Arrays.asList( new Integer[] { SafetyNetJavaThreatType.SUBRESOURCE_FILTER, diff --git a/components/safe_browsing/android/java/src/org/chromium/components/safe_browsing/BraveSafeBrowsingUtils.java b/components/safe_browsing/android/java/src/org/chromium/components/safe_browsing/BraveSafeBrowsingUtils.java index c09dba78d247..3327365f302a 100644 --- a/components/safe_browsing/android/java/src/org/chromium/components/safe_browsing/BraveSafeBrowsingUtils.java +++ b/components/safe_browsing/android/java/src/org/chromium/components/safe_browsing/BraveSafeBrowsingUtils.java @@ -144,8 +144,8 @@ public static int safetyNetToSafeBrowsingJavaThreatType(int safetyNetThreatType) // Priorities from most high to low private static final int SAFETY_NET_THREAT_PRIORITIES[] = { SafetyNetJavaThreatType.SOCIAL_ENGINEERING, - SafetyNetJavaThreatType.BILLING, SafetyNetJavaThreatType.POTENTIALLY_HARMFUL_APPLICATION, + SafetyNetJavaThreatType.BILLING, SafetyNetJavaThreatType.UNWANTED_SOFTWARE, SafetyNetJavaThreatType.SUBRESOURCE_FILTER, SafetyNetJavaThreatType.CSD_ALLOWLIST, From 2dd4ea9ee2bc0f3619b34e0869deccb314bd67b4 Mon Sep 17 00:00:00 2001 From: AlexeyBarabash Date: Fri, 25 Oct 2024 01:22:00 +0300 Subject: [PATCH 5/5] Fixed year 2024 --- .../chrome/browser/BraveSafetyNetThreatsPrioritiesTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/android/javatests/org/chromium/chrome/browser/BraveSafetyNetThreatsPrioritiesTest.java b/android/javatests/org/chromium/chrome/browser/BraveSafetyNetThreatsPrioritiesTest.java index d2e2490f3a9c..590b8848908c 100644 --- a/android/javatests/org/chromium/chrome/browser/BraveSafetyNetThreatsPrioritiesTest.java +++ b/android/javatests/org/chromium/chrome/browser/BraveSafetyNetThreatsPrioritiesTest.java @@ -1,4 +1,4 @@ -/* Copyright (c) 2023 The Brave Authors. All rights reserved. +/* Copyright (c) 2024 The Brave Authors. All rights reserved. * This Source Code Form is subject to the terms of the Mozilla Public * License, v. 2.0. If a copy of the MPL was not distributed with this file, * You can obtain one at https://mozilla.org/MPL/2.0/. */