1771685 - (CVE-2022-38473) Cross-origin frames can obtain top-level permissions b/c XSLT transform resets FeaturePolicy

Status: VERIFIED FIXED

(Core :: XSLT, defect, P2)

Description

[Armin Ebert [:arminius]]

Attachment: Proof of concept to be loaded in a cross-origin iframe

Cross-origin children can obtain top-level permissions using XSLT because transformation effectively resets the document's FeaturePolicy.

Impact

The attack potential is very similar to bug 1755081. An untrusted cross-origin <iframe> may hijack the top-level origin's permissions to access media devices like the webcam, or prompt for permissions in the top-level origin's name.

Details

During XSL transformation, several properties of the source document are transferred to the result document in URIUtils::ResetWithSource(). Currently this doesn't consider the document's FeaturePolicy.

However, it's necessary to explicitly set the correct FP here, because the result document never runs Document::StartDocumentLoad() => Document::InitFeaturePolicy() to set the policy itself. Hence, the FP remains at its permissive default which, in a nested context (viz. cross-origin iframe), is equivalent to a blanket delegation of all permissions by the top-level context.

Proof of concept

Load the attached PoC in a cross-origin frame. Click the buttons to demo microphone/fullscreen access. Note that parent doesn't require HTTPS, but the PoC itself needs to be served securely so that navigator.mediaDevices is available.

The fullscreen case even works if HTML-sandboxed via e.g.:

<iframe sandbox="allow-scripts" src="https://untrusted.example/poc-with-acao-header.xml"></iframe>

Comment 1

[Armin Ebert [:arminius]]

Attachment: Mochitest: child document retains original FeaturePolicy after XSL transform

Here's a mochitest which asserts that an XSL-transformed document has the same FeaturePolicy as before.

Comment 2

[Armin Ebert [:arminius]]

Not sure what's the correct approach, but here is just a very rough patch idea to transfer the FP state to the new document.

diff --git a/dom/xslt/base/txURIUtils.cpp b/dom/xslt/base/txURIUtils.cpp
--- a/dom/xslt/base/txURIUtils.cpp
+++ b/dom/xslt/base/txURIUtils.cpp
@@ -9,6 +9,7 @@
 #include "nsIPrincipal.h"
 #include "mozilla/LoadInfo.h"
 #include "mozilla/dom/nsCSPContext.h"
+#include "mozilla/dom/FeaturePolicy.h"
 
 using mozilla::dom::Document;
 using mozilla::net::LoadInfo;
