Closed Bug 1454242 Opened 6 years ago Closed 6 years ago

Login on https://testrail.stage.mozaws.net/ fails after same-site cookies set by cross site requests have been forbidden

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox-esr52 --- unaffected
firefox59 --- unaffected
firefox60 --- fixed
firefox61 --- fixed

People

(Reporter: aryx, Assigned: ckerschb)

References

Details

(Keywords: regression, Whiteboard: [kanban:https://webops.kanbanize.com/ctrl_board/2/6500][domsecurity-active])

Attachments

(2 files)

Bisection with Nightly on Windows confirms that the patches for bug 1452496 broke the login on https://testrail.stage.mozaws.net/

Steps to reproduce:
1. Open e.g. https://testrail.stage.mozaws.net/index.php?/plans/view/8760
2. Login with your LDAP and second factor.

Actual result:
File not found

An error occurred during a connection to testrail.stage.mozaws.net.

    Check the file name for capitalization or other typing errors.
    Check to see if the file was moved, renamed or deleted.

Expected result:
Testrail's own login form to login with LDAP shown.

needinfo for Miles as the op for testrail and Christoph as developer of bug 1452496 which has a pending beta uplift.
CC Julien and Ryan because of the pending uplift.
CC Andrei, Cristian and Rares in case SV might also work with testrail on Nightly.
Flags: needinfo?(miles)
Flags: needinfo?(ckerschb)
Whiteboard: [kanban:https://webops.kanbanize.com/ctrl_board/2/6500]
Assignee: server-ops-webops → nobody
Component: WebOps: IT-Managed Tools → DOM: Security
Flags: needinfo?(ckerschb)
Priority: -- → P1
Product: Infrastructure & Operations → Core
QA Contact: smani
Whiteboard: [kanban:https://webops.kanbanize.com/ctrl_board/2/6500] → [kanban:https://webops.kanbanize.com/ctrl_board/2/6500][domsecurity-active]
Valentin,

within Bug 1452496 we started to not allow same-site cookies being set from within a cross origin context. Unfortunately I used NS_IsSameSite() instead of simply consulting the third party util to determine whether a same-site cookie is allowed to be set. NS_IsSameSite uses the triggeringPrincipal for top-level loads [1] to determine whether samesite cookies in lax mode should be send. In this case we not only checked for sending same-site cookies, but also for setting, which is different and wrong.

In more detail, the URI https://testrail.stage.mozaws.net/index.php?/plans/view/8760 wants to set samesite cookie in 'lax' mode; when setting the cookie on the top-level page we used the triggeringPrincipal https://auth.mozilla.auth0.com/ which did not allow the same site cookie to be set. Simply using the thirdparty util within CanSetCookie() does the right check and causes the problem from comment 0 work correctly again.

[1] https://hg.mozilla.org/mozilla-central/rev/453b91e427d6#l1.25
Attachment #8968078 - Flags: review?(valentin.gosu)
Comment on attachment 8968078 [details] [diff] [review]
bug_1454242_same_site_cross_origin_context.patch

Review of attachment 8968078 [details] [diff] [review]:
-----------------------------------------------------------------

Glad we caught it early.
Attachment #8968078 - Flags: review?(valentin.gosu) → review+
Mark, since we need to uplift that change I suppose it's wise having an automated test for that as well. Please see a description of the problem in comment 1 - thanks!
Attachment #8968114 - Flags: review?(mgoodwin)
Comment on attachment 8968114 [details] [diff] [review]
bug_1454242_same_site_cross_origin_context_tests.patch

Review of attachment 8968114 [details] [diff] [review]:
-----------------------------------------------------------------

Good catch!
Attachment #8968114 - Flags: review?(mgoodwin) → review+
Pushed by mozilla@christophkerschbaumer.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c1fdf8c83da
Setting samesite cookie should not rely on NS_IsSameSiteForeign. r=valentin
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5e2ab3b7a26
Test samesite cookie on top-level page from cross-origin context. r=mgoodwin
Comment on attachment 8968078 [details] [diff] [review]
bug_1454242_same_site_cross_origin_context.patch

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1452496
[User impact if declined]: does not allow same-site cookies to be set if top-level page was triggered by cross origin page
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: not yet
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: Bug 1452496
[Is the change risky?]: no
[Why is the change risky/not risky?]: only affects same-site cookies
[String changes made/needed]: no
Attachment #8968078 - Flags: approval-mozilla-beta?
Comment on attachment 8968114 [details] [diff] [review]
bug_1454242_same_site_cross_origin_context_tests.patch

Approval Request Comment
see comment 7
Attachment #8968114 - Flags: approval-mozilla-beta?
Comment on attachment 8968078 [details] [diff] [review]
bug_1454242_same_site_cross_origin_context.patch

same-site cookies followup, approved for 60.0b13
Attachment #8968078 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8968114 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: needinfo?(miles)
We have several admin panels using lua-resty-openidc (Balrog, Normandy, probably others) that are affected by this.
https://hg.mozilla.org/mozilla-central/rev/3c1fdf8c83da
https://hg.mozilla.org/mozilla-central/rev/b5e2ab3b7a26
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.