1648825 - Fetch Metadata Headers contain invalid value for Sec-Fetch-Site for history manipulation

Status: RESOLVED FIXED

(Core :: DOM: Security, defect, P3)

Description

[terjanq]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_14_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/83.0.4103.116 Safari/537.36

Steps to reproduce:

PoC: https://terjanq.appspot.com/sec-fetch/reports/location-hash-bypass.html

1. On abc.com origin load xyz.com page in iframe.
2. Set location of the loaded iframe to xyz.com#123
3. Set location of the iframe to same-origin, i.e. abc.com
4. Call history.back() on the iframe

Please note that the xyz.com should set either Cache-Control: no-store or must-revalidate, because Firefox will likely load the page from cache otherwise, so there will be no additional request sent to the server.

Actual results:

After 4th step, the browser will send header Sec-Fetch-Site: none.

Expected results:

The browser should not send Sec-Fetch-Site: none, it is not yet clear what value it should be but Chrome sets cross-site as for an example.

Comment 1

[terjanq]

It's possible that the behaviour has the same root cause as https://bugzilla.mozilla.org/show_bug.cgi?id=1647128

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

Some thoughts about fixing this bug:

• First, (though not directly related to this bug) we should fix Bug 1698767 to avoid any unwanted side effects.
• Second, relying on the referrerInfo to be present to determine IsUserTriggeredForSecFetchSite is a semi optimal solution and might cause other problems, besides the mentioned history manipulation in this bug. For Sec-Fetch-User we rely on loadInfo->GetHasValidUserGestureActivation(), maybe that's an option to factor in here too?

Comment 3

[Niklas]

Attachment: Bug 1648825 - Add user activation flag to ChildSHistory::reload. r=ckerschb

Depends on D108035

Comment 4

[Niklas]

Hey Edgar,

While investigating this Bug i came across some code that you wrote and i have a couple of questions.

When the reload button is clicked by the user, the LoadURI method is called from here and by then i would have expected that mBrowsingContext->GetCurrentWindowContext()->HasValidTransientUserGestureActivation() would return true, since the load is triggered by the user through the reload button but it returns false.
What do you think, should it return true? If so, I feel like there is a missing call somewhere to WindowContext::NotifyUserGestureActivation() in the context of a user triggered reload.

Comment 5

[Edgar Chen [:edgar]]

(In reply to Niklas from comment #4)

When the reload button is clicked by the user, the LoadURI method is called from here and by then i would have expected that mBrowsingContext->GetCurrentWindowContext()->HasValidTransientUserGestureActivation() would return true, since the load is triggered by the user through the reload button but it returns false.

The click happens on the reload button which is in the browser window, so it only activates the WindowContext of the browsing window.
If the mBrowsingContext is the web content one, it would stay false as the user interacts with the browser, not the web content.

Comment 6

[Niklas]

Attachment: Bug 1648825 - Add user activation flag to goBack(). r=ckerschb

Depends on D108657

Comment 7

[Niklas]

Attachment: Bug 1648825 - Add user activation flag to goForward(). r=ckerschb

Depends on D109641

Comment 8

[Niklas]

Attachment: Bug 1648825 - Ensure that Sec-Fetch-Site is only 'none' if the load was user triggered. r=ckerschb

Depends on D109642

Comment 9

[Niklas]

AFAICT there was no way for the content process to know if a reload or history manipulation was triggered by the user or not.
In this patch i added a aUserActivation boolean to the ipc methods so the parent can tell the content process if it has a user activation. (this and this patch are analogous to that for back and forward).

Would this be a viable approach in your opinion? or would you have a different idea?

Comment 10

[Edgar Chen [:edgar]]

Has left some comments in phabricator.

Comment 11

[Niklas]

Attachment: Bug 1648825 - Add user activation flag to reload, goBack and goForward r=ckerschb

Comment 12

[Phabricator Automation]

Comment on attachment 9212500 [details]
Bug 1648825 - Add user activation flag to reload, goBack and goForward r=ckerschb

Revision D110245 was moved to bug 1708150. Setting attachment 9212500 [details] to obsolete.

Comment 13

[Niklas]

Attachment: Bug 1648825 - Add nsISHEntry::hasUserActivation r=ckerschb

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5aece2a17f5d
Add nsISHEntry::hasUserActivation r=ckerschb,smaug
https://hg.mozilla.org/integration/autoland/rev/d90566e41269
Ensure that Sec-Fetch-Site is only 'none' if the load was user triggered. r=ckerschb,marionette-reviewers,whimboo

Comment 15

[Andreea Pavel [:apavel]]

Backed out for failing bc at browser_navigation.js

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&selectedTaskRun=WWdVkk_AT_qh9x-bEDL19g.0&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=615fca6623860d9516ac173f71db02d2303e26f9

Failure log: https://treeherder.mozilla.org/logviewer?job_id=340321324&repo=autoland&lineNumber=6766

Backout: https://hg.mozilla.org/integration/autoland/rev/ec603555c196ad8e682cb73023fda275f379c984

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a25184e549ad
Add nsISHEntry::hasUserActivation r=ckerschb,smaug
https://hg.mozilla.org/integration/autoland/rev/a84a2e535ffd
Ensure that Sec-Fetch-Site is only 'none' if the load was user triggered. r=ckerschb,marionette-reviewers,whimboo

Comment 17

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/a25184e549ad
https://hg.mozilla.org/mozilla-central/rev/a84a2e535ffd