1452496 - Do not allow setting same-site cookies from a cross site request

Status: RESOLVED FIXED

(Core :: Networking: Cookies, defect, P1)

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1452496_discard_same_site_cookie.patch

Hey Valentin, unfortunately the test setup within TestCookie.cpp is complicated and cookies are always load in a cross site context. The problem happens because SetACookie() passes a nullptr as the channel [1] which is used to determine same or cross siteness.

It's unfortunate, but I guess not the end of the world because we have test coverage for all the cases within dom/security/test/general.

Acceptable?

[1] https://dxr.mozilla.org/mozilla-central/source/netwerk/test/TestCookie.cpp#74

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1452496_discard_same_site_cookie_tests.patch

Comment 3

[Daniel Veditz [:dveditz]]

Comment on attachment 8966169 [details] [diff] [review]
bug_1452496_discard_same_site_cookie.patch

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

::: netwerk/cookie/nsCookieService.cpp
@@ +3469,5 @@
>      return newCookie;
>    }
>  
> +  // If the new cookie is same-site but an a cross site context,
> +  // browser have to ignore the cookie.

typo: "in" a cross site context

I'd prefer "have to" => "must" (or even "MUST") since it's a spec requirement.

::: netwerk/test/TestCookie.cpp
@@ +782,5 @@
>      SetACookie(cookieService, "http://samesite.test", nullptr, "bogus=yes; samesite=bogus", nullptr);
> +    // please note that we can't test:
> +    //  * samesite=strict
> +    //  * samesite=lax
> +    // because TestCookie.cpp loads cookies in a cross origin fashion.

This is sad. Do we have other tests for the samesite attribute (I don't see any)? Also, if we can't set samesite from this test then the preceding 4 "samesite" tests are meaningless and you might as well delete them, too.

Any way to fake up a same-origin channel to pass in so we can test these?

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1452496_discard_same_site_cookie.patch

(In reply to Daniel Veditz [:dveditz] from comment #3)
> Any way to fake up a same-origin channel to pass in so we can test these?

In fact: yes. Please note that I on purpose created a new function that deals with same site cookies to make sure the update does not interfere with any other tests within that file. If needed, we can file a follow up to clean that up.

Comment 5

[Daniel Veditz [:dveditz]]

Comment on attachment 8966327 [details] [diff] [review]
bug_1452496_discard_same_site_cookie.patch

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

r=dveditz

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8bf36c469e22
Discard same-site cookie in cross site context. r=dveditz
https://hg.mozilla.org/integration/mozilla-inbound/rev/071ecf5e3680
Test for diiscarding same-site cookie in cross site context. r=mgoodwin

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8966170 [details] [diff] [review]
bug_1452496_discard_same_site_cookie_tests.patch

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1452496
[User impact if declined]: same-site cookies in cross origin context are allowed to be set.
[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 1286861 needs to be applied before this patch.
[Is the change risky?]: No
[Why is the change risky/not risky?]: Only affects same-site cookies.
[String changes made/needed]: no

Comment 8

[Raul Gurzau (:RaulG)]

Backed out 2 changesets (bug 1452496) for bustage on build/src/netwerk/test/TestNamedPipeService.cpp on a CLOSED TREE

Backout link: https://hg.mozilla.org/integration/mozilla-inbound/rev/7a6977dca03a1c363656137ccbd78626ed1a35bc

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=071ecf5e36808e9f9a96e15a0d019418a8846d5b

Log link: https://treeherder.mozilla.org/logviewer.html#?job_id=172890948&repo=mozilla-inbound&lineNumber=12026

Log snippet: 15:57:22     INFO -  mozmake.EXE[4]: Leaving directory 'z:/build/build/src/obj-firefox/ipc/chromium'
15:57:22     INFO -  mozmake.EXE[4]: Entering directory 'z:/build/build/src/obj-firefox/extensions/auth'
15:57:22     INFO -  mozmake.EXE[4]: Leaving directory 'z:/build/build/src/obj-firefox/extensions/auth'
15:57:23     INFO -  mozmake.EXE[4]: Entering directory 'z:/build/build/src/obj-firefox/config/external/icu/i18n'
15:57:23     INFO -  z:/build/build/src/sccache2/sccache.exe z:/build/build/src/vs2017_15.6.0/VC/bin/Hostx64/x86/cl.exe -Fotznames_impl.obj -c -DNDEBUG=1 -DTRIMMED=1 -DU_I18N_IMPLEMENTATION -DUCONFIG_NO_TRANSLITERATION -DUCONFIG_NO_REGULAR_EXPRESSIONS -DUCONFIG_NO_LEGACY_CONVERSION -DU_USING_ICU_NAMESPACE=0 -DU_NO_DEFAULT_INCLUDE_UTF_HEADERS=1 -DU_CHARSET_IS_UTF8 -DU_HAVE_NL_LANGINFO_CODESET=0 -Iz:/build/build/src/config/external/icu/i18n -Iz:/build/build/src/obj-firefox/config/external/icu/i18n -Iz:/build/build/src/intl/icu/source/common -Iz:/build/build/src/obj-firefox/dist/include -Iz:/build/build/src/obj-firefox/dist/include/nspr -Iz:/build/build/src/obj-firefox/dist/include/nss -MD -FI z:/build/build/src/obj-firefox/mozilla-config.h -DMOZILLA_CLIENT -utf-8 -TP -nologo -w15038 -wd5026 -wd5027 -Zc:sizedDealloc- -wd4091 -wd4577 -D_HAS_EXCEPTIONS=0 -W3 -Gy -Zc:inline -arch:SSE2 -Gw -wd4251 -wd4244 -wd4267 -wd4800 -wd4595 -we4553 -D_SILENCE_TR1_NAMESPACE_DEPRECATION_WARNING -GR- -Z7 -O1 -Oi -Oy- -GR -wd4005 -wd4333 -wd4996  -deps.deps/tznames_impl.obj.pp    z:/build/build/src/intl/icu/source/i18n/tznames_impl.cpp
15:57:23     INFO -  tznames_impl.cpp
15:57:23     INFO -  cl : Command line warning D9025 : overriding '/GR-' with '/GR'
15:57:23     INFO -  mozmake.EXE[4]: Leaving directory 'z:/build/build/src/obj-firefox/config/external/icu/i18n'
15:57:23     INFO -  mozmake.EXE[4]: Entering directory 'z:/build/build/src/obj-firefox/config/external/icu/i18n'
15:57:23     INFO -  mozmake.EXE[4]: Leaving directory 'z:/build/build/src/obj-firefox/config/external/icu/i18n'
15:57:23     INFO -  mozmake.EXE[4]: Entering directory 'z:/build/build/src/obj-firefox/netwerk/sctp/src'
15:57:23     INFO -  z:/build/build/src/sccache2/sccache.exe z:/build/build/src/vs2017_15.6.0/VC/bin/Hostx64/x86/cl.exe -Fosctp_sysctl.obj -c -DNDEBUG=1 -DTRIMMED=1 -DWIN32_LEAN_AND_MEAN -D_WIN32 -DWIN32 -D_CRT_RAND_S -DCERT_CHAIN_PARA_HAS_EXTRA_FIELDS -DOS_WIN=1 -D_UNICODE -DCHROMIUM_BUILD -DU_STATIC_IMPLEMENTATION -DUNICODE -D_WINDOWS -D_SECURE_ATL -DCOMPILER_MSVC -DSCTP_SIMPLE_ALLOCATOR=1 -DSCTP_PROCESS_LEVEL_LOCKS=1 -D__Userspace__=1 -DCALLBACK_API=1 -DSCTP_DEBUG=1 -D__Userspace_os_Windows=1 -D_LIB=1 -DSTATIC_EXPORTABLE_JS_API -DMOZ_HAS_MOZGLUE -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -Iz:/build/build/src/netwerk/sctp/src -Iz:/build/build/src/obj-firefox/netwerk/sctp/src -Iz:/build/build/src/obj-firefox/ipc/ipdl/_ipdlheaders -Iz:/build/build/src/ipc/chromium/src -Iz:/build/build/src/ipc/glue -Iz:/build/build/src/dom/base -Iz:/build/build/src/netwerk/base -Iz:/build/build/src/obj-firefox/dist/include -Iz:/build/build/src/obj-firefox/dist/include/nspr -Iz:/build/build/src/obj-firefox/dist/include/nss -MD -FI z:/build/build/src/obj-firefox/mozilla-config.h -DMOZILLA_CLIENT -utf-8 -TC -nologo -wd4091 -D_HAS_EXCEPTIONS=0 -W3 -Gy -Zc:inline -arch:SSE2 -Gw -wd4244 -wd4267 -we4553 -Z7 -O1 -Oi -Oy-  -deps.deps/sctp_sysctl.obj.pp    z:/build/build/src/netwerk/sctp/src/netinet/sctp_sysctl.c
15:57:23     INFO -  sctp_sysctl.c
15:57:23     INFO -  mozmake.EXE[4]: Leaving directory 'z:/build/build/src/obj-firefox/netwerk/sctp/src'
15:57:23     INFO -  mozmake.EXE[4]: Entering directory 'z:/build/build/src/obj-firefox/netwerk/test'
15:57:23     INFO -  z:/build/build/src/sccache2/sccache.exe z:/build/build/src/vs2017_15.6.0/VC/bin/Hostx64/x86/cl.exe -FoUnified_cpp_netwerk_test0.obj -c -Iz:/build/build/src/obj-firefox/dist/stl_wrappers -DNDEBUG=1 -DTRIMMED=1 -DWIN32_LEAN_AND_MEAN -D_WIN32 -DWIN32 -D_CRT_RAND_S -DCERT_CHAIN_PARA_HAS_EXTRA_FIELDS -DOS_WIN=1 -D_UNICODE -DCHROMIUM_BUILD -DU_STATIC_IMPLEMENTATION -DUNICODE -D_WINDOWS -D_SECURE_ATL -DCOMPILER_MSVC -DSTATIC_EXPORTABLE_JS_API -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -Iz:/build/build/src/netwerk/test -Iz:/build/build/src/obj-firefox/netwerk/test -Iz:/build/build/src/obj-firefox/ipc/ipdl/_ipdlheaders -Iz:/build/build/src/ipc/chromium/src -Iz:/build/build/src/ipc/glue -Iz:/build/build/src/obj-firefox/dist/include -Iz:/build/build/src/obj-firefox/dist/include/nspr -Iz:/build/build/src/obj-firefox/dist/include/nss -MD -FI z:/build/build/src/obj-firefox/mozilla-config.h -DMOZILLA_CLIENT -utf-8 -TP -nologo -w15038 -wd5026 -wd5027 -Zc:sizedDealloc- -wd4091 -wd4577 -D_HAS_EXCEPTIONS=0 -W3 -Gy -Zc:inline -arch:SSE2 -Gw -wd4251 -wd4244 -wd4267 -wd4800 -wd4595 -we4553 -D_SILENCE_TR1_NAMESPACE_DEPRECATION_WARNING -GR- -Z7 -O1 -Oi -Oy- -WX  -deps.deps/Unified_cpp_netwerk_test0.obj.pp    z:/build/build/src/obj-firefox/netwerk/test/Unified_cpp_netwerk_test0.cpp
15:57:23     INFO -  Unified_cpp_netwerk_test0.cpp
15:57:23     INFO -  z:/build/build/src/netwerk/test/TestNamedPipeService.cpp(251): error C3861: 'CreateEvent': identifier not found

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Raul Gurzau (:RaulGurzau) from comment #8)
> Backed out 2 changesets (bug 1452496) for bustage on
> build/src/netwerk/test/TestNamedPipeService.cpp on a CLOSED TREE

On it!

Comment 10

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8966170 [details] [diff] [review]
bug_1452496_discard_same_site_cookie_tests.patch

Cancelling the uplift request for now till I have fixed the issue within TestCookie.cpp

Comment 11

[Christoph Kerschbaumer [:ckerschb]]

So, I was able to fix the compile problem with regards to TestNamedPipeService.cpp.

But still there is one problem left (see dt1 within [1]). When we check for third-party-ness of a same-site cookie we apparently get something wrong for inline scripts. In particular we don't allow the same-site cookie to be set within CanSetCookie for [2] because:

aHostURI: http://test1.example.org/browser/devtools/client/storage/test/storage-cookies-samesite.html
aChannelURI: about:blank

I think inline scripts should be treated same-origin with the including context. I suppose the flag 'aHTTP' determines that, but also not entirely sure.

ni? a bunch of folks who might now the answer!

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=913f6240f3ceb38be8c4689595bd8c8efdfcb415
[2] https://dxr.mozilla.org/mozilla-central/source/devtools/client/storage/test/storage-cookies-samesite.html#12

Comment 12

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1452496_discard_same_site_cookie_inline_script_tests.patch

Hey Mark, as discussed over IRC, inline same site cookies should be allowed if in same origin context, but should be discarded in cross site context.

More explicitly:

[allowed] SiteA loads frameA
          frameA does document.cookie = value=bla, samesite=strict


[discard] Site loads frameB
          frameB does document.cookie = value=bla, samesite=strict

To make sure, do we agree that this is expected behavior? Please also verify my test which should map the scenario.

Still not sure how we can fix that in our code though.

Comment 13

[Christoph Kerschbaumer [:ckerschb]]

Dan, Francois, do keep you in the loop as well, please see comment 11 and comment 12.

Comment 14

[Christoph Kerschbaumer [:ckerschb]]

It seems that the root cause of the problem originates here [1], but even there the isForeign flag is already wrong in that particular case.

[1] https://dxr.mozilla.org/mozilla-central/source/netwerk/cookie/CookieServiceParent.cpp#259-274

Comment 15

[Honza Bambas (:mayhemer)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #11)
> So, I was able to fix the compile problem with regards to
> TestNamedPipeService.cpp.
> 
> But still there is one problem left (see dt1 within [1]). When we check for
> third-party-ness of a same-site cookie we apparently get something wrong for
> inline scripts. 

I think the best person to ask about how to recognize inline script security context is Boris.  (ni? on him)

> In particular we don't allow the same-site cookie to be set
> within CanSetCookie for [2] because:
> 
> aHostURI:
> http://test1.example.org/browser/devtools/client/storage/test/storage-
> cookies-samesite.html
> aChannelURI: about:blank
> 
> I think inline scripts should be treated same-origin with the including
> context. I suppose the flag 'aHTTP' determines that, but also not entirely
> sure.

Not sure to which code you are referring exactly, but I assume that aHTTP arg represents 'http-only' cookie (a flag sent by the server), that instructs that cookie to no be exposed on document.cookie (IIUC), it doesn't stand for aHTTP == false => cookie comes from an inline script.

> 
> ni? a bunch of folks who might now the answer!
> 
> [1]
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=913f6240f3ceb38be8c4689595bd8c8efdfcb415
> [2]
> https://dxr.mozilla.org/mozilla-central/source/devtools/client/storage/test/
> storage-cookies-samesite.html#12

Comment 16

[Boris Zbarsky [:bzbarsky]]

There should be nothing special for inline scripts.  That is, they should act just like <script src="something"> in terms of the cookie setter.

> aChannelURI: about:blank

What does the stack look like to the point where you're seeing this?

In general, the document.cookie setter uses the document's channel if there is one.  If not, it creates a dummy channel with the principal URI of the document.  So I don't see where about:blank would come from in devtools/client/storage/test/storage-cookies-samesite.html.

Comment 17

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1452496_discard_same_site_cookie.patch

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #16)
> There should be nothing special for inline scripts.  That is, they should
> act just like <script src="something"> in terms of the cookie setter.
> 
> > aChannelURI: about:blank
> 
> What does the stack look like to the point where you're seeing this?

Boris,

thanks for your speedy response. The problem originates here [1], where we create that dummy channel with about:blank as the URI (see also stacktrace underneath). I guess we could explicitly pass around the 'aIsForeign' flag (see attached patch) which fixes the problem and the tests runs to completion correctly. The only problem is that currently we only compute the 'isForeign' flag in case RequireThirdPartyCheck() returns true.

Would it be an option to *always* compute
mThirdPartyUtil->IsThirdPartyChannel(aChannel, aHostURI, &isForeign); ?

I am not sure what other implications that might have. Also within CookieServiceChild.cpp there are more calls to RequireThirdPartyCheck() which might also be affected.

[1] https://dxr.mozilla.org/mozilla-central/source/netwerk/cookie/CookieServiceParent.cpp#259-274

Assertion failure: false (blah), at /home/ckerschb/source/mc/netwerk/cookie/nsCookieService.cpp:3495
 0:49.92 GECKO(3480) #01: nsCookieService::CanSetCookie(nsIURI*, mozilla::net::nsCookieKey const&, nsCookieAttributes&, bool, CookieStatus, nsTDependentString<char>&, long, bool, bool, nsIChannel*, bool, bool&, mozIThirdPartyUtil*) (/home/ckerschb/source/mc/netwerk/cookie/nsCookieService.cpp:3495)
 0:49.92 GECKO(3480) #02: nsCookieService::SetCookieInternal(nsIURI*, mozilla::net::nsCookieKey const&, bool, CookieStatus, nsTDependentString<char>&, long, bool, bool, bool, nsIChannel*) (/home/ckerschb/source/mc/netwerk/cookie/nsCookieService.cpp:3530)
 0:49.92 GECKO(3480) #03: nsCookieService::SetCookieStringInternal(nsIURI*, bool, nsTDependentString<char>&, nsTString<char> const&, bool, bool, mozilla::OriginAttributes const&, nsIChannel*) (/home/ckerschb/source/mc/netwerk/cookie/nsCookieService.cpp:2221)
 0:49.93 GECKO(3480) #04: mozilla::net::CookieServiceParent::RecvSetCookieString(mozilla::ipc::URIParams const&, bool const&, nsTString<char> const&, nsTString<char> const&, mozilla::OriginAttributes const&, bool const&) (/home/ckerschb/source/mc/netwerk/cookie/CookieServiceParent.cpp:274)
 0:50.03 GECKO(3480) #05: mozilla::net::PCookieServiceParent::OnMessageReceived(IPC::Message const&) (/home/ckerschb/source/mc-obj-ff-dbg/ipc/ipdl/PCookieServiceParent.cpp:248)
 0:50.03 GECKO(3480) #06: mozilla::dom::PContentParent::OnMessageReceived(IPC::Message const&) (/home/ckerschb/source/mc-obj-ff-dbg/ipc/ipdl/PContentParent.cpp:3337)
 0:50.05 GECKO(3480) #07: mozilla::ipc::MessageChannel::DispatchAsyncMessage(IPC::Message const&) (/home/ckerschb/source/mc/ipc/glue/MessageChannel.cpp:2135)
 0:50.05 GECKO(3480) #08: mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message&&) (/home/ckerschb/source/mc/ipc/glue/MessageCha

Comment 18

[Boris Zbarsky [:bzbarsky]]

> where we create that dummy channel with about:blank as the URI

OK.  So if we expect this to not be about:blank, what is the stack to the corresponding SendSetCookieString?  

> Would it be an option to *always* compute

I don't know; I'm not really familiar with the cookie or third-party service code.

Comment 19

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #18)
> > where we create that dummy channel with about:blank as the URI
> 
> OK.  So if we expect this to not be about:blank, what is the stack to the
> corresponding SendSetCookieString?

 Assertion failure: false (blah), at /home/ckerschb/source/mc/netwerk/cookie/CookieServiceChild.cpp:554
 0:26.39 GECKO(9467) #01: mozilla::net::CookieServiceChild::SetCookieStringInternal(nsIURI*, nsIChannel*, char const*, char const*, bool) (/home/ckerschb/source/mc/netwerk/cookie/CookieServiceChild.cpp:554)
 0:26.39 GECKO(9467) #02: mozilla::net::CookieServiceChild::SetCookieString(nsIURI*, nsIPrompt*, char const*, nsIChannel*) (/home/ckerschb/source/mc/netwerk/cookie/CookieServiceChild.cpp:648)
 0:26.82 GECKO(9467) #03: nsHTMLDocument::SetCookie(nsTSubstring<char16_t> const&, mozilla::ErrorResult&) (/home/ckerschb/source/mc/dom/html/nsHTMLDocument.cpp:1171 (discriminator 1))
 0:26.88 GECKO(9467) #04: mozilla::dom::HTMLDocumentBinding::set_cookie(JSContext*, JS::Handle<JSObject*>, nsHTMLDocument*, JSJitSetterCallArgs) (/home/ckerschb/source/mc-obj-ff-dbg/dom/bindings/HTMLDocumentBinding.cpp:140)
 0:26.91 GECKO(9467) #05: bool mozilla::dom::binding_detail::GenericSetter<mozilla::dom::binding_detail::NormalThisPolicy>(JSContext*, unsigned int, JS::Value*) (/home/ckerschb/source/mc/dom/bindings/BindingUtils.cpp:3135)
 0:27.38 GECKO(9467) #06: js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) (/home/ckerschb/source/mc/js/src/vm/JSContext-inl.h:290)
 0:27.38 GECKO(9467) #07: js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) (/home/ckerschb/source/mc/js/src/vm/Interpreter.cpp:467)
 0:27.40 GECKO(9467) #08: InternalCall(JSContext*, js::AnyInvokeArgs const&) (/home/ckerschb/source/mc/js/src/vm/Interpreter.cpp:517)
 0:27.40 GECKO(9467) #09: js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) (/home/ckerschb/source/mc/js/src/vm/Interpreter.cpp:535)
 0:27.41 GECKO(9467) #10: js::CallSetter(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::Handle<JS::Value>) (/home/ckerschb/source/mc/js/src/vm/Interpreter.cpp:664)
 0:27.66 GECKO(9467) #11: SetExistingProperty(JSContext*, JS::Handle<js::NativeObject*>, JS::Handle<jsid>, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::Handle<js::NativeObject*>, JS::Handle<JS::PropertyResult>, JS::ObjectOpResult&) (/home/ckerschb/source/mc/js/src/vm/NativeObject.cpp:2786)
 0:27.67 GECKO(9467) #12: bool js::NativeSetProperty<(js::QualifiedBool)1>(JSContext*, JS::Handle<js::NativeObject*>, JS::Handle<jsid>, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::ObjectOpResult&) (/home/ckerschb/source/mc/js/src/vm/NativeObject.cpp:2814)
 0:28.30 GECKO(9467) #13: js::SetProperty(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::ObjectOpResult&) (/home/ckerschb/source/mc/js/src/vm/NativeObject.h:1658)
 0:28.31 GECKO(9467) #14: js::SetPropertyIgnoringNamedGetter(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::Handle<JS::PropertyDescriptor>, JS::ObjectOpResult&) (/home/ckerschb/source/mc/js/src/proxy/BaseProxyHandler.cpp:182)
 0:28.32 GECKO(9467) #15: mozilla::dom::DOMProxyHandler::set(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::ObjectOpResult&) const (/home/ckerschb/source/mc/dom/bindings/DOMJSProxyHandler.cpp:220)
 0:28.32 GECKO(9467) #16: js::Proxy::setInternal(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::ObjectOpResult&) (/home/ckerschb/source/mc/js/src/proxy/Proxy.cpp:403)
 0:28.33 GECKO(9467) #17: js::Proxy::set(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::ObjectOpResult&) (/home/ckerschb/source/mc/js/src/proxy/Proxy.cpp:413)
 0:28.34 GECKO(9467) #18: JSObject::nonNativeSetProperty(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::ObjectOpResult&) (/home/ckerschb/source/mc/js/src/vm/JSObject.cpp:1083)
 0:28.35 GECKO(9467) #19: js::SetProperty(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::ObjectOpResult&) (/home/ckerschb/source/mc/js/src/vm/NativeObject.h:1656)
 0:28.37 GECKO(9467) #20: SetPropertyOperation(JSContext*, JSOp, JS::Handle<JS::Value>, JS::Handle<jsid>, JS::Handle<JS::Value>) (/home/ckerschb/source/mc/js/src/vm/Interpreter.cpp:264)
 0:28.39 GECKO(9467) #21: Interpret(JSContext*, js::RunState&) (/home/ckerschb/source/mc/js/src/vm/Interpreter.cpp:2881)
 0:28.40 GECKO(9467) #22: js::RunScript(JSContext*, js::RunState&) (/home/ckerschb/source/mc/js/src/vm/Interpreter.cpp:417)
 0:28.40 GECKO(9467) #23: js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::AbstractFramePtr, JS::Value*) (/home/ckerschb/source/mc/js/src/vm/Interpreter.cpp:700)
 0:28.41 GECKO(9467) #24: js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*) (/home/ckerschb/source/mc/js/src/vm/Interpreter.cpp:733)
 0:28.43 GECKO(9467) #25: ExecuteScript(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSScript*>, JS::Value*) (/home/ckerschb/source/mc/js/src/jsapi.cpp:4702)
 0:28.45 GECKO(9467) #26: ExecuteScript(JSContext*, JS::AutoVector<JSObject*>&, JS::Handle<JSScript*>, JS::Value*) (/home/ckerschb/source/mc/js/src/jsapi.cpp:4721)
 0:28.46 GECKO(9467) #27: JS_ExecuteScript(JSContext*, JS::AutoVector<JSObject*>&, JS::Handle<JSScript*>, JS::MutableHandle<JS::Value>) (/home/ckerschb/source/mc/js/src/jsapi.cpp:4743)
 0:28.73 GECKO(9467) #28: nsJSUtils::ExecutionContext::CompileAndExec(JS::CompileOptions&, JS::SourceBufferHolder&, JS::MutableHandle<JSScript*>) (/home/ckerschb/source/mc/dom/base/nsJSUtils.cpp:268)
 0:28.77 GECKO(9467) #29: mozilla::dom::ScriptLoader::EvaluateScript(mozilla::dom::ScriptLoadRequest*) (/home/ckerschb/source/mc/dom/script/ScriptLoader.cpp:2327)
 0:28.78 GECKO(9467) #30: mozilla::dom::ScriptLoader::ProcessRequest(mozilla::dom::ScriptLoadRequest*) (/home/ckerschb/source/mc/dom/script/ScriptLoader.cpp:1956)
 0:28.79 GECKO(9467) #31: mozilla::dom::ScriptLoader::ProcessInlineScript(nsIScriptElement*, mozilla::dom::ScriptKind) (/home/ckerschb/source/mc/dom/script/ScriptLoader.cpp:1595)
 0:28.79 GECKO(9467) #32: mozilla::dom::ScriptLoader::ProcessScriptElement(nsIScriptElement*) (/home/ckerschb/source/mc/dom/script/ScriptLoader.cpp:1314)
 0:28.80 GECKO(9467) #33: mozilla::dom::ScriptElement::MaybeProcessScript() (/home/ckerschb/source/mc/dom/script/ScriptElement.cpp:141)
 0:28.91 GECKO(9467) #34: nsIScriptElement::AttemptToExecute() (/home/ckerschb/source/mc-obj-ff-dbg/dist/include/nsIScriptElement.h:247)
 0:28.91 GECKO(9467) #35: nsHtml5TreeOpExecutor::RunScript(nsIContent*) (/home/ckerschb/source/mc/parser/html/nsHtml5TreeOpExecutor.cpp:739)
 0:28.92 GECKO(9467) #36: nsHtml5TreeOpExecutor::RunFlushLoop() (/home/ckerschb/source/mc/parser/html/nsHtml5TreeOpExecutor.cpp:541)
 0:28.93 GECKO(9467) #37: nsHtml5ExecutorFlusher::Run() (/home/ckerschb/source/mc/parser/html/nsHtml5StreamParser.cpp:123)
 0:28.97 GECKO(9467) #38: mozilla::SchedulerGroup::Runnable::Run() (/home/ckerschb/source/mc/xpcom/threads/SchedulerGroup.cpp:337)
 0:28.97 GECKO(9467) #39: nsThread::ProcessNextEvent(bool, bool*) (/home/ckerschb/source/mc/xpcom/threads/nsThread.cpp:1100)
 0:28.99 GECKO(9467) #40: NS_ProcessNextEvent(nsIThread*, bool) (/home/ckerschb/source/mc/xpcom/threads/nsThreadUtils.cpp:519)
 0:29.02 GECKO(9467) #41: mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) (/home/ckerschb/source/mc/ipc/glue/MessagePump.cpp:97)
 0:29.02 GECKO(9467) #42: mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*) (/home/ckerschb/source/mc/ipc/glue/MessagePump.cpp:302)
 0:29.04 GECKO(9467) #43: MessageLoop::RunInternal() (/home/ckerschb/source/mc/ipc/chromium/src/base/message_loop.cc:327)
 0:29.05 GECKO(9467) #44: MessageLoop::RunHandler() (/home/ckerschb/source/mc/ipc/chromium/src/base/message_loop.cc:320)
 0:29.05 GECKO(9467) #45: MessageLoop::Run() (/home/ckerschb/source/mc/ipc/chromium/src/base/message_loop.cc:298)
 0:29.08 GECKO(9467) #46: nsBaseAppShell::Run() (/home/ckerschb/source/mc/widget/nsBaseAppShell.cpp:159)
 0:29.10 GECKO(9467) #47: XRE_RunAppShell() (/home/ckerschb/source/mc/toolkit/xre/nsEmbedFunctions.cpp:893)
 0:29.11 GECKO(9467) #48: mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*) (/home/ckerschb/source/mc/ipc/glue/MessagePump.cpp:269)
 0:29.11 GECKO(9467) #49: MessageLoop::RunInternal() (/home/ckerschb/source/mc/ipc/chromium/src/base/message_loop.cc:327)
 0:29.11 GECKO(9467) #50: MessageLoop::RunHandler() (/home/ckerschb/source/mc/ipc/chromium/src/base/message_loop.cc:320)

Comment 20

[Christoph Kerschbaumer [:ckerschb]]

Are you suggesting serializing the ChannelURI and that use that URI to create the dummy Channel?

Comment 21

[Boris Zbarsky [:bzbarsky]]

Oh, I see.  We're passing in the "host uri" but that's the URI of the document, but creating the dummy channel with about:blank as the channel URI.

Yes, we should be using the right channel URI here!  That's what happens in the non-e10s case, no?  What's happening here is that the document is creating a dummy channel with the right URI, and then the e10s cookie machinery is throwing it out, afaict.

Comment 22

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #21)
> Oh, I see.  We're passing in the "host uri" but that's the URI of the
> document, but creating the dummy channel with about:blank as the channel URI.
> 
> Yes, we should be using the right channel URI here!

Since we are on it, should we also serialize other bits of the channel?
ContentPolicyType?
SecurityFlags?
Or just the URI which is enough to make this particular test case work.

Comment 23

[Boris Zbarsky [:bzbarsky]]

I don't know.  Does the cookie service use those bits?  If it does, it should probably not lose them when crossing the e10s boundary....

Comment 24

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1452496_discard_same_site_cookie.patch

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #23)
> I don't know.  Does the cookie service use those bits?  If it does, it
> should probably not lose them when crossing the e10s boundary....

The cookie service does not ever query the loadinfo from the channel, hence I only serialized the channel URI which is then used to create the dummy channel.

Would you be willing to review that change?

Comment 25

[Mark Goodwin [:mgoodwin]]

Comment on attachment 8966924 [details] [diff] [review]
bug_1452496_discard_same_site_cookie_inline_script_tests.patch

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

I think we're good other than the fact that CROSS_ORIGIN looks fairly same-origin to me...

::: dom/security/test/general/test_same_site_cookies_from_script.html
@@ +25,5 @@
> +
> +SimpleTest.waitForExplicitFinish();
> +
> +const SAME_ORIGIN = "http://example.com/";
> +const CROSS_ORIGIN = "http://example.com/";

Shouldn't SAME_ORIGIN and CROSS_ORIGIN differ?

Comment 26

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8967111 [details] [diff] [review]
bug_1452496_discard_same_site_cookie.patch

There is a little more to it actually, let me put everything in one patch!

Comment 27

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1452496_discard_same_site_cookie_tests.patch

Rebased and some nits, carrying over r+ from mgoodwin!

Comment 28

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1452496_discard_same_site_gtests.patch

Separated the gtests which have already been r+ by dveditz within comment 5.

Comment 29

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1452496_discard_same_site_cookie_inline_script_tests.patch

(In reply to Mark Goodwin [:mgoodwin] from comment #25)
> Shouldn't SAME_ORIGIN and CROSS_ORIGIN differ?

Indeed, I realized that shortly after uploading when testing my patch.

Comment 30

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1452496_discard_same_site_cookie.patch

Valentin,

let me highlight and summarize the changes here:
a) Within Bug 1286861 we introduced the concept of NS_IsTopLevelForeign() which checked the same-siteness for top-level loads. Within this bug I realized that 'aForeign' does not always return the right value, in particular for inline scripts. I realized that RequireThirdPartyCheck() quite often guards the calculation of the third-partyness. Since we are going to uplift same-site cookies to beta I don't want to risk any breakage and hence I changed NS_IsTopLevelForeign() to NS_IsSameSiteForeign() which not only checks for top-level third-partyness but *always* performs the third party check required for same site cookies. I added automated tests which got reviewed by Dan and Mgoodwin.

b) Within RecvSetCookieString() we created a dummy channel using about:blank as the uri [1] instead of using the correct URI of the channel which also caused third-party checks to fail. I updated those bits and serialized the actual URI for the channel from child to parent.

Here is a TRY run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b97b00759720b1eb54c732b808501452cb845b68

Thanks,
  Christoph

[1] https://dxr.mozilla.org/mozilla-central/source/netwerk/cookie/CookieServiceParent.cpp#259-274

Comment 31

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8967281 [details] [diff] [review]
bug_1452496_discard_same_site_cookie.patch

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

Looks great!

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/453b91e427d6
Discard same-site cookie in cross site context. r=valentin
https://hg.mozilla.org/integration/mozilla-inbound/rev/532a1bb332ba
gtest for discarding same-site cookies in cross site context. r=dveditz
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b60b41eda62
Test for discarding same-site cookie in cross site context. r=mgoodwin
https://hg.mozilla.org/integration/mozilla-inbound/rev/87b552f9c09b
Test for discarding same-site cookies using inline scripts in cross origin context. r=mgoodwin

Comment 33

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8967275 [details] [diff] [review]
bug_1452496_discard_same_site_cookie_tests.patch

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1452496
[User impact if declined]: same-site cookies in cross origin context are
allowed to be set.
[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 1286861 needs to be
applied before this patch.
[Is the change risky?]: No
[Why is the change risky/not risky?]: Only affects same-site cookies.
[String changes made/needed]: no


Patches should be applied in the following order:

 0 U bug_1452496_discard_same_site_cookie: Bug 1452496: Discard same-site cookie in cross site context. r=valentin
 1 U bug_1452496_discard_same_site_gtests: Bug 1452496: gtest for discarding same-site cookies in cross site context. r=dveditz
 2 U bug_1452496_discard_same_site_cookie_tests: Bug 1452496: Test for discarding same-site cookie in cross site context. r=mgoodwin
 3 U bug_1452496_discard_same_site_cookie_inline_script_tests: Bug 1452496: Test for discarding same-site cookies using inline scripts in cross origin.

Comment 34

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8967276 [details] [diff] [review]
bug_1452496_discard_same_site_gtests.patch

Approval Request Comment
See comment 33

Comment 35

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8967279 [details] [diff] [review]
bug_1452496_discard_same_site_cookie_inline_script_tests.patch

Approval Request Comment
See comment 33

Comment 36

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8967281 [details] [diff] [review]
bug_1452496_discard_same_site_cookie.patch

Approval Request Comment
See comment 33

Comment 37

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

https://hg.mozilla.org/mozilla-central/rev/453b91e427d6
https://hg.mozilla.org/mozilla-central/rev/532a1bb332ba
https://hg.mozilla.org/mozilla-central/rev/2b60b41eda62
https://hg.mozilla.org/mozilla-central/rev/87b552f9c09b

Comment 38

[Julien Cristau [:jcristau] (back April 22)]

Comment on attachment 8967275 [details] [diff] [review]
bug_1452496_discard_same_site_cookie_tests.patch

followup for samesite cookies, approved for 60.0b13

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/f3ba925bbcdb
https://hg.mozilla.org/releases/mozilla-beta/rev/005ee449235a
https://hg.mozilla.org/releases/mozilla-beta/rev/cb7ae1cf71df
https://hg.mozilla.org/releases/mozilla-beta/rev/1e59dc322c46

Comment 40

[Ryan VanderMeulen [:RyanVM]]

Backed out for bustage. Looks like the newly-added gtest depends on bug 1439713, which is only on 61. Christoph, can you adjust the patch around that?

https://treeherder.mozilla.org/logviewer.html#?job_id=173698593&repo=mozilla-beta

Comment 41

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/12e79afb160d5574669e13664cbcac3c66375551

Comment 42

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1452496_discard_same_site_gtests_beta.patch

Ryan, you are right. All we have to do is use SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL instead of SEC_ONLY_FOR_EXPLICIT_CONTENTSEC_CHECK. I updated that within this patch, ran TestCookie locally and everything should be fine now.

Thanks!

Comment 43

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/releases/mozilla-beta/rev/84c492af301f
https://hg.mozilla.org/releases/mozilla-beta/rev/7aa5a506eaa0
https://hg.mozilla.org/releases/mozilla-beta/rev/d367a7b18722
https://hg.mozilla.org/releases/mozilla-beta/rev/9f6f88626ac4