mumble-voip / mumble

Mumble is an open-source, low-latency, high quality voice chat software.
https://www.mumble.info
Other
6.41k stars 1.12k forks source link

Image download should be disabled by default #2318

Closed frecel closed 6 years ago

frecel commented 8 years ago

Since there is no way to stop an image download once it has started (aside from killing mumble) the image download should be disabled by default. This is just to make it a bit harder for trolls to annoy people by deliberately linking to massive image files in the chat.

wkennington commented 8 years ago

Or linking http://192.168.100.1/reset.htm as an image, which causes a reset of the modem for any surfboard user.

Kissaki commented 8 years ago

Or linking http://192.168.100.1/reset.htm as an image, which causes a reset of the modem for any surfboard user.

Are you saying the router will reset for any network-local request to that URL without any additional checks? Pretty stupid by the router to do IMO.

We have a server side limit for embedded image blobs.

Being able to limit/control img-src-ing images would definitely be useful against trolls.

When I send myself a message with a larger image, it is replaced by a dot. This is in Mumble 1.3.0~1305. So no issue there!?

mkrautz commented 8 years ago

@wkennington that would be https://github.com/mumble-voip/mumble/issues/1388

frecel commented 8 years ago

We have a server side limit for embedded image blobs.

imagemessagelength?

Does that affect images sent as base 64 png or linked images? If it only affects linked images could I set it to 0 and effectively disable linking to images in the chat but still allow pasting base 64 images?

Kissaki commented 8 years ago

blob = base64, so not for linked images

WizardGed commented 8 years ago

I've been looking into fixing this and if there was a hook placed into the HTML parser that could drop img src's that have an external link that would solve the problem nearly completely. this could be done by reading the config and looking for a dropNetworkSrc=true set a variable in the HTML parser header that the main HTML Parser function can check and return an empty String or simply an anchor tag with the link URL instead. If that would be acceptable I can look into writing that bit as a patch to murmur itself. right now I will be using a modified MUMO plugin to solve the problem for older servers based on the following plugin https://github.com/Aciid/urltoimg-for-mumo/blob/master/urltoimg.py

mkrautz commented 8 years ago

It's a bit more complex, because I believe Qt's HTML subset for "rich text" supports CSS background-image, etc.

It's worth noting that that "Disable image downloads" (the client side) should work. It ensures Mumble will only show inline base64-images. (Data URLs.)

WizardGed commented 8 years ago

I Understand, I'll do some digging to see what exact tag properties are supported in the QML window and create a list of them that can be exploited. Until then I'll be using the following MUMO plugin I threw together to solve the problem by parsing image tags for URL's https://github.com/WizardGed/networkSrc-to-anchortags_MUMO

DeltoidDelta commented 7 years ago

This is still high security concern that must be addressed. A scenario where one could obtain IP address, and (D)DoSing that user persists, even intruding locational privacy. This is just worse as Skype IP resolving vulnerability, and must be resolved ASAP.

Unless it has been resolved already (which I hope dearly!), please bring your attention to this security vulnerability.

Kissaki commented 7 years ago

We do not have UI support for adding linked images. That means to send them, a user would actually have to write the <img tag himself in the send message dialogue source tab.

So it’s safe to remove support for that. We should probably still give a hint at what happens when that is removed in case a user does it (I think we remove other HTML without notice as well?), or download the image and put it in as a data blob when sanitizing the HTML.

DeltoidDelta commented 7 years ago

Ah, I see. Could you potentially implement click to view or some interactive measures where user has to give consent to download the external image, instead? This will not only solve the security issue, it will allow users to still be able to send images.

Removing the feature completely seems counterproductive, seeing how this feature seems rather essential for modern voip client.

I sincerely thank you for your consideration in regards to security.

mkrautz commented 7 years ago

You can still send images. Mumble has, for a long time, favoured inline images in messages. If you paste/drop an image in the Send Message dialog, it'll be embedded in the message via a data-URL. In fact, there is no way (except for manually typing <img src="http://...."/> in the source view) to send an image in a message that is fetched from a remote server.

DeltoidDelta commented 7 years ago

That's interesting. What was the rationale behind allowing remote image through html? I can see server owners may want to add static images to channels that way (assuming inline images are not possible to notes), but this asks question whether enabling html in the murmur server is safe practice.

mkrautz commented 7 years ago

I don't know, I wasn't the one who implemented it. I suspect the primary use-case was to allow remote images in the welcome message (sent by the server)...

DeltoidDelta commented 7 years ago

Okay, in this case, does it make sense to strip the rights of html remote images to regular users but keep the feature for people who has acl permission, and server itself?

mkrautz commented 7 years ago

Just to be clear: it makes sense to remove the ability to use external images outright.

However, perhaps it makes sense to make an exception for the server's welcome message. The argument for that is that it is typically set in murmur.ini, and pasting a big data URL in there seems insane (also, I suspect it can be hard for people to figure out how to get a data URL for a given image). The counter-argument would be that it should be possible for an admin to set the welcome message via the server, to make it easy to use data URLs.

ghost commented 6 years ago

Fixed with #3168.