1850019 - (CVE-2023-5758) XSS on the domain reader in the firefox browser application for IOS

Status: VERIFIED FIXED

(Firefox for iOS :: General, defect)

Description

[Irwan]

Hi team.
As explained in https://hackerone.com/reports/2117521, I will be moving a vulnerability issue in an IOS app, which should be reported on the bugzilla form.

I found XSS in the Reader mode feature on the Firefox for IOS application, maybe this is also affected on Android.
I see that the domain of the reader feature is http://localhost:6571,
You can see the domain reader when opening https://hackerone.com/irwanjugabro, and the reader detects "connections is not secure" when you use the reader feature.

production steps:

1. download the firefox browser for IOS
2. then open the domain from the following reader features:

• http://localhost:6571/reader-mode/page?url=javascript:alert(document.domain)

3. after that, then click on the reader mode feature, then XSS will be triggered, because the redirect url that has been set to be javascript:alert(1)

Impact

XSS on the domain reader in the firefox browser application for IOS

Comment 1

[Irwan]

POC VIDEOS :

• https://drive.google.com/file/d/1cYGnZnHf4MDa-wGVBrfnc9aVtpzgjblp/view?usp=sharing

Comment 2

[Irwan]

Hi all any update for this problem?

Comment 4

[Daniel Veditz [:dveditz]]

reader mode should have an allow-list of safe schemes and reject anything else. Ideally just http and https.

Comment 5

[:Gijs (he/him)]

(In reply to Daniel Veditz [:dveditz] from comment #4)

reader mode should have an allow-list of safe schemes and reject anything else. Ideally just http and https.

FWIW there's already an allowlist on desktop, and it allows http/https and local files: https://searchfox.org/mozilla-central/rev/a7e33b7f61e7729e2b1051d2a7a27799f11a5de6/toolkit/components/reader/AboutReader.sys.mjs#50-62

I imagine on iOS the local file option isn't realistic.

Comment 6

[lmarceau]

Tracking progress on this issue with JIRA ticket https://mozilla-hub.atlassian.net/browse/FXIOS-7330

Comment 7

[Nishant Bhasin]

I see the issue on Firefox iOS side where we are checking scheme == "http" && host == "localhost" && path == "/reader-mode/page" which isn't enough to stop attack like above. I was able to run the following on the url(s) to test some benign changes and they sure seem to interfere with the page.

http://localhost:6571/reader-mode/page?url=javascript:document.body.style.display='none';

http://localhost:6571/reader-mode/page?url=javascript:document.body.innerText='Hello';

http://localhost:6571/reader-mode/page?url=javascript:document.body.style.fontSize='50px';

Regarding the fix and reference

The fix involves checking the url and to not allow direct injection and instead sanitize the url or just straight up not accept it.

Best option would be mix of both.
a) Do not accept any harmful urls
b) Sanitize strings using our htmlEntityEncodedString method before sending it to reader mode

Added some references on where loading of the url happens, will investigate further if I find anything new.

** Reference links **
enableReaderMode: https://github.com/mozilla-mobile/firefox-ios/blob/main/Client/Frontend/Browser/BrowserViewController/BrowserViewController%2BReaderMode.swift#L123
disableReaderMode: https://github.com/mozilla-mobile/firefox-ios/blob/main/Client/Frontend/Browser/BrowserViewController/BrowserViewController%2BReaderMode.swift#L151
isReaderModeURL: https://github.com/mozilla-mobile/firefox-ios/blob/main/Shared/Extensions/URLExtensions.swift#L290
htmlEntityEncodedString: https://github.com/mozilla-mobile/firefox-ios/blob/main/Shared/Extensions/String%2BExtension.swift#L137

Comment 8

[Nishant Bhasin]

Made a PR with potential FIX: https://github.com/mozilla-mobile/firefox-ios/pull/16345

Will be on the look out for any more areas of sanitization

Comment 9

[:Gijs (he/him)]

