search

asciidoctor / asciidoctor-vscode

AsciiDoc support for Visual Studio Code using Asciidoctor
Other
323 stars 97 forks source link

Refine Content Security Policies #442

Closed ggrossetie closed 2 years ago

ggrossetie commented 2 years ago

The current CSP used in the WebView are actually controller by the extension.

I believe that we inherited from the Markdown extension:

https://github.com/microsoft/vscode/blob/c77dbe95658ae11ec6f58a75a945827eb8c47692/extensions/markdown-language-features/src/features/previewContentProvider.ts#L209-L229

While CSP are useful, I think we should refine the policies to align with all the built-in features that Asciidoctor provides.

For instance, if we are using SRI on scripts loaded from https://cdnjs.cloudflare.com/ then it's safe to use:

script-src https://cdnjs.cloudflare.com/ajax/libs/*

For extra safety, we could do:

script-src https://cdnjs.cloudflare.com/ajax/libs/mathjax/2.7.9/* https://cdnjs.cloudflare.com/ajax/libs/highlight.js/9.18.3/*

Currently, the user experience is not great and users are (mostly) confused by CSP. I think we could use relaxed policies by default until we can enforce better policies.

ggrossetie commented 2 years ago

For instance, if we are using SRI on scripts loaded from cdnjs.cloudflare.com then it's safe to use:

Reading the documentation again, it seems that we can only allow a domain so:

script-src https://cdnjs.cloudflare.com
danyill commented 2 years ago

Thanks for continuing to look into this @Mogztter :clap: I think there are some long-standing issues here that trouble users and I've struggled to find time to resolve so it's great to have a conversation partner :tada:

I believe that we inherited from the Markdown extension:

Correct.

Reading the documentation again, it seems that we can only allow a domain so:

I think you have precisely understood the issue -- whitelisting an entire CDN which is the only option (without using integrity hashes) is antithetical to the security principle of least privilege.

CDNs themselves are at best controversial from a security point of view:

https://httptoolkit.tech/blog/public-cdn-risks/ https://blog.radware.com/security/2017/03/cdn-security-not-enough-today/

My preference is to continue to go down the path we are going down. If we're going to whitelist by CDN we could give everyone a really good user experience and no security by eliminating the option. OK, it's not quite that bad but I doubt it would pass a security assessment.

Users should be able to open random Asciidoc files with pass-throughs and other problems from untrusted websites without this being an attack vector IMHO.

Another point of view is to note that recently there were changes in VS Code (perhaps because of problems) where you had to express some trust in your local folders that you are using. If that's true then access to remote resources should be at least as well controlled.

Currently, the user experience is not great and users are (mostly) confused by CSP. I think we could use relaxed policies by default until we can enforce better policies.

Mmmm... I agree about the confused bit :confused:. We have quite a few open issues related to this :confounded:. I think there are not so many places where we need this improvement and perhaps it is limited to:

I think if all those worked "out of the box" and you had reduced security levels otherwise that would be OK for 90% of users. End users don't need to understand (certainly not on day 1) that there are client-side scripts and calls to CDNs to embed them. And of course, from a user point of view, I would argue they shouldn't need to know (the abstraction is part of the tool, the user is there to write).

(I note that you've been discussing this here: https://github.com/asciidoctor/asciidoctor/issues/3728).

Now perhaps we have (at least) one option too many in our security settings -- 4 is a lot, especially for users who may not understand the insides of Asciidoctor / VSCode / web security etc.

I think we could integrate the workspace trust settings into the security settings. This has already been proposed for Markdown:

I think then our options should be renamed and there should be only two:

With only "Disable Security" and "Safe" I think the user options are clearer.

And while I'm thinking about this if we're going to address the security settings, then a change in security settings really should force a re-render as that increases the confusion for everyone (probably that should be a new issue).

I just noticed that it won't work when using the Asciidoctor CLI because we are only registering the custom syntax highlighter on Asciidoctor.js

Previously it has been proposed to remove the Asciidoctor CLI option.

I think it requires too much effort to continue to maintain parity between command-line / Ruby / some executable and Asciidoctor.js and I don't think we should try.

We should just document, "You can use Ruby but there are some limitations, please don't file issues" (so for instance Kroki and scroll sync don't work now).

danyill commented 2 years ago

Incidentally the security model that VS Code is working with has this as an architectural approach:

We want to ensure that even if arbitrary scripts can be injected into the html preview, nothing worse can happen to you than if you visited a website in Chrome. Some degree of security responsibility will still fall on extension authors, but we should certainly make exploits like the ~/passwords upload example difficult expose accidentally.

(see here)

While we're thinking about CSPs, there is also a CORS issue with the extension at the moment for loading the fonts specified in the Asciidoctor stylesheet. Maybe that is related or maybe it should be a separate issue.

image

It may be related to:

https://github.com/microsoft/vscode/issues/102959 https://github.com/microsoft/vscode/issues/72900

It appears as though fonts might have to be manually loaded using a proxy server (perhaps this is relevant).

What these all have in common is that Asciidoctor may need to provide additional capabilities to either download and embed resources or otherwise specify that they are whitelisted.

ggrossetie commented 2 years ago

Previously it has been proposed to remove the Asciidoctor CLI option. I think it requires too much effort to continue to maintain parity between command-line / Ruby / some executable and Asciidoctor.js and I don't think we should try.

Is there an existing issue? I can't find it. I also think we should remove the Asciidoctor CLI option but I would prefer to discuss it on a dedicated issue.

I think you have precisely understood the issue -- whitelisting an entire CDN which is the only option (without using integrity hashes) is antithetical to the security principle of least privilege. CDNs themselves are at best controversial from a security point of view:

Another way to look at it is to use an offline-first approach where we rely on a local copy of Highlight.js, Font Awesome and MathJax.

Asciidoctor (core) relies on CDN because we don't want to include third party libraries into the core but an extension can.

This approach has two main benefit. First, the preview will work without Internet access (which is quite nice). And second, it will solve most (if not all) our security concerns/issues with CSP. Feeding two birds with one scone!

The preview might also be a bit faster since we won't rely on the network anymore.

Now perhaps we have (at least) one option too many in our security settings -- 4 is a lot, especially for users who may not understand the insides of Asciidoctor / VSCode / web security etc.

I agree, we have too many options.

I don't think we should allow http at all in 2021. I would remove this option and document that all content used in the preview has to be served over https. But I expect many companies still use http behind firewalls for example and might have common resources... But perhaps that is not a very big issue.

Unfortunately, I think many companies are using http behind firewalls. Having said that, allowing http: scheme globally is probably a bad idea and it's pretty much equivalent to "Disable Security".

With only "Disable Security" and "Safe" I think the user options are clearer.

We might need a third option where users will be able to configure the CSP they want to apply. Or at least a way to allow them to whitelist domains. That way, if they are using a service (behind firewalls) over http then they can allow it without disabling CSP completely.

And while I'm thinking about this if we're going to address the security settings, then a change in security settings really should force a re-render as that increases the confusion for everyone (probably that should be a new issue).

👍🏻

While we're thinking about CSPs, there is also a CORS issue with the extension at the moment for loading the fonts specified in the Asciidoctor stylesheet. Maybe that is related or maybe it should be a separate issue.

It seems that the stylesheet is loaded from a domain vscode-webview:// but then the stylesheet is trying to load fonts from cdnjs. Another reason to use an offline-first approach and load resources/fonts directly from the extension 😉

danyill commented 2 years ago

It seems like we have landed on making standard Asciidoctor preview resources available offline (fonts, admonitions, mathjax, highlight) as the way forward. I'm good with that. We would probably have it as a user preference and default it to on? I guess this would require some templating or extensions and can probably be facilitated by a good set of npm modules.

For the security options, we could include a whitelist in the "Safe" security option as a user preference so we only had two options. If someone needed or wanted to use online resources they could whitelist a CDN or specify the domain for local resources as an explicit user choice.

WDYT?

ggrossetie commented 2 years ago

Fixed in https://github.com/asciidoctor/asciidoctor-vscode/pull/547