[iOS] - Add CSPs support in Chromium WebUI #26402

Brandon-T · 2024-11-06T19:15:04Z

Security Review

https://github.com/brave/reviews/issues/1792

Summary

Adds support for Content-Security-Policy and other headers to be set on WebUI responses so that WebKit can enforce such policies. See screenshots below.

Resolves brave/brave-browser#42138

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:

Screenshots:

Testing Overrides by adding script-src with a nonce:

WebKit sees the abovenonce:

Testing Default CSPs (no overrides):

Without this PR (Current AppStore Build - Same as Chrome-iOS):

The above matches Desktop:

github-actions · 2024-11-06T19:19:33Z

The security team is monitoring all repositories for certain keywords. This PR includes the word(s) "csp" 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.

stoletheminerals

@Brandon-T do we expose any mojo interfaces to chrome-untrusted? For example, does MojoJS get injected in these contexts? I believe @bridiver mentioned that the last time we tried to implement it, page permissions weren't too much different from chrome:// urls

chromium_src/ios/web/webui/crw_web_ui_scheme_handler.mm

Brandon-T · 2024-11-18T23:56:54Z

@Brandon-T do we expose any mojo interfaces to chrome-untrusted? For example, does MojoJS get injected in these contexts? I believe @bridiver mentioned that the last time we tried to implement it, page permissions weren't too much different from chrome:// urls

chromium-untrusted must have access to mojom APIs.

It's not injected by default but if we created an untrusted html page that loads the mojom script, it will work.

If an untrusted page programmatically loads the script, it'll work.

mojom bindings are available to chromium-untrusted as they're supposed to be afaik. I can probably block them, but how would it communicate with native code?

We would have to disable it via: https://source.chromium.org/chromium/chromium/src/+/main:ios/web/web_state/ui/crw_wk_ui_handler.mm;l=256;drc=a2ed1bfa50bdef7869f102cb1436f0c95cca812f;bpv=0;bpt=1

But https://chromium.googlesource.com/chromium/src/+/main/docs/chrome_untrusted.md states:

We recommend using Mojo to expose APIs to chrome-untrusted://. Mojo for chrome-untrusted:// works similarly to how it works for chrome:// with a few key differences:

Unlike chrome:// pages, chrome-untrusted:// pages don't get access to all renderer exposed Mojo interfaces by default. Use PopulateChromeWebUIFrameInterfaceBrokers to expose WebUI specific interfaces to your WebUI. See this CL for example.
The exposed interface has a different threat model: a compromised chrome-untrusted:// page could try to exploit the interface (e.g. sending malformed messages, closing the Mojo pipe).

Desktop has: https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/render_frame_host_impl.cc;l=16777;drc=a2ed1bfa50bdef7869f102cb1436f0c95cca812f

But iOS doesn't have a RenderFrameHost or any sort of Render Processes.

stoletheminerals · 2024-11-19T04:28:27Z

mojom bindings are available to chromium-untrusted as they're supposed to be afaik. I can probably block them, but how would it communicate with native code?

This document
https://docs.google.com/document/d/1Kw4aTuISF7csHnjOpDJGc7JYIjlvOAKRprCTBVWw_E4/edit?tab=t.0#heading=h.eh1u1uo66itu states:

Unlike trustworthy WebUI pages, which may request any renderer-exposed interface through Mojo.bindInterface(), an untrustworthy WebUIs is restricted to requesting its single bespoke Mojo interface registered in PopulateChromeWebUIFrameInterfaceBrokers(). Often, this interface is used as a top-level broker interface to expose additional interfaces to the untrustworthy WebUI.

So I guess the question is, can chrome-untrusted on iOS bind to any interface or is it limited in the same way as on desktop.

Brandon-T · 2024-11-19T07:33:49Z

mojom bindings are available to chromium-untrusted as they're supposed to be afaik. I can probably block them, but how would it communicate with native code?

This document https://docs.google.com/document/d/1Kw4aTuISF7csHnjOpDJGc7JYIjlvOAKRprCTBVWw_E4/edit?tab=t.0#heading=h.eh1u1uo66itu states:

