mozilla / codemoji

Mozilla Foundation project to support 2016 Encrypt campaign
Mozilla Public License 2.0
22 stars 19 forks source link

output text with $.text() #201

Closed cadecairos closed 8 years ago

cadecairos commented 8 years ago

@Pomax R?

This is my proposed fix, but I'm having issues getting this running locally. still need to confirm it works.

cc/ @brettgaylor @lovegushwa @simonwex

cadecairos commented 8 years ago

It fixed the xss issue, but it also breaks the decrypting animation.

abusedmedia commented 8 years ago

going to check right now

cadecairos commented 8 years ago

oh, I think I used .text where not necessary:

https://github.com/mozilla/codemoji/pull/201/files#diff-b5f4e1a2529ec05a48bf6968b1bf8aabL64

https://github.com/mozilla/codemoji/pull/201/files#diff-b5f4e1a2529ec05a48bf6968b1bf8aabL68

abusedmedia commented 8 years ago

Changing that part is a bit tricky for the animation part. Is there any other way to sanitize the string before to pass to .html ? ...I'm searching around

cadecairos commented 8 years ago

Changing that part is a bit tricky for the animation part.

I was escaping the special elements that were being used as part of the animation. Turns out I just need to escape the final output part.

abusedmedia commented 8 years ago

xss-free?

abusedmedia commented 8 years ago

I can confirm that changing only that part, the animation works

cadecairos commented 8 years ago

@abusedmedia awesome, thanks for checking it out!

While we're here - can you think of anywhere else codemoji might display the unencrypted message besides that output area?

abusedmedia commented 8 years ago

I think there are not any other place where the clear message is shown in the app... I'll double check, though

Pomax commented 8 years ago

based on code tracing this seems to be the only place where html() has a dangerous result; there is a text attribute that gets passed around as part of the decrypt functions but the value in it (despite itself being potentially dangerous) gets filtered through the twemoji call everywhere, which yields either SVG code or undefined, and so while leading to potentially crummy images, does not lead to a security issue as far as I can tell.

abusedmedia commented 8 years ago

I've been able to reproduce the issue and I can confirm the same conclusions of @Pomax

cadecairos commented 8 years ago

Alright then, Merging!