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

Root level script tag should not be executed #11483

Closed
hybridwebdev opened this issue Jul 1, 2020 · 7 comments · Fixed by #11487
Closed

Root level script tag should not be executed #11483

hybridwebdev opened this issue Jul 1, 2020 · 7 comments · Fixed by #11487

Comments

@hybridwebdev
Copy link

hybridwebdev commented Jul 1, 2020

Version

2.6.11

Reproduction link

https://codesandbox.io/s/cranky-moser-p90di?file=/src/main.js

Steps to reproduce

Put valid <script> element in template string

What is expected?

Script should be blocked from executing

What is actually happening?

Script executes


As the example illustrates, if a <script> element is the sole element passed into the template string of a component, it will execute said script block. However if it's pre-ceded by a valid element, eg: a div the script is blocked from execution by the renderer. You do not have to close said element as shown in examples, simply preceding the script tag with a valid opening tag suffices. I believe this is because the renderer automatically closes elements with missing closures.

@posva
Copy link
Member

posva commented Jul 1, 2020

Every one of them throws an Error telling you it contains a script tag and that isn't supported but I think it makes sense to block it in all situations.

That being said, if you are running templates provided by the user, note you are still responsible for making them safe

@hybridwebdev
Copy link
Author

hybridwebdev commented Jul 1, 2020

Every one of them throws an Error telling you it contains a script tag and that isn't supported but I think it makes sense to block it in all situations.

That being said, if you are running templates provided by the user, note you are still responsible for making them safe

Templates should only be responsible for mapping the state to the UI. Avoid placing tags with side-effects in your templates, such as <script>, as they will not be parsed.

Well, clearly this "error" is wrong, as they are indeed parsed and executed in my examples.

That being said, if you are running templates provided by the user, note you are still responsible for making them safe

That's a pretty reprehensible attitude. Clearly this is an issue with Vue, not end users. Sure validation is indeed a step that needs to be taken by developers. However in this particular case, Vue is giving the illusion of safety by:

a) Irresponsibly making declarations proven to be false (the parsing statement in the error code).
b) Blocking execution if wrapped in a valid element, but not if it's the sole element.
c) A glance through Vue's documentation would reaffirm that in this context execution should not be possible. A shining example of this would be specifically forcing developers to use the v-html directive to render raw html and warning of the inherit risk.

Let's not try and shift the blame here. Clearly this is an oversight with potentially grave consequences that needs to be addressed.

@shadowings-zy
Copy link
Contributor

I also encountered this problem in using vue.js. Not all <script> elements are blocked.
When the <script> tag is the root element of the component, it is rendered as usual.
When the <script> tag is a child element of the component, it is not rendered.

Is it a bug? or a feature?

I think block <script> in all situations maybe a better solution.
: )

@pikax
Copy link
Member

pikax commented Jul 2, 2020

That's a pretty reprehensible attitude. Clearly this is an issue with Vue, not end users. Sure validation is indeed a step that needs to be taken by developers.

If you allow your users to define the javascript (in this case compiling the template at runtime) that will run, you always need to sanitize it.
To vue using string templates is the same as a developer writing the code.

A shining example of this would be specifically forcing developers to use the v-html directive to render raw html and warning of the inherit risk.

Vue is not "render raw HTML", vue is rendering the Vue Template that you provide, vue template is not HTML (only HTML spec-compliant).

@yyx990803
Copy link
Member

yyx990803 commented Jul 2, 2020

A glance through Vue's documentation would reaffirm that in this context execution should not be possible. A shining example of this would be specifically forcing developers to use the v-html directive to render raw html and warning of the inherit risk.

https://vuejs.org/v2/guide/security.html#Rule-No-1-Never-Use-Non-trusted-Templates

Root level <script> tags should not be executed, that needs a fix, but just for consistent behavior because it doesn't make it any safer. If you are allowing arbitrary injection into your templates, there are a million other XSS attack vectors by using binding expressions with constructor hack, or image src, etc. The moment you give up control of your template, your site is insecure. Fixing script execution under that situation is like patching a hole on a ship that is already underwater.

@yyx990803 yyx990803 reopened this Jul 2, 2020
@hybridwebdev
Copy link
Author

hybridwebdev commented Jul 2, 2020

https://vuejs.org/v2/guide/security.html#Rule-No-1-Never-Use-Non-trusted-Templates

Your example clearly shows content contained in a div. If you look at my example and read my outline, I am talking about root level script tags. I clearly illustrate script tags wrapped in a node do not execute.

Root level <script> tags should not be executed, that needs a fix, but just for consistent behavior because it doesn't make it any safer. If you are allowing arbitrary injection into your templates, there are a million other XSS attack vectors by using binding expressions with constructor hack, or image src, etc. The moment you give up control of your template, your site is insecure. Fixing script execution under that situation is like patching a hole on a ship that is already underwater.

Whilst again, i do agree that data validation/sanitization is ultimately up to the developer, in this particular situation the vulnerability lies in VUE as by your own words "this needs to be fixed for consistency". This vulnerability comes from the inconsistency.

If you are allowing arbitrary injection into your templates, there are a million other XSS attack vectors by using binding expressions with constructor hack, or image src, etc.

This only makes me wonder what other undiscovered vulnerabilities may lie in the core code. For example, does this mean escaping must be done on attributes bound to variables?

Also, if being unable to "fully trust" VUE to properly sanitize markup it generates, then what purpose does v-html actually serve? Why force users to use it at all?

@yyx990803
Copy link
Member

yyx990803 commented Jul 3, 2020

Similar to how you should not be using user-provided content as your template, we do not recommend using v-html to insert user-provided content either. But v-html can be used to insert HTML you trust. It doesn't have to be user content. It's the user's responsibility to use the API correctly, just like how eval() and new Function() exists in JavaScript, but you shouldn't call them on arbitrary user provided strings.

The point about the link is that you should not use non-trusted content in your template. It does not matter whether it's wrapped by a div or not, because when you use non-trusted content in your template you site is pretty much vulnerable to any kind of XSS attacks and a straight up <script> is least of your concern.

I'm not interested in explaining this further to you since that seems to entail explaining how front end security works from the ground up. This is not a vulnerability, period.

@vuejs vuejs locked as too heated and limited conversation to collaborators Jul 3, 2020
shadowings-zy added a commit to shadowings-zy/vue that referenced this issue Jul 3, 2020
…ll be executed' in issue#11483

Root level <script> tags should not be executed, for consistent behavior. So I remove the code in
<script> tag when the <script> tag is the root element of the template.

fix vuejs#11483
shadowings-zy added a commit to shadowings-zy/vue that referenced this issue Jul 3, 2020
@posva posva added the has PR label Jul 3, 2020
shadowings-zy added a commit to shadowings-zy/vue that referenced this issue Jul 3, 2020
@posva posva changed the title Template string render vulnerability Root level script tag should not be executed Aug 3, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants