1889066 - (CVE-2024-5687) Incorrect triggeringPrincipal used in new windows to load manually entered URLs

Status: RESOLVED FIXED

(Firefox for Android :: Browser Engine, defect, P1)

Description

[Daniel Veditz [:dveditz]]

Created from bug 1863662 comment 16 by jackyzy823 (see also bug 1863662 comment 10)

This is not only related to ReaderView.

This is quite a deep security bug. It is related to security design.

If a url is manually typed and opened in a already loaded tab (this tab has a parent tab, so it has parentId and referrerSessionId ) , should the triggerPrinciple, csp, referrerInfo of the parent tab be inherited?

For example

1. Open "https://example.com"
2. Long click on "More Information" and click open link in new tab
3. Switch to the new created tab which url is https://www.iana.org/help/example-domains
4. Manually input a url "https://httpbin.org/headers" in the addressbar
5. Observe the "Referer : example.com" is in the page. (this should not happens)

This is not limited to windows where the user long-clicks to open a new window: it also seems to be the same if a site opens a new window on a link press. I'm not sure yet if target=_blank has this effect, but window.open() sure does.

Example 1:

• open https://twitter.com/firefox/status/1774831670795129122. in the second tweet click the mzl.la/firefoxsauce link which opens a new window
• type (or paste) https://httpbin.org/headers into the URL bar

result: referrer will be twitter.com rather than m.youtube.com

Example 2:

• go to any story on https://www.cnn.com
• click in an ad, which opens a new window
• type (or paste) https://httpbin.org/headers into the URL bar

result: referrer will be cnn.com rather than the site that's showing the ad.

Notice also that the Sec-Fetch- headers are wrong. Sec-Fetch-Site: should say "none", but it says "cross-site", and the Sec-Fetch-User: ?1 is missing entirely. I get the same incorrect results if I use a bookmark rather than typing it.

Even from the home page in a brand new tab the Sec-Fetch- headers show something is wrong with the triggering principal: there's no -User header to show manual entry and the -Site says "cross-site" instead of "none". It looks like in this case we're using the Home page itself as the originator or something.

A "wrong referrer" may not be the end of the world, but it shows a scary confusion and is probably just the tip of the iceberg of potential problems. We have a ton of security policy checks in Gecko that critically depend on using the correct principals. And getting into this bad state doesn't depend on convincing a user to long-press; a malicious site could do it themselves if they're interesting enough to get the user to click on something (like a big panel covering 3/4 of the screen saying "This is not a paywall, but journalism isn't free. Please subscribe and support the information you rely on [sign up] [log in] [continue to article]"

Comment 1

[Daniel Veditz [:dveditz]]

I also hid two other comments from jackyzy823 that talked about the underlying security issue. I will reproduce them here:

bug 1863662 comment 10

This is bug is quite old .

The change from Bug 1856731 trigger the bug, previous readerview's url is loaded via WebExtension tab api, after this PR , it is loaded with store.dispatch(EngineAction.LoadUrlAction(it.id, readerUrl))

So the bug appears. and it also cause the bug 1875625 which don't work for a very very long time ( in my test v89 still don't work).

The key is
https://searchfox.org/mozilla-mobile/rev/c4e2718f82a8a0f40028bf2637312b152720f8ac/firefox-android/android-components/components/browser/engine-gecko/src/main/java/mozilla/components/browser/engine/gecko/GeckoEngineSession.kt#210

and

https://searchfox.org/mozilla-central/rev/a2709c2f0e31b2ad1a7b6447117626a34063fe57/mobile/android/modules/geckoview/GeckoViewNavigation.sys.mjs#189

It looks like Gecko/GeckoView couldn't handle the security principal properly?

So that the error

Security Error: Content at <The parent tab which opens new tab> may not load or link to <moz-ext/view-source scheme in the new opened tab>

occurs.

bug 1863662 comment 11

Or Fenix's bug?

LoadUrlAction / geckosession.loadurl couldn't make gecko resetting content principal because parentSession is always passed ?

https://searchfox.org/mozilla-mobile/rev/c4e2718f82a8a0f40028bf2637312b152720f8ac/firefox-android/android-components/components/browser/state/src/main/java/mozilla/components/browser/state/engine/middleware/EngineDelegateMiddleware.kt#55-83

https://searchfox.org/mozilla-mobile/rev/c4e2718f82a8a0f40028bf2637312b152720f8ac/firefox-android/android-components/components/feature/session/src/main/java/mozilla/components/feature/session/SessionUseCases.kt#90-119

Comment 2

[Daniel Veditz [:dveditz]]

Attachment: stage two

Comment 3

[Daniel Veditz [:dveditz]]

Attachment: testcase (save it and load from somewhere else)

Comment 4

[Daniel Veditz [:dveditz]]

If you load "testcase" here on bugzilla you won't learn much: it needs to be on a different origin than "stage two". You can download it and open it as a file. You can perform one of the tests by long-pressing on the stage two attachment link and opening it in a new tab (this bug is on BMO, but stage two will be on bug1889066.bmoattachments.org)

Comment 5

[jackyzy823]

Thanks for paying attention to this.

Some more context:

1. In Fenix part, the concept : parent could be easily messed up.

   1. It could mean the structure of tabs tray , for example, if you close a child tab, fenix will auto jump to its parent tab.
   2. It could also mean parent session ,where the child session's referrerInfo, csp , triggerPrincipal, contextId inherited from.

   A structural parent won't necessarily be the parent session. For example , manually load url in the child tab. Its structural parent is still the same, but it should have no parent session.

2. GeckoView part , the concept : referrerInfo and ( triggerPrincipal , csp ) could be easily messed up. I think these two concepts would better be handle separately .

   For example, like bug 1807537 , in this situation referrer should not be sent, but ( triggerPrincipal , csp ) should be following its parent.

---

I've made several approaches to fix this issue.

1. https://github.com/jackyzy823/gecko-dev/tree/bug-1875250-dont-inherit-principal changes Fenix and GeckoView both side. you need to DISALLOW inherit parent session explicitly.
2. https://github.com/jackyzy823/gecko-dev/tree/bug-1863662-dont-inherit-principal-only-fenix-side changes Fenix side, you need to DISALLOW inherit parent session explicitly. TODO: find more use cases which need to DISALLOW inherit parent session explicitly (like open from bookmark/collection/history/topsites)
3. https://github.com/jackyzy823/gecko-dev/tree/bug-1863662-allow-inherit-principal-only-some-situations-on-mobile-side changes Fenix side, you need to ALLOW inherit parent session explicitly , otherwise no parent session would be default . Currently only context menu actions and windowfeature (mostly for _blank , or window.open) are ALLOWed to inherit parent session. TODO: how to handle MigratePrivateTabUseCase/DuplicateTabUseCase and find more use cases which need to ALLOW inherit parent session explicitly

Comment 6

[Daniel Veditz [:dveditz]]

My testcase doesn't work quite as I expected. I might have a too simplistic assumption on what's going on (I have not looked at this under a debugger). There's probably a way to show the principal confusion can also lead to problems with <iframe sandbox> and CSP inheritance.

Comment 7

[Daniel Veditz [:dveditz]]

In Fenix part, the concept : parent could be easily messed up.

The Fenix layer should not be trying to determine the parent based on Fenix browser things. Only the document itself knows the truth (the Gecko-internal document, not the limited web-accessible subset). Every tiny difference between what Fenix thinks and what Gecko thinks is a crack that could give an attacker a way in.

GeckoView part , the concept : referrerInfo and ( triggerPrincipal , csp ) could be easily messed up. I think these two concepts would better be handle separately .

Documents logically (per spec!) have a "security policy", which encompasses all the settings for referrer policy, csp, sandboxing and so on. If you get your principal from the document it will link to the LoadInfo and have all these things, plus a set of "origin attributes" which include private browsing, containers, storage partitioning("cookie jars") and others. If you artificially create a new principal from a URL string it won't have any of those origin attributes things, it will only have scheme+host+port. That can be used in some situations, but only extremely carefully. It's a warning sign to see that, but it's not always wrong.

Eventually we want to create a literal "security policy" object for documents, but currently LoadInfo (including the principal it references) is close, plus a few extra bits we have to do manually.

For example, like bug 1807537 , in this situation referrer should not be sent, but ( triggerPrincipal , csp ) should be following its parent.

The user is taking an action in the browser. It's OK for the browser to reach in and change things at that point. It doesn't have to follow 100% of the rules that navigations and fetches wholly within the web context have to follow.

If you're opening a link in a non-private page into private browsing (or vice-versa) there's a good argument that nothing from the non-private page should carry over. The mobile and Firefox security devs should get together and make sure we're consistent.

Comment 8

[[:owlish] 🦉 PST]

The bug does not reproduce for us in desktop and in GVE. It means it's either in AC or Fenix layer

Comment 9

[[:owlish] 🦉 PST]

We used Reference Browser and narrowed it down to AC being the culprit

Comment 10

[[:owlish] 🦉 PST]

So, the problem is that AC is setting a referrer if an AC tab object has a parent tab object (meaning, if this tab was opened from another tab) - here https://searchfox.org/mozilla-mobile/rev/c4e2718f82a8a0f40028bf2637312b152720f8ac/firefox-android/android-components/components/browser/engine-gecko/src/main/java/mozilla/components/browser/engine/gecko/GeckoEngineSession.kt#210 The problem there is that AC doesn't take into account consequent navigation in the tab (like user entering a URL), which leads to this bug.

The setting of the referrer was introduced as a solution for this bug

We will be looking for a solution that does not regress the bug I linked above.

Comment 11

[Jeff Boek [:boek]]

We found a solution that fixes 1522829 without causing this regression. I'm going to spend some time tomorrow to ensure that we're not knowingly causing another regression and see if we can write some better tests for this.

:dveditz this is the first sec bug we're fixing in Fenix since moving to the mono-repo. Do we need to do anything special when opening / landing the patch?

Comment 12

[Daniel Veditz [:dveditz]]

:dveditz this is the first sec bug we're fixing in Fenix since moving to the mono-repo. Do we need to do anything special when opening / landing the patch?

Where is the patch? It needs to be in Phabricator, and security patches should be linked to a security bug so phabricator knows to restrict access to it and keep it hidden. You should be following the guidelines in Fixing Security Bugs. When the patch is ready to land it will then need Secuity Approval, which is done on the bugzilla attachment and not in phabricator. Those two documents link to each other so if you remember where to find one you can find the other, and you can always find the Security Approval one from the link in the yellow "SECURITY ISSUE" banner in a security bug.

Comment 13

[Daniel Veditz [:dveditz]]

If there's a bounty for this bug credit goes to jackyzy823 for reporting it. The original bug 1863662 was reported (and handled) as a functional bug until they dug in deeper.

Comment 14

[Jeff Boek [:boek]]

Attachment: Bug 1889066 - Adds property to LoadUrlAction to determine if we should include a parentEngineSession when loading a URL

Comment 15

[Jeff Boek [:boek]]

Attachment: Bug 1889066 - Adds tests to test includeParent

Depends on D206815

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/84ea5c275334
Adds property to LoadUrlAction to determine if we should include a parentEngineSession when loading a URL r=android-reviewers,amejiamarmol

Comment 17

[Donal Meehan [:dmeehan]]

Landing a sec-high rated bug without sec-approval?

Comment 18

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/be4c46af8f00
Backed out changeset 84ea5c275334 for causing Engine related android build bustages. CLOSED TREE

Comment 19

[Chris Peterson [:cpeterson]]

Priority P1 because this bug has been assigned to a squad/group.

Comment 20

[Donal Meehan [:dmeehan]]

Comment 18 missing an need-info on the assignee.
:boek this needs sec-approval before landing this again.

Comment 21

[Jeff Boek [:boek]]

We'll have an updated patch up this morning and will request sec-approval.

Comment 22

[Jeff Boek [:boek]]

Attachment: sec-review

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Not sure.
Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
I think it's possible for someone to deduce what the problem was based on this patch.
Which branches (beta, release, and/or ESR) are affected by this flaw, and do the release status flags reflect this affected/unaffected state correctly?
All Fenix channels are affected.
If not all supported branches, which bug introduced the flaw?

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
We should be able to backport this patch without risk.
How likely is this patch to cause regressions; how much testing does it need?
Unlikely to cause a regression
Is the patch ready to land after security approval is given?
yes

Comment 23

[Tom Ritter [:tjr]]

Comment on attachment 9395345 [details]
Bug 1889066 - Adds property to LoadUrlAction to determine if we should include a parentEngineSession when loading a URL

Approved to land and uplift

Comment 24

[Donal Meehan [:dmeehan]]

FYI :tjr/:boek This did not land in time for Fx126.
We are now in RC week for Fx126.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/737c6c63866d
Adds property to LoadUrlAction to determine if we should include a parentEngineSession when loading a URL r=android-reviewers,amejiamarmol

Comment 26

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/737c6c63866d

Comment 27

[Jeff Boek [:boek]]

Could we uplift in a dot release?

Comment 28

[Donal Meehan [:dmeehan]]

(In reply to Jeff Boek [:boek] from comment #27)

Could we uplift in a dot release?

:boek we currently don't ship secbugs in dot releases

Comment 29

[Tom Ritter [:tjr]]

Attachment: advisory.txt

Comment 30

[BugBot [:suhaib / :marco/ :calixte]]

a month ago, tjr placed a reminder on the bug using the whiteboard tag [reminder-test 2024-06-25] .

boek, please refer to the original comment to better understand the reason for the reminder.

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ed8d85433651
Adds tests to test includeParent r=android-reviewers,amejiamarmol

Comment 32

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/ed8d85433651