(In reply to Nishant Bhasin from comment #8)

Made a PR with potential FIX: https://github.com/mozilla-mobile/firefox-ios/pull/16345

Will be on the look out for any more areas of sanitization

Sorry, I was on PTO for this.

I'm pretty concerned by this fix. Unfortunately I cannot test it because I don't have an iDevice. AFAICT this tries to break the XSS by escaping quotes, which htmlEntityEncodedString happens to do. But there is no particular reason that the JS URL would need to use quotes at all, and escaping those quotes doesn't do anything to break opening the JS URL, only JS URLs that happen to use quotes. In fact, even comment 0 has an example (using alert(document.domain)) that doesn't use quotes.

I don't see any changes in the patch that ensure that the scheme/protocol used for the url param is one of a safelist. Was there some reason not to go with this approach, and limit the pages on which reader mode can be invoked to http and https ones, thus avoiding this problem completely?

Comment 10

[Nishant Bhasin]

Apologies for late reply as I was on PTO as well.

Reader mode runs on localhost and unfortunately with the current implementation we cannot simply stop users from accessing reader mode localhost urls and only stick to http / https. The issue isn't that reader mode is accessed from localhost. The issue is that the button pressed again for reader mode injects html / js.

With the above PR the idea isn't to not allow JS but to sanitize it so even if something does get injected it has no effect on the actual page. Its also the fact the the url isn't where this is happening but is actually happening inside a query param. The fix won't allow for a url to even load.

http://localhost:6571/reader-mode/page?url=javascript:alert(document.domain) will fail to even load with the fix mentioned above, it has been tested.

Ideally we should scrap this reader mode in favour of the one that Desktop uses but I won't go into those details as its much larger discussion and a bigger project. However let me know if there is a case that above PR missed.

Happy to further discuss this in detail.

Comment 11

[:Gijs (he/him)]

I had a conversation with Nishant about the patch. I am reassured that it does reject all JS URLs passed to reader mode, so I think for the purposes of this bug we're all good. I was confused about what the tests are testing (primarily the escaping) vs. what the implementation actually ends up doing (rejecting URLs without a host, which includes all JS URLs).

In terms of defence in depth, I do still think it would be good to have an allowlist of valid schemes for the url param (ie enforce that scheme is either http or https), but that could happen as a follow-up / separate ticket.

At this point, should this bug be closed and marked as fixed in 119? :-)

Comment 12

[Irwan]

Hi all.

just asking, more than 1 months have passed, is there any update on this issue?

Thank you

Comment 13

[lmarceau]

I marked this bug QA needed. Once that's done then the bug will be closed and we'll create sec advisories for v119.

Comment 14

[Andrei Bodea[:andrei]]

Verified as fixed on v119 (34997) with iPhone 13 Pro (15.7.1.)

Please note that I tried on an older version to figure out or to see if I'm able to reproduce this issue like it was mentioned in the description or the video attached.
So on the v118 version (more than 1 month old build) I tried the following:

• Open FF for iOS
• Open the domain from the following reader features:
  http://localhost:6571/reader-mode/page?url=javascript:alert(document.domain)
• after that, click on the reader mode feature, then XSS will be triggered because the redirect URL has been set to be javascript:alert(1)
  So after those steps when I tap on the Reader mode I have: Open this link in external app? (javascript:alert(document.domain).
  So like this, I was able to reproduce the issue.

When I tried on v119 (34997) where the fix is made - after repeating the steps from above when tapping on the reader mode nothing happened and the webpage just enter or leave the reader mode state. Nothing regarding `javascript:alert(document.domain) or anything related to that.

Can you please confirm this is the correct verification or do I need to check something more? @Irwan, @Nish

Please note that after this I will add a simply verification comment on the Jira ticket as it's a security issue and I will not add anything related to this on the Jira.

Comment 15

[Irwan]

Hi, thanks for tagging me here.

I think your retest is perfect on v119,
I can't try the restest on v119 because that version hasn't been released to the public yet, so I can't download it.

Cheers

Comment 16

[Andrei Bodea[:andrei]]

Awesome, thank you for the fast response Irwan!
Then based on your comment, I can close this issue and also the Jira one, this means that next week we will have the RC week, and on the 24 October the build will be released and you will be able to double-check it at that time too.

Comment 17

[Irwan]

Hi @andrei
thank you for your reply.

As we know that you have closed this ticket to be resolved, just ask if this problem is worth getting a bounty.

Once again, thank you.
Cheers

Comment 18

[lmarceau]

:irwan For bounty, developers are not the persons to take those decisions, so we can't answer this particular question. You should write at [e-mail] to contact the bug bounty team. The bug has the correct flags to be looked at by the bounty team now that it has been resolved, so they'll be able to answer you on this matter. Thank you!

Comment 19

[Irwan]

Hi @lmarceau, thanks for the follow up.
I'm sorry I don't understand that, I will try to send it to the intended email as stated.

Kind regards

Comment 20

[Daniel Veditz [:dveditz]]

I imagine on iOS [needing to use] the local file: option isn't realistic.
...
the implementation actually ends up [...] rejecting URLs without a host, which includes all JS URLs

That rule would reject file:/// URLs also, because they don't have a host. We ended up needing to allow file:/// urls on Android; are you sure you won't need them on iOS? It came up when documents were shared with Firefox from another app. In some cases a file would be downloaded in the other app. Sharing them moved or copied them into a place Firefox for Android could read and the OS gave us a file:/// URL pointing to it. A particularly common case this came up in was when Fenix added support for being a PDF-handler, although it came up sometimes prior to that if an app downloaded or generated a local media file.

Apple being Apple it's quite possible sharing is done through an OS API with richer options and no need for a lowly file:/// URL.

Comment 21

[Daniel Veditz [:dveditz]]

Nevermind: skimmed over comment 8 because I didn't notice the non-linkified URL

(In reply to :Gijs (he/him) from comment #11)
> I had a conversation with Nishant about the patch.

Where is the patch? It's not in this bug and there's no Jira issue or github PR linked in this bug's "see also" section

Comment 22

[Irwan]

Hi everyone.

thank you for the bounty.

Comment 23

[lmarceau]

Attachment: advisory.txt