@@ -73,6 +74,21 @@ void URIUtils::ResetWithSource(Document*
   aNewDoc->SetReferrerInfo(sourceDoc->GetReferrerInfo());
   aNewDoc->SetEmbedderPolicy(sourceDoc->GetEmbedderPolicy());
 
+  mozilla::dom::FeaturePolicy* sourceFP = sourceDoc->FeaturePolicy();
+  mozilla::dom::FeaturePolicy* newFP = aNewDoc->FeaturePolicy();
+  newFP->SetInheritedDeniedFeatureNames(
+      sourceFP->InheritedDeniedFeatureNames());
+
+  const auto& declaredString = sourceFP->DeclaredString();
+  if (sourceFP->GetSelfOrigin() && !declaredString.IsEmpty()) {
+    newFP->SetDeclaredPolicy(sourceDoc, sourceFP->DeclaredString(),
+                             sourceFP->GetSelfOrigin(),
+                             sourceFP->GetSrcOrigin());
+  }
+  for (auto& featureName : sourceFP->AttributeEnabledFeatureNames()) {
+    newFP->MaybeSetAllowedPolicy(featureName);
+  }
+
   // Inherit the csp if there is one
   nsCOMPtr<nsIContentSecurityPolicy> csp = sourceDoc->GetCsp();
   if (csp) {

That's essentially the approach taken to sync the FP over IPC from here: https://searchfox.org/mozilla-central/rev/7751fef9eeb3db0a07ae4680daa2a62bd8f49882/dom/security/featurepolicy/FeaturePolicyUtils.cpp#255-264,291-302

Comment 3

[Daniel Veditz [:dveditz]]

Christoph: what other security policies do we need to worry might not be carried over into a generated document if we're getting this one wrong?

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Daniel Veditz [:dveditz] from comment #3)

Christoph: what other security policies do we need to worry might not be carried over into a generated document if we're getting this one wrong?

I guess it's best to look at docshell code whenever we reload a document, or also create an about:blank document.

ReloadDocument might be a good starting point to look what we copy over, because it's not only policies, but also base-uri, triggeringPrincipal, sandbox-flags, ...

We should really have a container-struct which simplifies copying things over. It's on our roadmap and we have Bug 1739442 on file because we consistently run into this problem.

Comment 5

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Peter is working on this.

Comment 6

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

The severity field is not set for this bug.
:mccr8, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 7

[Peter Van der Beken [:peterv]]

Attachment: WIP: Bug 1771685 - Init feature policy correctly.

Comment 8

[Peter Van der Beken [:peterv]]

Attachment: Bug 1771685 - Init feature policy. r?farre!

Comment 9

[Peter Van der Beken [:peterv]]

Attachment: Bug 1771685 - Testcase. r?farre!

Depends on D151516

Comment 10

[Peter Van der Beken [:peterv]]

Comment on attachment 9284953 [details]
Bug 1771685 - Init feature policy. r?farre!

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Patch makes it fairly obvious that it's related to XSLT and feature policy.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Yes
• Which older supported branches are affected by this flaw?: All
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: Should just apply.
• How likely is this patch to cause regressions; how much testing does it need?: Fairly safe, makes sure we apply the feature policy restrictions for XSLT documents.
• Is Android affected?: Yes

Comment 11

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Comment on attachment 9284953 [details]
Bug 1771685 - Init feature policy. r?farre!

Approved to land and uplift

Comment 12

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Comment on attachment 9284954 [details]
Bug 1771685 - Testcase. r?farre!

Can land the test case on/after Oct 5th

Comment 13

[Andrew McCreight [:mccr8]]

Peter's out for a bit, so I'll try to do the landing and uplift requests.

Comment 14

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

Init feature policy. r=farre
https://hg.mozilla.org/integration/autoland/rev/47cd8a35c5953974b051612bae3a80657b05814e
https://hg.mozilla.org/mozilla-central/rev/47cd8a35c595

Comment 15

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

Since nightly and release are affected, beta will likely be affected too.
For more information, please visit auto_nag documentation.

Comment 16

[Dianna Smith [:diannaS]]

Approved for 104.0b5

https://hg.mozilla.org/releases/mozilla-beta/rev/2d846959d7b3

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Landed for 91.13esr.
https://hg.mozilla.org/releases/mozilla-esr91/rev/b503b79de4ed

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Landed for 102.2esr.
https://hg.mozilla.org/releases/mozilla-esr102/rev/858d7fde5f6a

Comment 19

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Attachment: advisory.txt

Comment 20

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Attachment: advisory.txt

Comment 21

[Rares Doghi, Desktop QA]

This issue is Verified as fixed in our latest 91.13.0esr, 102.3.0esr, 104.0.2, 105.0b7 and 106.0a1 (2022-09-04).

Comment 22

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

5 months ago, Tom Ritter [:tjr] placed a reminder on the bug using the whiteboard tag [reminder-test 2022-10-5] .

peterv, please refer to the original comment to better understand the reason for the reminder.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2195a23cce7d
Testcase. r=farre

Comment 24

[Sandor Molnar[:smolnar]]

Backed out for causing mochitest failures in dom/security/featurepolicy/test/mochitest/test_xslt.html

Backout link: https://hg.mozilla.org/integration/autoland/rev/9762f32b72b6d7ba543dc8de722932a3f074c5e8

Push with failures

Failure log: TEST-UNEXPECTED-FAIL | dom/security/featurepolicy/test/mochitest/test_xslt.html | Test timed out