Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: default policy in every Virtual DOM Environment #431

Merged
merged 1 commit into from
May 31, 2023

Conversation

dgitin
Copy link
Contributor

@dgitin dgitin commented May 24, 2023

No description provided.

@dgitin dgitin requested a review from jdalton May 24, 2023 17:50
@dgitin dgitin force-pushed the dgitin/venv-default-policy branch from 3839c1d to 98a334a Compare May 24, 2023 18:01
@@ -80,6 +80,7 @@ const ESGlobalKeys = [
'Symbol',
// 'SyntaxError', // Reflective
// 'TypeError', // Reflective
'trustedTypes',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this file is about language intrinsics, trustedTypes is not an intrinsic, it is just a DOM API.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was the only way to avoid having it remapped, as far as my knowledge on the near-membrane code could help me. Any other way to prevent it from being remapped ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that is the thing, you should remap it, and distort it but only after creating the default policy for the iframe right before remapping everything.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here is my hunch on why this is wrong: if you attempt to use the defaultPolicy object from global after the iframe is detached, it will blow up, because there is no active window behind it. it works for eval because eval is an intrinsic, but probably not for other DOM APIs. That is a hunch, but I don't see a test for that here, let's try it. Basically, inside the sandbox, something like eval(trustedTypes.defaultPolicy. createScript('1')), and that might throw because the DOM API fails due to the iframe being disconnected, while something like eval(1) might succeed just fine.

@dgitin dgitin force-pushed the dgitin/venv-default-policy branch from 98a334a to 35c2dd8 Compare May 24, 2023 18:02
Copy link
Contributor

@caridy caridy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to me that remapping is something that we could do, but distort the defaultPolicy instance with the one created prior to patching the iframe. That should be equivalent but more elegant.

@@ -80,6 +80,7 @@ const ESGlobalKeys = [
'Symbol',
// 'SyntaxError', // Reflective
// 'TypeError', // Reflective
'trustedTypes',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this file is about language intrinsics, trustedTypes is not an intrinsic, it is just a DOM API.

// DOM APIs from XSS attacks. Use distortions to intercept, sanitize
// and sign content for script URLs and HTML strings.
// @ts-ignore trustedTypes does not exist on GlobalObject
if (typeof redWindow.trustedTypes !== 'undefined') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should probably be configurable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

configurable now.

// @ts-ignore trustedTypes does not exist on GlobalObject
if (typeof redWindow.trustedTypes !== 'undefined') {
// @ts-ignore trustedTypes does not exist on GlobalObject
redWindow.trustedTypes.createPolicy('default', {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be done prior to remap rather than after remap, in which case you don't have to use the intrinsics trick.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The remap will just overwrite everything, I did that and I couldn't get the policy to be installed. Adding it as an intrinsic prevents remapping which is a problem when we define our default policy. So this is the only place to do it and it has to be an intrinsic even though the language doesn't define it as one it was the only way I found to prevent it from being remapped and interfering with the rest of the things (remapping & main window default policy).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the policy is installed before the remap, that is the key here, after that, who cares much about what they see. It is true that you might also need to distort the defaultPolicy to do nothing since the default will kick-in if you return a string, it is sort of redundant.

@dgitin
Copy link
Contributor Author

dgitin commented May 24, 2023

It seems to me that remapping is something that we could do, but distort the defaultPolicy instance with the one created prior to patching the iframe. That should be equivalent but more elegant.

The policy needs to be declared as being part of this window, not distorted. Once things are remapped declaring a default policy in the virtual env becomes a problem so if you know a more elegant way of achieving the same result I'd like to know about it.

@dgitin dgitin force-pushed the dgitin/venv-default-policy branch from 35c2dd8 to 466ba66 Compare May 30, 2023 18:31
@github-actions
Copy link

Metric Coverage Percent Covered / Total
Statements 80.75% ( 822/1018 )
Branches 61.24% ( 256/418 )
Functions 72.48% ( 79/109 )
Lines 80.49% ( 792/984 )
Total 73.74%

See detailed coverage

Copy link
Contributor

@jdalton jdalton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's configurable and done before remapping!

@dgitin dgitin requested a review from caridy May 31, 2023 18:03
Copy link
Contributor

@caridy caridy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for making these changes. can we add some tests for this?

@dgitin
Copy link
Contributor Author

dgitin commented May 31, 2023

thanks for making these changes. can we add some tests for this?

Testing is problematic there is absolutely nothing set up at the moment to support this scenario. First, there is no policy to assert on in the virtual environment because the entire trustedTypes object gets remapped. Second, testing with Trusted Types is quite the challenge on this repo, the code needs to load once, declare the policies and then assert that evaluation is possible in the environment but not in the main window which would confirm that virtual environment run with their own TT policy. So definitely not as simple as just writing a test. The test is easy, the setting up, quite the opposite.

@dgitin dgitin merged commit e975627 into main May 31, 2023
@dgitin dgitin deleted the dgitin/venv-default-policy branch May 31, 2023 18:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants