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

Script tag is not escaped when using :option="" in form-select #1974

Closed
MerrySean opened this issue Jul 30, 2018 · 17 comments

Comments

@MerrySean
Copy link

commented Jul 30, 2018

I was trying to do some XSS in my project and I got some un-shown text. Which I found out that it was the cause of the script tag that was not escaped.. So I tried doing a v-for loop inside an <option> tag and use
{{ }} (Double brackets) to escaped the script tag. I would like to make a pull request sadly bootstrap_vue is using a render less components (I don't really know but it's using render function) which I don't have much knowledge about. So I just submitted an Issue here.

by the way you can replicate the problem using the Live documentation on Bootstrap_vue site.

My browser: Opera
Operating System: Fedora LXDE

@Remigius2011

This comment has been minimized.

Copy link

commented Aug 15, 2018

same here - browser is chrome.

@Remigius2011

This comment has been minimized.

Copy link

commented Aug 15, 2018

this occurs in b-form-radio-group as well as in b-form-select. please note that this is a severe security problem!

@tmorehouse

This comment has been minimized.

Copy link
Member

commented Aug 22, 2018

Could you provide an example bit of code or a JS Fiddle with a test case?

@Remigius2011

This comment has been minimized.

Copy link

commented Aug 22, 2018

Oh, this is quite easy - it even works interactively in the online documentation when you change the first option line in https://bootstrap-vue.js.org/docs/components/form-radio to the following:

    { text: '\'"><img/src=x onerror="alert(1)">', value: 'first' },

or respectively in https://bootstrap-vue.js.org/docs/components/form-select :

   { value: null, text: '\'"><img/src=x onerror="alert(1)">' },

