1890748 - (CVE-2024-6601) Cross-origin containers can obtain permissions of the top-level origin

Status: RESOLVED FIXED

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

Description

[Andreas Farre [:farre]]

Because of how a cross origin container gets its feature policy it is possible that we initialize the feature policy before the window context has become the browsing context's current window context.

This means that a cross origin could get the default policy because of timing issues. This can be seen in the intermittent test failures from trying to land the test in bug 1755081 Comment 38. The solution is to pass the feature policy through the load info instead, so that it's available when the document starts to load.

Comment 1

[Andreas Farre [:farre]]

Of course, we should post-pone landing the test from bug 1755081 until this has been fixed (patch incoming).

Comment 2

[Andreas Farre [:farre]]

Attachment: Bug 1890748 - Move responsibility of FeaturePolicy initialization to nsILoadInfo. r=freddyb!

Comment 3

[Andreas Farre [:farre]]

Attachment: Bug 1890748 - Add WPT for policy in transient about blank. r=freddyb!

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a710fd347db2
Move responsibility of FeaturePolicy initialization to nsILoadInfo. r=freddyb,necko-reviewers,jesup,dom-core,sefeng

Comment 5

[Andreas Farre [:farre]]

Ni to myself to land the test in https://phabricator.services.mozilla.com/D140817 and https://phabricator.services.mozilla.com/D207141

Comment 6

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

Backed out for build bustage in Document.cpp:
https://hg.mozilla.org/integration/autoland/rev/b99c1865de0e5728811ce3b7b59fd0700ba61489

Push with bustage
Failure log

[task 2024-05-24T14:43:43.284Z] 14:43:43     INFO -  /builds/worker/fetches/sccache/sccache /builds/worker/fetches/clang/bin/clang++ --sysroot /builds/worker/fetches/sysroot-x86_64-linux-gnu -o Unified_cpp_dom_base2.o -c  -I/builds/worker/workspace/obj-build/dist/stl_wrappers -I/builds/worker/workspace/obj-build/dist/system_wrappers -include /builds/worker/checkouts/gecko/config/gcc_hidden.h -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -fstack-protector-strong -fstack-clash-protection -DNDEBUG=1 -DTRIMMED=1 -DMOZ_HAS_MOZGLUE -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -DSTATIC_EXPORTABLE_JS_API -I/builds/worker/checkouts/gecko/dom/base -I/builds/worker/workspace/obj-build/dom/base -I/builds/worker/checkouts/gecko/dom/battery -I/builds/worker/checkouts/gecko/dom/events -I/builds/worker/checkouts/gecko/dom/media -I/builds/worker/checkouts/gecko/dom/network -I/builds/worker/checkouts/gecko/caps -I/builds/worker/checkouts/gecko/docshell/base -I/builds/worker/checkouts/gecko/dom/base -I/builds/worker/checkouts/gecko/dom/file -I/builds/worker/checkouts/gecko/dom/geolocation -I/builds/worker/checkouts/gecko/dom/html -I/builds/worker/checkouts/gecko/dom/ipc -I/builds/worker/checkouts/gecko/dom/storage -I/builds/worker/checkouts/gecko/dom/svg -I/builds/worker/checkouts/gecko/dom/xml -I/builds/worker/checkouts/gecko/dom/xslt/xpath -I/builds/worker/checkouts/gecko/dom/xul -I/builds/worker/checkouts/gecko/extensions/spellcheck/src -I/builds/worker/checkouts/gecko/gfx/2d -I/builds/worker/checkouts/gecko/image -I/builds/worker/checkouts/gecko/js/xpconnect/loader -I/builds/worker/checkouts/gecko/js/xpconnect/src -I/builds/worker/checkouts/gecko/js/xpconnect/wrappers -I/builds/worker/checkouts/gecko/layout/base -I/builds/worker/checkouts/gecko/layout/forms -I/builds/worker/checkouts/gecko/layout/generic -I/builds/worker/checkouts/gecko/layout/style -I/builds/worker/checkouts/gecko/layout/xul -I/builds/worker/checkouts/gecko/netwerk/base -I/builds/worker/checkouts/gecko/netwerk/protocol/http -I/builds/worker/checkouts/gecko/netwerk/url-classifier -I/builds/worker/checkouts/gecko/parser/htmlparser -I/builds/worker/checkouts/gecko/security/manager/ssl -I/builds/worker/checkouts/gecko/third_party/xsimd/include -I/builds/worker/checkouts/gecko/widget -I/builds/worker/checkouts/gecko/xpcom/ds -I/builds/worker/checkouts/gecko/netwerk/sctp/datachannel -I/builds/worker/workspace/obj-build/ipc/ipdl/_ipdlheaders -I/builds/worker/checkouts/gecko/ipc/chromium/src -I/builds/worker/workspace/obj-build/dist/include -I/builds/worker/workspace/obj-build/dist/include/nspr -I/builds/worker/workspace/obj-build/dist/include/nss -DMOZILLA_CLIENT -include /builds/worker/workspace/obj-build/mozilla-config.h -D_GLIBCXX_USE_CXX11_ABI=0 -fno-rtti -pthread -fno-sized-deallocation -fno-aligned-new -ffunction-sections -fdata-sections -fno-math-errno -fno-exceptions -fcrash-diagnostics-dir=/builds/worker/artifacts -fno-exceptions -fPIC -gdwarf-4 -Xclang -load -Xclang /builds/worker/workspace/obj-build/build/clang-plugin/libclang-plugin.so -Xclang -add-plugin -Xclang moz-check -O2 -fno-omit-frame-pointer -funwind-tables -Werror -Wall -Wbitfield-enum-conversion -Wempty-body -Wformat-type-confusion -Wignored-qualifiers -Wpointer-arith -Wshadow-field-in-constructor-modified -Wsign-compare -Wtautological-constant-in-range-compare -Wtype-limits -Wno-error=tautological-type-limit-compare -Wunreachable-code -Wunreachable-code-return -Wunused-but-set-parameter -Wno-invalid-offsetof -Wclass-varargs -Wempty-init-stmt -Wfloat-overflow-conversion -Wfloat-zero-conversion -Wloop-analysis -Wno-range-loop-analysis -Wenum-compare-conditional -Wenum-float-conversion -Wno-deprecated-anon-enum-enum-conversion -Wno-deprecated-enum-enum-conversion -Wno-deprecated-this-capture -Wcomma -Wimplicit-fallthrough -Wstring-conversion -Wno-inline-new-delete -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wno-error=free-nonheap-object -Wno-error=atomic-alignment -Wno-error=deprecated-builtins -Wformat -Wformat-security -Wno-psabi -Wthread-safety -Wno-error=builtin-macro-redefined -Wno-vla-cxx-extension -Wno-unknown-warning-option -I/builds/worker/fetches/sysroot-x86_64-linux-gnu/usr/include/gtk-3.0 -I/builds/worker/fetches/sysroot-x86_64-linux-gnu/usr/include/pango-1.0 -I/builds/worker/fetches/sysroot-x86_64-linux-gnu/usr/include/glib-2.0 -I/builds/worker/fetches/sysroot-x86_64-linux-gnu/usr/lib/x86_64-linux-gnu/glib-2.0/include -I/builds/worker/fetches/sysroot-x86_64-linux-gnu/usr/include -I/builds/worker/fetches/sysroot-x86_64-linux-gnu/usr/include/cairo -I/builds/worker/fetches/sysroot-x86_64-linux-gnu/usr/include/pixman-1 -I/builds/worker/fetches/sysroot-x86_64-linux-gnu/usr/include/freetype2 -I/builds/worker/fetches/sysroot-x86_64-linux-gnu/usr/include/libpng12 -I/builds/worker/fetches/sysroot-x86_64-linux-gnu/usr/include/gdk-pixbuf-2.0 -I/builds/worker/fetches/sysroot-x86_64-linux-gnu/usr/include/gio-unix-2.0/ -I/builds/worker/fetches/sysroot-x86_64-linux-gnu/usr/include/harfbuzz -I/builds/worker/fetches/sysroot-x86_64-linux-gnu/usr/include/atk-1.0 -I/builds/worker/fetches/sysroot-x86_64-linux-gnu/usr/include/at-spi2-atk/2.0 -I/builds/worker/fetches/sysroot-x86_64-linux-gnu/usr/include/at-spi-2.0 -I/builds/worker/fetches/sysroot-x86_64-linux-gnu/usr/include/dbus-1.0 -I/builds/worker/fetches/sysroot-x86_64-linux-gnu/usr/lib/x86_64-linux-gnu/dbus-1.0/include -I/builds/worker/fetches/sysroot-x86_64-linux-gnu/usr/include/gtk-3.0/unix-print -pthread -fno-strict-aliasing -ffp-contract=off  -MD -MP -MF .deps/Unified_cpp_dom_base2.o.pp   Unified_cpp_dom_base2.cpp
[task 2024-05-24T14:43:43.284Z] 14:43:43     INFO -  In file included from Unified_cpp_dom_base2.cpp:38:
[task 2024-05-24T14:43:43.286Z] 14:43:43    ERROR -  /builds/worker/checkouts/gecko/dom/base/Document.cpp:3908:9: error: Refcounted variable 'this' of type 'mozilla::dom::Document' cannot be captured by a lambda
[task 2024-05-24T14:43:43.287Z] 14:43:43     INFO -   3908 |         mFeaturePolicy->InheritPolicy(aContainerFeaturePolicy);
[task 2024-05-24T14:43:43.287Z] 14:43:43     INFO -        |         ^
[task 2024-05-24T14:43:43.288Z] 14:43:43     INFO -  /builds/worker/checkouts/gecko/dom/base/Document.cpp:3908:9: note: Please consider using a smart pointer
[task 2024-05-24T14:43:43.288Z] 14:43:43    ERROR -  /builds/worker/checkouts/gecko/dom/base/Document.cpp:3916:11: error: Refcounted variable 'this' of type 'mozilla::dom::Document' cannot be captured by a lambda
[task 2024-05-24T14:43:43.288Z] 14:43:43     INFO -   3916 |           mFeaturePolicy->InheritPolicy(containerFeaturePolicy);
[task 2024-05-24T14:43:43.288Z] 14:43:43     INFO -        |           ^
[task 2024-05-24T14:43:43.288Z] 14:43:43     INFO -  /builds/worker/checkouts/gecko/dom/base/Document.cpp:3916:11: note: Please consider using a smart pointer
[task 2024-05-24T14:43:43.289Z] 14:43:43     INFO -  2 errors generated.
[task 2024-05-24T14:43:43.289Z] 14:43:43    ERROR -  gmake[4]: *** [/builds/worker/checkouts/gecko/config/rules.mk:689: Unified_cpp_dom_base2.o] Error 1
[task 2024-05-24T14:43:43.289Z] 14:43:43     INFO -  gmake[4]: Leaving directory '/builds/worker/workspace/obj-build/dom/base'

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9b708a1dc402
Move responsibility of FeaturePolicy initialization to nsILoadInfo. r=freddyb,necko-reviewers,jesup,dom-core,sefeng

Comment 8

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

Backed out for causing browser-chrome failures in browser_permission_delegate_geo.js:
https://hg.mozilla.org/integration/autoland/rev/c13e7cf787c87a01ef7c70b1a3043fb3d9e8890e

Push with failures
Failure line

[task 2024-05-27T15:57:03.655Z] 15:57:03     INFO - Entering test bound testExplicitlyAllowedInChain
[task 2024-05-27T15:57:03.656Z] 15:57:03     INFO - Buffered messages logged at 15:56:24
[task 2024-05-27T15:57:03.656Z] 15:57:03     INFO - Console message: [JavaScript Warning: "Partitioned cookie or storage access was provided to https://example.org/browser/browser/base/content/test/permissions/permissions.html because it is loaded in the third-party context and dynamic state partitioning is enabled."]
[task 2024-05-27T15:57:03.657Z] 15:57:03     INFO - Console message: [JavaScript Warning: "Partitioned cookie or storage access was provided to https://example.org/browser/browser/base/content/test/permissions/permissions.html because it is loaded in the third-party context and dynamic state partitioning is enabled."]
[task 2024-05-27T15:57:03.658Z] 15:57:03     INFO - Console message: [JavaScript Warning: "Partitioned cookie or storage access was provided to https://example.org/browser/browser/base/content/test/permissions/permissions.html because it is loaded in the third-party context and dynamic state partitioning is enabled."]
[task 2024-05-27T15:57:03.660Z] 15:57:03     INFO - Console message: [JavaScript Warning: "Partitioned cookie or storage access was provided to https://example.org/browser/browser/base/content/test/permissions/permissions.html because it is loaded in the third-party context and dynamic state partitioning is enabled."]
[task 2024-05-27T15:57:03.660Z] 15:57:03     INFO - Buffered messages finished
[task 2024-05-27T15:57:03.660Z] 15:57:03     INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/permissions/browser_permission_delegate_geo.js | Test timed out - 

Comment 9

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

There are some r+ patches which didn't land and no activity in this bug for 2 weeks.
:farre, could you have a look please?
If you still have some work to do, you can add an action "Plan Changes" in Phabricator.
For more information, please visit BugBot documentation.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/017d0d5be68b
Move responsibility of FeaturePolicy initialization to nsILoadInfo. r=freddyb,necko-reviewers,jesup,dom-core,sefeng

Comment 11

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

https://hg.mozilla.org/mozilla-central/rev/017d0d5be68b

Comment 12

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

The patch landed in nightly and beta is affected.
:farre, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox128 to wontfix.

For more information, please visit BugBot documentation.

Comment 13

[Andreas Farre [:farre]]

I'm not really sure if this is important enough to warrant an uplift, but I think I need to defer to someone on the security team.

Comment 14

[Christoph Kerschbaumer [:ckerschb}]

(In reply to Andreas Farre [:farre] from comment #13)

I'm not really sure if this is important enough to warrant an uplift, but I think I need to defer to someone on the security team.

Yes, I think it's worth an uplift. Thank you!

Comment 15

[Andreas Farre [:farre]]

Comment on attachment 9395993 [details]
Bug 1890748 - Move responsibility of FeaturePolicy initialization to nsILoadInfo. r=freddyb!

Beta/Release Uplift Approval Request

• User impact if declined: Then cross-origin embeds/objects (and probably iframes) can obtain permissions of the top-level origin through carefully crafted, albeit highly intermittent, loads.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): I've marked it as medium since it does change how cross-origin permissions are communicated, which is a significant change. It does follow the old flow, but is more pro-active which is fixing the issue.
• String changes made/needed:
• Is Android affected?: Yes

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: The fact that it's possibly, albeit intermittent, to obtain permissions that should be disallowed warrants an ESR uplift.
• User impact if declined: Then cross-origin embeds/objects (and probably iframes) can obtain permissions of the top-level origin through carefully crafted, albeit highly intermittent, loads.
• Fix Landed on Version: 129
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): I've marked it as medium since it does change how cross-origin permissions are communicated, which is a significant change. It does follow the old flow, but is more pro-active which is fixing the issue.

Comment 16

[Andreas Farre [:farre]]

Tests landed on 129 in https://phabricator.services.mozilla.com/D140817, but not verified.
Tests for this patch are on https://bugzilla.mozilla.org/attachment.cgi?id=9395994, but haven't landed yet.

Comment 17

[Donal Meehan [:dmeehan - PTO - Back Oct 13]]

Comment on attachment 9395993 [details]
Bug 1890748 - Move responsibility of FeaturePolicy initialization to nsILoadInfo. r=freddyb!

Approved for 128.0b9

Comment 18

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/78504ab399e7

Comment 19

[Donal Meehan [:dmeehan - PTO - Back Oct 13]]

:farre there are conflicts when grafting to esr115. Could you please attach a rebased patch?

Comment 20

[Andreas Farre [:farre]]

Attachment: WIP: Bug 1890748 - Move responsibility of FeaturePolicy initialization to nsILoadInfo. r=freddyb,necko-reviewers,jesup,dom-core,sefeng

Comment 21

[Andreas Farre [:farre]]

I submitted a rebased patch as WIP on phabricator: D215192

Do I need to have that reviewed as well?

Comment 22

[Donal Meehan [:dmeehan - PTO - Back Oct 13]]

(In reply to Andreas Farre [:farre] from comment #21)

Do I need to have that reviewed as well?

Thanks, no need for a review on rebased patches. I can take it from here.

Comment 23

[Donal Meehan [:dmeehan - PTO - Back Oct 13]]

Comment on attachment 9395993 [details]
Bug 1890748 - Move responsibility of FeaturePolicy initialization to nsILoadInfo. r=freddyb!

Approved for 115.13esr.

Comment 24

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/74026d54da53

Comment 25

[Simon Friedberger [:simonf]]

Attachment: advisory.txt

Comment 26

[Simon Friedberger [:simonf]]

Attachment: advisory.txt