1775235 - Unable to login with Google on Pinterest.com

Status: VERIFIED FIXED

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

Description

[Rares Doghi, Desktop QA]

Attachment: PinterestFox.mp4

** Found in**

• Firefox Nightly 103.0a1 (2022-06-20)

Affected versions

• Firefox Nightly 103.0a1 (2022-06-20)

Affected platforms

• all

Steps to reproduce

1. Reach gmail.com and login.
2. Reach pinterest.com and select Connect with Google.
3. Select the google account.

Expected result

• User should be able to login without issues.

Actual result

• An error occurs when the user selects the Google account.

Regression range

• First bad: 28cf8d7e972301be7da0472bc7448deb06555c17
• Last good: 5096ce509f67ae285838599ef3c65f6ee2ae9bb2
• Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=5096ce509f67ae285838599ef3c65f6ee2ae9bb2&tochange=28cf8d7e972301be7da0472bc7448deb06555c17
• Potentially regressed by: Bug 1605305

Comment 1

[Daniel Veditz [:dveditz]]

Tom: can you take a look? Might be either Pinterest or Google making assumptions when they detect Firefox and not ready for an unexpected Origin header (e.g. what happens if you spoof a chrome UA?)

Comment 2

[Daniel Veditz [:dveditz]]

[Tracking Requested - why for this release]: Breaking Pinterest is probably bad

Comment 3

[Tom Schuster (MoCo)]

I am going to debug this more in-depth tomorrow. It would be helpful if someone could test if this still reproduces with network.http.origin.redirectTainted set to false.

Comment 4

[Tom Schuster (MoCo)]

So the problem is related to an XHR POST request to https://accounts.google.com/gsi/issue with a now null origin:

POST /gsi/issue?user_id=<...>
Host: accounts.google.com
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:103.0) Gecko/20100101 Firefox/103.0
Accept: */*
Accept-Language: en-US,en;q=0.5
Accept-Encoding: gzip, deflate, br
X-Requested-With: XmlHttpRequest
Content-Type: application/x-www-form-urlencoded;charset=utf-8
Content-Length: 0
Origin: null
Alt-Used: accounts.google.com
Connection: keep-alive
Cookie: <....>
Sec-Fetch-Dest: empty
Sec-Fetch-Mode: cors
Sec-Fetch-Site: same-origin
TE: trailers

I think this must be a bug in how we now check for CORS mode, because per Step 2. in the spec we should always add an Origin header with response tainting CORS. I implemented this check using mLoadInfo->GetTainting() previously we were using aChannel->GetCorsMode(&corsMode).

Kershaw any ideas? Is GetTainting even the right method to use, or are we maybe missing some kind of MaybeIncreaseTaint call somewhere?

Comment 5

[Rares Doghi, Desktop QA]

I tried the network.http.origin.redirectTainted set to False and I was still able to reproduce the issue in our latest build. @Tom Schuster

Comment 6

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

The severity field for this bug is set to S3. However, the bug is marked as tracked for firefox103 (nightly).
:tschuster, could you consider increasing the severity of this tracked bug?

For more information, please visit auto_nag documentation.

Comment 7

[Tom Schuster (MoCo)]

I tried the network.http.origin.redirectTainted set to False and I was still able to reproduce the issue in our latest build. @Tom Schuster

Thanks for checking this. It seems it's another issue after all.

So this really seems to be related to GetTainting vs GetCorsMode. GetTainting returns LoadTainting::Basic but GetCorsMode is 2 (CORS_MODE_CORS).

Comment 8

[Kershaw Chang [:kershaw]]

I think this must be a bug in how we now check for CORS mode, because per Step 2. in the spec we should always add an Origin header with response tainting CORS. I implemented this check using mLoadInfo->GetTainting() previously we were using aChannel->GetCorsMode(&corsMode).

Kershaw any ideas? Is GetTainting even the right method to use, or are we maybe missing some kind of MaybeIncreaseTaint call somewhere?

I think GetTainting is the right method to use. There might be a bug somewhere else.

Comment 9

[Kershaw Chang [:kershaw]]

Looks like the problem is in ReferrerInfo::ShouldSetNullOriginHeader.
I can login with google after adding the code below back.

--- a/dom/security/ReferrerInfo.cpp
+++ b/dom/security/ReferrerInfo.cpp
@@ -426,6 +426,12 @@ bool ReferrerInfo::ShouldSetNullOriginHeader(net::HttpBaseChannel* aChannel,
   MOZ_ASSERT(aChannel);
   MOZ_ASSERT(aOriginURI);
 
+  uint32_t corsMode = CORS_NONE;
+  NS_ENSURE_SUCCESS(aChannel->GetCorsMode(&corsMode), false);
+  if (corsMode == CORS_USE_CREDENTIALS) {
+    return false;
+  }
+
   nsCOMPtr<nsIReferrerInfo> referrerInfo;
   NS_ENSURE_SUCCESS(aChannel->GetReferrerInfo(getter_AddRefs(referrerInfo)),
                     false);

Comment 10

[Tom Schuster (MoCo)]

That is ~basically (before/after the GET/HEAD check) the same as replacing the GetTainting call with GetCorsMode in SetOriginHeader. ReferrerInfo::ShouldSetNullOriginHeader now is only supposed to implement Step 3.1. of append-a-request-origin-header, which doesn't do anything with CORS.

Comment 11

[Tom Schuster (MoCo)]

Interesting. I just realized that this must exactly be the test I described in bug 1768185: "Origin header and POST same-origin fetch cors mode with Referrer-Policy no-referrer". Per spec (and our current behavior) this should be null, but Chrome and Safari send the origin. Seems like the spec might simply be wrong.

Comment 12

[Tom Schuster (MoCo)]

Attachment: Bug 1775235 - Always send the real Origin for non GET/HEAD requests with request mode CORS. r?kershaw

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ec72038ed93c
Always send the real Origin for non GET/HEAD requests with request mode CORS. r=kershaw,necko-reviewers

Comment 14

[Pascal Chevrel:pascalc]

Unless I am missing something, it doesn't need tracking for 102 as the regression is affecting 103 only.

Comment 15

[Norisz Fay [:noriszfay]]

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

Comment 16

[Rares Doghi, Desktop QA]

This issue is Verified as Fixed in our latest build 20220623035450.

Comment 18

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

Set release status flags based on info from the regressing bug 1605305