1478280 - samesite=strict prevents reading the cookie after a xhr request

Status: NEW

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

Description

[alexanderbh]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/67.0.3396.99 Safari/537.36

Steps to reproduce:

1. Make a xhr GET request to an URL that responds with a Set-Cookie xxx; samesite=strict header.

2. After the request is completed access document.cookie


Actual results:

The cookie from the GET request is not in document.cookie


Expected results:

I expect to be able to read the cookie. I am able to read the cookie if the request responds with samesite=lax.

Comment 1

[alexanderbh]

NOTE: It works if I refresh the page manually after the XHR request is done. Then I can actually read the cookie. But not after the XHR request is done without refresh.

Comment 2

[Rares Doghi, Desktop QA]

Hi, This is a bit technical for me, but I will give it a shot if you can tell me the exact steps you used to make the XHR GET request and what is the URL you were using that had the "samesite=strict"  header.

Comment 3

[alexanderbh]

It is not technical I think. But it requires you to make a XHR request where the website and the endpoint is on the same host. And then have the host response with a samesite=strict cookie.

When you have that the following code will show that the cookie cannot be read:

        let xsrfRequest = {
            uri: "endpoint on the same host as this request is made from",
            options: {
                method: "GET",
                credentials: "include"
            }
        };
        fetch(xsrfRequest.uri, xsrfRequest.options)
        .then(() => {
            console.log(document.cookie); // The cookie from the response above is not here!
        });

Comment 4

[Rares Doghi, Desktop QA]

Hi, this is a bit to advanced for me, but I think the Network > Cookies component might be a start for this issue.

Comment 5

[Junior [inactive]]

Hello :ckerschb
I saw you solved lots of samesite issues.
Do you have any idea of this?

Thanks.

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

302 to :mgoodwin who is the right person for same-site questions. I am happy to help implement changes if needed though.

Comment 7

[Selena Deckelmann :selenamarie :selena]

:mgoodwin - can you comment?

Comment 8

[Mark Goodwin [:mgoodwin]]

(In reply to alexanderbh from comment #3)
> It is not technical I think. But it requires you to make a XHR request where
> the website and the endpoint is on the same host. And then have the host
> response with a samesite=strict cookie.
> 
> When you have that the following code will show that the cookie cannot be
> read:
> 
>         let xsrfRequest = {
>             uri: "endpoint on the same host as this request is made from",
>             options: {
>                 method: "GET",
>                 credentials: "include"
>             }
>         };
>         fetch(xsrfRequest.uri, xsrfRequest.options)
>         .then(() => {
>             console.log(document.cookie); // The cookie from the response
> above is not here!
>         });


I'm not sure whether I'm misunderstanding the problem or just struggling to reproduce it. Can you try the following for me, please:
* open the document in a new tab and open the devtools.
* Switch to the "network" tab.
* Navigate to https://computerist.org/1478280.html - observe that the request to this document has no cookies set.
* Switch to the console tab in the devtools toolbox.
* Press the button.

Is this illustrating the behavior you want?

Comment 9

[Mark Goodwin [:mgoodwin]]

Aha. It occurs to me that this could be racy - if the cookie is set on the document *after* the XHR triggers the callback, it'll be empty.

Comment 10

[Mark Goodwin [:mgoodwin]]

Testing this on OS X confirms the behavior differs sometimes. I'd be greatly surprised if this is specific to SameSite and I'm not sure it's necessarily even a bug (is the order of the change to document.cookie / invocation of the callback even defined?). I'll see what I can discover.

Comment 11

[Mark Goodwin [:mgoodwin]]

I've updated the test document I linked to in comment #8 to include a similar test for a non-SameSite cookie. My intention was to test whether the behaviour is the same for all cookies that are set via XHR. Unfortunately I can no longer reproduce this issue on OS X today.

(In reply to Mark Goodwin [:mgoodwin] from comment #10)
(is the order of the change to document.cookie / invocation of
> the callback even defined?).

My reading of the spec suggests it may not be defined behaviour.

Regardless, Chromium, Safari and Edge seem to behave consistently in the cookies being available; this, plus the fact that cookie values can't be extracted from the response headers, suggest to me that we should probably treat this as a bug.  I'm pretty sure it's not SameSite specific, though.

Comment 12

[Mark Goodwin [:mgoodwin]]

(In reply to Mark Goodwin [:mgoodwin] from comment #11)
> ... cookies that are set via XHR ...

* via Fetch

Comment 13

[Mark Goodwin [:mgoodwin]]

Hi Kershaw, could you confirm my suspicion that this isn't a SameSite specific issue?

Comment 14

[Kershaw Chang [:kershaw]]

(In reply to Mark Goodwin [:mgoodwin] from comment #13)
> Hi Kershaw, could you confirm my suspicion that this isn't a SameSite
> specific issue?

I agree with you this looks like not a SameSite specific issue.
But I'd like to make sure if the test document in comment #8 is the same case as the reporter encountered.

Hi Reporter,
Could you take a look at comment #8 and let us know what do you think?
Thanks.

Comment 15

[Kershaw Chang [:kershaw]]

I just found that I can reproduce the issue with the test html in comment #8.

Assign to myself.

Comment 16

[Kershaw Chang [:kershaw]]

:mgoodwin,

I found this is really a Samesite specific issue. 
With the test html in comment #8, the problem is that sometimes NS_IsSameSiteForeign(aChannel, aHostURI) [1] returns true and causes CookieService skip the Samesite cookie [2]. However, NS_IsSameSiteForeign should return false.
In the case that NS_IsSameSiteForeign returns true, I found that the triggeringPrincipal's uri of the channel [3] is "moz-nullprincipal:{52c34ec8-649b-7e46-ac1d-259c021ce957}", so IsThirdPartyChannel thinks this is a cross origion request.

Honestly, I have no idea why we could get a moz-nullprincipal uri. I think maybe we should treat this as an empty uri and this could also fix this bug.

What do you think?




[1] https://searchfox.org/mozilla-central/rev/c0b26c40769a1e5607a1ae8be37fe64df64fc55e/netwerk/cookie/CookieServiceChild.cpp#580
[2] https://searchfox.org/mozilla-central/rev/c0b26c40769a1e5607a1ae8be37fe64df64fc55e/netwerk/cookie/CookieServiceChild.cpp#408
[3] https://searchfox.org/mozilla-central/rev/c0b26c40769a1e5607a1ae8be37fe64df64fc55e/netwerk/base/nsNetUtil.cpp#2180

Comment 17

[Mark Goodwin [:mgoodwin]]

(In reply to Kershaw Chang [:kershaw] from comment #16)
> Honestly, I have no idea why we could get a moz-nullprincipal uri. I think
> maybe we should treat this as an empty uri and this could also fix this bug.

I'm also not clear on why we'd be getting a moz-nullprincipal URI; probably :ckerschb would have more of an idea.

> What do you think?

Surely treating this as an empty URI means that we'd be missing the check on *actual* cross site requests?

Comment 18

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Kershaw Chang [:kershaw] from comment #16)
> I found this is really a Samesite specific issue. 
> With the test html in comment #8, the problem is that sometimes
> NS_IsSameSiteForeign(aChannel, aHostURI) [1] returns true and causes
> CookieService skip the Samesite cookie [2].

What does 'sometimes' mean in that case? Same input variables or different input variables? Or is the code racy?

> However, NS_IsSameSiteForeign should return false.
> In the case that NS_IsSameSiteForeign returns true, I found that the
> triggeringPrincipal's uri of the channel [3] is
> "moz-nullprincipal:{52c34ec8-649b-7e46-ac1d-259c021ce957}", so
> IsThirdPartyChannel thinks this is a cross origion request.
> 
> Honestly, I have no idea why we could get a moz-nullprincipal uri. I think
> maybe we should treat this as an empty uri and this could also fix this bug.

A moz-nullprincipal usually reflects the security context of a opaque origin (e.g data: URI) or a sandboxed iframe. The NullPrincipal (which holds a URI of moz-nullprincipal:{...}) is only same-origin within itself and with nothing else.

We sometimes use a NullPrincipal as a secure default in case we can't query the correct Principal for whatever reasons. E.g. when deserializing Principals and something goes wrong.

If we have a live testcase then we could certainly trace this on down. Only looking at the testcode in comment 8 doesn't provide enough context to say where exactly the problem happens.

Comment 19

[Mark Goodwin [:mgoodwin]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #18)
> What does 'sometimes' mean in that case? Same input variables or different
> input variables? Or is the code racy?

The code could be racy, or it could be something platform specific (I suspect the former but I could be wrong); when I was testing, I saw the problem on OS X but not Windows. I've since tried on Windows again and I seem to have the issue there too. Kershaw what OS were you testing on?

Comment 20

[Kershaw Chang [:kershaw]]

(In reply to Mark Goodwin [:mgoodwin] from comment #19)
> (In reply to Christoph Kerschbaumer [:ckerschb] from comment #18)
> > What does 'sometimes' mean in that case? Same input variables or different
> > input variables? Or is the code racy?
> 
> The code could be racy, or it could be something platform specific (I
> suspect the former but I could be wrong); when I was testing, I saw the
> problem on OS X but not Windows. I've since tried on Windows again and I
> seem to have the issue there too. Kershaw what OS were you testing on?

I was testing on OSX.

Let me show you how do I reproduce this.
1. Run firefox with a whole new profile.
2. Navigate to https://computerist.org/1478280.html.
3. Open the console and click the "Samesite" button.
   You should see "document.cookie is: foo=bar; document is: https://computerist.org/1478280.html". 
4. Close the firefox and start it again.
5. Repeat step 2 and 3.
   You should see "document.cookie is:  document is: https://computerist.org/1478280.html".
   The cookie is missing because th request "https://computerist.org/same_site/test.txt" is loaded from cache and we don't cache sookie headers, so I think this is normal.
6. Leave the "1478280.html" tab open and go to preferences and clear all the caches and cookies.
7. Go back to "1478280.html" tab and clear the "Samesite" button again.
   You can see that the cookie string "foo=bar" is still missing, but the cookie string "wibble=blah" for non-samesite is still there.

I can 100% reproduce with the steps above. I think maybe the NullPrincipal is caused because I cleared the cookies and caches at step 6? Anyway, I'll take a look why we got a NullPrincipal in channel's loadinfo. Or maybe someone knows?

The reason why I use "sometimes" is that I think this is not the only case that a channel having a null triggering principal. Maybe we have more cases.

Comment 21

[Kershaw Chang [:kershaw]]

I finally figured out where the moz-nullprincipal uri was coming from. It's actually from [1].
Aparently, some documents could be loaded with null triggering principal.

So, the problem is: If a document is loaded with a null triggering principal and it wants to read a Samesite cookie, cookie service will not return the cookie back because |isSameSiteForeign| at [2] is true.
The reason why NS_IsSameSiteForeign returns true is that we use moz-nullprincipal [3] to check if this is a third party channel.

I am not sure how to deal with moz-nullprincipal uri in NS_IsSameSiteForeign.
Christoph, what do you think?


[1] https://searchfox.org/mozilla-central/rev/55895c49f55073d82d977cb74ec1d3a71ae4b25f/browser/components/sessionstore/ContentRestore.jsm#200-210
[2] https://searchfox.org/mozilla-central/rev/55895c49f55073d82d977cb74ec1d3a71ae4b25f/netwerk/cookie/CookieServiceChild.cpp#580
[3] https://searchfox.org/mozilla-central/rev/55895c49f55073d82d977cb74ec1d3a71ae4b25f/netwerk/base/nsNetUtil.cpp#2180

Comment 22

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Kershaw Chang [:kershaw] from comment #21)
> I finally figured out where the moz-nullprincipal uri was coming from. It's
> actually from [1].
> Aparently, some documents could be loaded with null triggering principal.

Thanks for digging into the problem and finding the root cause. So it seems that Bug 1490257 regressed that. Within that bug we replaced the 'null' fallback for the triggeringPrincipal and created an explicit NullPrincipal. Now in the old world, the 'null' was passed all the way to docshell and got translated into a SystemPrincipal.

You could check if that causes the problem by changing the following line within ContentRestore.jsm

let triggeringPrincipal = loadArguments.triggeringPrincipal
                          ? Utils.deserializePrincipal(loadArguments.triggeringPrincipal)
-                         : Services.scriptSecurityManager.createNullPrincipal({});
+                         : Services.scriptSecurityManager.getSystemPrincipal());

Now moving forward, it would be interesting to figure out why there was no triggeringPrincipal set on the loadArguments. I guess we should file a follow up and investigate. If the above change fixes the problem then I think we should just perform that change, so we can uplift that change and then follow up on it.

Comment 23

[Jonathan Kingston [:jkt] he/him]

It might be worth that we aren't having a serialization issue with the correct principal:
https://searchfox.org/mozilla-central/rev/55895c49f55073d82d977cb74ec1d3a71ae4b25f/toolkit/modules/sessionstore/SessionHistory.jsm#450

Comment 24

[Kershaw Chang [:kershaw]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #22)
> (In reply to Kershaw Chang [:kershaw] from comment #21)
> > I finally figured out where the moz-nullprincipal uri was coming from. It's
> > actually from [1].
> > Aparently, some documents could be loaded with null triggering principal.
> 
> Thanks for digging into the problem and finding the root cause. So it seems
> that Bug 1490257 regressed that. Within that bug we replaced the 'null'
> fallback for the triggeringPrincipal and created an explicit NullPrincipal.
> Now in the old world, the 'null' was passed all the way to docshell and got
> translated into a SystemPrincipal.
> 
> You could check if that causes the problem by changing the following line
> within ContentRestore.jsm
> 
> let triggeringPrincipal = loadArguments.triggeringPrincipal
>                           ?
> Utils.deserializePrincipal(loadArguments.triggeringPrincipal)
> -                         :
> Services.scriptSecurityManager.createNullPrincipal({});
> +                         :
> Services.scriptSecurityManager.getSystemPrincipal());
> 
> Now moving forward, it would be interesting to figure out why there was no
> triggeringPrincipal set on the loadArguments. I guess we should file a
> follow up and investigate. If the above change fixes the problem then I
> think we should just perform that change, so we can uplift that change and
> then follow up on it.

Actually, the triggeringPrincipal was set in the loadArguments. The null principal is coming from [1].
The problem in this bug is fixed with the following change.

--- a/browser/components/newtab/lib/PlacesFeed.jsm
+++ b/browser/components/newtab/lib/PlacesFeed.jsm
@@ -237,17 +237,17 @@ class PlacesFeed {
   }

   /**
    * Open a link in a desired destination defaulting to action's event.
    */
   openLink(action, where = "", isPrivate = false) {
     const params = {
       private: isPrivate,
-      triggeringPrincipal: Services.scriptSecurityManager.createNullPrincipal({}),
+      triggeringPrincipal: Services.scriptSecurityManager.getSystemPrincipal(),
     };

     // Always include the referrer (even for http links) if we have one
     const {event, referrer, typedBonus} = action.data;
     if (referrer) {
       params.referrerPolicy = Ci.nsIHttpChannel.REFERRER_POLICY_UNSAFE_URL;
       params.referrerURI = Services.io.newURI(referrer);
     }

However, I am not suref if the correct way to fix this bug is changing all the places where triggeringPrincipal is set to null principal. Or should we just add another check for moz-nullprincipal uri in NS_IsSameSiteForeign?

What do you think, Christoph?


[1] browser/components/newtab/lib/PlacesFeed.jsm#245

Comment 25

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Kershaw Chang [:kershaw] from comment #24)
> However, I am not suref if the correct way to fix this bug is changing all
> the places where triggeringPrincipal is set to null principal. Or should we
> just add another check for moz-nullprincipal uri in NS_IsSameSiteForeign?

First, it's good that this change fixes the problem. I think we should just land that which converts that particular change introduced within Bug 1490257. And then we need to uplift that change, right?

Second, I am slightly confused by what you mean with 'changing all the places' - what are all the places? Are there similar problems that I am not aware of or is it just that particular one?

Third, we need to file a follow up bug which provides the correct triggeringPrincipal within PlacesFeed.jsm. Could you please file that and ni? jonathan kingston on it. He can help with fixing.

Thanks!

Comment 26

[Kershaw Chang [:kershaw]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #25)
> (In reply to Kershaw Chang [:kershaw] from comment #24)
> > However, I am not suref if the correct way to fix this bug is changing all
> > the places where triggeringPrincipal is set to null principal. Or should we
> > just add another check for moz-nullprincipal uri in NS_IsSameSiteForeign?
> 
> First, it's good that this change fixes the problem. I think we should just
> land that which converts that particular change introduced within Bug
> 1490257. And then we need to uplift that change, right?
> 
> Second, I am slightly confused by what you mean with 'changing all the
> places' - what are all the places? Are there similar problems that I am not
> aware of or is it just that particular one?
> 
> Third, we need to file a follow up bug which provides the correct
> triggeringPrincipal within PlacesFeed.jsm. Could you please file that and
> ni? jonathan kingston on it. He can help with fixing.
> 
> Thanks!

I'd rather fix the code in PlacesFeed.jsm in this bug since I am sure doing this can fix the problem here.

I can't really tell if the changes made in Bug 1490257 could cause another potential problem. I think we should file a follow up bug for this.

Comment 27

[Kershaw Chang [:kershaw]]

Attachment: Bug 1478280 - Provides the correct triggeringPrincipal within PlacesFeed.jsm

When a document opened with a null triggeringPrincipal tries to read samesite cookies, this will fail because NS_IsSameSiteForeign returns true. To fix this, we have to provide the correct triggeringPrincipal at the first place where the document is loaded.

Comment 28

[Kershaw Chang [:kershaw]]

Given that the root cause of this bug is about triggeringPrincipal, it looks like I am not the right one to fix this bug.
So, set the componment to dom:security and unassign myself.

Comment 29

[rutgerstorm]

We are kinda experiencing a similiar problem but only when you get redirected from another domain to the domain which is setting the cookie.

So I can reproduce this if I have domain A (http://redirect.test.local) redirect by 302 to domain B (http://otherdomain.bug.local). After arriving on the http://otherdomain.bug.local we do a XHR call to an api which returns a Set-Cookie: test=test; domain=otherdomain.bug.local; path=/; samesite=strict then document.cookie will be empty. If you go directly to http://otherdomain.bug.local then we don't experience the problem.

I have a HAR file which shows the most simple flow I could make to reproduce it. If that's useful information please let me know and I will attach it.
I tested this with version 67.0 up to nightly.

Since I see nothing in this issue saying anything about redirects ect. is this the same bug or do I need to log a different one?

Comment 30

[Christoph Kerschbaumer [:ckerschb]]

(In reply to rutgerstorm from comment #29)

Since I see nothing in this issue saying anything about redirects ect. is this the same bug or do I need to log a different one?

I am pretty sure we are facing the same underlying issue as outlined in comment 27 and 28.

Jonathan, you have been working on providing the correct triggeringPrincipal for all top-level loads (Bug 1333030 and dependencies). It seems that in some cases we are falling back to NullPrincipal which causes to break same-site strict cookies.

Could you please take a look - I think we should fix the underlying triggeringPrincipal problem.

Comment 31

[Christoph Enzmann]

We have the same issue. We use an angularjs webapp. When samesite was set to strict angularjs was unable the read the cookie. With samesite set to lax everything works fine. It doesn't occur when working with a localhost uri. This is 100% reproducible in our code.

Comment 32

[Odin Hørthe Omdal]

We are also seeing this. Though there are no XHR requests involved (so might be different, though sounds very similar). Angularjs webapp.

We do a redirect from A (sub.redirect-from.com) to B (sub.redirect-to.com), inside redirect-to.com we do:

document.cookie = 'test_cookie=true; domain=sub.redirect-to.com; path=/; samesite=strict; secure';
document.location.reload()

The reload does NOT send the cookie to the backend, and this cookie is ALSO not available in document.cookie causing this code to just infinitely reload. A user-reload will not fix it, but going to the URL bar and pressing enter does allow the document.cookie to be read.

Comment 33

[Tom S]

Christoph, is Jonathan unavailable?

Comment 34

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Tom Schuster [:evilpie] from comment #33)

Christoph, is Jonathan unavailable?

I just had a quick sync with Jonathan, he will take a look asap, but probably not before next week. Sorry for the delay here.

Comment 35

[Jonathan Kingston [:jkt] he/him]

From memory I think this was a triggering principal issue but I never got around to fixing it. Removing NI sorry.