Bug 293657 - HTML injection in KWin Killer
Summary: HTML injection in KWin Killer
Status: RESOLVED FIXED
Alias: None
Product: kwin
Classification: Plasma
Component: core (show other bugs)
Version: 4.7.4
Platform: Ubuntu Linux
: NOR normal
Target Milestone: 4.9 Beta 1
Assignee: KWin default assignee
URL: https://git.reviewboard.kde.org/r/104989
Keywords:
Depends on:
Blocks:
 
Reported: 2012-02-08 17:31 UTC by Marco Cimmino
Modified: 2012-05-19 09:56 UTC (History)
1 user (show)

See Also:
Latest Commit:
Version Fixed In: 4.9.0
mgraesslin: ReviewRequest+


Attachments
Example of the issue (59.26 KB, image/png)
2012-02-08 17:32 UTC, Marco Cimmino
Details
Reproducible example (524 bytes, application/x-gzip)
2012-04-12 06:28 UTC, Marco Cimmino
Details
Proposed patch (1.05 KB, patch)
2012-04-12 07:12 UTC, Marco Cimmino
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Marco Cimmino 2012-02-08 17:31:15 UTC
Version:           unspecified (using KDE 4.7.4) 
OS:                Linux

See below.

Reproducible: Always

Steps to Reproduce:
0. have a program with window title with some html
1. make the program to hang (while (1) do something)
2. try to close it and wait for the termination warning dialog

Actual Results:  
html is rendered (see the screenshot)

Expected Results:  
html is not rendered
Comment 1 Marco Cimmino 2012-02-08 17:32:51 UTC
Created attachment 68629 [details]
Example of the issue
Comment 2 Martin Flöser 2012-04-08 20:10:17 UTC
Do you have a test case for this issue?
Comment 3 Marco Cimmino 2012-04-12 06:28:31 UTC
Created attachment 70330 [details]
Reproducible example

Yes, this is a short example that creates a window and when you'll try to close freezes.
Trying to close it multiple times will reproduce the issue.

To compile (Qt is needed):
qmake
make
Comment 4 Marco Cimmino 2012-04-12 07:12:44 UTC
Created attachment 70331 [details]
Proposed patch

Proposed patch, I didn't try it, so might even not compile.

best
Marco Cimmino
Comment 5 Marco Cimmino 2012-05-14 02:03:12 UTC
KDE 4.8.2 also affected. Any other things you need to fix this bug??
Comment 6 Thomas Lübking 2012-05-14 10:21:10 UTC
I guess by comment #2
Martin either wondered whether 
- you've an exploit where this causes some vulnerability (you have not because qlabel can handle html, but "injection" sounds like) or 
- there's an actual client using html tags in the title (ie. you've a special interest) because

otherwise this "bug"
- only affects text rendering
- in the itself rare case of a hanging client (a *buggy* client)
- which probably doesn't exist anyway

and is by this rather theoretic what raises the question whether the fix of this looooong standing "issue" justifies even remotely risking a regression in a minor release.
Comment 7 Martin Flöser 2012-05-14 17:45:14 UTC
btw patches on bug reports get easily lost. You might want to propose it on reviewboard.kde.org
Comment 8 Marco Cimmino 2012-05-14 18:27:20 UTC
(In reply to comment #6)
> I guess by comment #2
> Martin either wondered whether 
> - you've an exploit where this causes some vulnerability (you have not
> because qlabel can handle html, but "injection" sounds like) or 
> - there's an actual client using html tags in the title (ie. you've a
> special interest) because
> 
> otherwise this "bug"
> - only affects text rendering
> - in the itself rare case of a hanging client (a *buggy* client)
> - which probably doesn't exist anyway
> 
> and is by this rather theoretic what raises the question whether the fix of
> this looooong standing "issue" justifies even remotely risking a regression
> in a minor release.

Developers that spend more time finding excuses to not fix a bug rather than fixing it they are funny to me.
Especially when there is a proposed patch already that needs just to be reviewed.
Really do you think that a couple of escaped description strings can 'risking a regression'?
Funny, just funny.
Comment 9 Marco Cimmino 2012-05-14 18:29:02 UTC
But I understand your point, you don't fix even more important issues leaving non working features across different major versions, this bug from your point of view is not even a bug.

I apologize if I tried to make KDE a bit better.
Comment 10 Lydia Pintscher 2012-05-14 18:42:39 UTC
Marco, I can see you are frustrated. Thank you for your patch. Please understand that the KWin team is probably one of the ones in KDE who take their bug reports very seriously and who are usually on top of their reports. A change that might seem trivial to you can still have serious unintended consequences. Martin isn't saying this out of malice or anything. He's just very careful in minor releases out of experience.
To move this forward: Can you answer Martin's questions?
Comment 11 Thomas Lübking 2012-05-14 19:10:06 UTC
He didn't reply to Martin but to my speculations on why this patch wasn't dealt
(I wasn't sure whether Martin is currently abroad or so and didn't want to leave him alone)

Turns out that Martin isn't away and the bug probably just dropped off the table.

I was unfortunately a bit straight in expressing that the issue is rather minor and esp. not really an html injection (what would be a severe bug but is not possible in this context at all) and am really sorry because I may have offended others expectations.
Comment 12 Marco Cimmino 2012-05-14 19:22:59 UTC
(In reply to comment #11)
> He didn't reply to Martin but to my speculations on why this patch wasn't
> dealt
> (I wasn't sure whether Martin is currently abroad or so and didn't want to
> leave him alone)
> 
> Turns out that Martin isn't away and the bug probably just dropped off the
> table.
> 
> I was unfortunately a bit straight in expressing that the issue is rather
> minor and esp. not really an html injection (what would be a severe bug but
> is not possible in this context at all) and am really sorry because I may
> have offended others expectations.

Surely QLabel has a subset of HTML, sorry I should have said RichText:
http://qt-project.org/doc/qt-4.8/richtext-html-subset.html

Surely it supports also images and you can for example include a nice png that uses the _many_ vulnerabilities discovered in libpng and still _unpatched_ in Qt 4.8.1 (that ships libpng 1.5.4 still!!!).
So for dynamically linked libpng this is not an issue, for statically linked it is.

One can argue that if you runned the code this is already enough to make stuff happens, but is not true, there might be other ways to hang a client, it is not so rare as you think.
Potentially one can write a simple web page with html tags in the title and then a simple JavaScript that hangs the whole browser.
Modern browsers should have their own dialog to prevent this, but eh... you're relying on other's work now to save your ass, don't you? ;)

Said this we can all agree on the fact this is not a very common and severe security issue, change the topic if makes you feel better, but answering like you did is far from what I expected.

Just FYI: when a person sends me a patch for an issuet the first two words I say are "Thank you" whatever the patch is for and how is written. I call it respect.
Comment 13 Martin Flöser 2012-05-14 20:02:18 UTC
On Monday 14 May 2012 19:22:59 you wrote:
> Surely it supports also images and you can for example include a nice png
> that uses the _many_ vulnerabilities discovered in libpng and still
> _unpatched_ in Qt 4.8.1 (that ships libpng 1.5.4 still!!!).
> So for dynamically linked libpng this is not an issue, for statically linked
> it is.
ldd /home/martin/kde/lib/kde4/libexec/kwin_killer_helper | grep png
        libpng12.so.0 => /lib/x86_64-linux-gnu/libpng12.so.0

As a matter of fact it just does not matter whether it is possible or not to 
inject anything in that dialog. There is no security issue with it. It is not 
possible to have privilege escalation as the dialog is running as a user. To 
become a vulnerability another application has to be vulnerable in the first 
place. If that is achieved there are easier ways to compromise the system than 
html inject into the killer dialog and use at least one more vulnerability 
there.
> but answering like you did is far from what I expected.
actually I think it's quite valid to figure out whether there is a real threat 
or if it is just a rendering issue. Given that a discussion of technical 
merits is quite important and of course we have to judge whether a fix should 
be considered for a bug fix release if we are internally days before the next 
feature release.
> Just FYI: when a person sends me a patch for an issuet the first two words I
> say are "Thank you" whatever the patch is for and how is written. I call it
> respect.
Let me say "Thank you" and sorry that I missed the patch. We have a high 
number of bug comments each day and it's very easy to miss a single comment. 
We don't use the bug tracker to exchange patches, which means it's not the 
place to look for those and to review those.
Comment 14 Thomas Lübking 2012-05-14 21:01:18 UTC
(In reply to comment #12)
> Surely it supports also images
> and you can for example include a nice png
yes, local ones - you didn't actually try "kdialog --msgbox '<img src="http://some.img">'", did you?

so what you have to do to cause somthing is:
a) have a Qt client with vulnerable static libpng (no distro, i'd say)
b) have a malicious png
c) *store it in a known local path*
d) alter the title of the browser
e) hang the browser (you won't do that in a java script but we will assume you're doing this with some other client you control anyway)
f) have the user click the close button (instead of killing the browser)
to hopefully gain control over a kdialog process with user rights.

I'd frankly stop when i can do (c) and do worse instead (like fixing crontabs or so)

So, thank you for pointing out that we've a rendering issue in the kill helper dialog - could occasionally be nasty or even confusing - but unless you have an exploit for a compromising html injection (actually no matter whether it requires a weak browser, vulnerability chains are still a valid issue) that does not include a former security breach, it's not sth. severe, sorry.
Comment 15 Martin Flöser 2012-05-19 09:56:58 UTC
Git commit a8cdd876878ffebb4a550613bc41a7fcbc8c3995 by Martin Gräßlin.
Committed on 19/05/2012 at 10:20.
Pushed by graesslin into branch 'master'.

Escape HTML tags in window caption in Aurorae and Killer

The unescaped tags are interpreted as HTML by Aurorae decorations
and the KWin killer. Escaping the tags ensures that the text is not
rendered incorrectly.
FIXED-IN: 4.9.0
REVIEW: 104989

M  +1    -0    kwin/clients/aurorae/src/qml/aurorae.qml
M  +5    -0    kwin/killer/killer.cpp

http://commits.kde.org/kde-workspace/a8cdd876878ffebb4a550613bc41a7fcbc8c3995