Unlike trustworthy WebUI pages, which may request any renderer-exposed interface through Mojo.bindInterface(), an untrustworthy WebUIs is restricted to requesting its single bespoke Mojo interface registered in PopulateChromeWebUIFrameInterfaceBrokers(). Often, this interface is used as a top-level broker interface to expose additional interfaces to the untrustworthy WebUI.

So I guess the question is, can chrome-untrusted on iOS bind to any interface or is it limited in the same way as on desktop.

I believe an untrusted frame would be able to access whatever interfaces are bound to the main-frame.
https://source.chromium.org/chromium/chromium/src/+/main:ios/web/webui/mojo_facade.mm;l=101;drc=a2ed1bfa50bdef7869f102cb1436f0c95cca812f?q=GetInterfaceBinderForMainFrame&ss=chromium%2Fchromium%2Fsrc

is called no matter which frame or scheme calls it. So:

var pHandle = Mojo.createMessagePipe();
Mojo.bindInterface('optimization_guide_internals.mojom.PageHandlerFactory', pHandle.handle0);

From the skus-internals page in a separate iFrame, will call it. In WebUI on iOS, all interfaces are registered through that function, so I'm pretty sure the untrusted would be able to access it the same way.

We don't have: https://source.chromium.org/chromium/chromium/src/+/main:content/public/browser/per_web_ui_browser_interface_broker.h;l=23;drc=a2ed1bfa50bdef7869f102cb1436f0c95cca812f;bpv=0;bpt=1

or https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/chrome_browser_interface_binders.cc;l=1188;drc=7fb345a0da63049b102e1c0bcdc8d7831110e324;bpv=0;bpt=1

or https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/chrome_content_browser_client_receiver_bindings.cc;l=367;drc=a2ed1bfa50bdef7869f102cb1436f0c95cca812f;bpv=0;bpt=1

or a RenderHost frame. Chrome iOS doesn't have a registry to register any WebUI, whereas Desktop registers every WebUI regardless of if it's trusted or not.

Chrome's regular ones are registered here: https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/chrome_browser_interface_binders.cc;l=768;drc=7fb345a0da63049b102e1c0bcdc8d7831110e324;bpv=0;bpt=1

I'm not sure if it's even possible to replicate that behaviour and fix it on iOS.

Interfaces for trustworthy WebUIs should be bound in [PopulateChromeWebUIFrameBinders].

Already doesn't apply to iOS. We would have to figure out a way to do all of that for both trusted and untrusted if we want this to follow Desktop or this doc.

stoletheminerals · 2024-11-19T14:20:54Z

I believe an untrusted frame would be able to access whatever interfaces are bound to the main-frame

This unfortunately defeats the purpose of chrome-untrusted. I think we should decouple CSP logic (which lgtm) from this PR and figure out how to make chrome-untrusted secure.

github-actions · 2024-11-19T15:23:10Z

The security team is monitoring all repositories for certain keywords. This PR includes the word(s) "policy, csp" 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.

Brandon-T · 2024-11-19T16:12:52Z

chrome-untrusted support has now been removed from this PR via the revert commit: c8f38a6

Only Content-Security-Policy additions/overrides remain.

bridiver · 2024-12-17T23:07:53Z

chromium_src/ios/web/webui/url_fetcher_block_adapter.h