(don't forget the backslash to escape the single quote). This shows a dialog box, which is harmless. Obviously, this should not happen, it should show the text, which is quite useless unless it is treated as executable code.

@Remigius2011

This comment has been minimized.

Copy link

commented Aug 27, 2018

@tmorehouse have you been able to reproduce the problem or do you need additional assistence?

@Remigius2011

This comment has been minimized.

Copy link

commented Sep 6, 2018

@tmorehouse I have had a look at the source code of form-select. I think, the problem is in line 24 of b-form-select.js, which is currently:

domProps: { innerHTML: option.text, value: option.value }

this means, the (untrusted) data contained in option.text is directly used as innerHTML, which allows to inject JavaScript code using the payload I have supplied above. Assuming the text is fed from a database that contains user entries, a malicious user is able to insert code that is executed in another user's browser. The line should imho be something like:

domProps: { textContent: option.text, value: option.value }

see also https://github.com/vuejs/vue/blob/master/src/platforms/web/server/modules/dom-props.js.

The same applies to form-radio, see line 35 of form-radio-group.js:

[h('span', { domProps: { innerHTML: option.text } })]

should imho be:

[h('span', { domProps: { textContent: option.text } })]

I havent checked whether other components are affected, too, but any use of innerHTML is suspicious - only trusted content should go in there at any rate.

@tmorehouse

This comment has been minimized.

Copy link
Member

commented Sep 6, 2018

This would prevent any HTML markup from being passed.

Shouldn't your script, which is accessing the remote data, or sending the user supplied input to your database be sanitizing the user input before storing it (which is typically a good practice)?

@Remigius2011

This comment has been minimized.

Copy link

commented Sep 6, 2018

imho: nope! app developers would then end up doubling their code for sanitizing all texts...

Seriously, see e.g. the description of v-html in the vuejs doc: https://vuejs.org/v2/guide/syntax.html#Raw-HTML

If you have a property called options.text you are alluding this is interpreted as text and hence passed to a text-like property (domProps.textContent in this case). Otherwise it should be called options.html (which could exist simultaneously and be passed into innerHTML. Otherwise, this should be clearly documented to avoid security risks (I assume more than 90% of app developers who pass in user input will not sanitize it and have a security vulnerability in their app). Please feel free to consult resources of the security folklore to make an informed decision - but at least add some documentation that the property options.text is not secure (i.e. treated as HTML - similar to the remarks for v-html) and explicit options should be used instead if the content is untrusted (e.g. select controls filled from database content and/or user input).

@philippedebridiers

This comment has been minimized.

Copy link

commented Sep 19, 2018

I confirm this behavior, i test with this options.text <iframe id="inlineFrameExample" src="https://www.free.fr"></iframe> and the website is called when the page is displayed.

@Remigius2011

This comment has been minimized.

Copy link

commented Sep 23, 2018

@PhilippeWCS , effectively you can have any javscript code executed on behalf of an innocent user - including but not limited to executing XHR requests - without this being noticed (just leave out the alert).

See also here (link to the documentation of vue-i18n that warns about using v-html): https://kazupon.github.io/vue-i18n/guide/formatting.html

In contrast, vue-bootstrap just treats text as html without even warning the developers.

@tmorehouse tmorehouse closed this in 6dde0cb Nov 4, 2018

tmorehouse added a commit that referenced this issue Nov 4, 2018

tmorehouse added a commit that referenced this issue Nov 4, 2018

Revert "feat(security): Strip HTML script tags before inserting conte…
…nt into DOM. Fixes #1974,#1665 (#2129)" (#2135)

This reverts commit 6dde0cb.

Reverts #2129

Merged onto master instead of dev by mistake

tmorehouse added a commit that referenced this issue Nov 4, 2018

feat(security): Strip HTML script tags before inserting content into …
…DOM. Fixes #1974,#1665 (#2134)

* fixed a typo (#1931)

* Create utils/strip-sripts.js

Utility for removing script tags from injected HTML (i.e. for use with v-html or domProps.innerHTML)

Prevents user supplied input form injecting scripts into the DOM

* mixins/form-options.js use new striptScripts util

* Update button-group.js

Remove validator of size prop... to allow for custom CSS defined sizes

* Update card-body.js

* Update dropdown.js

* Update form-group.js

* Update input-group.js

* Update jumbotron.js

* Update modal.js

* Update nav-item-dropdown.js

* Update progress-bar.js

* Update table.js

* pagination mixin: add stripScripts and remove temporary button styling

* Minor update to table readme
@tmorehouse tmorehouse referenced this issue Nov 14, 2018
81 of 89 tasks complete
@CAR7OGRAPH3R

This comment has been minimized.

Copy link

commented Jan 16, 2019

Hi @tmorehouse

Apologies for commenting on an issue that has already been closed - please let me know if you would prefer a new issue be raised instead.

An XSS vulnerability was recently found in one of our applications that uses a b-form-select element populated from user supplied data ( as mentioned above ).

I have simplified and replicated the scenario ( that does not use <script> tags ) here.

When speaking with the developers ( who admit the validation of this data can and will be improved ), their understanding was that the data would only ever be treated as text, not markup.

OWASP recommend never providing untrusted data to innerHTML, and to use textContent instead - would this be possible?

@Remigius2011

This comment has been minimized.

Copy link

commented Jan 16, 2019

@CAR7OGRAPH3R see conversation above - this was exactly my suggestion - unfortunately to no avail. the work-around is to use <b-form-select>{{ my content goes here }}</b-form-select>.

note also that other controls may be affected, too, which has been established for b-form-radio-group, but there may be others, which have not been investigated yet. therefore it is imho strongly advisable to perform a thourough vulnerability testing whern uising this lib.

imho sanitizing html is a difficult task to say the least. there are many ways to circumvent sanitizing, some of which are easy to be found, like:

https://support.portswigger.net/customer/portal/articles/2590821-xss-beating-html-sanitizing-filters
https://www.owasp.org/index.php/XSS_Filter_Evasion_Cheat_Sheet

but there are numerous others. it is not only necessary to handle each in the sanitizing code, but also (recurrently!) in testing, which is a big effort and also error prone. therefore i prefer the easy and deterministic method of not treating untrusted content as html. period.

there are sources on handling untrusted content (with some hints on esacping html), like:

https://www.owasp.org/index.php/XSS_(Cross_Site_Scripting)_Prevention_Cheat_Sheet

@jpicton

This comment has been minimized.

Copy link

commented Jan 16, 2019

@tmorehouse as outlined above, the merged "fix" for this issue is insufficient for preventing XSS. This issue has caught us out and introduced a security flaw in our application that was only just recently identified.

Has anybody thought to report this vulnerability to the NPM security advisory yet? It would aid others who are unwittingly using a vulnerable version of bootstrap-vue.

@pi0 pi0 reopened this Jan 20, 2019

pi0 added a commit that referenced this issue Jan 20, 2019

feat(security): Strip HTML script tags before inserting content into …
…DOM. Fixes #1974,#1665 (#2134)

* fixed a typo (#1931)

* Create utils/strip-sripts.js

Utility for removing script tags from injected HTML (i.e. for use with v-html or domProps.innerHTML)

Prevents user supplied input form injecting scripts into the DOM

* mixins/form-options.js use new striptScripts util

* Update button-group.js

Remove validator of size prop... to allow for custom CSS defined sizes

* Update card-body.js

* Update dropdown.js

* Update form-group.js

* Update input-group.js

* Update jumbotron.js

* Update modal.js

* Update nav-item-dropdown.js

* Update progress-bar.js

* Update table.js

* pagination mixin: add stripScripts and remove temporary button styling

* Minor update to table readme
@pi0

This comment has been minimized.

Copy link
Member

commented Jan 21, 2019

Other than clearification letter I've created two PRs (Hotfix #2477 + Fix for next RC #2479). Still talking with the team that if it makes sense to avoid using HTML everywhere or not. For inputs and forms, this makes sense. But for areas like modal, it is more convenient for users passing HTML.

@Remigius2011

This comment has been minimized.

Copy link

commented Jan 21, 2019

@pi0 imho the solution could be to support both text and html, but to avoid developers to use html inadvertently. vue itself follows this approach by providing both v-text and v-html properties and a warning not to pass untrusted content into v-html (see one of my comments above). in a similar way, when passing an options array, if passing items as:

{ value: 'a', text: 'This is First option' }

the text should be treated as text (i.e. passed the dom as domProps: { textContent: option.text, value: option.value }) whereas html could be provided like:

{ value: 'a', html: 'This is First <b>HTML</b> option' }

and passed to the dom as domProps: { innerHTML: option.html, value: option.value }. even for people who don't read warnings, there is at least a chance for them to be aware that they are passing html into the control if the property is named html.

@pi0

This comment has been minimized.

Copy link
Member

commented Jan 21, 2019

@Remigius2011 We are going to advise users using slots if need passing HTML. Does it look good to you?

@Remigius2011

This comment has been minimized.

Copy link

commented Jan 21, 2019

@pi0 i have already changed my code to use slots after discovering the vulnerabilities in my app. so actually it is up to you and the team to decide on whether to minimize changes in bootstrap-vue or the likelyhood of other developers to introduce vulnerabilities in her/his app (i would opt for the latter if i were to decide).

i agree with your point in the clarification letter that separating trusted from untrusted input is not (and cannot be) the task of bootstrap-vue (and hence any html sanitization - including removal of <script> tags, which is neither necessary nor sufficient - can be dropped), but providing separate channels for trusted (html) and untrusted (text) data is important, and it vould avoid confusion and wrong expectations if the respective properties were named and documented accordingly. but clearly, the two channels are already there: the text property for trusted html and the slot for untrusted text.

@pi0 pi0 closed this in #2479 Feb 5, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.