Vscode: Trusted Types policy usage

[jrieken]

The editor view layer uses innerHTML for performance reasons and we aren't ready to rework that (see https://github.com/microsoft/vscode/issues/106285#issuecomment-689414623) This is a use-case of a custom policy, e.g one which doesn't escape things. There might be other, still unknown, places where policies are required, e.g when rendering markdown. This is the item to track such policy usages.

[jrieken]

fyi @annkamsk

[jrieken]

I have pushed a few changes that make sure we have a trusted types polyfill (comes view the AMD loader) and typings. Also, the loader has got an option to created trusted script urls. With that most TrustedScriptURL warning are gone. Line 36 in the snippet below is a sample of a loader config to enable trusted types (policies are insecure)

https://github.com/microsoft/vscode/blob/133d95e61892e304af8763b6df1412af5a87f32d/src/vs/code/browser/workbench/workbench-dev.html#L32-L49

[annkamsk]

fyi @engelsdamien @koto @Siegrift

[Siegrift]

Hi, I am currently working on a few enhancements in tsec, specifically on the following things

1) Improving type awareness such that assignments of Trusted Types (TT) are allowed.
We plan to support various common patterns such as casting TT value to string or using an unwrapper function. We will use the existing TT typing from definitely typed. Any particular reason why you created the custom ones?

2) Tsec as language service plugin
This one is in review internally, but after it's merged you will be able to see the violations in the IDE right as you type.

3) Support for tsec fixers
You can create a Fixer which can produce a suggested fix for a given violation.
This one is currently only planned, and it's a bigger change but the implementation should start soon. The question is:

Can you imagine a fixer tailored for VSCode?

I see that you added a createTrustedScriptURL builder, but I am not sure if this function is available everywhere and whether it's a good fit for a fixer. The fixer will have access to the full source code, but has only limited information about the violations. Basic fixers we are thinking about replace the dangerous value with sanitizer call (and maybe imports the sanitizer) or offers using safe alternative (e.g. innerText instead of innerHTML).


If you feel this deserves a separate issue to discuss this on, we can create one and move the discussion there.

[jrieken]

Any particular reason why you created the custom ones?

I didn't expect them to exist already ;-) Also, I have only added a very minimal api-only polyfill so I didn't spec more of the trusted types API. Also wondering how to do it properly when assigning e.g. TrustedHTML to innerHTML - it accepts a string only and I didn't like 'foo.innerHTML = tt.toString()' too much. Anyways, let me think about this again, esp with tsec in mind. The task will be to make the polyfill more complete

I see that you added a createTrustedScriptURL builder, but I am not sure if this function is available everywhere and whether it's a good fit for a fixer.

Yeah, the createTrustedScriptURL is very specific to the AMD loader. That's something we use in different contexts and not something were we can enable trusted types by default. That's why I added the config option to pass the builder which is basically TrustedTypePolicyOptions#createScriptURL.

Not sure if I can image a tailored VS Code fixer - violations I have seen so far all very different. There might some low-hanging fixers for cases when the empty string is assigned. A common violation was the population of style-tags via innerHTML - we have replaced those with textContent which would be a candidate for automation.

This one is in review internally, but after it's merged you will be able to see the violations in the IDE right as you type.

Very excited about that!

[engelsdamien]

I have pushed a few changes that make sure we have a trusted types polyfill (comes view the AMD loader) and typings. Also, the loader has got an option to created trusted script urls. With that most TrustedScriptURL warning are gone. Line 36 in the snippet below is a sample of a loader config to enable trusted types (policies are insecure)

https://github.com/microsoft/vscode/blob/133d95e61892e304af8763b6df1412af5a87f32d/src/vs/code/browser/workbench/workbench-dev.html#L32-L49

One thing to note is that with this current setup, it might be easy to degrade security in the future without realizing, due to the fact that the validation function far away from the actual policy creation, so changes in the former might not look suspicious to a reviewer, but are actually security relevant.

We are currently working on open sourcing our set of Trusted Types builders, but our general approach is to try to fully encapsulate the policy behaviour so that consumers of the builders cannot change the security property of the resulting types.

One of the tricks that we use to ensure that a value is safe is by using template literals to separate what comes from the program's source vs what is dynamic. (when it comes from the program source, there is no risk for it to be user controlled). This might work in this case since all your paths seem to be in the program's source.

So you could define a function like follows:

// This remains private, all security considerations stay in this file.
const policy = trustedTypes.createPolicy('amdLoader', {createScriptURL: (value: string) => value});

export function scriptURL(templateObj: TemplateStringsArray): TrustedScriptURL {
 if (!templateObj.length === 1) { throw "Cannot interpolate in script urls"; }
 if (!templateObj[0].startsWith('/')) { throw "Only absolute paths are allowed in script urls"; }
 return policy.createScriptURL(`${window.location.origin}${templateObj[0]}`);
}

And then use it anywhere you want like follows:

<script> 
    self.require = { 
        baseUrl: scriptURL`/static/out`, 
        recordStats: true, 
        paths: { 
            'vscode-textmate': scriptURL`/static/remote/web/node_modules/vscode-textmate/release/main`, 
            'vscode-oniguruma': scriptURL`/static/remote/web/node_modules/vscode-oniguruma/release/main`, 
            ...
        } 
    }; 
 </script> 

[Siegrift]

Also wondering how to do it properly when assigning e.g. TrustedHTML to innerHTML - it accepts a string only and I didn't like 'foo.innerHTML = tt.toString()' too much.

Unfortunately, there is no support for this in TS due to this issue. Also, you approach (elem.innerHTML = tt.toString()) will not work because it will stringify the value and you'll get a Trusted Type violation.
For now, you need to lie to the compiler somehow. For example, elem.innerHTML = tt as unknown as string (tt is still a Trusted Type and TS is happy as well).

This pattern might be tedious so you might want to use unwrapper function to unwrap the trusted value: elem.innerHTML = unwrap(tt), where const unwrap = (tt: TrustedHTML) => tt as unknown as string.

[jrieken]

@Siegrift Can you help with the tsec tool. Since a couple of weeks it seems broken (misses files in its distribution). We have brought it in via https://github.com/microsoft/vscode/commit/ac7bf0384bc8f236809ab12fc5f0445d5cbf2769. Is there now a different way to use it, like via npm?

[Siegrift]

@jrieken Can you provide more info about the failure? I tried cloning vs code repo and tsec works for me without problems. There weren't any suspicious changes in the repo recently which could cause such breakage.

[jrieken]

This is confusing... I was getting this error even after a refresh install. Now it seems to be working 😕

jrieken@Johanness-MBP-2 vscode % yarn tsec-compile-check
yarn run v1.19.1
$ node_modules/tsec/bin/tsec -p src/tsconfig.json --noEmit
/bin/sh: node_modules/tsec/bin/tsec: No such file or directory
error Command failed with exit code 127.

[joaomoreno]

  ERR Failed to execute 'invoke' on 'CreateHTMLCallback': The provided callback is no longer runnable.: Error: Failed to execute 'invoke' on 'CreateHTMLCallback': The provided callback is no longer runnable.
    at ViewLayerRenderer._finishRenderingNewLines (file:///home/joao/Work/vscode/out/vs/editor/browser/view/viewLayer.js:378:60)
    at ViewLayerRenderer._finishRendering (file:///home/joao/Work/vscode/out/vs/editor/browser/view/viewLayer.js:438:26)
    at ViewLayerRenderer.render (file:///home/joao/Work/vscode/out/vs/editor/browser/view/viewLayer.js:288:22)
    at VisibleLinesCollection.renderLines (file:///home/joao/Work/vscode/out/vs/editor/browser/view/viewLayer.js:263:37)
    at ViewLines.renderText (file:///home/joao/Work/vscode/out/vs/editor/browser/viewParts/lines/viewLines.js:427:32)
    at View._actualRender (file:///home/joao/Work/vscode/out/vs/editor/browser/view/viewImpl.js:242:33)
    at file:///home/joao/Work/vscode/out/vs/editor/browser/view/viewImpl.js:213:40
    at safeInvokeNoArg (file:///home/joao/Work/vscode/out/vs/editor/browser/view/viewImpl.js:366:20)
    at View._renderNow (file:///home/joao/Work/vscode/out/vs/editor/browser/view/viewImpl.js:213:13)
    at View.restoreState (file:///home/joao/Work/vscode/out/vs/editor/browser/view/viewImpl.js:266:18)
    at CodeEditorWidget.restoreViewState (file:///home/joao/Work/vscode/out/vs/editor/browser/widget/codeEditorWidget.js:605:38)
    at DiffEditorWidget.restoreViewState (file:///home/joao/Work/vscode/out/vs/editor/browser/widget/diffEditorWidget.js:649:38)
    at TextDiffEditor.restoreTextDiffEditorViewState (file:///home/joao/Work/vscode/out/vs/workbench/browser/parts/editor/textDiffEditor.js:111:33)
    at TextDiffEditor.setInput (file:///home/joao/Work/vscode/out/vs/workbench/browser/parts/editor/textDiffEditor.js:80:49)
    at async EditorControl.doSetInput (file:///home/joao/Work/vscode/out/vs/workbench/browser/parts/editor/editorControl.js:130:17)
    at async EditorControl.openEditor (file:///home/joao/Work/vscode/out/vs/workbench/browser/parts/editor/editorControl.js:48:35)
    at async file:///home/joao/Work/vscode/out/vs/workbench/browser/parts/editor/editorGroupView.js:735:40
    at async EditorGroupView.restoreEditors (file:///home/joao/Work/vscode/out/vs/workbench/browser/parts/editor/editorGroupView.js:326:13)

[koto]

When the error occurs, are you inside a Chrome devtools debugging session? This might be https://bugs.chromium.org/p/chromium/issues/detail?id=1131368.

[jrieken]

Yeah. @joaomoreno saw this when running/debugging VS Code from within VS Code

[koto]

Then it's very likely that bug (sorry!), it should not manifest itself during regular execution, only when the e.g. one is inside a breakpoint. By now we have a good idea on how to fix this.