Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add script protection mechanisms to SVGScriptElement #524

Closed
wants to merge 1 commit into from
Closed

Add script protection mechanisms to SVGScriptElement #524

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 39 additions & 9 deletions spec/index.bs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,7 @@ spec:ECMA-262; urlPrefix: https://tc39.github.io/ecma262/
spec: HTML; urlPrefix: https://html.spec.whatwg.org/
type: dfn; text: prepare the script element; url: prepare-the-script-element
type: dfn; text: The text insertion mode; url: parsing-main-incdata
type: dfn; text: The rules for parsing tokens in foreign content; url: parsing-main-inforeign
type: dfn; text: reentrant invocation of the parser; url: nestedParsing
type: dfn; text: get the text steps; url: get-the-text-steps
type: dfn; text: set the inner text steps; url: set-the-inner-text-steps
Expand Down Expand Up @@ -1070,8 +1071,8 @@ Given a {{TrustedType}} type (|expectedType|), a [=realm/global object=] (|globa

Given an {{HTMLScriptElement}} (|script|), this algorithm performs the following steps:

1. If |script|'s [=script text=] value is not equal to its [=child text content=],
set |script|'s [=script text=] to the result of executing [$Get Trusted Type compliant string$], with the following arguments:
1. If |script|'s [=HTMLScriptElement/script text=] value is not equal to its [=child text content=],
set |script|'s [=HTMLScriptElement/script text=] to the result of executing [$Get Trusted Type compliant string$], with the following arguments:
* {{TrustedScriptURL}} as |expectedType|,
* |script|'s {{Document}}'s [=relevant global object=] as |global|,
* |script|'s [=child text content=] attribute value,
Expand Down Expand Up @@ -1177,14 +1178,21 @@ This document modifies {{HTMLScriptElement}}s. Each script has:
through a compliant sink. Equivalent to script's
[=child text content=]. Initially an empty string.

This document also modifies {{SVGScriptElement}}s. Each script has:

: an associated string <dfn export for="SVGScriptElement">script text</dfn>.
:: A string, containing the body of the script to execute that was set
through a compliant sink. Equivalent to script's
[=child text content=]. Initially an empty string.

#### The {{HTMLScriptElement/innerText}} IDL attribute #### {#the-innerText-idl-attribute}

The {{HTMLScriptElement/innerText}} setter steps are:

1. Let |value| be the result of calling [$Get Trusted Type compliant string$] with
{{TrustedScript}}, [=this=]'s [=relevant global object=], the given value, `HTMLScriptElement innerText`, and
`script`.
1. Set [=this=]'s [=script text=] value to |value|.
1. Set [=this=]'s [=HTMLScriptElement/script text=] value to |value|.
1. Run [=set the inner text steps=] with [=this=] and |value|.

The {{HTMLScriptElement/innerText}} getter steps are:
Expand All @@ -1199,7 +1207,7 @@ empty string instead, and then do as described below:
1. Let |value| be the result of calling [$Get Trusted Type compliant string$] with
{{TrustedScript}}, [=this=]'s [=relevant global object=], the given value, `HTMLScriptElement textContent`, and
`script`.
1. Set [=this=]'s [=script text=] value to |value|.
1. Set [=this=]'s [=HTMLScriptElement/script text=] value to |value|.
1. Run [=set text content=] with [=this=] and |value|.

The {{HTMLScriptElement/textContent}} getter steps are:
Expand All @@ -1213,7 +1221,7 @@ Update the {{HTMLScriptElement/text}} setter steps algorithm as follows.
1. <ins>Let |value| be the result of calling [$Get Trusted Type compliant string$] with
{{TrustedScript}}, [=this=]'s [=relevant global object=], the given value, `HTMLScriptElement text`, and
`script`.</ins>
1. <ins>Set [=this=]'s [=script text=] value to the given value.</ins>
1. <ins>Set [=this=]'s [=HTMLScriptElement/script text=] value to the given value.</ins>
1. [=String replace all=] with the given value within [=this=].


Expand All @@ -1228,7 +1236,7 @@ The {{HTMLScriptElement/src}} setter steps are:

#### Setting slot values from parser #### {#setting-slot-values-from-parser}

This document modifies the HTML parser to set the [=script text=] value when the script is created.
This document modifies the HTML parser to set the [=HTMLScriptElement/script text=] value when the script is created.

Modify the [=The text insertion mode=] algorithm as follows:

Expand All @@ -1237,7 +1245,7 @@ Modify the [=The text insertion mode=] algorithm as follows:
<dd>
<p>...</p>

<ins><p>Set <var>script</var>'s [=script text=] value to its [=child text content=].</p></ins>
<ins><p>Set <var>script</var>'s [=HTMLScriptElement/script text=] value to its [=child text content=].</p></ins>

<p>If the <span>active speculative HTML parser</span> is null, then <span>prepare the script
element</span> <var>script</var>. This might cause some script to execute, which might cause
Expand All @@ -1248,7 +1256,27 @@ Modify the [=The text insertion mode=] algorithm as follows:
</dd>
</dl>

Issue: The above algorithm doesn't account for the case when the script element's content is changed mid-parse. Implementors should ensure they protect against this case. See [https://github.com/w3c/trusted-types/issues/507](https://github.com/w3c/trusted-types/issues/507).
This document also modifies the HTML parser to set the [=SVGScriptElement/script text=] value when the script is created.

Modify the [=The rules for parsing tokens in foreign content=] as follows:

<dl class="switch">
<dt id="scriptForeignEndTag">An end tag whose tag name is "script", if the <span>current
node</span> is an <span>SVG <code>script</code></span> element</dt>
<dd>
<p>...</p>

<ins><p>Set <var>script</var>'s [=SVGScriptElement/script text=] value to its [=child text content=].</p></ins>

<p>If the <span>active speculative HTML parser</span> is null and the user agent supports SVG,
then <a href="https://www.w3.org/TR/SVGMobile12/script.html#ScriptContentProcessing">Process the
SVG <code data-x="">script</code> element</a> according to the SVG rules.

<p>...</p>
</dd>
</dl>

Issue: The above algorithms don't account for the case when the script element's content is changed mid-parse. Implementors should ensure they protect against this case. See [https://github.com/w3c/trusted-types/issues/507](https://github.com/w3c/trusted-types/issues/507).

#### Slot value verification #### {#slot-value-verification}

Expand All @@ -1270,10 +1298,12 @@ The first few steps of the [=prepare the script element=] algorithm are modified
run when the parser tries to run it, but it is later executed after a script dynamically
updates it, it will execute in an async fashion even if the <code id=script-processing-model:attr-script-async-5><a href=https://html.spec.whatwg.org/#attr-script-async>async</a></code> attribute isn't set.</p>
<li><ins><p>Execute the [$Prepare the script text$] algorithm on <var>el</var>. If that algorithm threw an error, then return.</p></ins></li>
<li><p>Let <var ignore="">source text</var> be <var>el</var>'s <del><a id=script-processing-model:child-text-content href=https://dom.spec.whatwg.org/#concept-child-text-content data-x-internal=child-text-content>child text content</a>.</del> <ins>[=script text=] value.</ins>
<li><p>Let <var ignore="">source text</var> be <var>el</var>'s <del><a id=script-processing-model:child-text-content href=https://dom.spec.whatwg.org/#concept-child-text-content data-x-internal=child-text-content>child text content</a>.</del> <ins>[=HTMLScriptElement/script text=] value.</ins>
<li>...
</ol>

Issue: There's no proper definition for the processing of SVG script elements. However, you should apply a similar change to the processing of {{SVGScriptElement}}s.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@annevk can you think of a better way to word this, or a way to actually spec this?

I could try patching https://www.w3.org/TR/SVGMobile12/script.html#ScriptContentProcessing but it's a bit lose on the details and is fairly old, so idk that we'd ever upstream the patching.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The long term solution is probably for HTML to define both script elements, similar to how they are implemented.

Short term we might get away with some issue markers?

It's still not entirely clear to me that "script text" is a good approach as it essentially doubles the storage cost. What happened to some kind of boolean that we'd invalidate?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The boolean bit is currently only used as part of the "have the children been changed by an API during parsing" code.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem with a Boolean bit is how to allow the specific IDL and the "actually parsed" stuff but nothing else. I'm currently unable to think how you would do that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything else that mutates would end up flipping the bit? It might help if we went through all the entry points we are concerned about or why this would be hard for some of them.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still at 1 boolean. Presumably if a script set is trustworthy you can just set the existing bit to trustworthy in the end.

Copy link
Member Author

@lukewarlow lukewarlow Jul 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yeah actually we don't need a parse specific one anymore and provided it's all synchronous you're right you could set the "trusted" bit to true after the children changed steps (which would set it to false for all API calls).

If we make children changed always untrust API changes I think that solves any weirdness from mutation events too. Because we'd fail safe and the script would be untrusted. Because we only need the 3 sanctioned APIs to behave properly wrt to mutation events.

Thinking on it some more I think that could work?

cc @otherdaniel what do you think about that (Assuming we can get children changed steps spec to correctly tell us if it's API or parser)?

Relatively small changes needed (assuming my mental model is right) such as that below and then updating the children changed steps and the prepare script steps:

image

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking on this some more it would also fix #517 (different handling of new lines leading to TT errors)

Copy link
Member Author

@lukewarlow lukewarlow Jul 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay so I tried implementing this and hit a snag.

https://html.spec.whatwg.org/#prepare-the-script-element:~:text=When%20a%20script%20element%20el%20that%20is%20not%20parser%2Dinserted%20experiences

This bit of the HTML spec is ran inside of children changed steps (at least in WebKit and I believe Chromium from a quick look), so prepare the script runs before the isTrusted flag is set back to true at the end of setInnerText.

So that seems to make the above plan not possible?

Edit: I think I've got a way that works using two booleans.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've opened a new draft PR which I'll use to do this new model (and will also apply to svg as best it can) #533


### HostEnsureCanCompileStrings ### {#host-ensure-can-compile-strings}

JavaScript contains an <span>implementation-defined</span> <a href="https://tc39.es/ecma262/#sec-hostensurecancompilestrings">HostEnsureCanCompileStrings</a>(<var>realm</var>, <ins><var>parameterStrings</var>,
Expand Down
Loading