EnsureCSPDoesNotBlockStringCompilation: Explain why we need to check TrustedScript's data (and add tests)

[fred-wang]

cc @koto @lukewarlow

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

• If bodyString is not equal to bodyArg’s data, set isTrusted to false.
• if parameterStrings[index] is not equal to arg’s data, set isTrusted to false.

but I couldn't find any explanation in the CSP spec or in https://github.com/tc39/proposal-dynamic-code-brand-checks / https://tc39.es/proposal-dynamic-code-brand-checks

I was first thinking maybe this is because PerformEval/CreateDynamicFunction and EnsureCSPDoesNotBlockStringCompilation can be executed in different processes so we can't trust the data pass via IPC communication, but I'm not quite sure about that and there is not indication in the spec that would suggest that.

The other thing I considered is the fake TrustedScript object mentioned in https://w3c.github.io/trusted-types/dist/spec/#dom-trustedtypepolicyfactory-ishtml ; it's easy to create a fake TrustedScript that serializes to something different from its data. Indeed, CreateDynamicFunction would then produce bodyString and parameterStrings that are different from bodyArg and parameterArgs's data and the sourceString produced would need validation. However, in the case of PerformEval, HostGetCodeForEval will always be called first to extract data from TrustedType objects so this mismatch can't happen.

Finally, I didn't find any TrustedScript WPT test in content-security-policy/, while a run of trusted-types/ tests does not seem to hit any string mismatch. So I believe we don't have any test coverage for these conditions.

So to summarize:

1. Explaining why these string checks are needed would be great, probably as a note in the CSP spec.
2. That would help to write WPT tests to cover these checks (I plan to add some for new Function, based on fake TrustedScript objects).
3. (minor) If they are indeed not necessary for direct/indirect evals, then implementers could optimize a bit things by skipping the string checks in that case.

[fred-wang]

OK, I tried https://phabricator.services.mozilla.com/D230236 but the FakeTrustedScript object does not implement TrustedScript (IIUC https://webidl.spec.whatwg.org/#implements seems stronger than "is an instance of"). So although the algorithm is called with potential string mismatch, the string check is never reached anyway. So it's still not clear why these checks are necessary if objects implementing TrustedScript always serializes the same as their internal data...

[koto]

What likely needs confirming is whether WebIDL's implements indeed would return false for any kind of objects that were created outside of policies or TT APIs. The slot checks and comparing the strings were introduced in the past to assert that, but the implements was added later.

[lukewarlow]

The string check is related to forging the toString on a trusted script object.

It's possible there doesn't exist test coverage yet (it might be added by my WebKit pr which is yet to be merged).

Off the top of my head I think you're correct that they probably can be skipped for eval (would have to check my WebKit PR).

[fred-wang]

What likely needs confirming is whether WebIDL's implements indeed would return false for any kind of objects that were created outside of policies or TT APIs.

If that's the case, then probably isHTML(value) and friends could just be redefined as "return whether value implements TrustedHTML" (but this is a discussion for the Trusted Types spec).

The string check is related to forging the toString on a trusted script object.

OK, so that would be like in https://phabricator.services.mozilla.com/D230236 but replacing fake = new FakeTrustedScript("a", "A") with fake = policyNoChange.createScript("a"); fake.toString = () => { return "A"; }. I guess that would work indeed (haven't tried yet).

It's possible there doesn't exist test coverage yet (it might be added by my WebKit pr which is yet to be merged).

Do you mean https://github.com/WebKit/WebKit/pull/27878 ? I can't find any match for "toString" there...

Off the top of my head I think you're correct that they probably can be skipped for eval (would have to check my WebKit PR).

:+1:

[fred-wang]

Regarding tests, I've updated https://phabricator.services.mozilla.com/D230369 and they cover the case when isTrusted becomes false, due to string mismatch (by forging a toString on a trusted script object).

[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 https://github.com/w3c/webappsec-csp/issues/697