Escape chat messages at the server by atsampson · Pull Request #939 · jamulussoftware/jamulus

atsampson · 2021-02-01T22:42:51Z

Update my patch to strip HTML from chat messages from #380, as discussed in #890 (@hoffie).

One thing to test with this before merging is that URLs containing & still get link-ified correctly.

hoffie · 2021-02-01T23:10:04Z

One thing to test with this before merging is that URLs containing & still get link-ified correctly.

Uh, good point. I just tried and they still work.

In fact, I think your PR fixes another (hypothetical) problem. Before your PR, https://example.org?foo=1&bar=1 incorrectly linked to https://example.org?foo=1&bar=1 (this is probably true for all html entities). With your PR, this problem no longer occurs.

ann0see · 2021-02-02T17:09:11Z

Hmm. Escaping server sided isn‘t really a secure way to fix the issue in my opinion. A quick fix, yes. But not the final way to solve the original problem.

hoffie · 2021-02-02T20:29:54Z

Escaping server sided isn‘t really a secure way to fix the issue in my opinion. A quick fix, yes. But not the final way to solve the original problem.

I assume you are suggesting to handle it on the target client instead?
I agree it would be better, but this would either require stopping server-side message formatting (i.e. prefixing the user name) or it would require transferring the individual parts of the message (user, text) to the client seperately. The latter would probably equal some (possibly incompatible) change in the protocol, which is probably not desired. :)

Which remaining security issues do you see after merging this PR? "Just" the general UDP-related spoofing things?

ann0see · 2021-02-03T07:05:23Z

I assume you are suggesting to handle it on the target client instead?

Yes. Exactly. Send e.g. json to the client, validate that and then display the message.

Which remaining security issues do you see after merging this PR? "Just" the general UDP-related spoofing things?

That's also an issue. I could also imagine that somebody sets a welcome message with "maliscious" html which we do not escape.

hoffie · 2021-02-03T20:06:14Z

Send e.g. json to the client, validate that and then display the message.

Agreed, but as far as I can see this would require a protocol change/extension. If we were to do that, I guess it would make sense to combine it with some other possible improvements in that area.

I could also imagine that somebody sets a welcome message with "maliscious" html which we do not escape.

Agreed, but I think we will always have to trust the server to a specific level. Clients should not have to trust the server as much as that it could run arbitrary code or something, but regarding the content I think there is nothing one could do. An attempt at comparing it with some other area: Facebook could always impersonate anyone on their platform because they have the required access. No need for any formatting hacks, cross site scripting or anything. They (or here: the Jamulus server) could just change the content.
So, I think we have to live with this part. I would vote for simply trusting Welcome messages as they can only be changed by server operators. Server operators are always in a position to cause denial of service (just stop the server) or do impersonation (run a modified Jamulus server code or something).

I am more concerned that by now a valid use case for allowing (some) HTML has been mentioned by @passing in #890 (comment). I don't know yet how to keep such things still working.

passing · 2021-02-05T20:04:46Z

I am more concerned that by now a valid use case for allowing (some) HTML has been mentioned by @passing in #890 (comment). I don't know yet how to keep such things still working.

@hoffie
a different way could be a possibility to send multi-line messages that are monospaced. So maybe some button to switch from the "default mode" (single line, proportional font) to "tab mode" (multiline, monospaced). With that, a converter that creates html would also no longer be needed.

pljones · 2021-02-07T10:27:00Z

OK, can we summarise:

This does prevent any client of any version - when connected to a server with this fix - using CSS (and, indeed, wider) spoofing in chat
This does not address protocol level attacks of a client

Protection from 2 is not, in my view, the scope of this ticket and shouldn't block adoption of the a solution for a known issue.

softins

Just tried this out and it works fine, as expected.

ann0see · 2021-02-09T10:04:44Z

src/server.cpp

        QTime::currentTime().toString ( "hh:mm:ss AP" ) + ") <b>" +
        ChanName.toHtmlEscaped() +
-        "</b></font> " + strChatText;
+        "</b></font> " + strChatText.toHtmlEscaped();


I assume it works as intended. Should I test it again?

I think the change is trivial enough just to approve. I tested it:

ann0see · 2021-02-09T11:20:26Z

BTW: is this already documented in the ChangeLog?

softins · 2021-02-09T12:17:19Z

BTW: is this already documented in the ChangeLog?

It appears not. Historically, who added items to the ChangeLog? The contributor of a change, or Volker?

EDIT: Looking at the history for ChangeLog, it appears Volker did so himself. If we need contributors to add their own entries, we need to document this in the contribution guidelines.

ann0see · 2021-02-09T12:36:33Z

If we need contributors to add their own entries, we need to document this in the contribution guidelines.

Agree. Probably the contributing file must be updated too.

ann0see · 2021-02-10T17:04:48Z

Just merged it and documented it in the changelog.

PR jamulussoftware#939 disabled HTML usage in chat messages by escaping all user input at the server side for security reasons. There is demand to keep basic text formatting working for sharing lyrics/chords in a sane way. This PR re-enables certain safe tags again to enable such use cases. This works by keeping the existing message escaping, but converting selected safe HTML tags back into their parsable form. To avoid worsening security again, this PR deliberately - does not permit CSS or any color changes which could enable user impersonation, - does not accept any attributes in HTML tags, - does not accept invalid HTML (e.g. start tags without end tags), - does not accept nested HTML tags (except for <br>s within <pre>...</pre>). References: jamulussoftware#1021 jamulussoftware#1524

Escape chat messages at the server.

6ffd1fd

softins approved these changes Feb 8, 2021

View reviewed changes

softins requested review from a team, ann0see, gilgongo and pljones and removed request for a team, gilgongo and pljones February 8, 2021 23:46

ann0see reviewed Feb 9, 2021

View reviewed changes

ann0see approved these changes Feb 9, 2021

View reviewed changes

ann0see merged commit 3fa9421 into jamulussoftware:master Feb 10, 2021

ann0see added a commit that referenced this pull request Feb 10, 2021

Add #939

b9ed4f5

hoffie mentioned this pull request Feb 17, 2021

Sticky text alignment in chat #869

Closed

passing mentioned this pull request Feb 18, 2021

Sharing lyrics with chords (multiline monospaced messages) in the Chat #1020

Closed

hoffie mentioned this pull request Apr 27, 2021

Server: Permit simple HTML in chat messages again #1571

Closed

pljones added this to the Release 3.7.0 milestone Nov 24, 2024

Comments

Conversation

atsampson commented Feb 1, 2021

Uh oh!

hoffie commented Feb 1, 2021

Uh oh!

ann0see commented Feb 2, 2021

Uh oh!

hoffie commented Feb 2, 2021

Uh oh!

ann0see commented Feb 3, 2021

Uh oh!

hoffie commented Feb 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

passing commented Feb 5, 2021

Uh oh!

pljones commented Feb 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

softins left a comment

Choose a reason for hiding this comment

Uh oh!

ann0see Feb 9, 2021

Choose a reason for hiding this comment

Uh oh!

softins Feb 9, 2021

Choose a reason for hiding this comment

Uh oh!

ann0see commented Feb 9, 2021

Uh oh!

softins commented Feb 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ann0see commented Feb 9, 2021

Uh oh!

ann0see commented Feb 10, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

hoffie commented Feb 3, 2021 •

edited

Loading

pljones commented Feb 7, 2021 •

edited

Loading

softins commented Feb 9, 2021 •

edited

Loading