Chat messages are escaped in an unsafe way

[Nicolai Hähnle]

At the moment, '<' is replaced with '{' in chat messages in the NetHost to avoid escaping out of rich-text in chat message displays. There are two things wrong with this:

1. This is potentially insecure, because we rely on the host doing this replacement. A malicious host could trivially disable this replacement and target individual clients with nasty spam in richtext messages.

2. Clearly it would be nicer if players could actually write '<' in their messages.

A much better approach would be to transfer chat messages verbatim, and to perform any necessary escaping in a proper way in ChatMessage::toPrintable().

[Nicolai Hähnle (nha)]

I'm torn as to where this should be targeted. I don't believe this can be used to DoS somebody, let alone execute arbitrary code; then again, those are famous last words when it comes to network security, and I feel really, really uneasy about knowingly releasing code that doesn't sanitize its inputs properly.

I very strongly tend towards moving the < to { substitution to ChatMessage::toPrintable, where it logically belongs.

Properly fixing the escaping so that we can render < signs also in chat messages could be left to build17, though.

[SirVer (sirver)]

potential security issues should be handled. I do not trust myself to realize a Ddosable code and I would hate if widelands could be used to compromise a user in any kind of way. Better safe than sorry; we should maybe also enforce an arbitrary length to the messages to avoid memory overflow attacks from the losing player - Don't know if this is already the case.

I agree that proper escaping is not crucial for build 16.

[Nicolai Hähnle (nha)]

I think I fixed the escaping properly as best as possible given the constraint that the meta server and dedicated server actually use richtext for formatting their messages. Without changing their behaviour, I don't see a reasonable way to lock things down further, and the image loading (which is what I was most suspicious about) is disabled in the current code.

[Chuck Wilder (chuckw20)]

Thanks for watching our backs!

On Thu, Feb 24, 2011 at 1:01 PM, Launchpad Bug Tracker <
<email address hidden>> wrote:

> ** Branch linked: lp:widelands
>
> --
> You received this bug notification because you are subscribed to
> widelands.
> https://bugs.launchpad.net/bugs/723379
>
> Title:
> Chat messages are escaped in an unsafe way
>
> Status in Widelands:
> Fix Committed
>
> Bug description:
> At the moment, '<' is replaced with '{' in chat messages in the
> NetHost to avoid escaping out of rich-text in chat message displays.
> There are two things wrong with this:
>
> 1. This is potentially insecure, because we rely on the host doing
> this replacement. A malicious host could trivially disable this
> replacement and target individual clients with nasty spam in richtext
> messages.
>
> 2. Clearly it would be nicer if players could actually write '<' in
> their messages.
>
> A much better approach would be to transfer chat messages verbatim,
> and to perform any necessary escaping in a proper way in
> ChatMessage::toPrintable().
>

[SirVer (sirver)]

Released in build16-rc1