Skip to content

Adding a few more badcharacters#1083

Closed
mrbrutti wants to merge 2 commits intohandlebars-lang:masterfrom
mrbrutti:master
Closed

Adding a few more badcharacters#1083
mrbrutti wants to merge 2 commits intohandlebars-lang:masterfrom
mrbrutti:master

Conversation

@mrbrutti
Copy link
Copy Markdown

It would be interesting to contemplate adding even more characters.

- Adding these special characters to support better escaping. Testing adding also more characters that could result on XSS, such as ";", "(" and ")" . Currently this library is vulnerable to attacks such as <a href={{foo}}/> where foo is "test onload=alert(1)" resulting on the string <a href=test onload=alert(1)/>
@kpdecker
Copy link
Copy Markdown
Collaborator

This method is intended for escaping HTML content only, not javascript. Could you show me an example of what you are using this for that you'd like this change?

@mrbrutti
Copy link
Copy Markdown
Author

Well, I was adding a few more cases to add common special characters that most libraries include. With that said, I found this when I was testing this edge case <a href={{foo}}/> when { 'foo' : 'test.com onload=alert(1)'} which will be translated as <a href=test.com onload=alert(1)/> which will end on a XSS vulnerability.

@kpdecker
Copy link
Copy Markdown
Collaborator

Ping me at kpdecker at gmail with the repo case.
On Tue, Aug 25, 2015 at 3:39 PM Matias P. Brutti [email protected]
wrote:

Well, I was adding a few more cases to add common special characters that
most libraries include. With that said, I found this when I was testing
this edge case when { 'foo' : 'test.com onload=alert(1)'} which will be
translated as which will end on a XSS vulnerability. http://test.com


Reply to this email directly or view it on GitHub
#1083 (comment)
.

@kpdecker
Copy link
Copy Markdown
Collaborator

Sorry that was formatted oddly in my email. No need to ping. This isn't the right fix as you are escaping to a different language and thus creating invalid data. Escaping = would be sufficient to mitigate, no?

@mrbrutti
Copy link
Copy Markdown
Author

I was going to send another patch escaping ‘(‘ , ')’ and ‘=‘. The reason why I added the other special characters as well is because technically those should be escaped as well, but it could potentially break things if someone is trying to paste something on <p>{{text}}</p> which has \n. Although that will have to be a design decision from your library. 👍

With that said, technically this is more of a fault from developers not inserting the correct “ or ‘ to contain the value of the html Attribute, but it is such a common usage that I think you should try to fix it, by escaping a few more characters as you mentioned and I have recommended above.

@kpdecker kpdecker added this to the 4.0 milestone Aug 26, 2015
@kpdecker kpdecker closed this in 83b8e84 Sep 1, 2015
@kpdecker
Copy link
Copy Markdown
Collaborator

kpdecker commented Sep 1, 2015

Thanks for bringing this up. I went with the = escape as that seemed like the least amount of risk for users. In the compatibility notes I will also note that users should generally opt for quoted attributes whenever possible.

CalebFenton added a commit to srcclr/open-core that referenced this pull request Nov 21, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants