CSP sample for eval and Function

[lukewarlow]

This issue is to discuss the exact specifics of the CSP violation sample.

Chrome currently has some oddly specific behaviour which isn't specced.

eval('alert(1)'); -> eval|alert(1) - This direct eval example follows the spec and makes sense.

eval?.('alert(1)'); -> eval|alert(1) - This indirect eval example follows the spec and makes sense.

new Function('param1', 'body'); -> Function|(param1\n) {\nbody\n}) - This function constructor example, doesn't match the spec. I would expect something like Function|function anonymous(param1\n) {\nbody\n} which is what is currently specced.

const GeneratorFunction = function* () {}.constructor;
new GeneratorFunction('param1', 'body');

-> eval|(function* anonymous(param1\n) {\nbody\n}) This GeneratorFunction constructor example doesn't match the spec, and seems inconsistent with Function constructor.

As specced this should be Function|function* anonymous(param1\n) {\nbody\n}

const AsyncFunction = async function () {}.constructor;
new AsyncFunction('param1', 'body');

-> eval|(async function anonymous(param1\n) {\nbod This AsyncFunction constructor example doesn't match the spec, and is inconsistent with Function constructor within Chromium.

As specced this should be Function|async function anonymous(param1\n) {\nbody

[lukewarlow]

This issue is to check what we want the behaviour to be here.

If the (async function anonymous block isn't considered useful that is fine but we must spec how to strip that from the beginning of the sample. Or more accurately we should spec such that the sample is just built from the bodyString and parameterStrings list rather than being the code string that then gets mangled?

So I guess 2 questions,

1. do we keep or strip the "prefix" from the sample?
2. Should non-general function constructor usages just be marked as eval? If that's the case it begs the question should we even bother with the distinction?

[lukewarlow]

@koto @mbrodesser-Igalia @annevk @otherdaniel

ccing for thoughts.

For context, https://chromium-review.googlesource.com/c/chromium/src/+/2235621 removed the "prefix" from Function.

https://chromium-review.googlesource.com/c/chromium/src/+/2584024 (by @otherdaniel) seems to be where Chrome's current decision on Function vs eval comes from.

[lukewarlow]

My gut feeling is that its maybe not worth trying to be clever with stripping prefixes off sample strings? But if we do we should probably be stripping them off generator / async functions aswell. But then that might mean we're losing useful context?

Perhaps we should instead of just doing eval or Function also do GeneratorFunction, AsyncFunction, AsyncGeneratorFunction?

People's thoughts would be much appreciated.

[lukewarlow]

Worth noting that the issue between eval vs Function (or something else) for the exotic function types also applies to default policies too which also gets the "sink" as eval in these cases.

[lukewarlow]

Coming back to this in my WebKit patch I currently have Function|async function anonymous(param1\n) {\nbody etc correctly identifying Functions as such. This is correct behaviour imo, the only thing that might be questionable is whether we strip the prefixes (async function anonymous)?

[mbrodesser-Igalia]

Coming back to this in my WebKit patch I currently have Function|async function anonymous(param1\n) {\nbody etc correctly identifying Functions as such. This is correct behaviour imo, the only thing that might be questionable is whether we strip the prefixes (async function anonymous)?

@otherdaniel do you remember why async function anonymous was removed? https://chromium-review.googlesource.com/c/chromium/src/+/2235621 's commit message and comments don't reveal it.

[lukewarlow]

The issue https://issues.chromium.org/issues/40672094 seems to just say that having function anonymous takes up space of the 40 characters and that's not super useful information. Which I do understand but Chrome's implementation was never fed back into the spec, and is also not consistent. As I say any of the more exotic function types still report as eval with all the extra text.

Could we perhaps just increase the number of characters we report in the sample? Trusted Types already doesn't obey the general 40 character limit (the sink name is extra). And that would prevent any need to strip out text from the sample?

[otherdaniel]

As I remember, this was indeed about the 40 char limit. We got user reports (from our own ISE) that the CSP violation reports were difficult to match up to actual source code, because -- especially with mini-fied source code -- there wasn't much "unique" information left in the sample. Apologies for not feeding this back into the spec.

The fourty character limit comes from CSP (see the note at the end of https://www.w3.org/TR/CSP3/#framework-violation) and specifically refers to "first 40 characters of an inline script", not field length. I think this is why we felt compelled to stick to 40 chars of script sample; and why the sink name didn't count.

Could we perhaps just increase the number of characters we report in the sample? Trusted Types already doesn't obey the general 40 character limit (the sink name is extra). And that would prevent any need to strip out text from the sample?

I'm happy to reconsider any of this, given how ad-hoc the current implementation is. But I do think we stayed within letter and intent of the CSP spec with the 40 char limit; while a longer sample text probably wouldn't.