1748693 - Cookie without Secure attribute not sent over HTTP when set from HTTPS page

Status: VERIFIED FIXED

(Core :: Networking, defect, P1)

Description

[Michael Cordover [:mjec]]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:96.0) Gecko/20100101 Firefox/96.0

Steps to reproduce:

This bug was observed on an internal network, so I've replaced our corporate network domain with example.com. The true corporate domain is also on a registered <something>.com domain.

This behavior is new in Firefox 96, and not reproducible in Firefox 95.0.2. I can reproduce it on Linux and MacOS. Chrome 96 on Linux does not have this behavior.

Steps to reproduce:

1. Visit https://auth.example.com, which sets two cookies with the following response headers:

Set-Cookie: auth=value; Domain=.example.com; Expires=Thu, 06-Jan-2022 05:42:55 GMT; HttpOnly; Path=/

Set-Cookie: auth.secure=value; Domain=.example.com; Expires=Thu, 06-Jan-2022 05:42:55 GMT; Secure; HttpOnly; Path=/; SameSite=None

2. Visit http://site.example.com/

If this is not intended behavior and it would be helpful to do so, I can set up a public site to reproduce this.

Actual results:

No cookie request header is sent to http://site.example.com/

Expected results:

The non-secure cookie ("auth") should be sent with the request to http://site.example.com/

Comment 1

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

The Bugbug bot thinks this bug should belong to the 'Core::Networking' component, and is moving the bug to that component. Please revert this change in case you think the bot is wrong.

Comment 2

[Kershaw Chang [:kershaw]]

Niklas, could you take a look?
Thanks.

Comment 3

[Niklas]

This is most likely a bug in our schemeful samesite implementation.

With schemeful samesite enabled, the navigation to http://site.example.com/ is considered cross-site as the schemes don't match. The auth cookie defaults to SameSite=Lax and should be send as long as the request to http://site.example.com/ is a top-level navigation.

Our implementation compares schemes in two locations:

1. https://searchfox.org/mozilla-central/rev/6b4e19ad33650fdf9cd8529cd68eeb98bff1b935/netwerk/cookie/CookieCommons.cpp#618
2. https://searchfox.org/mozilla-central/rev/6b4e19ad33650fdf9cd8529cd68eeb98bff1b935/netwerk/cookie/CookieCommons.cpp#506

[1] is responsible for this bug as it does not account for the SameSite value leading cookies to be ignored here.

Comment 4

[Niklas]

Attachment: Bug 1748693: Add test r=ckerschb!

Comment 5

[Ksenia Berezina [:ksenia]]

We've got a report in https://github.com/webcompat/web-bugs/issues/97492, where an issue with http://auction.co.kr/ seems to be similar to the behaviour described in this bug.

Comment 6

[Tom S. (please needinfo tschuster)]

See also https://www.reddit.com/r/firefox/comments/s3iych/south_korea_cant_sign_in_to_some_websites_after/. This seems to affect a number of Korean websites.

Comment 7

[Thomas Wisniewski [:twisniewski]]

Ben, do you have any insights here? Should we match Chrome for now?

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Is this a pref we can flip remotely via Normandy?

Comment 9

[Benjamin VanderSloot [:bvandersloot]]

(In reply to Thomas Wisniewski [:twisniewski] from comment #7)

Ben, do you have any insights here? Should we match Chrome for now?

Bringing in the engineers that made the pref flip.

Comment 10

[Daniel Veditz [:dveditz]]

Thomas: the regressing bug was intended to make Firefox match Chrome's behavior (for the last year-plus), but as Niklas pointed out our implementation missed the mark. We can back this out (or pref-flip) which will revert to a more relaxed behavior which should make these sites work, but it won't "match Chrome".

Is this a pref we can flip remotely via Normandy?

Yes, flip network.cookie.sameSite.schemeful back to false

There were two other SameSite-related prefs changed in 96 but unless we know we've implemented them incorrectly and are breaking sites that "work on Chrome" I'd prefer to leave those alone for now. Like this "schemeful" change, these were all intended to make Firefox match the (updated) spec and match Chrome's behavior.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Clearly not an S3 if it's blocking rollout.

Comment 12

[Daniel Veditz [:dveditz]]

This is broken because we're comparing schemes out of context, as Niklas pointed out in comment 3. The "SchemefulSameSite" pref means just what it says -- it should only care about schemes when SameSite matters.

CookieService::GetCookieForURI doesn't even need to call MaybeCompareSchemesWithLogging: the correct pref-dependent scheme comparison was already computed as part of the aIsSameSiteForeign argument in each of the three places that call it. The fix: Delete the call entirely. Troubling it's up to the callers to get it right in several places (what if we need to add another caller?), but as of now it's used consistently and correctly. Since this (and aIsSafeTopLevelNav) are computed from the HostURI and channel that are also passed in along with it we could simplify this if we wanted.

Both CookieService::GetCookieStringFromDocument and CookieServiceChild::GetCookieStringFromDocument call MaybeCompareSchemes, and like the previous case it's simply wrong because we're comparing schemes without any regard for whether SameSite cookies are involved. The symptoms would be missing cookies when read from document.cookie when a "site" has both https: and http: subdomains. Unfortunately, it won't be as easy to fix. The pref-dependent scheme check needs to be done around the code a little ways before:

    if (thirdParty &&
        !CookieCommons::ShouldIncludeCrossSiteCookieForDocument(cookie)) {
      continue;
    }

The pref-dependent scheme check needs to be built into the concept of thirdParty, but unfortunately that's calculated with our standard ThirdPartyUtils that only compare domains schemelessly. We can't change the basic concept of thirdPartyUtils to consider schemes because it's used for cookies blocking, tracking protection, and other things that need to remain schemeless for now. Could try to add an optional schemeful argument or parallel methods but that could get pretty deep. Maybe just walk the window tree in a new special-purpose CookieCommon function.

I'm not sure we're getting this right in SetCookieStringFromDocument and SetCookieStringFromHttp either, but in those cases we're permissive--possibly letting sites create SameSite cookies we should drop--and won't be breaking sites. We can spin that into a separate bug.

Comment 13

[Daniel Veditz [:dveditz]]

Just realized I was ignoring network.cookie.sameSite.noneRequiresSecure when I analyzed this. That shouldn't make a difference because that's checked when the explicit "None" cookie is created, and checking the secure flag when we are deciding to use the cookie should be enough.

We definitely should remove the MaybeCompareSchemes check from both versions of GetCookieStringFromDocument -- that's blocking completely legit cookies and may break working sites. Fixing it correctly will be work, but what if we simply don't replace it with anything?

1. We don't need to worry about https://good.site.example framing http://insecure.site.example -- the mixed content blocker prevents that case.
2. There is still the potential for cookie injection attacks from http://insecure.site.example framing https://good.site.example that "schemeful samesite" (along with "lax by default") was intended to prevent. There are several existing cookie features that https://good.site.example could be using for protection, like secure cookies and cookie prefixes, but "lax by default" is trying to help sites that haven't thought about these issues and schemeful is part of that.

In the short term I'm fine not fully supporting "schemeful same site" for document.cookie. We should fix this web compat site-breaking bug ASAP and can file a bug to fix this case later.

Comment 14

[Frederik Braun [:freddy]]

Attachment: Bug 1748693 - remove MaybeCompareSchemes(Logging), disable samesite-schemeful for release r?dragana!,ckerschb,dveditz

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Regressing feature was disabled on Fx96 by way of bug 1750257 and bug 1750264.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Disabled for 97 by way of bug 1751435.

Comment 17

[Daniel Veditz [:dveditz]]

I filed bug 1756213 on the thirdParty vs schemeful issue described in comment 12, which can be fixed separately as noted in comment 13

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a5abe44e15f4
remove MaybeCompareSchemes(Logging), disable samesite-schemeful for release  r=dveditz,dragana

Comment 20

[Natalia Csoregi [:nataliaCs]]

Backed out for causing /cookies/ failures

• backout: https://hg.mozilla.org/integration/autoland/rev/4e8f1acdb35732562992bfc941791da6f971a18c
• push: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&revision=a5abe44e15f42812c090ec7885b76367392cbda8
• failures:
  • TEST-UNEXPECTED-FAIL | /cookies/schemeful-same-site/schemeful-subresource.tentative.html | Cross-scheme subresources cannot sent lax/strict cookies - assert_not_equals: SameSite=lax cookies cannot be sent to cross-scheme subresources got disallowed value "0.6292409694853295"
  • TEST-UNEXPECTED-FAIL | TestCookie.TestCookieMain | Value of: CheckResult(cookie.get(), MUST_CONTAIN, "test=security3")
  • TEST-UNEXPECTED-FAIL | /cookies/attributes/secure.https.html | Set cookie for Secure attribute - assert_equals: The cookie was set as expected. expected "test=1" but got "testb=20; testB=8; test=1"

Comment 21

[Tom S. (please needinfo tschuster)]

This try push still has a lot of orange, but it's all intermittents as far as I can tell.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d6d0415a9d79
remove MaybeCompareSchemes(Logging), disable samesite-schemeful for release  r=ckerschb,dveditz,dragana

Comment 23

[Tom Schuster (MoCo)]

Attachment: Bug 1748693 - Remove unused const. r?freddyb

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/11741cb22c8d
Remove unused const.

Comment 25

[Tom Schuster (MoCo)]

Attachment: Bug 1748693 - Remove unused cookie rejection strings. r?freddyb

Comment 26

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/d6d0415a9d79
https://hg.mozilla.org/mozilla-central/rev/11741cb22c8d

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/38058f1dd921
Remove unused cookie rejection strings. r=freddyb,necko-reviewers,dragana
https://hg.mozilla.org/integration/autoland/rev/18a65893fe7d
Add test r=ckerschb

Comment 28

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/38058f1dd921
https://hg.mozilla.org/mozilla-central/rev/18a65893fe7d

Comment 29

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

Since the status are different for nightly and release, what's the status for beta?
For more information, please visit auto_nag documentation.

Comment 30

[Monica Chiorean]

Could not reproduce the issue on Ubuntu 20.4, using build without fix 96.0.1 (20220113185450). Can you please confirm issue is fixed on latest Beta (https://archive.mozilla.org/pub/firefox/candidates/100.0b9-candidates/)? Thank you.

Comment 31

[Michael Cordover [:mjec]]

This is fixed for me on 100.0b9 (on x86_64 Arch Linux), regardless of the value of network.cookie.sameSite.schemeful.