candy icon indicating copy to clipboard operation
candy copied to clipboard

Remote code execution vulnerability with XHTML-IM enabled

Open arcriley opened this issue 7 years ago • 18 comments

Candy lacks a proper HTML sanitizer allowing remote javascript code execution which can be used to send spam from other user's accounts, steal account credentials, or exploit browser vulnerabilities.

This hack was done with a simple img onload="" to send a message via Candy's own API. It could just have easily made everyone's clients send their login credentials to the room.

screenshot from 2016-12-31 23-09-01

arcriley avatar Jan 01 '17 04:01 arcriley

It would have been more appropriate to disclose this privately before posting it publicly.

I'll look into solutions when I can. In the meantime we're now in a slightly awkward position. If anyone else has a ready fix, PRs would be greatly appreciated.

benlangfeld avatar Jan 01 '17 17:01 benlangfeld

Sounds interesting. I will do some research I may do a PR on this issue it looks awesome! Great find.

On Sun, Jan 1, 2017 at 12:26 PM Ben Langfeld [email protected] wrote:

It would have been more appropriate to disclose this privately before posting it publicly.

I'll look into solutions when I can. In the meantime we're now in a slightly awkward position. If anyone else has a ready fix, PRs would be greatly appreciated.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/candy-chat/candy/issues/498#issuecomment-269911442, or mute the thread https://github.com/notifications/unsubscribe-auth/AMphBu9g0yvgx94AXY8dgrgT7RGZqf_Iks5rN-HDgaJpZM4LYqXt .

jdc20181 avatar Jan 01 '17 18:01 jdc20181

I would have sent it to your security list if there was one.

I believe the correct patch would be informing users that XHTML-IM should be disabled and removing support for it entirely in the next release. It is simply too difficult to sanitize XHTML-IM correctly, and several modern clients (such as Conversations) has come to this same conclusion.

Consider if you spend the time to write a clean-tree sanitizer; walk through the incoming message node by node, and when valid nodes are found pass the supported attributes to a function which re-creates that node in your clean tree. All someone would need to do to attack this is send an img src="javascript:" or something similar, and each time this is discovered you'd have to go back to make your sanitizer check for something extra. There's no winning this game.

There's also a privacy issue img elements; if I want to discover the IP address of another user all I would need to do is send an image to them privately through a MUC service and check the HTTP server logs for which IP was used to access it. Their IP address reveals their geographic region, with the help of 3rd party lookup services it can show some of their Internet history, or it could be used to launch a ddos attack.

arcriley avatar Jan 01 '17 20:01 arcriley

I'm not a project maintainer just a active developer. Thanks for the input. You outta create a pull request. Simply - Add a security notice in the ReadMe- with the info you provided here. I'm sure they'd appreciate it. DDOS attacks are mainly on your server. Not necessarily in the code. If you maintain a good security on your server shouldn't have a issue.

Additionally some configuring is required with this to make it work. So if you wanna use it you should know that you need to make sure it's secure-

Like said before PR are welcome I'll look into. Could be something cool for me to do.

jdc20181 avatar Jan 01 '17 20:01 jdc20181

DDoS attacks are primarily executed against servers because individual users rarely have their IP addresses exposed like this, but they used to happen on IRC against individual users all the time (and are much easier to accomplish given the limited bandwidth many users have). What changed to stop this is most IRC servers started masking user hostnames/IPs.

arcriley avatar Jan 01 '17 20:01 arcriley

Ah I see- that's cool. Thanks for the info!

jdc20181 avatar Jan 01 '17 21:01 jdc20181

If I worked on a PR and somehow get Something Like this integrated would this help? I am trying to figure out a easy way to add a hotfix. As it seems like its a easy fix. I also suggest adding your own PR with the Security message. Write so no one is alarmed but proceed with caution. and it is reccomended you have xhtml-im disabled.

jdc20181 avatar Jan 02 '17 00:01 jdc20181

This ticket is full of win! Candy Dev, where do you explicitly say where security issues should be reported? (security isn't mentioned in GitHub readme, or your .io page). Bug reporter, XSS is not RCE, at all, in any way (although, bonus points for the Super Troopers image in your XSS PoC). In the mean time, please continue with this disclosure... =)

attritionorg avatar Jan 02 '17 06:01 attritionorg

@attritionorg We don't have an explicit security disclosure process, that's our failure and I will fix it. In the absence of something explicit, emailing the maintainer directly would be a reasonable assumption to make. Instead I now have this public disclosure to scramble after like a headless chicken without any advance warning.

benlangfeld avatar Jan 02 '17 12:01 benlangfeld

This issue was discovered publicly by one of our Google Code-in students, who was writing a plugin for Candy, but used an onclick="" from the sender side. This led to a discussion as to why this wasn't a good solution, and moreso, the security implications of javascript even being allowed.

If you'd like help addressing this we have 2-3 students who are extremely familiar with Candy who would love to clean this up.

It would be great to get you back into the XSF, applications for Q1 2017 are now open.

arcriley avatar Jan 02 '17 18:01 arcriley

If your team has time to prepare a solution, that would be much appreciated! The time I have available to work on Candy is unfortunately very limited since I don't actively use it any more.

benlangfeld avatar Jan 02 '17 18:01 benlangfeld

Arc you are welcome to have anyone contribute. Just create a PR. On Mon, Jan 2, 2017 at 1:40 PM Ben Langfeld [email protected] wrote:

If your team has time to prepare a solution, that would be much appreciated! The time I have available to work on Candy is unfortunately very limited since I don't actively use it any more.

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/candy-chat/candy/issues/498#issuecomment-270006855, or mute the thread https://github.com/notifications/unsubscribe-auth/AMphBgJQLZQpbkVs07Y0i569GLwkFx6sks5rOUSQgaJpZM4LYqXt .

jdc20181 avatar Jan 02 '17 19:01 jdc20181

Was this ever fixed? There a link to the commit etc?

attritionorg avatar Jul 14 '17 02:07 attritionorg

No-one has yet proposed a fix.

benlangfeld avatar Jul 14 '17 12:07 benlangfeld

The fix is to disable support for HTML in messages. Candy isn't the only client with this kind of vulnerability, and once you start to look at the problem nothing short of writing a xhtml-im rendering engine will safely stop these kinds of attacks.

On Jul 14, 2017 5:37 AM, "Ben Langfeld" [email protected] wrote:

No-one has yet proposed a fix.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/candy-chat/candy/issues/498#issuecomment-315349204, or mute the thread https://github.com/notifications/unsubscribe-auth/AB1A8U5CMZupotd9aXGe0IE-qRESuWQnks5sN2EOgaJpZM4LYqXt .

arcriley avatar Jul 14 '17 17:07 arcriley

Do you have a pull request?

benlangfeld avatar Jul 14 '17 17:07 benlangfeld

Do you have a bug bounty for this?

On Fri, Jul 14, 2017 at 10:43 AM, Ben Langfeld [email protected] wrote:

Do you have a pull request?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/candy-chat/candy/issues/498#issuecomment-315421764, or mute the thread https://github.com/notifications/unsubscribe-auth/AB1A8Wg78Y-rgbW_V_Ijf7Oqnwie1o3Oks5sN6jdgaJpZM4LYqXt .

arcriley avatar Aug 01 '17 00:08 arcriley

No.

benlangfeld avatar Aug 04 '17 12:08 benlangfeld