The built-in bridges have been updated. Added a request for built-in bridges.

[boocmp]

Resolves brave/brave-browser#37896

Once a day (if user has opened the Tor window) we request new built-in bridges config from torproject and save it in the local state.

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:

• The associated issue milestone is set to the smallest version that the
  changes has landed on
• All relevant documentation has been updated, for instance:
  • https://github.com/brave/brave-browser/wiki/Deviations-from-Chromium-(features-we-disable-or-remove)
  • https://github.com/brave/brave-browser/wiki/Proxy-redirected-URLs
  • https://github.com/brave/brave-browser/wiki/Fingerprinting-Protections
  • https://github.com/brave/brave-browser/wiki/Brave%E2%80%99s-Use-of-Referral-Codes
  • https://github.com/brave/brave-browser/wiki/Web-Compatibility-Exceptions-in-Brave
  • https://github.com/brave/brave-browser/wiki/QA-Guide
  • https://github.com/brave/brave-browser/wiki/P3A

Test Plan:

[fmarier]

(if user has opened the Tor window)

I didn't see an explicit check for that in the PR. Am I right in assuming that the profile service will not run at all unless a user has opened a private window with Tor?

I reviewed the Tor parts and the overall logic, but I would appreciate if someone else could do a thorough code review.

[fmarier]

Noting that this is a Tor-related request not going over Tor, but it's intended in this case because Tor may not be accessible without a bridge. Also, it's the same request for all users.

[fmarier]

nit: "Built-in"

[github-actions]

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

Description

This PR makes several changes to the Tor component in the Brave browser to support fetching and using builtin bridges from the Tor bridges server. The main motivation seems to be to allow automatically updating the list of builtin bridges used by Brave's Tor windows, rather than having them hardcoded.

Changes

Changes

browser/tor/tor_profile_service_factory.cc

• Passes the original profile to the TorProfileServiceImpl constructor in addition to the BrowserContext

browser/ui/webui/settings/brave_tor_handler.cc

• Changes some callback parameters from std::unique_ptr<std::string> to std::optional<std::string>
• Updates a method to use the new network::SimpleURLLoader::BodyAsStringCallback typedef instead of the deprecated network::SimpleURLLoader::BodyAsStringCallbackDeprecated

components/tor/DEPS

• Adds a dependency on services/data_decoder/public

components/tor/pref_names.h

• Adds a new preference kBuiltinBridgesRequestTime to store the last time the builtin bridges were requested

components/tor/tor_profile_service.cc

• Registers the new kBuiltinBridgesRequestTime preference

components/tor/tor_profile_service_impl.cc

• Adds a BuiltinBridgesRequest class to handle fetching updated builtin bridges
• TorProfileServiceImpl now takes the original profile in its constructor
• On construction, TorProfileServiceImpl may start a BuiltinBridgesRequest to update bridges if enough time has passed
• Handles the result of the BuiltinBridgesRequest in OnBuiltinBridgesResponse

components/tor/tor_utils.cc

• Moves the hardcoded builtin bridge definitions here
• Adds support for loading bridges from a dictionary in BridgesConfig

Security Hotspots

1. The BuiltinBridgesRequest fetches from a hardcoded URL kTorBuiltinBridgesFetchUrl. Ensure this URL is controlled by Brave and the contents can be trusted. Fetching bridges from an untrusted source could compromise Tor privacy.

2. The fetched bridges are stored in Local State prefs. Access to these prefs should be restricted to prevent malicious extensions from reading sensitive bridge info.

3. The simple URL loader used in BuiltinBridgesRequest doesn't follow redirects or save cookies, which is good. Make sure it cannot be reconfigured to follow redirects to untrusted destinations.

4. BuiltinBridgesRequest parses the response as JSON but the parsing is done in an isolated context via data_decoder::DataDecoder::ParseJsonIsolated which mitigates risk of a malicious payload.

So in summary, the main security considerations are ensuring the hardcoded bridges fetch URL remains trusted, and access to the Local State prefs storing bridges is restricted. The changes look good overall from a security perspective but those areas are worth extra scrutiny in code review.

[boocmp]

I didn't see an explicit check for that in the PR. Am I right in assuming that the profile service will not run at all unless a user has opened a private window with Tor?

Yes, You're right.