1909168 - Implement a fast-path for injection sinks when trusted types aren't used

Status: RESOLVED FIXED

(Core :: DOM: Security, task)

Description

[Mirko Brodesser (:mbrodesser-Igalia), offline until 10th of Feb]

See https://phabricator.services.mozilla.com/D216170#7453441.

Let's implement default policy support first. That needs to be done anyway and will shed light on whether this fast-path is indeed required.

Comment 1

[Mirko Brodesser (:mbrodesser-Igalia), offline until 10th of Feb]

@Daniel: not yet working on this. Will presumably in the future.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

This is definitely a blocker for trusted types. DoesSinkTypeRequireTrustedTypes is rather slow.

Comment 3

[Mirko Brodesser (:mbrodesser-Igalia), offline until 10th of Feb]

A ./mach try perf run (still in progress) with TT enabled by default: https://treeherder.mozilla.org/perfherder/compare?originalProject=try&originalRevision=2aada6496bec8cf06b92125509ff8a42386f0fd2&newProject=try&newRevision=e40d1333e2836a8ef7bbd432e25ca0d8ec6818da&framework=13 to avoid premature optimization.

Edit: above perf run seems to miss the commits for TT, except the one flipping the pref.

Comment 4

[Frédéric Wang (:fredw)]

Assigning myself, since I will give this a try.

Comment 5

[Frédéric Wang (:fredw)]

(In reply to Mirko Brodesser (:mbrodesser-Igalia) from comment #0)

Let's implement default policy support first. That needs to be done anyway and will shed light on whether this fast-path is indeed required.

Since you suggested that I take a look at bug 1909168 but didn't mention bug 1903717, and per comment 2, I assume we should do the fast path anyway, so no need to wait for default policy to be implemented. So I'll remove the corresponding dependency, but please correct me if I'm making a wrong assumption.

Comment 6

[Frédéric Wang (:fredw)]

Attachment: Bug 1909168 - Add fast paths for sink checks when trusted types are not used. r=smaug

This introduces a boolean on nsCSPPolicy / nsIContentSecurityPolicy
that is initially false and only set to true after a
REQUIRE_TRUSTED_TYPES_FOR_DIRECTIVE directive is added to the policy
/ set of policies. This provides fast check and early return when such
a directive is absent:

• TrustedTypeUtils's DoesSinkTypeRequireTrustedTypes
• TrustedTypeUtils's ShouldSinkTypeMismatchViolationBeBlockedByCSP
• nsCSPPolicy::AreTrustedTypesForSinkGroupRequired

This also makes nsCSPPolicy::hasDirective() fast for
nsIContentSecurityPolicy::REQUIRE_TRUSTED_TYPES_FOR_DIRECTIVE.

Comment 7

[Mirko Brodesser (:mbrodesser-Igalia), offline until 10th of Feb]

(In reply to Frédéric Wang (:fredw) from comment #5)

(In reply to Mirko Brodesser (:mbrodesser-Igalia) from comment #0)

Let's implement default policy support first. That needs to be done anyway and will shed light on whether this fast-path is indeed required.

Since you suggested that I take a look at bug 1909168 but didn't mention bug 1903717, and per comment 2, I assume we should do the fast path anyway, so no need to wait for default policy to be implemented. So I'll remove the corresponding dependency, but please correct me if I'm making a wrong assumption.

Corrected the dependency.

Comment 8

[Frédéric Wang (:fredw)]

This doesn't seem to be fast enough place for the flag.
We need something like this on Document level so that we don't even need to access any CSP object at all when calling insertAdjacentHTML or especially innerHTML.
Any extra memory lookup somewhere like in innerHTML tends to show up in performance profiles.
In a relevant method we do have always access to Document, and we should be able to use that quickly to prevent any need to call any TT code.
https://searchfox.org/mozilla-central/rev/4ac5b1f6cc9d8c186986f52369a2d9a537d10474/dom/base/Element.cpp#4085,4122

I tried something in https://phabricator.services.mozilla.com/D226882, however this is making https://searchfox.org/mozilla-central/source/testing/web-platform/tests/trusted-types/trusted-types-createHTMLDocument.html fail...

https://searchfox.org/mozilla-central/rev/b655023a1978bcad8cc2fb038b89a058b45e91a7/dom/security/trusted-types/TrustedTypeUtils.cpp#228

ends up using a mDoc != pinnedDoc here, and this is obtained via multiple memory lookup:

https://searchfox.org/mozilla-central/rev/b655023a1978bcad8cc2fb038b89a058b45e91a7/dom/base/nsGlobalWindowInner.cpp#5863
https://searchfox.org/mozilla-central/rev/b655023a1978bcad8cc2fb038b89a058b45e91a7/dom/base/nsGlobalWindowInner.cpp#2526
https://searchfox.org/mozilla-central/rev/b655023a1978bcad8cc2fb038b89a058b45e91a7/dom/security/trusted-types/TrustedTypeUtils.cpp#201

So I'm not really sure what to do... am I suppose to propagate the flag from the nsGlobalWindowInner's mDoc to pinnedDoc?

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Ok, so trusted-types-createHTMLDocument.html creates "data" documents. Those don't have their own globals.
In such case we could get the flag from scopeObject->mDoc. A bit slow still, but better than nothing.

So, could the check be (pseudo code)
if (doc->UsesTrustedTypes() || (doc->IsLoadedAsData() && doc->GetScopeObject()->GetExtantDoc()->UsesTrustedTypes())) {
...
}

That might be still a bit too slow. Could use some profiling.
If it is too slow, then inner window could have a list to all the documents which have it as the scope object. And whenever TT status changes, all the documents would be updated.

Comment 10

[Frédéric Wang (:fredw)]

Thanks, I've updated D226882 to handle IsLoadedAsData() specially, let's see if that's good enough.

I can't find the any result for the link from comment 3, so I resent a try perf link checking the effect of turning on dom.security.trusted_types.enabled:

https://perf.compare/compare-results?baseRev=47edea7b487da5f1a453b800dc37b71d11e578a2&newRev=a4ef3fe2d6fb732b2268bca114f0b94f31e77a00&baseRepo=try&newRepo=try&framework=1

And this is the same with https://phabricator.services.mozilla.com/D226882?id=936730 applied:

https://perf.compare/compare-results?baseRev=47edea7b487da5f1a453b800dc37b71d11e578a2&newRev=d2df3641c8b19a46a0fa74f71a2de682c1232155&baseRepo=try&newRepo=try&framework=1

Comment 11

[Frédéric Wang (:fredw)]

Attachment: innerhtml_nonsimple_nostyle_data_document_test.xhtml

Attached is a modified version of https://mozilla.pettay.fi/moztests/innerhtml_nonsimple_nostyle_test.xhtml that sets innerHTML on a "data" document.

Comment 12

[Frédéric Wang (:fredw)]

In the profile I'm obtaining for innerhtml_nonsimple_nostyle_test.xhtml with dom.security.trusted_types enabled and without D226882, GetTrustedTypesCompliantString represents a small (but big enough to be visible on the flame graph) portion of mozilla::dom::Element::SetInnerHTML.

The profile with D226882 for that test case shows the same amount of time as when dom.security.trusted_types is disabled. So as expected this patch addresses the case of non-data documents without trusted types.

Attachment 9434166 [details] is a modified version of the test case that uses "data" document instead. The profile with D226882 shows something similar to what I described above: GetTrustedTypesCompliantString represents a small but visible portion of mozilla::dom::Element::SetInnerHTML.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cb0ae7dcb963
Add fast paths for sink checks when trusted types are not used. r=smaug

Comment 14

[pstanciu]

https://hg.mozilla.org/mozilla-central/rev/cb0ae7dcb963