Skip to content

Comments

HTML-escape chat messages at the server#380

Merged
corrados merged 1 commit intojamulussoftware:masterfrom
atsampson:escapechat
Jun 21, 2020
Merged

HTML-escape chat messages at the server#380
corrados merged 1 commit intojamulussoftware:masterfrom
atsampson:escapechat

Conversation

@atsampson
Copy link
Contributor

At present, whatever text you include in your user name or chat messages is passed through directly to the chat display window and interpreted as HTML, which I don't think is intentional. This commit fixes it by having the server escape the message when it adds the channel name and timestamp.

It's not an ideal fix because the client is still trusting arbitrary HTML sent by the server (as it does for server welcome messages, and while external links don't work, cf #360, it can include things like <img> elements that reference local files, which isn't very nice). So maybe it'd be worth thinking about introducing a new type of protocol message where the channel name and text are separate fields?

@corrados
Copy link
Contributor

At present, whatever text you include in your user name or chat messages is passed through directly to the chat display window and interpreted as HTML, which I don't think is intentional.

Actual it is intentional. E.g. a lot of users make use of HTML in their server welcome message.

Why do you want to change this behaviour? Have you seen issues when running your fuzzing tests? BTW: It is really great that you spend time on checking the security of the Jamulus software, thanks for that!

Regarding the channel name (i.e. your change ChanName.toHtmlEscaped()) I think it would be ok to strip out the HTML because here I really don't see a need for it.

@atsampson
Copy link
Contributor Author

I've updated the patch so it only escapes the channel name. (What got me looking at this was that I set my name to <i>AdamS to test the status file change, then tried all the other places where the name is shown and found it appeared in italics in the chat too...)

I'm not sure that QTextBrowser is really intended to be safe against people sending arbitrary bits of HTML to it, but at least the subset of HTML it currently understands is pretty small, so most of the things you can do look annoying rather than dangerous.

One thing that concerns me is that you can make it open arbitrary local paths as resources - e.g. with something like <img src="/usr/share/pixmaps/xterm_48x48.xpm"> or <p style="background: url(/usr/share/pixmaps/xterm_48x48.xpm)">hello (or indeed a never-ending device file, which hangs Jamulus). If it's possible to disable loading resources entirely then that would be a good idea - is it possible to override loadResource with a dummy implementation, or can you think of an easier way?

@corrados corrados merged commit 1b3e49f into jamulussoftware:master Jun 21, 2020
@corrados
Copy link
Contributor

Right now the chat window QTextBrowser does not have links or external links activated. So I assume that we do not have a problem right now. But we have a feature request: #360 which want external links to work. I already have written in the comments that I have security concerns with it. I'll reference your comment in that Issue.

@corrados
Copy link
Contributor

Sorry, I was not reading your comment correctly. You are concerned not about the links but resources. Can you please create a separate Issue for that?

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