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

optionToHtml does not escape HTML characters in option label #264

Closed
davidcostanzo opened this issue Dec 28, 2015 · 5 comments
Closed

optionToHtml does not escape HTML characters in option label #264

davidcostanzo opened this issue Dec 28, 2015 · 5 comments

Comments

@davidcostanzo
Copy link

My web page includes a <select> whose <option> labels may include escaped HTML characters. These are not escaped in the control which multiple-select creates. In most cases this is unnoticeable. In some cases, it causes some rendering problems. However, because the choices in my Web page may be populated by one user and viewed by another user, in the worst case it's a security risk, as it may give one user the ability to inject a <script> into another user's Web page.

For example, if I turn this into a multiple-select:

<select name="choice" multiple="multiple">
  <option value="1" >Text</option>
  <option value="2" >Italics &lt;i&gt; Tag</option>
</select>

The second option should look like "Italics <i> Tag", not "Italics Tag"

The bug is in optionToHtml. The "text" variable is read as text (and so the HTML is unescaped), but then processed by JQuery as HTML. Here is a patch of how I fixed this in my repository. The diff is done using subversion (not git) and is based on multiple-select-1.2.0 (not master), so you won't be able to apply the patch automatically, but it's simple enough to apply by hand.

@wenzhixin
Copy link
Owner

Any pull request is welcome.

@wenzhixin
Copy link
Owner

Fixed in #255.

@davidcostanzo
Copy link
Author

I didn't see textTemplate documented as part of the interface, so I didn't know what its purpose was. I can see its value and I can see that my fix negated this value. However, I still think this is a bug. Shouldn't multiple-select be secure by default--that is, shouldn't the default textTemplate return $elem.html(), not $elm.text()? Is there a compelling reason to have it return text()?

For example, suppose my Web application allowed an administrator to bulk delete anonymous form posts by their title. The HTML is generated on the server using a server-side template that properly escapes all of the form posts which it inserts into the HTML. This app would appear to work fine, but then what happens when an anonymous post include a <script> in the title. Using a regular <select> is secure because the HTML is escaped. However, if I used multiple-select as documented, an administrator could end up executing untrusted JavaScript from the context of their session.

To illustrate this, consider this snippet:

<select id="ms" multiple="multiple">
   <option selected="selected" value="1">Simple</option>
   <option value="2">Italics &lt;i&gt; Tag</option>
   <option value="3">Bold &lt;b&gt; Tag</option>
   <option value="4">Script &lt;script&gt;alert('do something evil')&lt;/script&gt;</option>
</select>

On my browser (FF) the plain HTML shows everything correctly (as HTML tags). When I enlighten the <select> using $('#ms').multipleSelect(); I get a JavaScript alert before I make any selection. The alert is benign, but it's arbitrary code controlled by an attacker and could be made malicious. None of the docs suggest that I must use textTemplate to mitigate this risk.

@wenzhixin
Copy link
Owner

I think you are right, I have updated the default to $el.html().

@davidcostanzo
Copy link
Author

Cool. Thanks for giving me a second chance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants