1469916 - (CVE-2018-12402) SameSite Cookies leakage via "Save Page As..."

Status: VERIFIED FIXED

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

Description

[1lastBr3ath]

Attachment: Contains php files for easier reproduction

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/67.0.3396.87 Safari/537.36

Steps to reproduce:

1. Download the attached file and host it locally under "save" directory
2. Make request.log world writable (i.e. chmod 666 request.log)
3. Run the following bash command to follow requests coming in;
$ tail -f request.log| sed -En '/GET|Cookie:/p'
3. Open http://localhost/save/cookies.php?url=http://127.0.0.1/save/save.php
4. Observe cookies received, only cookies with no SameSite attributes are received
5. Save the page with Ctrl+S or via Context menu > Save Page As...
6. Observe received cookies, all cookies are received

Note:
Only those tags  (with external reference) in "save.php" leak cookies


Actual results:

SameSite cookies sent for crossdomain requests


Expected results:

SameSite cookies shouldn't have been sent

Comment 1

[:Gijs (he/him)]

Mark, just flagging this up to you as well... :-)

Comment 2

[Daniel Veditz [:dveditz]]

file: urls have no referrer, I wonder if we mess up on whether the sub-resources are 3rd party requests or not? I'm not sure how we could (comparing "" to any host shouldn't be true), but if that's the problem we might also be sending cookies when the user has disabled 3rd-party cookies.

Comment 3

[1lastBr3ath]

I don't think that's what the issue is because if I browse file URL directly, it doesn't send SameSite cookies at all. It only happens when saving the page and only with those 4 tags.

Comment 4

[:Gijs (he/him)]

This is all because nsIWebBrowserPersist is a giant mess. We have other bugs about this. They're all not particularly severe, but there's a bunch of them and we should just clean it up. I'll give this a poke.

Comment 5

[Phabricator Automation]

Attachment: Bug 1469916 - use principals in webbrowserpersist code, r?ckerschb,jkt

Bug 1469916 - use principals in webbrowserpersist code to ensure proper OA, samesite cookie behavior, etc.

Comment 6

[:Gijs (he/him)]

So... I wrote up a fairly ugly big patch to try to teach webbrowserpersist some manners about principals. I think this will also fix some other issues we have on file, like how we fetch URLs outside of containers when saving pages from a container tab etc...

The problem I'm having is that although with the patch we pass a loading principal to the load triggered by webbrowserpersist, it seems we end up calling https://dxr.mozilla.org/mozilla-central/rev/681eb7dfa324dd50403c382888929ea8b8b11b00/netwerk/protocol/http/HttpBaseChannel.cpp#3424-3439

(HttpBaseChannel::AddCookiesToRequest), which via GetCookieStringFromHttp and GetCookieStringCommon calls

  mThirdPartyUtil->IsThirdPartyChannel(aChannel, aHostURI, &isForeign);

with aChannel == e.g. the channel for the alternate stylesheet in the testcase, and aHostURI... is the URI for that channel! Understandably, the third party util code then declares that we can send samesite cookies, because the host uri matches the channel URI. I don't really see how this is happening yet works fine "normally", ie I'm not sure what webbrowserpersist is doing differently/wrong in how it creates a channel (once it has a loading principal like with this patch). Mark/Christoph, can either of you take a look?

Comment 7

[:Gijs (he/him)]

Egh, didn't mean to unassign.

Comment 8

[Christoph Kerschbaumer [:ckerschb, back on Aug 10th}]

(In reply to :Gijs (he/him) from comment #6)
> So... I wrote up a fairly ugly big patch to try to teach webbrowserpersist
> some manners about principals. I think this will also fix some other issues
> we have on file, like how we fetch URLs outside of containers when saving
> pages from a container tab etc...

Thanks for updating this part of the code - I didn't like falling back to using the systemPrincipal within WebBrowserPersist in the first place. One tiny nit, could we call this principal "triggeringPrincipal", because that's what it really is in the end - it's the principal that triggered the load for those cases.

Bonus points for adding an assertion within saveURI: If saveURI gets passed a referrer, the triggeringPrincipal must be a CodebasePrincipal that matches the URI of the referrer. But we could do that in a follow up of course. Thomas is working on referrer policy, maybe I'll just file another bug and have him take a look.

> The problem I'm having is that although with the patch we pass a loading
> principal to the load triggered by webbrowserpersist, it seems we end up
> calling
> https://dxr.mozilla.org/mozilla-central/rev/
> 681eb7dfa324dd50403c382888929ea8b8b11b00/netwerk/protocol/http/
> HttpBaseChannel.cpp#3424-3439
> 
> (HttpBaseChannel::AddCookiesToRequest), which via GetCookieStringFromHttp
> and GetCookieStringCommon calls
> 
>   mThirdPartyUtil->IsThirdPartyChannel(aChannel, aHostURI, &isForeign);
> 
> with aChannel == e.g. the channel for the alternate stylesheet in the
> testcase, and aHostURI... is the URI for that channel!

Shouldn't aHostURI be the URI of the document in that case? Or is that only the problem for stylesheets that aHostURI matches the URI of the stylesheet itself? I couldn't find precisely the code where we end up calling |mThirdPartyUtil->IsThirdPartyChannel(aChannel, aHostURI, &isForeign);|, do you have a stacktrace for that? Or can you provide a DXR link? Then I could take another look.

Besides the principal changes here, I suppose that never worked in the first place and most likely we have a bug where aHostURI is the URI of the stylesheet instead of the document.

Comment 9

[:Gijs (he/him)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #8)
> (In reply to :Gijs (he/him) from comment #6)
> > The problem I'm having is that although with the patch we pass a loading
> > principal to the load triggered by webbrowserpersist, it seems we end up
> > calling
> > https://dxr.mozilla.org/mozilla-central/rev/
> > 681eb7dfa324dd50403c382888929ea8b8b11b00/netwerk/protocol/http/
> > HttpBaseChannel.cpp#3424-3439
> > 
> > (HttpBaseChannel::AddCookiesToRequest), which via GetCookieStringFromHttp
> > and GetCookieStringCommon calls
> > 
> >   mThirdPartyUtil->IsThirdPartyChannel(aChannel, aHostURI, &isForeign);
> > 
> > with aChannel == e.g. the channel for the alternate stylesheet in the
> > testcase, and aHostURI... is the URI for that channel!
> 
> Shouldn't aHostURI be the URI of the document in that case? Or is that only
> the problem for stylesheets that aHostURI matches the URI of the stylesheet
> itself? I couldn't find precisely the code where we end up calling
> |mThirdPartyUtil->IsThirdPartyChannel(aChannel, aHostURI, &isForeign);|, do
> you have a stacktrace for that? Or can you provide a DXR link? Then I could
> take another look.

I think it *should* be but it isn't, yeah. The DXR link for the IsTPC channel is:

https://dxr.mozilla.org/mozilla-central/rev/681eb7dfa324dd50403c382888929ea8b8b11b00/netwerk/cookie/nsCookieService.cpp#2034-2036

> Besides the principal changes here, I suppose that never worked in the first
> place and most likely we have a bug where aHostURI is the URI of the
> stylesheet instead of the document.

This would seem to be surprising to me, but I suppose it's possible. Should be easy to test?

Comment 10

[Christoph Kerschbaumer [:ckerschb, back on Aug 10th}]

(In reply to :Gijs (he/him) from comment #6)
> (HttpBaseChannel::AddCookiesToRequest), which via GetCookieStringFromHttp
> and GetCookieStringCommon calls
> 
>   mThirdPartyUtil->IsThirdPartyChannel(aChannel, aHostURI, &isForeign);
> 
> with aChannel == e.g. the channel for the alternate stylesheet in the
> testcase, and aHostURI... is the URI for that channel! Understandably, the
> third party util code then declares that we can send samesite cookies,
> because the host uri matches the channel URI. I don't really see how this is
> happening yet works fine "normally", ie I'm not sure what webbrowserpersist
> is doing differently/wrong in how it creates a channel (once it has a
> loading principal like with this patch). Mark/Christoph, can either of you
> take a look?

Hey Gijs,

I haven't debugged this, but I just started to look at the code and already realized, it's not the mThirdPartyUtil->IsThirdPartyChannel() check within GetCookieStringCommon, but the NS_IsSameSiteForeign() [0] that determines whether a request is foreign with respect to same-site cookies.

Now, NS_IsSameSiteForeign() only does special things for TYPE_DOCUMENT and TYPE_SUBDOCUMENT [1], but not for TYPE_OTHER. Question is, the channel we create within nsWebBrowserPersist::SaveURIInternal is that only for loads of TYPE_DOCUMENT, right? If so, then I guess we could change the type from TYPE_OTHER to TYPE_DOCUMENT and presumably NS_IsSameSiteForeign() returns the correct result then.

What do you think?

[0] https://dxr.mozilla.org/mozilla-central/source/netwerk/cookie/nsCookieService.cpp#2045
[1] https://dxr.mozilla.org/mozilla-central/source/netwerk/base/nsNetUtil.cpp#2117-2190

Comment 11

[:Gijs (he/him)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #10)
> (In reply to :Gijs (he/him) from comment #6)
> > (HttpBaseChannel::AddCookiesToRequest), which via GetCookieStringFromHttp
> > and GetCookieStringCommon calls
> > 
> >   mThirdPartyUtil->IsThirdPartyChannel(aChannel, aHostURI, &isForeign);
> > 
> > with aChannel == e.g. the channel for the alternate stylesheet in the
> > testcase, and aHostURI... is the URI for that channel! Understandably, the
> > third party util code then declares that we can send samesite cookies,
> > because the host uri matches the channel URI. I don't really see how this is
> > happening yet works fine "normally", ie I'm not sure what webbrowserpersist
> > is doing differently/wrong in how it creates a channel (once it has a
> > loading principal like with this patch). Mark/Christoph, can either of you
> > take a look?
> 
> Hey Gijs,
> 
> I haven't debugged this, but I just started to look at the code and already
> realized, it's not the mThirdPartyUtil->IsThirdPartyChannel() check within
> GetCookieStringCommon, but the NS_IsSameSiteForeign() [0] that determines
> whether a request is foreign with respect to same-site cookies.
> 
> Now, NS_IsSameSiteForeign() only does special things for TYPE_DOCUMENT and
> TYPE_SUBDOCUMENT [1], but not for TYPE_OTHER. Question is, the channel we
> create within nsWebBrowserPersist::SaveURIInternal is that only for loads of
> TYPE_DOCUMENT, right? If so, then I guess we could change the type from
> TYPE_OTHER to TYPE_DOCUMENT and presumably NS_IsSameSiteForeign() returns
> the correct result then.
> 
> What do you think?
> 
> [0]
> https://dxr.mozilla.org/mozilla-central/source/netwerk/cookie/
> nsCookieService.cpp#2045
> [1]
> https://dxr.mozilla.org/mozilla-central/source/netwerk/base/nsNetUtil.
> cpp#2117-2190

SaveURIInternal gets used both for the actual document and for anything required by the document. In this case the problematic request that has cookies is for a stylesheet <link> element, so I would think that a non-document type is correct... and I'd expect aHostURI to be the document URI, with the channel using the <link> element's URI, and thus those 2 not matching...

Comment 12

[Christoph Kerschbaumer [:ckerschb, back on Aug 10th}]

(In reply to :Gijs (he/him) from comment #11)
> SaveURIInternal gets used both for the actual document and for anything
> required by the document. In this case the problematic request that has
> cookies is for a stylesheet <link> element, so I would think that a
> non-document type is correct... and I'd expect aHostURI to be the document
> URI, with the channel using the <link> element's URI, and thus those 2 not
> matching...

It took me a while to page all of that stuff back in. Here is the info you were looking for:

1) Really the information whether a channel is third party or not does not 'really' depend on the provided HostURI, but is computed from the loadinfo. In particular, the URI of the loadingPrincipal (which refelcts the actual hostURI) is compared to the channelURI within IsThirdPartyChannel here [1].

2) What you pointed out in comment 6 however does not really make sense to me, we are passing mURI as the hostURI to GetCookieStringFromHttp() within the HttpBaseChannel [2]. Please note that channel::GetURI() also return mURI, so the hostURI and the channelURI are always identical. In my opinion there is a bug here and we should pass mDocumentURI instead of mURI within HttpBaseChannel [2].

Honza (I guess he is out) or Valentin, what do you think regarding point (2) here?


[1] https://dxr.mozilla.org/mozilla-central/source/dom/base/ThirdPartyUtil.cpp#224-239
[2] https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/HttpBaseChannel.cpp#3436

Comment 13

[Valentin Gosu [:valentin] {{ PTO 3 July - 3 August }}]

I think you're right. Even in the IDL it's specified that first argument is supposed to be the URI of the document.

https://searchfox.org/mozilla-central/rev/14cb8f1c238735ba1abe18ad44e39808983c2572/netwerk/cookie/nsICookieService.idl#126-127,143

Comment 14

[1lastBr3ath]

Attachment: poc.html

Sorry to interrupt again but the issue doesn't seem to affect only those 4 tags, rather all tags. It's hard to reproduce consistently. The attached HTML should trigger requests for all tags when saved. Please refer to reproduction steps given in my initial report.

Comment 15

[:Gijs (he/him)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #12)
> 2) What you pointed out in comment 6 however does not really make sense to
> me, we are passing mURI as the hostURI to GetCookieStringFromHttp() within
> the HttpBaseChannel [2]. Please note that channel::GetURI() also return
> mURI, so the hostURI and the channelURI are always identical. In my opinion
> there is a bug here and we should pass mDocumentURI instead of mURI within
> HttpBaseChannel [2].

So the next question I'm wondering about is... why isn't this broken all the time? i.e. step 4 in comment 0 is also what I see - when the page is loaded without using 'save page as' the correct cookies are sent. But the URLs should be the same, right? (Unfortunately I don't have a build to hand so I can't easily check at the moment - will try to do this later today or tomorrow if nobody else gets there before me...)

Comment 16

[:Gijs (he/him)]

(In reply to :Gijs (he/him) from comment #15)
> (In reply to Christoph Kerschbaumer [:ckerschb] from comment #12)
> > 2) What you pointed out in comment 6 however does not really make sense to
> > me, we are passing mURI as the hostURI to GetCookieStringFromHttp() within
> > the HttpBaseChannel [2]. Please note that channel::GetURI() also return
> > mURI, so the hostURI and the channelURI are always identical. In my opinion
> > there is a bug here and we should pass mDocumentURI instead of mURI within
> > HttpBaseChannel [2].
> 
> So the next question I'm wondering about is... why isn't this broken all the
> time? i.e. step 4 in comment 0 is also what I see - when the page is loaded
> without using 'save page as' the correct cookies are sent. But the URLs
> should be the same, right? (Unfortunately I don't have a build to hand so I
> can't easily check at the moment - will try to do this later today or
> tomorrow if nobody else gets there before me...)

As usual the answer to seemingly confusing states is more mundane than one might expect.

First off, the reason we block cookies in the normal pageload case isn't the checks referenced here so far, but these:

https://dxr.mozilla.org/mozilla-central/rev/681eb7dfa324dd50403c382888929ea8b8b11b00/dom/base/ThirdPartyUtil.cpp#226-235

copy-paste for convenience:

  if (!doForce) {
    if (nsCOMPtr<nsILoadInfo> loadInfo = aChannel->GetLoadInfo()) {
      parentIsThird = loadInfo->GetIsInThirdPartyContext();
      if (!parentIsThird &&
          loadInfo->GetExternalContentPolicyType() != nsIContentPolicy::TYPE_DOCUMENT) {
        // Check if the channel itself is third-party to its own requestor.
        // Unforunately, we have to go through the loading principal.
        nsCOMPtr<nsIURI> parentURI;
        loadInfo->LoadingPrincipal()->GetURI(getter_AddRefs(parentURI));
        rv = IsThirdPartyInternal(channelDomain, parentURI, &parentIsThird);
        if (NS_FAILED(rv))
          return rv;
      }

... and then we return true (for "is this a third party channel") if parentIsThird is true.

Now, the reason this doesn't help with 'save as' is also trivial, it turns out - doForce is true, so we skip this check.

Why? Well, nsWebBrowserPersist code has had this flag since https://bugzilla.mozilla.org/show_bug.cgi?id=437174 got fixed (so that people could use the "disable third party cookies" pref and still get cookies in background requests, ie requests without a window), 9 years ago, and since then we've not really updated this stuff...

The last few comments seem to suggest this flag was no longer necessary a few months later. :-\

In any case, I would expect our new principal work to mean that you shouldn't need a docshell for us to make the correct third party cookie decisions, and clearly here the flag is messing with the desired outcome. I'll experiment with fixing this, rather than fixing the mURI/mDocumentURI confusion in the cookie code (which I expect has more fallout - there's not a lot of in-tree usage of the THIRD_PARTY_FORCE_ALLOW flag, it looks like.

Comment 17

[:Gijs (he/him)]

(In reply to :Gijs (he/him) from comment #16)
> In any case, I would expect our new principal work to mean that you
> shouldn't need a docshell for us to make the correct third party cookie
> decisions, and clearly here the flag is messing with the desired outcome.
> I'll experiment with fixing this, rather than fixing the mURI/mDocumentURI
> confusion in the cookie code (which I expect has more fallout - there's not
> a lot of in-tree usage of the THIRD_PARTY_FORCE_ALLOW flag, it looks like.

This turns out to *also* not be trivial to just get rid of this flag everywhere, because we use a system principal for some of these loads, and the third party cookie code tries to get a URI off it, which fails and therefore leaves the cookie code deciding this is a third-party load (safe defaults) and doesn't send cookies (if you disable third party cookies, per bug 437174). I tested this with the InstallTrigger code by uploading a self-hosted add-on on AMO and disabling 3rd party cookies, and then manually running InstallTrigger.install() fails. It seems it'd need to have its principal fixed, too, in order for that to work.

The original rationale here (cf. bug 437174 comment 27 and further, bug 421494) seems to also include the idea that system loads (sometimes from add-ons) can't be trusted to always send third party cookies. I don't know how that works these days, I don't see any webextension-related code using this flag... It's also interesting that we made the default behavior break all cookies for privileged requests, but privileged requests can work around with a flag (which add-ons could of course also use if they wanted...) which then results in *all* cookies being sent, even the ones it really shouldn't send. Feels like the cure is worse than the disease. :-(

Thankfully, favicons (which is a case that's cited repeatedly in the discussion about sending samesite cookies) is being fixed in bug 1453751. So with that, I think the "force cookies" flag should be kill-able.

In any case... hopefully with the fixes in this patch I can ensure that we pass a non-system-principal in all the browser callsites of webbrowserpersist, and get rid of the flag there. Once patches in this bug are finished, reviewed and landed, I'll file a follow-up bug to get the cookie service to use mDocumentURI, and a follow-up bug to kill off this footgun flag - pretty sure we can just pass better principals instead of system principal in the other callsites eventually, and then we can kill the flag.

Comment 18

[:Gijs (he/him)]

Comment on attachment 8987073 [details]
Bug 1469916 - use principals in webbrowserpersist code, r?ckerschb,jkt

Setting flags here to make sure people see the requests.

This should be ready for review now. :-)

Comment 19

[Christoph Kerschbaumer [:ckerschb, back on Aug 10th}]

(In reply to :Gijs (he/him) from comment #17)
> In any case... hopefully with the fixes in this patch I can ensure that we
> pass a non-system-principal in all the browser callsites of
> webbrowserpersist, and get rid of the flag there. Once patches in this bug
> are finished, reviewed and landed, I'll file a follow-up bug to get the
> cookie service to use mDocumentURI, and a follow-up bug to kill off this
> footgun flag - pretty sure we can just pass better principals instead of
> system principal in the other callsites eventually, and then we can kill the
> flag.

Thanks Gijs, that sounds like a good path forward to me. It's still surprising to me that using mURI instead of mDocumentURI has been there for so many years.

Comment 20

[Phabricator Automation]

Comment on attachment 8987073 [details]
Bug 1469916 - use principals in webbrowserpersist code, r?ckerschb,jkt

Christoph Kerschbaumer [:ckerschb] has approved the revision.

https://phabricator.services.mozilla.com/D1766

Comment 21

[Christoph Kerschbaumer [:ckerschb, back on Aug 10th}]

Comment on attachment 8987073 [details]
Bug 1469916 - use principals in webbrowserpersist code, r?ckerschb,jkt

Phabricator and bugzilla r+ ;-)

Comment 22

[Phabricator Automation]

Comment on attachment 8987073 [details]
Bug 1469916 - use principals in webbrowserpersist code, r?ckerschb,jkt

Jonathan Kingston [:jkt] has approved the revision.

https://phabricator.services.mozilla.com/D1766

Comment 23

[:Gijs (he/him)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/be11a32900298eb6fd4d18ad21b9a699995254c3

Comment 24

[:Gijs (he/him)]

Filed bug 1473012 to remove the cookie forcing flags from docshell/webnavigation, and bug 1473011 to change the http/cookie code to use mDocumentURI.

Comment 25

[:Gijs (he/him)]

(In reply to :Gijs (he/him) from comment #23)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> be11a32900298eb6fd4d18ad21b9a699995254c3

Backed out for orange in any tests that touched lightweight themes (forgot to update LWTManager which calls nsIWebBrowserPersist.saveURI() ) as well as browser_jsonview_save_json.js (which ended up trying to use a null principal in the parent for a blob URI created in the child for that null principal), and clean-up errors from the new tests that were windows only and seemed to be a race condition with deleting the temporarily-saved file created by the test. Fixed, pushed to try ( https://treeherder.mozilla.org/#/jobs?repo=try&revision=1e4b3c5f27825008069bf58cd1e6d96db6e01c64 - green, except for the mochitest-plain-1 chunks that are from pre-existing orange from another cset on inbound - forgot to rebase onto m-c for the trypush).

Re-pushing:

https://hg.mozilla.org/integration/mozilla-inbound/rev/9a2b02fe351bd65f6850a5c80b91dd0eec4a878a

Also, I think this will fix bug 1463365 so marking that as a dep.

Comment 26

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

https://hg.mozilla.org/mozilla-central/rev/9a2b02fe351b

Comment 27

[1lastBr3ath]

Is this eligible for bounty? And CVE?

Comment 28

[Ryan VanderMeulen (PTO, back July 27)]

Gijs, I assume we want to let this ride the trains? :)

(In reply to 1lastBr3ath from comment #27)
> Is this eligible for bounty? And CVE?

If you want to nominate this bug for bounty consideration, please follow the directions from the page below:
https://www.mozilla.org/security/client-bug-bounty/

Comment 29

[:Gijs (he/him)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #28)
> Gijs, I assume we want to let this ride the trains? :)

Yeah, there was enough fallout (that should be fixed now...) which didn't trigger any unittest failures, and it's a big enough patch, that I'd be pretty wary of uplifting this, esp. given it's sec-low.

I could be convinced about taking a roll-up of all of this on ESR as a correctness fix once this has made it to release without incidents. I *think* I set the flags right for this, let me know if you need anything else for that.

Comment 30

[1lastBr3ath]

I don't really understand what you guys are talking about. Should I assume that it's not eligible because it's sec-low?

Comment 31

[Ryan VanderMeulen (PTO, back July 27)]

(In reply to :Gijs (he/him) from comment #29)
> I could be convinced about taking a roll-up of all of this on ESR as a
> correctness fix once this has made it to release without incidents. I
> *think* I set the flags right for this, let me know if you need anything
> else for that.

Yes, fix-optional will keep it on the radar.

(In reply to 1lastBr3ath from comment #30)
> I don't really understand what you guys are talking about. Should I assume
> that it's not eligible because it's sec-low?

That's not a decision Gijs or myself makes. Follow the directions linked from comment 28 if you want bounty consideration.

Comment 32

[Daniel Veditz [:dveditz]]

Does not meet the severity requirements for the bug bounty.

Comment 33

[Cristian Craciun]

Hello, 

I'm the QA assigned to verify this issue and I've encountered some difficulties when trying to test it. I've followed the steps from the description but I don't have any information written in the request.log file even though I've made the file writable. 

Could you please provide some extra information regarding the STR or can you aid into verifying the fix?

Comment 34

[1lastBr3ath]

Hi,
I'm not sure why it wouldn't work. Are you using same Firefox that I and others used? If not, probably, it's already fixed.
Or can you please check your server log for any errors? Or you can also use requestbin (https://requestbin.fullcontact.com/) to log requests and see if it gets the cookies or receives any requests at all.

Let me know if you need any further help.
Thanks,
Prakash

Comment 35

[Ryan VanderMeulen (PTO, back July 27)]

Did you want to consider taking this for ESR 60.3 or wait until the 60.4 cycle?

Comment 36

[:Gijs (he/him)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #35)
> Did you want to consider taking this for ESR 60.3 or wait until the 60.4
> cycle?

Hm. Let's wait for now. Note that the dep tree of this bug is significant, we'd want to land this + bug 1473509 + bug 1473507 + bug 1487263

Comment 37

[Emil Ghitta, QA [:emilghitta]]

Hi 1lastBr3ath,

We didn't managed to reproduce this issue (in order to confirm if this is fixed or not). Could you help us by confirming that this is fixed using Firefox beta 63 ? 

Thank you!

Comment 38

[1lastBr3ath]

Yeah, I cannot reproduce it either, must have been fixed. Tested and verified on Windows 10, Firefox 63.0 beta.

Comment 39

[:Gijs (he/him)]

Marking verified per comment #38.

Comment 40

[Liz Henry (:lizzard)]

I'd rather let this wait for the next major ESR. Unless someone wants to argue strongly for taking all these patches from here and the ones mentioned in comment 36 for ESR 60.4.0.

Comment 41

[Jorg K (CEST = GMT+2)]

Gijs, did you mean to remove bug 1581253 from the list of regressions? You took off bug 1519732 which was certainly caused here.
EDIT: Scrap that, you did add bug 1519732, but it's crossed out since we fixed it. Sorry about the noise.

Comment 42

[:Gijs (he/him)]

(In reply to Jorg K (GMT+2) (reduced availability 14-19 of Sept.) from comment #41)

Gijs, did you mean to remove bug 1581253 from the list of regressions? You took off bug 1519732 which was certainly caused here.
EDIT: Scrap that, you did add bug 1519732, but it's crossed out since we fixed it. Sorry about the noise.

Indeed, though I now think it's sufficiently clear that though the dialog in question was broken earlier by this bug (which is/was filed and fixed in bug 1519732), the new incarnation was not in fact a regression from this bug, so I'm going to remove the newer bug after all. :-)