Vscode: We should try to make the message extension <-> WebView communication more secure

Created on 24 Apr 2018 · 14Comments · Source: microsoft/vscode

Tests https://github.com/Microsoft/vscode/issues/48453

While reading through the documentation I stumbled over https://github.com/Microsoft/vscode-docs/blob/vnext/docs/extensions/webview.md#do-not-trust-messages-received-from-webviews which outlines the problem however doesn't provides any reasonable solution.

@alexandrudima and I brain stormed over lunch a little and we should try to make this more secure. An idea is:

since the web view sends messages JS must be enabled
the script should contain and 'active' or 'init' method which we try to call. Not sure how to actually do this. May be we need to inject something into the provided html using a template marker.
the method should take a context which offers method to send and receive message (we will not use the global window properties)
the methods uses tokens to ensure that on the receiving side we know that the message came from the right sender.
we capture the postMessage function and check before we call it to ensure no one tempers with that function (for example to spy on the token we use).

The provider of the web view can still leak the context. However if he does so it is his fault since the communication didn't rely on global available functions available to everyone.

verified

Source

dbaeumer

👍1

Most helpful comment

I've merged in a change that adds an acquireVsCodeApi function inside webviews. This api object now offers the only way to post messages back to vscode. It can only be acquired once. I've already prototyped this approach in the markdown preview and will be updating the webview example and documentation. The change also explicitly disallows access to window.parent from inside the webview

I apologize if I can across as dismissive on this issue. I am very happy with the end result and really appreciate all the help getting it to this point. Having thought so much about security, I just found it hard that so much attention was being given to this specific problem when we have done so much other work to prevent problems like this from even happening in the first place. But you are absolutely right that we should make it as easy as possible for extension authors to do the right thing security-wise. This change is the correct step in that direction

mjbvz on 26 Apr 2018

👍4 ❤2

All 14 comments

I believe this is important to consider for April, as we have the API in stable and cannot change this after the fact without breaking everybody.

alexdima on 24 Apr 2018

Look like we already inject a pre script into the Web view src\vs\workbench\parts\webview\electron-browser\webview-pre.js

dbaeumer on 24 Apr 2018

@dbaeumer Perhaps that section is a bit too scary as written. This is the correct best practice but it also requires a lot of other breakdowns:

If you write a webview that allows script injection
And you failed to use a proper content security policy
And you registered a command or handler that runs rm -rf / or something
An attacker could craft a malicious workspace or malicious content to exploit this
And if that attacker can get a user to load this malicious workspace or content, and also invoke the extension's webview in a way that allows the script injection
Then there could be problems
So you as the extension author should at least make sure you prompt the user from vscode before executing rm -rf / blindly just because the webview told you so

I'm not sure how to solve this well, or even if this is even an actual (non-theortical) problem.

If a webview allows script injection when it constructs its html, it is very difficult to prevent an attacker from posing as the webview's javascript and just registering their own activate method. A token based approach could work but then you have the problem of getting the token into the webview in a way that an evil script could not see

The reverse case also doesn't apply. The webview can trust that the extension is sane because if an attacker can compromise an extension itself, webview communication is the least of your problems

mjbvz on 24 Apr 2018

I've pushed an update to the docs that makes this section less alarming. I think that the current api is fine when used along with the other security best practices. But extension author should be aware that they should validate messages from webviews if they are using these messages to perform potentially dangerous operations

mjbvz on 25 Apr 2018

@mjbvz

Here is a sketch on how to build a secure token-based communication channel.

Option 1. If you can generate the HTML

<html>

<head>
    <!-- The following script is generated by VS Code -->
    <script>
var vscode = {
    acquirePostMessage: (function () {
        var secretToken = '723yrkjbvkjvbaoey241241dscdbsg'; // <- random generated and checked for every message.
        var originalPostMessage = window.postMessage.bind(window);
        var originalStringify = JSON.stringify.bind(JSON);
        return function () {
            vscode.acquirePostMessage = function() {
                throw new Error(`postMessage already acquired`);
            }
            return function(msg) {
                originalPostMessage(originalStringify({ msg: msg, secretToken: secretToken }));
            }
        }
    })()
}
    </script>
</head>
<body>

<!-- The following script is written by an extension author -->
<script>
(function() {
    var postMessage = vscode.acquirePostMessage();
    // ...
    // as long as postMessage is not leaked to a global everything is fine
})();
</script>

</body>
</html>

(Better) Option 2: If you can call into the script written by an extension author

More or less same as above
except the secure post message function is not available from a global, it is passed in to a window.activate that is to be written by the extension author.

I don't believe we should document the pitfalls, I believe we should build API that leads to pitfalls being avoided by extension authors.

alexdima on 25 Apr 2018

@alexandrudima nice work

@mjbvz

But extension author should be aware that they should validate messages from webviews if they are using these messages to perform potentially dangerous operations

What is your recommendation how an extension author should do this?

dbaeumer on 25 Apr 2018

@dbaeumer The recommendation is that extensions prompt the user before performing any dangerous operations. In fact, all commands really should also do this (because of command uris) but many do not

@alexandrudima Thanks for the explanation but I don't think it solves the problem. For example, if I write a webview that generates the html:

function getHtml() {
    const content = vscode.window.activeTextEditor.document.getText()
    return `
       <div>${content}</div>

<script>
(function() {
    var postMessage = vscode.acquirePostMessage();
})();
</script>
`
}

If the the text content of the document contains a script tag that invokes vscode.acquirePostMessage();, we are back to square one. The only solution that I can think of is to do like the electron webview does for preload scripts. This script is run before anything on the page, and in a separate context that has additional capabilities that normal webpage script do not

I do not think we should do this

mjbvz on 25 Apr 2018

@mjbvz The idea is the following:

this gives a way for someone who tries to write a secure communication channel to be able to use one.
this does not make it impossible for someone to fail. You can also break it if you stick postMessage into a global -- which I already mentioned -- or, as you point out, you can fail by injecting malicious JavaScript before the extension's intended JavaScript. There are probably a lot more ways to fail.

But what is the alternative for an extension using a webview? Popping up dialog boxes for every received message?

The urgency is given by the move of the API to stable. We will not be able to easily add this protection in the future without breaking everyone.

alexdima on 25 Apr 2018

I do not think this is a real problem. I mean, yes it is a problem and I documented it because we want people to be aware of it, and yes security in depth is always preferable, but as an extension author I really have to do a sequence of things incorrectly in order to make this is a problem:

Enable scripts in my webview
Fail to use a proper content security policy
Fail to sanitize content throughly (and in a way that is exploitable)
Setup a command that does something dangerous when invoked

However I do like the idea of getting rid of window.parent part and introducing a custom "vscode" namespace that has a postMessage function on it. That gives us more flexibility to change how the webview is implemented in the future, as well as allowing us to introduce other API if we ever need to

mjbvz on 26 Apr 2018

If we introduce the vscode namespace in webviews, I cannot think of good arguments why we should not use the acquire type flow, besides that it makes coding a bit more of a pain. I'll take a look at this. Unsure if we should have an acquirePostMessage method or just an acquireVsCodeApi method

mjbvz on 26 Apr 2018

@mjbvz and I looked through the necessary changes, code sample, etc. We'll make changes along the lines of option 2 for April. We'll also add a couple of recommendations for extensions authors. The issue remains that extension authors are on the hook to write secure extensions.