+                                                           \
+ public:                                                   \
+  const network::mojom::URLResponseHeadPtr getResponse() { \
+    return response_.Clone();                              \


what is this for?

Chromium has: https://source.chromium.org/chromium/chromium/src/+/main:ios/web/webui/crw_web_ui_scheme_handler.mm;l=89?q=CRWWebUISchemeHandler&ss=chromium%2Fchromium%2Fsrc

they completely ignore the headers from the response (and they use * for the Access-Control-Allow-Origin header. They hardcode headers). So to add the actual response headers, I need to retrieve it, and add it here (in this PR):

brave-core/chromium_src/ios/web/webui/crw_web_ui_scheme_handler.mm

Lines 64 to 77 in c8f38a6

const network::mojom::URLResponseHeadPtr responseHead =

fetcher->getResponse();

if (responseHead) {

const scoped_refptr<net::HttpResponseHeaders> headers =

responseHead->headers;

if (headers) {

// const std::string& raw_headers = headers->raw_headers();

NSMutableDictionary* responseHeaders =

[strongSelf parseHeaders:headers];

if (![responseHeaders objectForKey:@"Content-Type"]) {

[responseHeaders setObject:mimeType forKey:@"Content-Type"];

}

can you please add comments explaining?

bridiver · 2024-12-17T23:08:16Z

chromium_src/ios/web/webui/url_fetcher_block_adapter.mm

+  GetFinalURL();                               \
+  }                                            \
+                                               \
+  response_ = url_loader_->TakeResponseInfo(); \


same as above, what is this for?

It's an override for: https://source.chromium.org/chromium/chromium/src/+/main:ios/web/webui/url_fetcher_block_adapter.mm;l=43;bpv=0;bpt=1

So that I can retrieve the response that contains the headers, so that I can give them to WebKit's WKURLSchemeHandler here:

brave-core/chromium_src/ios/web/webui/crw_web_ui_scheme_handler.mm

Line 65 in c8f38a6

fetcher->getResponse();

and pass them to WebKit. It's used in the files linked above, as part of this PR.

same as above, please add comments

bridiver · 2024-12-17T23:24:33Z

chromium_src/ios/web/public/webui/url_data_source_ios.h

+
+#include <cstdint>
+
+namespace network::mojom {


don't forward declare enums

bridiver · 2024-12-17T23:25:17Z

chromium_src/ios/web/public/webui/url_data_source_ios.h

+  virtual std::string GetContentSecurityPolicy(                 \
+      network::mojom::CSPDirectiveName directive) const;        \
+                                                                \
+ private:                                                       \


what is the purpose of this?

bridiver · 2024-12-17T23:26:30Z

chromium_src/ios/web/webui/url_data_source_ios.mm

+}
+
+bool URLDataSourceIOS::ShouldServiceRequest(const GURL& url) const {
+  return URLDataSourceIOS::ShouldServiceRequest_ChromiumImpl(url);


doesn't this also need to include kChromeUIUntrustedScheme?

if it doesn't then overriding this is pointless

bridiver · 2024-12-17T23:30:32Z

chromium_src/ios/web/webui/url_data_source_ios.mm

+}  // namespace
+
+namespace web {
+bool URLDataSourceIOS::ShouldAddContentSecurityPolicy() const {


adding this seems pointless if it's always going to return true

chromium_src/ios/web/webui/crw_web_ui_scheme_handler.mm

bridiver · 2024-12-17T23:35:40Z

chromium_src/ios/web/webui/url_data_manager_ios_backend.mm

+#define ShouldDenyXFrameOptions ShouldDenyXFrameOptions());   \
+  job->set_add_content_security_policy(                       \
+      source->source()->ShouldAddContentSecurityPolicy());    \
+  job->set_content_security_policy_object_source(             \


this is already called in URLDataManagerIOSBackend::StartRequest, why are we calling it again?

bridiver · 2024-12-19T00:47:23Z

ios/browser/ui/webui/public/BUILD.gn

+import("//build/config/features.gni")
+import("//mojo/public/tools/bindings/mojom.gni")
+
+source_set("public") {


public doesn't make sense inside ios/browser, this is more equivalent to URLDataSourceIOSImpl because it's a brave specific implementation.

bridiver · 2024-12-19T00:49:27Z

ios/browser/ui/webui/public/brave_web_ui_ios_data_source.mm

+  std::move(callback).Run(response);
+}
+
+std::string BraveWebUIIOSDataSource::GetMimeType(


why aren't we subclassing WebUIIOSDataSourceImpl to get this stuff? We don't want to duplicate it

I guess it isn't public, but per dm I think we can have BraveWebUIIOSDataSource own an instance of WebUIIOSDataSourceImpl that is created through WebUIIOSDataSource::Create and then proxy these methods to it

github-actions · 2025-01-16T17:18:28Z

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

Description

This PR introduces significant changes to the Brave iOS browser, primarily focusing on enhancing WebUI functionality and security. It adds support for the chrome-untrusted scheme, implements Content Security Policy (CSP) controls, and modifies the behavior of WebUI data sources.

Possible Issues

The implementation of IsWebUIMessageAllowedForFrame in brave_web_client.mm always returns true, which might be overly permissive.
The chrome-untrusted scheme is added, but the logic for handling untrusted interfaces is commented out, potentially leaving a security gap.

Security Hotspots

The addition of the chrome-untrusted scheme in profile_ios.mm could introduce new attack vectors if not properly secured.
The modification of Content Security Policy settings in brave_url_data_source_ios.mm might affect the security of WebUI pages if not carefully implemented.
The IsWebUIMessageAllowedForFrame method in brave_web_client.mm always returns true, which could potentially allow unauthorized messages.

Changes

Changes

ios/chrome/browser/shared/model/profile/profile_ios.mm:
- Added support for the chrome-untrusted scheme.
ios/components/webui/web_ui_url_constants.cc and .h:
- Defined the kChromeUIUntrustedScheme constant.
ios/web/public/web_client.h and ios/web/web_client.mm:
- Added IsWebUIMessageAllowedForFrame method.
ios/web/public/web_state.h and ios/web/web_state/web_state.mm:
- Implemented AddUntrustedInterface and BindUntrustedInterface methods.
ios/web/webui/crw_web_ui_scheme_handler.mm:
- Modified response processing to add default headers.
ios/web/webui/mojo_facade.mm:
- Implemented IsWebUIMessageAllowedForFrame method.
ios/browser/brave_web_client.h and .mm:
- Added support for untrusted interfaces and WebUI message handling.
ios/browser/ui/webui/public/brave_url_data_source_ios.h and .mm:
- Implemented custom Content Security Policy controls.
ios/browser/ui/webui/public/brave_web_ui_ios_data_source.h and .mm:
- Created a new WebUI data source with enhanced security features.

sequenceDiagram
    participant WebUI
    participant BraveWebUIIOSDataSource
    participant BraveURLDataSourceIOS
    participant WebClient
    WebUI->>BraveWebUIIOSDataSource: Request data
    BraveWebUIIOSDataSource->>BraveURLDataSourceIOS: Get content
    BraveURLDataSourceIOS->>WebClient: Get resource
    WebClient-->>BraveURLDataSourceIOS: Return resource
    BraveURLDataSourceIOS->>BraveURLDataSourceIOS: Apply CSP
    BraveURLDataSourceIOS-->>BraveWebUIIOSDataSource: Return content
    BraveWebUIIOSDataSource-->>WebUI: Serve content

Brandon-T self-assigned this Nov 6, 2024

Brandon-T requested a review from a team as a code owner November 6, 2024 19:15

kylehickinson requested a review from a team November 6, 2024 19:15

github-actions bot added puLL-Merge needs-security-review labels Nov 6, 2024

github-actions bot assigned kdenhartog and thypon Nov 6, 2024

stoletheminerals reviewed Nov 18, 2024

View reviewed changes

chromium_src/ios/web/webui/crw_web_ui_scheme_handler.mm Outdated Show resolved Hide resolved

Brandon-T requested a review from stoletheminerals November 19, 2024 07:46

Brandon-T force-pushed the feature/ios-chromium-untrusted branch from f207166 to e8024f1 Compare November 19, 2024 15:18

Brandon-T force-pushed the feature/ios-chromium-untrusted branch 2 times, most recently from 9075cd7 to c8f38a6 Compare November 19, 2024 16:00

Brandon-T changed the title ~~[iOS] - Add CSPs and Chromium Untrusted to WebUI~~ [iOS] - Add CSPs support in Chromium WebUI Nov 19, 2024

stoletheminerals approved these changes Nov 19, 2024

View reviewed changes

bridiver reviewed Dec 17, 2024

View reviewed changes

chromium_src/ios/web/webui/crw_web_ui_scheme_handler.mm Outdated Show resolved Hide resolved

bridiver reviewed Dec 17, 2024

View reviewed changes

Brandon-T force-pushed the feature/ios-chromium-untrusted branch from c8f38a6 to 762e56e Compare December 18, 2024 22:19

Brandon-T requested a review from a team as a code owner December 18, 2024 22:19

Brandon-T force-pushed the feature/ios-chromium-untrusted branch 3 times, most recently from 85e7120 to 014a4ae Compare December 18, 2024 22:25

Brandon-T requested a review from bridiver December 19, 2024 00:36

bridiver reviewed Dec 19, 2024

View reviewed changes

nuo-xu approved these changes Dec 19, 2024

View reviewed changes

Brandon-T force-pushed the feature/ios-chromium-untrusted branch from 9514589 to 1a1a6d5 Compare December 24, 2024 15:12

Brandon-T force-pushed the feature/ios-chromium-untrusted branch 4 times, most recently from a5b8533 to 6ff07ce Compare January 8, 2025 20:29

Brandon-T force-pushed the feature/ios-chromium-untrusted branch from 6ff07ce to f56d5f0 Compare January 14, 2025 18:54

Brandon-T force-pushed the feature/ios-chromium-untrusted branch 3 times, most recently from 742e538 to 3e2cb38 Compare January 16, 2025 18:07

Brandon-T added 2 commits January 16, 2025 13:07

Add Chromium CSPs

7c17675

Add WebUI Untrusted Frame Validation

31db77d

Brandon-T force-pushed the feature/ios-chromium-untrusted branch from 3e2cb38 to 31db77d Compare January 16, 2025 18:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[iOS] - Add CSPs support in Chromium WebUI #26402

[iOS] - Add CSPs support in Chromium WebUI #26402

Brandon-T commented Nov 6, 2024 •

edited

Loading

github-actions bot commented Nov 6, 2024

stoletheminerals left a comment

Brandon-T commented Nov 18, 2024 •

edited

Loading

stoletheminerals commented Nov 19, 2024

Brandon-T commented Nov 19, 2024 •

edited

Loading

stoletheminerals commented Nov 19, 2024 •

edited

Loading

github-actions bot commented Nov 19, 2024

Brandon-T commented Nov 19, 2024

bridiver Dec 17, 2024

Brandon-T Dec 17, 2024 •

edited

Loading

bridiver Dec 19, 2024

bridiver Dec 17, 2024

Brandon-T Dec 17, 2024

bridiver Dec 19, 2024

bridiver Dec 17, 2024

bridiver Dec 17, 2024

bridiver Dec 17, 2024

bridiver Dec 17, 2024

bridiver Dec 17, 2024

bridiver Dec 17, 2024

bridiver Dec 19, 2024

bridiver Dec 19, 2024 •

edited

Loading

bridiver Dec 19, 2024 •

edited

Loading

github-actions bot commented Jan 16, 2025

Changes

	const network::mojom::URLResponseHeadPtr responseHead =
	fetcher->getResponse();
	if (responseHead) {
	const scoped_refptr<net::HttpResponseHeaders> headers =
	responseHead->headers;
	if (headers) {
	// const std::string& raw_headers = headers->raw_headers();

	NSMutableDictionary* responseHeaders =
	[strongSelf parseHeaders:headers];

	if (![responseHeaders objectForKey:@"Content-Type"]) {
	[responseHeaders setObject:mimeType forKey:@"Content-Type"];
	}

[iOS] - Add CSPs support in Chromium WebUI #26402

Are you sure you want to change the base?

[iOS] - Add CSPs support in Chromium WebUI #26402

Conversation

Brandon-T commented Nov 6, 2024 • edited Loading

Security Review

Summary

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Screenshots:

github-actions bot commented Nov 6, 2024

stoletheminerals left a comment

Choose a reason for hiding this comment

Brandon-T commented Nov 18, 2024 • edited Loading

stoletheminerals commented Nov 19, 2024

Brandon-T commented Nov 19, 2024 • edited Loading

stoletheminerals commented Nov 19, 2024 • edited Loading

github-actions bot commented Nov 19, 2024

Brandon-T commented Nov 19, 2024

Choose a reason for hiding this comment

Brandon-T Dec 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bridiver Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

bridiver Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Jan 16, 2025

Description

Possible Issues

Security Hotspots

Changes

Brandon-T commented Nov 6, 2024 •

edited

Loading

Brandon-T commented Nov 18, 2024 •

edited

Loading

Brandon-T commented Nov 19, 2024 •

edited

Loading

stoletheminerals commented Nov 19, 2024 •

edited

Loading

Brandon-T Dec 17, 2024 •

edited

Loading

bridiver Dec 19, 2024 •

edited

Loading

bridiver Dec 19, 2024 •

edited

Loading