search

valor-software / ng2-select

Angular based replacement for select boxes
http://valor-software.github.io/ng2-select/
MIT License
675 stars 585 forks source link

Injection attack potential #242

Open cherrydev opened 8 years ago

cherrydev commented 8 years ago

Because ng-select just dumps the content of the text attribute into [innerHtml] that makes it vulnerable to javascript injection attacks and it is certainly not obvious from the documentation that the developer needs to escape everything inside of text in order to make it secure.

Instead, I recommend allowing a template for the item to be passed in from another attribute on SelectItem or (a more general purpose solution) to provide an [itemTemplate] @Input on the component itself. In order for this to work you will need to construct the list items as separate components with a dynamic template. here is an example of how to create a child component whose template is specified dynamically by a parent component.

Another general-purpose solution would be to allow a component factory to be passed to ng-select that displays the items. You would need to publish an API of which @Inputs and events the component would need to handle.

The general purpose solutions would probably require a major re-work of the component, though, but would also add a the possibility of very flexible behaviour for the selection list, such as trees.

DennisSmolek commented 8 years ago

@cherrydev the attack would only be on your own system, any server or point where this would matter should have it's own injection protection. There isn't anything to stop me from adding my own elements, even my own javascript via developer tools.

I'm no expert but if I can only change my own browser code I dont understand the security risk.. Can you explain?

cherrydev commented 8 years ago

Just imagine any multi-user system where User A can craft malicious content that will be displayed, and therefore execute on User B's browser, and therefore with any credentials of User B.

DennisSmolek commented 8 years ago

Yes, but when User A inputs the malicious code it only exists on their own system. When you submit it to the server to be stored in the database it gets sanitized so before user B see’s the results it’s processed. Stripping HTML, JS code, etc. is pretty standard in that regard… I get that injection happens and can be a problem but I don’t think it matters as much on the front end for a single user.. Maybe I’m wrong?

On Jun 15, 2016, at 3:43 PM, cherrydev notifications@github.com wrote:

Just imagine any multi-user system where User A can craft malicious content that will be displayed, and therefore execute on User B's browser, and therefore with any credentials of User B.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/valor-software/ng2-select/issues/242#issuecomment-226314052, or mute the thread https://github.com/notifications/unsubscribe/ABVRPOAPdvdGH_yirrxNohv-CuSgvVCuks5qMGP_gaJpZM4Iyh_P.

0cv commented 8 years ago

Attack possibilities may not be obvious but it looks like you anticipated what needed to be done. With the latest version of Angular2, each item in the selection throws now a warning in the console WARNING: sanitizing HTML stripped some content (see http://g.co/ng/security#xss).

I think the HTML to build the selection shall be sanitized like mentioned here https://angular.io/docs/ts/latest/guide/security.html#!#xss

cherrydev commented 8 years ago

Another thing I'd point out is that using [innerHtml] at all in Angular2 is pretty much giving up on using the framework in the way it was designed. I'm pretty much re-writing ng2-select for my own specific usage right now. One of the non-specific changes I made was fixing the above to choose a component selector (<select-option-item>) that the ng2-select component will pass the current item to in order to render. The component that satisfies that selector, though, is not provided by the ng2-select component itself, but instead is provided by a parent component. This effectively allows it to be customizable.