XSS vulnerability on p-tooltip #1558

Closed
bjorneven opened this Issue Dec 12, 2016 · 1 comment

Projects

None yet

2 participants

@bjorneven
bjorneven commented Dec 12, 2016 edited

I'm submitting a ... (check one with "x")

[x ] bug report => Search github for a similar issue or PR before submitting
[ ] feature request => Please check if request is not on the roadmap already https://github.com/primefaces/primeng/wiki/Roadmap
[ ] support request => Please do not submit support request here, instead see http://forum.primefaces.org/viewforum.php?f=35

During a security check, p-tooltip with the following pTooltip

<input size="100" [(ngModel)]="xss_var"/>
<abbr pTooltip="{{xss_var}}"> test xss</abbr>

gives an alert-box at hover when xss_var is

Test XSS <img src='/eeeeerrrrrorrrr.jpg' onerror='javascript:alert(123)'>

If this is an intentional feature, I think it needs to be documented clearly. For example an [escaped] attribute to turn on and off escaping.

Plunkr Case (Bug Reports)
See following plnkr for reproduction
http://plnkr.co/edit/p3Km9oGRXuVw6zAKfcqH

Expected behavior
It should be documented whether or not the variable bindings to pTooltip accepts raw html without any escaping. Coming from JSF this used to be a non-issue with primefaces.

Please tell us about your environment:
Using latest node, prime and angular 2.3 and typescript.

@bjorneven bjorneven changed the title from p-tooltip accepts raw html without escaping to XSS vulnerability: p-tooltip accepts raw html without escaping Dec 15, 2016
@bjorneven bjorneven changed the title from XSS vulnerability: p-tooltip accepts raw html without escaping to XSS vulnerability: p-tooltip should sanitize before setting innerHtml text Dec 15, 2016
@bjorneven

Reading the source code makes me realize that this is the culprit:
https://github.com/primefaces/primeng/blob/master/components/tooltip/tooltip.ts#L112

tooltipText.innerHTML = this.text;
A better solution would perhaps be to sanitize with https://angular.io/docs/ts/latest/api/platform-browser/index/DomSanitizer-class.html ?

@cagataycivici cagataycivici changed the title from XSS vulnerability: p-tooltip should sanitize before setting innerHtml text to XSS vulnerability on p-tooltip Feb 1, 2017
@cagataycivici cagataycivici self-assigned this Feb 1, 2017
@cagataycivici cagataycivici added a commit that closed this issue Feb 1, 2017
@cagataycivici cagataycivici Fixed #1558 665ca5e
@cagataycivici cagataycivici added this to the 2.0-RC3 milestone Feb 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment