Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Redone SafeBrowsing by using SafeBrowsingApiHandler and SafetyNetClient #25842

Merged
merged 1 commit into from
Oct 15, 2024

Conversation

AlexeyBarabash
Copy link
Contributor

@AlexeyBarabash AlexeyBarabash commented Oct 7, 2024

Resolves brave/brave-browser#41407

This PR brings back Safe Browsing by SafetyNetClient API internally with supplying modified codes into SafeBrowsingApiHandler.

Submitter Checklist:

  • I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
  • Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • Checked the PR locally:
    • npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
    • npm run presubmit wiki, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed)

Reviewer Checklist:

  • A security review is not needed, or a link to one is included in the PR description
  • New files have MPL-2.0 license header
  • Adequate test coverage exists to prevent regressions
  • Major classes, functions and non-trivial code blocks are well-commented
  • Changes in component dependencies are properly reflected in gn
  • Code follows the style guide
  • Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

  1. open https://testsafebrowsing.appspot.com/
  2. Click (1), (2), (4) and (6)
    Expected to see interstitial Safe Browsing page with a warnings, similar to the ones which displays current Stable 1.70.123 Chromium: 129.0.6668.89

@AlexeyBarabash AlexeyBarabash added the CI/skip Do not run CI builds (except noplatform) label Oct 7, 2024
@AlexeyBarabash AlexeyBarabash self-assigned this Oct 7, 2024
@AlexeyBarabash AlexeyBarabash force-pushed the safe_browsing_over_safety_net branch 2 times, most recently from ff8c1af to 9b6df6f Compare October 8, 2024 12:15
@AlexeyBarabash AlexeyBarabash removed the CI/skip Do not run CI builds (except noplatform) label Oct 8, 2024
@AlexeyBarabash AlexeyBarabash marked this pull request as ready for review October 8, 2024 20:28
@AlexeyBarabash AlexeyBarabash force-pushed the safe_browsing_over_safety_net branch from 9b6df6f to a7d2b27 Compare October 8, 2024 20:29
Copy link
Contributor

github-actions bot commented Oct 8, 2024

The security team is monitoring all repositories for certain keywords. This PR includes the word(s) "safebrowsing" and so security team members have been added as reviewers to take a look.

No need to request a full security review at this stage, the security team will take a look shortly and either clear the label or request more information/changes.

Notifications have already been sent, but if this is blocking your merge feel free to reach out directly to the security team on Slack so that we can expedite this check.

@kdenhartog
Copy link
Member

I don't spot anything that's blocking in my review, but will liket @stoletheminerals give a look and do sign off

@fmarier
Copy link
Member

fmarier commented Oct 9, 2024

I am also planning to take a look tomorrow.

Copy link
Contributor

github-actions bot commented Oct 9, 2024

The security team is monitoring all repositories for certain keywords. This PR includes the word(s) "safebrowsing, safe browsing" and so security team members have been added as reviewers to take a look.

No need to request a full security review at this stage, the security team will take a look shortly and either clear the label or request more information/changes.

Notifications have already been sent, but if this is blocking your merge feel free to reach out directly to the security team on Slack so that we can expedite this check.

Copy link
Member

@SergeyZhukovsky SergeyZhukovsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

++

Copy link
Contributor

@deeppandya deeppandya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@samartnik samartnik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

// Delegate is used to turn off safe browsing option as every
// request is
// delayed when it's turned on and not working
mBraveSafeBrowsingApiHandlerDelegate.turnSafeBrowsingOff();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this update the toggle in the UI settings to OFF? We need to make sure we don't claim that Safe Browsing is ON when we know it's not. This is important.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a follow-up, we should detect this case ahead of time and gray out the Safe Browsing setting entirely to make it clear that it cannot be enabled.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am re-checking this.
At current cr130 Nightly where SafeBrowsing doesn't work, the setting is not grayed and is toggled on.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so the only thing we need to do is that if we detect that Safe Browsing is not working (e.g. it fails to initialize), we need to have the UI toggle OFF in order to reflect that Safe Browsing is not actually ON. Otherwise we'll be claiming that we protect users when we're not.

Graying out when it's not available can be left as a follow-up issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just re-checked the case on emulator without Google Play service.

The line

mBraveSafeBrowsingApiHandlerDelegate.turnSafeBrowsingOff();

causes Safe Browsing to be turned off in preferences.

Screenshot from 2024-10-14 16-09-10

Attempt to turn it on gives the message that it needs Google Play Services

Screenshot from 2024-10-14 16-09-32

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the other side, after fresh Brave installation, when any site was't visited, then the preferences disaplay Safe Browsing/Standard protection is on

image

In anyway attempt to change this setting gives the error

image

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's create a follow up for that as it is how it used to be, plus majority of user base is using Google Play. We can handle it later as currently nightly is without protection at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fmarier
With the commit 8563732 I modified the way we display Safe Browsing preference, taking to attention Google Play Service API, so the case when Protection is on but in fact the protection does not work because there is no Google Play Services is fixed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have removed the mentioned commit after discussion with @SergeyZhukovsky .
ChromiumPlayServicesAvailability.isGooglePlayServicesAvailable is an expensive call, marked as If at all possible, do not use this

The chance of that user without Google Play Services API will open preference without prior opening of any web site is low.

Current implementation turns the Safe Browsing into off after first navigation to any web site.

So I am going to merge is once CI will be green.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow-up issue assigned to me:

brave/brave-browser#41621

Fixes brave/brave-browser#41407

Related Chromium change
https://source.chromium.org/chromium/chromium/src/+/e5700c49b75254cc1201a3bbea59b20b06328a27

Remove remaining functions in the interface.

	Internal reference was removed in https://crrev.com/i/7648757

	OBSOLETE_HISTOGRAMS=No longer logged because URLs are checked through
	the new GMSCore Safe Browsing API.

	Bug: 40935425
	Change-Id: I3ab1e5783395c63586bd4c5163541161027004bb
	Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5837639
@AlexeyBarabash AlexeyBarabash force-pushed the safe_browsing_over_safety_net branch from dd4c302 to 8563732 Compare October 14, 2024 19:30
@AlexeyBarabash AlexeyBarabash requested a review from a team as a code owner October 14, 2024 19:30
Copy link
Contributor

[puLL-Merge] - brave/brave-core@25842

Description

This PR reintroduces Safe Browsing functionality for Android that was previously removed or commented out. It implements a Brave-specific version of Safe Browsing that utilizes Google's SafetyNet API while maintaining Brave's privacy-focused approach.

Changes

Changes

  1. android/java/apk_for_test.flags:

    • Added a keep rule for SafeBrowsingSettingsFragment.getSafeBrowsingSummaryString.
  2. android/java/org/chromium/chrome/browser/BraveApplicationImplBase.java:

    • Uncommented and updated the SafeBrowsing API handler initialization.
  3. android/java/org/chromium/chrome/browser/app/BraveActivity.java:

    • Reintroduced SafeBrowsing-related code, including initialization, shutdown, and delegate methods.
  4. android/javatests/org/chromium/chrome/browser/BytecodeTest.java:

    • Added tests for SafeBrowsing-related classes and methods.
  5. browser/safe_browsing/android/java/src/org/chromium/chrome/browser/safe_browsing/settings/:

    • Added BraveSafeBrowsingSettingsFragment.java to handle SafeBrowsing settings specific to Brave.
  6. build/android/bytecode/:

    • Added BraveSafeBrowsingSettingsFragmentClassAdapter.java for bytecode manipulation.
  7. build/android/config.gni:

    • Uncommented the Brave SafeBrowsing Java dependency.
  8. components/safe_browsing/android/:

    • Updated BraveSafeBrowsingApiHandler.java to implement the new SafeBrowsing API.
    • Added BraveSafeBrowsingUtils.java for utility functions related to threat types and conversions.

Possible Issues

  1. The reintroduction of SafeBrowsing functionality might impact performance or user experience if not properly optimized.
  2. There could be potential conflicts with Brave's existing privacy features that may need to be reconciled.

Security Hotspots

  1. BraveSafeBrowsingApiHandler.java: The implementation of SafeBrowsing API calls and threat handling should be carefully reviewed to ensure it doesn't introduce any vulnerabilities or privacy leaks.
  2. BraveSafeBrowsingUtils.java: The conversion between different threat types should be accurate to prevent misclassification of threats.

Overall, this PR appears to be a significant update to Brave's SafeBrowsing implementation on Android, reintroducing functionality while maintaining Brave's privacy-focused approach. Careful testing and review of the SafeBrowsing logic and its integration with the rest of the app will be crucial to ensure both security and performance are maintained.

@AlexeyBarabash AlexeyBarabash force-pushed the safe_browsing_over_safety_net branch from 8563732 to b827f72 Compare October 14, 2024 20:44
@AlexeyBarabash AlexeyBarabash merged commit b04bb97 into master Oct 15, 2024
17 checks passed
@AlexeyBarabash AlexeyBarabash deleted the safe_browsing_over_safety_net branch October 15, 2024 07:00
@github-actions github-actions bot added this to the 1.73.x - Nightly milestone Oct 15, 2024
brave-builds added a commit that referenced this pull request Oct 15, 2024
brave-builds added a commit that referenced this pull request Oct 15, 2024
@brave-builds
Copy link
Collaborator

Released in v1.73.12

@kjozwiak
Copy link
Member

Verification PASSED on Pixel 6 running Android 15 using the following build(s):

Brave | 1.73.13 Chromium: 130.0.6723.44 (Official Build) canary (64-bit)
--- | ---
Revision | 4e9c8031b2adb98c47678c7cb565c8800a728270
OS | Android 15; Build/AP41.240823.009; 35; REL

Using the STR/Cases outlined via #25842 (comment), ensured that the interstitial pages are being displayed as per the following:

  • ensured that clicking on Back to safety works without any issues/failures
  • ensured that clicking on Only visit this unsafe site works without any issues/failures
  • ensured that the interstitial pages re-appears once a user has restarted if they've proceeded to an unsafe website
Example Example Example Example
Screenshot_20241015-201505 Screenshot_20241015-201511 Screenshot_20241015-201521 Screenshot_20241015-201530

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Cr130] Safe Browsing is broken in Cr130
10 participants