EnsureCSPDoesNotBlockStringCompilation: Why do we need to call "Get Trusted Type compliant string" when isTrusted is true?

[fred-wang]

cc @koto @lukewarlow

See https://w3c.github.io/webappsec-csp/#can-compile-strings

Steps 5-8 are as follows:

5. Let sourceToValidate be a new TrustedScript object created in realm whose data is set to codeString if isTrusted is true, and codeString otherwise.
6. Let sourceString be the result of executing the Get Trusted Type compliant string algorithm, with TrustedScript, realm, sourceToValidate, compilationSink, and 'script'.
7. If the algorithm throws an error, throw an EvalError.
8. If sourceString is not equal to codeString, throw an EvalError.

When isTrusted is false, then it makes sense to make sure codeString is not modified by "Get Trusted Type compliant string".

But when isTrusted is true, step 5) will just create a TrustedType with data set to codeString, step 6) will set set sourceString to codeString (https://www.w3.org/TR/trusted-types/#abstract-opdef-get-trusted-type-compliant-string exits early at step 1 and per https://www.w3.org/TR/trusted-types/#trusted-script stringified input is the same as returning the codeString data) ; step 7) will see no errors thrown and step 8) will see sourceString equal to codeString.

So why not just do If isTrusted is true then set sourceString to codeString, otherwise do the "Get Trusted Type compliant string" steps?

[fred-wang]

When isTrusted is false, then it makes sense to make sure codeString is not modified by "Get Trusted Type compliant string".

Assuming there is a situation in #49371 where isTrusted can become false because of a mismatch between the passed arg's data and the passed string, I'm actually not even sure if step 8 is correct:

• With a silly default policy that performs no changes, the algorithm would continue with the unmodified codeString. But there is no reason why codeString would be safer than bodyString or parameterStrings.
• With a default policy that "sanitizes" codeString, the algorithm will throw an error due to a mismatch with codeString, while "Get Trusted Type compliant string" is actually meant to provide some kind of safety.

So maybe when isTrusted=false what we want instead is to redo the work of CreateDynamicFunction to build the source string of the function from the trusted data and then call "Get Trusted Type compliant string" on it. That would be similar to what we do for document.write: https://html.spec.whatwg.org/multipage/dynamic-markup-insertion.html#document-write-steps is doing. But then the "kind" argument would need to be propagated via HostEnsureCanCompileStrings.

A less strict approach, is to remove condition 8 so the algorithm can continue with the sourceString that has been "sanitized" by the default policy.

[lukewarlow]

It's explicitly designed so that if the default policy changes the string then it rejects for eval and Function.

Loosening that would require ecma changes that would require going back to them to request. It's stricter specifically based on historical feedback from tc39.

[fred-wang]

OK, agree it provides some stronger check but I still feel this is less intuitive than something like what document.write() do. If kept as is, probably a note in the spec explaining the reasoning would help for people who don't have the full context of the tc39 discussions.

And I still don't see why this is necessary when isTrusted=true.

[fred-wang]

The tests added in https://phabricator.services.mozilla.com/D230369 now check step 8 i.e. throw iff the codeString is modified by "Get Trusted Type compliant string" (eval-function-constructor.html is using a null default policy, so algorithm is exiting at step 7).

[fred-wang]

I just realized I opened this to the wrong repo. I meant to do it in CSP spec repo.

[fred-wang]

Moved to w3c/webappsec-csp#698