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

[iOS] [hackerone] Make sure original document security settings apply to Reader Mode #36259

Closed
stoletheminerals opened this issue Feb 4, 2024 · 3 comments · Fixed by brave/brave-core#22276
Assignees
Labels
OS/iOS Fixes related to iOS browser functionality priority/P2 A bad problem. We might uplift this to the next planned release. QA Pass - iPhone QA/Yes release-notes/exclude security

Comments

@stoletheminerals
Copy link

Reader mode sends pre-defined security headers. This may cause security issues if the original page has a stricter policy.
More details: #35797

@stoletheminerals stoletheminerals added security priority/P2 A bad problem. We might uplift this to the next planned release. labels Feb 4, 2024
@diracdeltas
Copy link
Member

diracdeltas commented Feb 6, 2024

CSP

iOS sets a CSP header here that is already very restrictive: https://github.com/brave/brave-ios/blob/cc0db0c0a920868087167c2e397d77d756ec66a1/Sources/Brave/Frontend/Browser/Handlers/ReaderModeHandler.swift#L73. However we should lock it down further to this:

default-src 'none'; base-uri 'none'; form-action 'none'; frame-ancestors 'none'; sandbox; upgrade-insecure-requests; img-src *; style-src \(InternalURL.baseUrl) '\(ReaderModeHandler.readerModeStyleHash)'; font-src \(InternalURL.baseUrl); script-src 'nonce-\(setTitleNonce)'"]

sandbox is most likely to break something, if that is the case we can loosen it.

In addition, if we detect that the page sets img-src in CSP via either an HTTP header or a meta element, we should apply that policy instead of img-src *. Note that if you send a second CSP header or add it as a meta element, it overrides img-src * because the most restrictive policy should be applied automatically.

Subresource Integrity

for https://developer.mozilla.org/en-US/docs/Web/Security/Subresource_Integrity, we need to make sure the HTML integrity and crossorigin attributes from the original page are preserved in the reader mode page on all elements.

Non-CSP headers

In addition to modifying the CSP, the reader mode HTTP response should set a few more security headers in https://github.com/brave/brave-ios/blob/cc0db0c0a920868087167c2e397d77d756ec66a1/Sources/Brave/Frontend/Browser/Handlers/ReaderModeHandler.swift#L69

Namely:

X-Frame-Options: DENY
X-Content-Type-Options: nosniff
Referrer-Policy:  no-referrer

@iccub iccub transferred this issue from brave/brave-ios Feb 21, 2024
@iccub
Copy link

iccub commented Feb 21, 2024

transferred to brave-browser repo

@soner-yuksel soner-yuksel changed the title [hackerone] Make sure original document security settings apply to Reader Mode [iOS] [hackerone] Make sure original document security settings apply to Reader Mode Feb 26, 2024
@iccub iccub added the OS/iOS Fixes related to iOS browser functionality label Apr 2, 2024
@brave-builds brave-builds added this to the 1.67.x - Nightly milestone Apr 16, 2024
@hffvld hffvld added the QA/In-Progress Indicates that QA is currently in progress for that particular issue label Jun 18, 2024
@hffvld
Copy link
Contributor

hffvld commented Jun 18, 2024

Verified on iPhone 14 using version(s):

Device/OS: iPhone 14 / iOS 17.5.1
Brave build: 1.67 (118)
BraveCore: 1.67.118 (126.0.6478.71)

STEPS:

  1. Launch Brave
  2. Go to https://js176.gitlab.io/tools/deeplinks/2010/06/encrypt-web-https-everywhere-firefox-extension
  3. Three-dot menu > Settings > Appearance
  4. Enable Speedreader > Verify

ACTUAL RESULTS:

  • Verified that Speedreader-enabled view rendered, without the image, as per the test plan

1 2
1 2
1 2

@hffvld hffvld added QA Pass - iPhone and removed QA/In-Progress Indicates that QA is currently in progress for that particular issue labels Jun 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OS/iOS Fixes related to iOS browser functionality priority/P2 A bad problem. We might uplift this to the next planned release. QA Pass - iPhone QA/Yes release-notes/exclude security
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants