Refine Content Security Policies

[ggrossetie]

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]

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]

Thanks for continuing to look into this @Mogztter 👏
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 🎉

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 😕. We have quite a few open issues related to this 😖. I think there are not so many places where we need this improvement and perhaps it is limited to:

• MathJax for equations
• Highlightjs for syntax highlighting (out of the box)
• Font Awesome for icons, admonitions etc.

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: asciidoctor/asciidoctor#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.

• AllowInsecureContent: -- 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.
• AllowInsecureLocalContent:: -- This is "I know my local drive" and perhaps is superseded by the recent security changes in the workspace. If you trust the workspace this option is redundant. It's worth pointing out from a security point of view we should perhaps examine why we're using unsafe for processing of documents. Probably to ease access to the filesystem anyway (see here).
• AllowScriptsAndAllContent: -- This is the "I know what I'm doing just get out of my way" option.
• Strict: -- I think this should be retained as a default and we should find solutions for the issues (sorry! 😢 )

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:

• AllowInsecureContent: -- Eliminate this option
• AllowInsecureLocalContent:: -- Eliminate this option and replace with "workspace trust"
• AllowScriptsAndAllContent: -- Rename to "Disable Security: Allow custom scripts and styles (local resources and the internet)". Or rename to "Trust Document: Any scripts and styles used both locally and from the internet are safe". I think the "Disable Security" option is better because it flags a concern.
• Strict: -- Rename to "Safe: Only allow built-in Asciidoctor features" (strict is too obscure). Safe also maps onto an Asciidoctor concept.

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]

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.

It may be related to:

microsoft/vscode#102959
microsoft/vscode#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]

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]

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]

Fixed in #547