Project

Profile

Help

HostedRedmine.com has moved to the Planio platform. All logins and passwords remained the same. All users will be able to login and use Redmine just as before. Read more...

Bug #857342

closed

Qt: more care over HTML-escaping

Added by Jacob Nevins over 3 years ago. Updated over 3 years ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
gui-qt
Sprint/Milestone:
Start date:
Due date:
% Done:

0%

Estimated time:

Description

The Qt client frequently takes strings from the ruleset, city names, usernames etc, and interpolates them into HTML contexts without escaping them.

I've audited for cases where strings were used in the presence of obvious HTML tags in the code. I have probably missed some cases where the code doesn't add tags, but the Qt::AutoText rules ("mainly checks whether there is something that looks like a tag before the first line break") would allow variable text to trigger HTML rendering. I've tried to take a little more care over player-controlled strings (player names, city names) but I have probably missed some of those too. (The most restrictive city name vetting in is_allowed_city_name doesn't let through any of <, >, or ".)

Tooltips are a bit of a crapshoot, because of their Qt::AutoText behaviour which is not easy to change. I've erred on the side of not escaping, which lets "<i>Silly</i>" through; the alternative could mangle "Fine & Sensible" into "Fine &amp; Sensible". If we wanted to fix this properly, I think we'd have to make our own tooltip override and use it everywhere.

I've not looked at the ruleset editor or modpack installer.

The forthcoming patch is definitely not a complete solution, but it improves things a lot.

This could obviously be argued to be a security risk. I don't see any obvious horrors like <script> in the Qt HTML subset, but this bug does expose the Qt HTML interpreter/renderer as an attack surface; I don't know if there are other opportunities for mischief (DoS etc).

(I think it would be nice if there were a type transition between plain text and HTML -- the coder currently has to keep track by hand of whether a QString is supposed to contain text or HTML -- but if we want that, we'd have to invent it ourselves.)

(This was epic yak-shaving on the way to #857341.)


Files

26-beastly.patch (8.25 KB) 26-beastly.patch Various difficult names in rulesets (mostly stub) for testing. Not for commit. Jacob Nevins, 2020-01-26 02:43 AM
26_alien_html.sav.bz2 (30.1 KB) 26_alien_html.sav.bz2 Beastly city names in alien savegame, for testing. Jacob Nevins, 2020-01-26 02:44 AM
m-qt-html-escape.patch (29 KB) m-qt-html-escape.patch master Jacob Nevins, 2020-01-26 02:49 AM
30-qt-html-escape.patch (29 KB) 30-qt-html-escape.patch S3_0 Jacob Nevins, 2020-01-26 02:49 AM
26-qt-html-escape.patch (28.9 KB) 26-qt-html-escape.patch S2_6 Jacob Nevins, 2020-01-26 02:49 AM
Actions #3

Updated by Jacob Nevins over 3 years ago

  • Status changed from Resolved to Closed

Also available in: Atom PDF