jashkenas / backbone

Give your JS App some Backbone with Models, Views, Collections, and Events
http://backbonejs.org
MIT License
28.1k stars 5.39k forks source link

Persistent DOM XSS found #1540

Closed b1shan closed 12 years ago

b1shan commented 12 years ago

Hello folks- example at http://backbonejs.org/examples/todos/index.html is vulnerable to DOM XSS.

Since it uses localStorage this can turn more grave by mimicking a persistent XSS by chaining Clickjacking, Drag & Drop or in a shared environment.

PoC exploit code: enter following in the input box and hit enter "" <img src=0 onerror=alert(0)>

Bish

knowtheory commented 12 years ago

I can't say i'm surprised. It is just a little demo app. That said, a pull request sanitizing the contents pulled from the form prior to creating a new Todo I'm sure would be welcome.

b1shan commented 12 years ago

And people get inspired from these demo apps ending up using in production code.

I suggest using Handlebars / Mustache as your view engine so you get the benefit of auto escaping.

The web is plagued with XSS and projects like Backbone can do good to community by adopting safer methods as defaults. We all know with HTML5 these things are going to be even more wicked.

knowtheory commented 12 years ago

You seem to be under the impression that I have disagreed with you when I haven't. Like I said, pull requests welcome.

The purpose of the todo app is to demonstrate Backbone's basic feature set (which is also, i presume, why @addyosmani has adapted it to the myriad MVWTF frameworks out there), which is why I am unsurprised that as a simple toy app, it doesn't sanitize its input (yet).

That said, I don't think the main demo app should (or will) switch to handlebars/mustache, since again, the point is to show off the basic features of Backbone.

addyosmani commented 12 years ago

I'm with @knowtheory on this one. As a toy/learning application, it's purpose is to just give developers the most basic view of what the framework is capable of.

Including Handlebars/Mustache here (if we're saying people are likely to copy to production, which is scary) might also be taken as Backbone suggesting users should be opting for a specific templating library over another, which you probably don't want to start arguments over.

That said, if a PR with auto-escaping is merged here, I'll happily consider for my TodoMVC apps.

factormystic commented 12 years ago

There's value in doing it the a right way even in the examples, to help beginners get on a safe track. Can the templated properties simply be wrapped with _.escape? No additional dependencies needed.

jashkenas commented 12 years ago

Even easier than that -- it's just a matter of using <%- value %> to interpolate.

b1shan commented 12 years ago

as @factormystic agrees, my only request is to have examples that are "by design safe". Requiring devs to sanitize user input manually hasn't worked else XSS wouldn't still be the #1 web security issue even after more than a decade. I am no Backbone guru. But Mustache or equivalent does a fair job. If you have something even simpler, great.

I agree ith @addyosmani that Backbone might be promoting something in that manner, but then there is YUI Y.App http://yuilibrary.com/yui/docs/app/app-contributors.html that uses Handlebars. May be you could just put a notice that "you could use one of your choice" like YUI has done in main sections.

knowtheory commented 12 years ago

The Backbone docs already note that you can use the templating library of your choice: http://backbonejs.org/#FAQ-tim-toady

knowtheory commented 12 years ago

Fixed this with 71d0fe3dccffc01f7badba5af4b2cae1c5af1aa8

Disappointed that @b1shan is trolling the issue rather than having submitted a change, but oh well.

b1shan commented 12 years ago

@knowtheory i did not understand your disappointment. Intention was definitely not that.

I am not saying you did not fix it.

I am sharing my concern of not promoting safe by default security practices amongst developers. This is a common industry issue that has been pointed by security folks earlier as well. In my other tweet I pointed why Chrome XSS Auditor DOM XSS protection won't protect most users. That doesn't meet Chrome is bad. It's just that people need to know what they are getting at the end of the day,

b1shan commented 12 years ago

And please note, I am doing a responsible disclosure like I did with Chrome bug report. Only after you fixed it, I shared it.