Processing Instructions

[otherdaniel]

whatwg/html#12118 proposes to parse XML-style Processing Instructions in HTML.

Not sure about timeline, and/or if there's much to do for us (yet), but that would invalidate the assertion in step 1.1 of https://wicg.github.io/sanitizer-api/#sanitize-core.

[zcorpan]

cc @foolip @noamr

Options:

• Always drop PIs
• Config to allow all PIs or remove all PIs
• Config to allow certain PI targets
• Config to allow certain PI targets and allow/block pseudo-attributes, similar to elements

[noamr]

• Config to allow certain PI targets

I think this. Maybe allow/remove.

• Config to allow certain PI targets and allow/block pseudo-attributes, similar to elements

[foolip]

The plan is to retain the original unparsed data even if we successfully parse it as attributes, so that makes it a bit challenging to sanitize attributes. If we did, I think we would have to update the data field after sanitizing so that nothing can ride along in it.

But I think just allowing or rejecting at the target level makes sense.

[mozfreddyb]

I think we could introduce a boolean switch, similar to what we have for comments and data-. We should disallowing them by default but still allow users to re-enable them. They can't directly be used for script execution (right? right?) and that's our core criterion for what is/isn't allowable

[otherdaniel]

Precedents:

• DOMPurify -- drops PI unconditionally. src
• Closure HTML sanitizer -- drops PI unconditionally. src

Not sure whether these precedents are relevant in this particular case, though. This is meant to accompany a change in the underlying parser, which existing code can't have known about.

[noamr]

I think we could introduce a boolean switch, similar to what we have for comments and data-.

That's. ok

We should disallowing them by default but still allow users to re-enable them. They can't directly be used for script execution (right? right?)

right!

[annevk]

We should probably think a little bit about the long term and whether we want an allowlist & blocklist for PI targets. Is there a good use case for allowing all of them without scrutiny? Because unlike data- these might incur some user agent processing down the line. We don't know what's going to happen in that regard. And we don't have a separate userland namespace here currently.

[noamr]

We should probably think a little bit about the long term and whether we want an allowlist & blocklist for PI targets. Is there a good use case for allowing all of them without scrutiny? Because unlike data- these might incur some user agent processing down the line. We don't know what's going to happen in that regard. And we don't have a separate userland namespace here currently.

It's a good question.

It really depends on what people are going to use them for beyond markers, and why my initial intuition was to do this by target name, which can be roughly be mapped to a "feature".

It might be that people will start using them for their own purposes, in which cases they might want to allow their own targets and not necessarily give a blank slate to markers or to any other processing instruction that the platform might attach some behavior to.

[annevk]

Maybe pis (list of target names; though pis reads a bit weird), blockPIs (list of target names), and customPIs (boolean; PIs whose target name contains a hyphen).

[noamr]

Maybe pis (list of target names; though pis reads a bit weird), blockPIs (list of target names), and customPIs (boolean; PIs whose target name contains a hyphen).

{
   processingInstructions: ["marker"],
   removeProcessingInstructions: ["dangerous"],
   allowCustomProcessingInstructions: true
}

?

[mozfreddyb]

Thinking about this some more, here's what I think:
Just as with HTML, we will need to decide for all PIs whether they should be allowed by default, allowable or blocked. We therefore need the processingInstructions, removeProcessingInstructions configuration properties.

For "custom" processing instructions (what is this?), we should behave as we do with custom elements. Adding them to the allow list will allow them. It is the web page developer's responsibility to ensure these custom things are safe. Given that they are custom, we can not guarantee safety from the browser

Now, for all PIs that exist, we need to make a judgement call:

• Can they lead to script execution? If so, they are not allowed by default and cannot be allowed in the config.
• Can they lead to some other attacks but no XSS? They can be allowed by the user.
• Does neither apply? Allowed by default.

It seems that the actual behavior for upcoming processing instructions is not yet fully specified, but looking at whatwg/html#11542 (comment) for <?marker ...>, it looks like this would be used to replace/modify HTML like so (modulo grammar errors:

<div><?marker start="foo"?>
<!-- this will be replaced?-->
<?marker end="foo"></div>

Later, out-of-order content could then supply information that will be inserted into the marker. E.g.,
<template contentmarker=foo>....</template>

When accepting HTML containing this marker PI, the Sanitizer can not know whether future content will include scripts (e.g., <template contentmarker=foo><img src=x onerror=alert(1)></template>.

Based on this, I think the Sanitizer can not allow the marker PI because it also violates the assumption that setHTML() won't lead to script execution at a later point (see also: Trusted Types-alignment)?

P.S. I am not concerned with someone misusing the Sanitizer (eg setHTML) to confuse the page and inserting in the wrong slot. The template element is not allowed by default,

[zcorpan]

Similar to comments, they could probably be used for mXSS.

Also if the page uses declarative patching for something and has sanitized untrusted content for something else, the untrusted content could insert markers to hijack where the patch goes.

I think a good default is to remove all processing instructions.

[otherdaniel]

Similar to comments, [...]
I think a good default is to remove all processing instructions.

I agree. One follow-up:

For comments, we made the choice that they're default-on for the "unsafe" variants and default-off for the safe ones (https://wicg.github.io/sanitizer-api/#sanitizer-canonicalize-the-configuration, step 8), on grounds that setHTMLUnsafe with an empty config shouldn't modify the parse tree. Would this apply here, too?

[annevk]

I agree that for safe they should be removed by default. For unsafe they should be kept because we don't want to mutate the tree indeed.

I'm not sure we want the API to mimic comments. Elements seem more appropriate as these have a target and pseudo-attributes, all of which seem reasonable to be able to address. (Though we could start out small with just allowing or blocking targets.)

And then as we introduce processing instructions that have a processing model attached to them into HTML, we'll have to carefully evaluate whether they can be allowed in safe at all or whether they need to be added to removeUnsafe().