Project

General

Profile

Bug #857342

Qt: more care over HTML-escaping

Added by Jacob Nevins 6 months ago. Updated 6 months ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
gui-qt
Target version:
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.)

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

History

#3 Updated by Jacob Nevins 6 months ago

  • Status changed from Resolved to Closed

Also available in: Atom PDF