[w3c/editing] Security Review (#315)

I'm from Chrome Security, taking a look at the explainer.md. Here are some thoughts.

## API Affordances

The API should not require the caller to proactively do the right thing — just as people forget to proactively check the sending `event.origin` in `postMessage`, they're going to forget to explicitly mark unsanitized formats as such. This code snippet from the explainer shows the problem:

```
// This format 'text/custom' is not sanitized by the Clipboard API. It will be
// pickled if the format is specified in the {unsanitized: []} formats list.
const customText = new Blob(
  ['<custom_markup>pickled_text</custom_markup>'], {type: 'text/custom'});

// Clipboard format ordering: Pickled formats will be written before sanitized
// formats by the browser, since they're more "custom" and likely more targeted
// towards this use case.
const clipboardItem = new ClipboardItem({
  'text/plain': text,       /* Sanitized format. */
  'text/custom': customText /* Pickled format. This new format will be accepted
                            and written without rejection, as long as the new
                            unsanitized list contains this format. */
}, 
// → PEOPLE WILL FORGET THIS:
{unsanitized: ['text/custom']} /* This new list specifies the pickled format
                          'text/custom'. */
);
navigator.clipboard.write([clipboard_item]);
```

I propose instead that the specification should say that implementations MUST pickle any clipboard contents that they don't already sanitize. (And then you can remove the `unsanitized` field from the `ClipboardItem` constructor.)

### Callers Are Malicious

A key reason for this is that the reader of clipboard contents must assume the writer is malicious. A malicious clipboard writer will intentionally not mark their attack payload as `unsanitized`.

I see that you have considered this ("Alternative Considered: unsanitized if not supported by the Async Clipboard API"), but I don't understand the argument against it. I don't see that it is equivalent to the `unsanitized: true` design alternative: it *would* be possible to distinguish sanitized from unsanitized clipboard contents, because we all know in advance what the sanitized MIME formats are. (Right?)

In any case, this feature simply must ensure that clipboard readers have to opt in to reading potentially dangerous clipboard contents. We cannot trust callers to get it right.

### Explicitness Is Good

By contrast, it is good that a caller of `read` has to explicitly ask for unsanitized clipboard contents. I.e. keep this:

```
// Pickling read example. ClipboardItems returned by clipboard.read() may 
// contain pickled formats.
const clipboardItems = await navigator.clipboard.read(
    {unsanitized: ['text/custom']} /* This new list specifies the pickled format
                              'text/custom' for all read ClipboardItems. */
  );
```

## Existing OS And Application Expectations

There's a very real problem that mainstream OSs will eagerly deserialize *and execute* COM/NSObject/other objects they find and can recognize in the clipboard, leading to a severe risk of exploitation of existing software that doesn't know it's receiving input from the wacky and wild internet. (See e.g. [Compromised renderers can set arbitrary clipboard formats](https://bugs.chromium.org/p/chromium/issues/detail?id=352395). This feature essentially opens up an attack path to all web origins that was previously only open to those who had already compromised the renderer process.)

Therefore, the specification must be super clear on what format the pickled stuff will be in, and ensure that the pickling format itself is not hard to parse (thus creating *new* bugs and vulnerabilities). And that format must not be something that existing software will eagerly decode and deserialize — it should be simple, yet opaque to existing software.

### Opting In To Trouble Is Not Sufficient

Under the "Risks" section, no mention is made of the fact that the unsanitized pickled data is still very likely to be unsafe for existing implementations that were not designed and implemented for the internet threat model. It is likely to happen that native app developers will opt in to receiving unsanitized formats, and then feed them directly to their existing implementations. The specification must warn developers that they should only feed these unsanitized inputs to implementations that are hardened against internet garbage. (The *minimum* standard is intense fuzzing, such as provided by [OSSFuzz](https://google.github.io/oss-fuzz/).)

## The Privacy And Security Section

### PII

The PII issue is a concern, but probably not the biggest. It should get a dedicated paragraph. The paragraph it's in starts with an unrelated topic sentence ("This feature introduces custom clipboard formats with unsanitized content that will be exposed to both native apps and websites.").

### Why Pickling

From the perspective of an application developer, all this pickling business may seem like an extra hoop to jump through with no clear reason. The explainer should clearly link the "whole new world of attacks" discussion with the opt-in-un-pickle requirement and with the requirement that developers harden their native implementations against internet malice. (The user gesture requirement, discussed below, is not a real defense against the whole new world of attacks, and should not appear in the same paragraph.)

### User Gesture Requirement

The utility and meaning of the user gesture gate/mitigation may have been lost. As written in the explainer, I don't see that it protects much, and from the perspective of a developer might just seem like more security theater.

If (and only if) we require that the clipboard write (and possibly read) operations can only be called from within an event handler handling a `trusted` `event` (i.e. an `event` attested by the browser to have come from a true human-generated UI gesture event), can we interpret the event as indicating a person's intent. That would help us get some confidence that the write (or read) operation is not happening unilaterally, at the request of a potentially malicious web origin.

I don't think the [transient activation](https://html.spec.whatwg.org/multipage/interaction.html#transient-activation) mechanism suffices to indicate a person's intent.

But as written, that requirement and marginal benefit is not clearly spelled out in the explainer. (Developers will not click on the extended privacy-security doc.)

### No Permission

I can imagine being OK with this, for the reasons you give (comprehensibility, fatigue), in a later review after these other issues are hammered out.

## Minor Nits

Nits: Use "HTML" consistently throughout, not "html". Document could use some copy editing.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/w3c/editing/issues/315