dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.46k stars 4.76k forks source link

Make `mono` CSP Compliant #68374

Closed mingyaulee closed 2 years ago

mingyaulee commented 2 years ago

In the effort to remove unsafe-eval CSP requirement from Blazor WASM, the mono runtime also needs to be CSP compliant by removing the need for unsafe-eval.

Although issue https://github.com/dotnet/runtime/issues/59416 has been resolved, the runtime is still not CSP compliant, with the following lines requiring the unsafe-eval CSP:

https://github.com/dotnet/runtime/blob/14584c60b41ddc361442539f78bc3d54d3ab3ea2/src/mono/wasm/runtime/method-binding.ts#L113

https://github.com/dotnet/runtime/blob/0d889f56d2c79f6ac73c218147e3a5a07883fb8e/src/mono/wasm/runtime/method-calls.ts#L631

This might not be applicable since it is only for debugging: https://github.com/dotnet/runtime/blob/0d889f56d2c79f6ac73c218147e3a5a07883fb8e/src/mono/wasm/runtime/debug.ts#L216

dotnet-issue-labeler[bot] commented 2 years ago

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

ghost commented 2 years ago

Tagging subscribers to 'arch-wasm': @lewing See info in area-owners.md if you want to be subscribed.

Issue Details
In the effort to [remove `unsafe-eval` CSP requirement from Blazor WASM](https://github.com/dotnet/aspnetcore/issues/37787), the mono runtime also needs to be CSP compliant by removing the need for `unsafe-eval`. Although issue https://github.com/dotnet/runtime/issues/59416 has been resolved, the runtime is still not CSP compliant, with the following lines requiring the `unsafe-eval` CSP: https://github.com/dotnet/runtime/blob/14584c60b41ddc361442539f78bc3d54d3ab3ea2/src/mono/wasm/runtime/method-binding.ts#L113 https://github.com/dotnet/runtime/blob/0d889f56d2c79f6ac73c218147e3a5a07883fb8e/src/mono/wasm/runtime/method-calls.ts#L631 This might not be applicable since it is only for debugging: https://github.com/dotnet/runtime/blob/0d889f56d2c79f6ac73c218147e3a5a07883fb8e/src/mono/wasm/runtime/debug.ts#L216
Author: mingyaulee
Assignees: -
Labels: `arch-wasm`, `untriaged`, `area-VM-meta-mono`
Milestone: -
keesschollaart81 commented 2 years ago

Hi @pavelsavara I see you removed it from the 7.0 timeline after it being triaged and put on the roadmap quite recently. Out of curiosity (as this is quite an important feature for me), what is the story here, too complicated/much work?

pavelsavara commented 2 years ago

We are generating interop thunks (JS code for marshaling) on runtime, so we need to use new Function. Also we plan to add new C# API wrapping JS new Function so that people could create JS functions.

We may be able to re-visit interop and generate it on compile time as part of AOT in the future. We may also try to implement linker trimming to include JS code in the future, which would link-out usage of new Function if you didn't use it's API in your code. Both of those things are non-trivial, realistically not doable in Net7 time frame.

Are there other ways how CSP compliance could be achieved ?

mingyaulee commented 2 years ago

This is very important for the browser extension development, since chrome is moving to manifest v3 and all the extensions has to be compliant. Generally, writing JavaScript code in string and executing is a bad idea in the first place and should be avoided at all cost. If this fix does not make it to .Net 7 release, no one would be able to develop new browser extensions using Blazor because the chrome web store will not accept it.

keesschollaart81 commented 2 years ago

We host our (Blazor WASM) web app on a vendor-managed (static files) webserver. As per their security policy, with their latest version, they require the web apps to be CSP compliant: 'Industry standard security'. I guess this requirement will pop-up more and more?

Aragas commented 2 years ago

We're too disappointed that it won't make the 7.0 release! The Bannerlord community created a WASM extension for Vortex by NexusMods, an electron application that is the main tool for modding games, but we can't use it in production builds due to CSP. For safety, the CSP disables unsafe-eval, as it runs both trusted extensions developed by NexusMods and community extensions, developed by third party entities. We hoped that we could reuse our existing C# codebase for this, keeping the ecosystem consistent with C# as the only language required for modding.

lewing commented 2 years ago

Thanks for the feedback, these are good points. We're looking at options that might be able resolve this properly. I'll update this issue once we've had more time to discuss them.

pavelsavara commented 2 years ago

Is there any eval or new Function in the emscripten code we use ?

keesschollaart81 commented 2 years ago

I guess not if DYNAMIC_EXECUTION is set to 0?

pavelsavara commented 2 years ago

My current understanding is that the JS code could contain the eval() and it only throws when the statement is executed. That is, the script is not rejected during parsing, but during execution, right ?

Something like Uncaught EvalError: Refused to evaluate a string as JavaScript because 'unsafe-eval' is not an allowed source of script in the following Content Security Policy directive: "default-src 'self'"

That means we could have C# API and implementation for new Function in the runtime (for unit tests and such) and CSP would be OK, until that API was executed.

mingyaulee commented 2 years ago

Yes that is correct. In my testing, I am able to load a Blazor browser extension in Chrome and only get an error when the JS code is executed, i.e. when one of the extension page is opened.

This is the first error that is shown in the browser console when trying to initialize the application. image

kg commented 2 years ago

This is very important for the browser extension development, since chrome is moving to manifest v3 and all the extensions has to be compliant. Generally, writing JavaScript code in string and executing is a bad idea in the first place and should be avoided at all cost. If this fix does not make it to .Net 7 release, no one would be able to develop new browser extensions using Blazor because the chrome web store will not accept it.

While we will continue to make efforts to find CSP mitigations for you and other users, I'd like to politely suggest that you re-read the manifest V3 requirements in the Web Store Program Policies and note that a fix for this CSP issue would not bring you into compliance with those policies.

Policies like those are of course the result of a company carefully balancing their business interests against customers' interests, and in this case your chosen approach may be running up against the new V3 policies as some very popular extensions have.

mingyaulee commented 2 years ago

I understand that CSP compliance is only one of the required policies and there are others that you have to follow as well, but this is a basic security policy not just for the use case of developing extensions, as pointed out by others as well.

mingyaulee commented 2 years ago

@pavelsavara @kg I attempted to update the TS codes to remove unsafe evaluation of JavaScript during runtime in this commit.

I am able to run the Blazor template application without unsafe-eval CSP for the Home and Counter page. However when I navigate to the 'Fetch data' page, the HttpMessageHandler also contains JavaScript code in the following line.

https://github.com/dotnet/runtime/blob/446e5ad932ba693d64225bd2939e977832a642e2/src/libraries/System.Net.Http/src/System/Net/Http/BrowserHttpHandler/BrowserHttpHandler.cs#L46

Of course, I can resolve this by creating a new function in dotnet.js and use the interop to call the function instead but I'm wondering if there another approach that would be taken by the team?

pavelsavara commented 2 years ago

The streamingSupportedis in works already but it's just small thing. HTTP is dependent on JS in the browser (there is no TCP socket there etc). We still need to generate the JS code to make even basic interop work. The code you deleted would be missing for that.

As Larry said, we are working hard to enable this. Our current ideas are:

There is no quick hack AFAIK.

Thank you for your patience. Please keep the feedback and ideas coming!

pavelsavara commented 2 years ago

I looked at your change again, I guess you are implementing the "interpreted" interop. The code you are changing is "old" interop, we will try to do something similar to the upcoming "new" interop.

mingyaulee commented 2 years ago

I'm not quite sure I understand what you meant, but I hope the direction that we're heading to is to completely remove string JavaScript from being executed directly, by using something like a C# expression evaluation approach, i.e.

Instead of Function.apply(Function, ["return typeof Response !== 'undefined'"]), use

// expression = {
//   type: "equal",
//   left: {
//     type: "call",
//     function: "typeof",
//     arguments: [{
//       type: "variable",
//       value: "Response"
//     }]
//   },
//   right: {
//     type: "constant",
//     value: "undefined"
//   }
// }

// Where the evaluator looks something like this
function evaluateExpression(expression) {
  switch (expression.type) {
    case "equal":
      return evaluateExpression (expression.left) === evaluateExpression (expression.right);
    case "andAlso":
      return evaluateExpression (expression.left) && evaluateExpression (expression.right);
    // Other types like "call" to represent function calls, "constant" to represent value, etc.
  }
}

Reference: https://docs.microsoft.com/en-us/dotnet/api/system.linq.expressions.expression

kg commented 2 years ago

LINQ support in the BCL has been frozen, so you should not expect a brand-new LINQ equivalent in our JS bindings. (It may be possible for LINQ itself to work, but it would be interpreted)

mingyaulee commented 2 years ago

You may have misunderstood what I was saying. I wasn't expecting a LINQ equivalent in JS bindings. I was just suggesting one of the possible solutions to not write JS code inside the runtime libraries (both in JS and C#), for example

https://github.com/dotnet/runtime/blob/46f53e387e0396542f7dcefa2bb7627ef3a2bf1d/src/mono/wasm/runtime/method-binding.ts#L235-L241

So the reference to LINQ was merely an idea of what can be implemented in place of raw JavaScript string.

veikkoeeva commented 2 years ago

I was looking into this case also. Maybe relevant is recent discussion regarding WASM loading in extensions and v3 manifest at https://bugs.chromium.org/p/chromium/issues/detail?id=1318922.

mingyaulee commented 2 years ago

Awesome work @pavelsavara!

I assume merging to main means it's intended for .Net 8. Will this change be merged to release in .Net 7?

pavelsavara commented 2 years ago

@mingyaulee Do you have non-Blazor use case which would make that effort/risk compelling? See https://github.com/dotnet/runtime/pull/74441#issuecomment-1225672619 for context.

mingyaulee commented 2 years ago

@pavelsavara i believe there are other use cases as mentioned in comments in this issue #issuecomment-1148460934 and #issuecomment-1148674528, and also DotNetJs which is capable of running in many JS runtimes are all currently limited by the CSP restriction from the runtime.

Aragas commented 2 years ago

We would really appreciate if this feature will be included to .NET7, as the only alternative for us is to compile our code to NativeAOT and create Node-API bindings for each platform, increasing the final size of our distribution

pavelsavara commented 2 years ago

@Aragas this fixes CSP only for new interop with [JSImport], [JSExport]. Does it mean that you already adopted it ? Is there other feedback about the new interop ?

Aragas commented 2 years ago

@Aragas this fixes CSP only for new interop with [JSImport], [JSExport]. Does it mean that you already adopted it ? Is there other feedback about the new interop ?

Oh, I wasn't aware of that! AFAIK DotNetJS didn't migrate to the new interop layer, there's still an issue (https://github.com/Elringus/DotNetJS/issues/75) active for that. I guess @Elringus could comment on this!

elringus commented 2 years ago

Well, the projects I'm using DotNetJS with wouldn't benefit much from the new interop or CSP compliance, so I'm a bit reluctant here. I'm mostly looking to the various modularization feats in .NET 7, which will hopefully allow us to drop the custom .NET runtime build. If CSP compliance would also be available in .NET 7 though, there will be more incentivise to try the new interop.