Bug 321778 - Status bar shows unsanitized strings (possible denial of service)
Summary: Status bar shows unsanitized strings (possible denial of service)
Status: RESOLVED FIXED
Alias: None
Product: dolphin
Classification: Unclassified
Component: bars: status (show other bugs)
Version: 2.2
Platform: Other Linux
: NOR normal (vote)
Target Milestone: ---
Assignee: Dolphin Bug Assignee
URL:
Keywords: investigated, reproducible
Depends on:
Blocks:
 
Reported: 2013-06-30 02:17 UTC by Fabio D'Urso
Modified: 2013-08-05 00:16 UTC (History)
1 user (show)

See Also:
Latest Commit:
Version Fixed In: 4.11.0


Attachments
Konqueror screenshot (83.04 KB, image/png)
2013-07-25 13:48 UTC, Fabio D'Urso
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Fabio D'Urso 2013-06-30 02:17:32 UTC
I've been playing with symlinks and, since you can basically put anything as target, I've tried:
  ln -s '<font color="red">Hello</font>!' test-html
This results in a red "Hello" being shown in the status bar on mouse over.

Then the evil me tried:
  ln -s '<img src="/dev/urandom">' test-freeze
This freezes dolphin on mouse over. The UI becomes unresponsive, CPU goes to 100% and it keeps slowly allocating memory. So I thought it was something worth a bug report :)


Reproducible: Always




A similar issue happens if I create a regular file named "<font color=red>Hello" (the status bar text becomes red on mouse over). Therefore, the issue seems to affect the status bar in general, not only symlink preview texts.
Comment 1 Frank Reininghaus 2013-06-30 12:18:55 UTC
Thanks for the bug report! I can confirm the problem.
Comment 2 Fabio D'Urso 2013-07-01 01:50:52 UTC
I've tried the naive approach: 
diff --git a/dolphin/src/statusbar/dolphinstatusbar.cpp b/dolphin/src/statusbar/dolphinstatusbar.cpp
index 6f734ed..1489191 100644
--- a/dolphin/src/statusbar/dolphinstatusbar.cpp
+++ b/dolphin/src/statusbar/dolphinstatusbar.cpp
@@ -62,6 +62,7 @@ DolphinStatusBar::DolphinStatusBar(QWidget* parent) :
     // Initialize text label
     m_label = new QLabel(this);
     m_label->setWordWrap(true);
+    m_label->setTextFormat(Qt::PlainText);
     m_label->installEventFilter(this);
 
     // Initialize zoom widget

It fixes the issue in dolphin, however...
I've found out that the same thing happens on konqueror, but only if the filename starts with <html> or <qt> (see KonqStatusBarMessageLabel::Private::isRichText in lib/konq/konq_statusbarmessagelabel.cpp).
I hope I'm not hijacking the bug: I've had a look at the code and I see that konqueror uses dolphinpart too, but statusbar messages follow a different code path (DolphinPart::slotRequestItemInfo instead of DolphinViewContainer::showItemInfo).

I'll try to come up with a better patch tomorrow :)
Comment 3 Frank Reininghaus 2013-07-01 16:09:23 UTC
Thanks for your analysis, Fabio! I also thought that something like the Qt::PlainText approach might work, but you're right that this does not help much for the DolphinPart.

An alternative might be to use Qt's escape() function:

http://qt-project.org/doc/qt-4.8/qt.html#escape

I'm wondering if it might make sense to do the escaping in KFileItem::getStatusBarInfo() though. However, that might break applications that expect that function to return plain text. According to lxr.kde.org, there are not many users of that function, maybe one could check if there is a problematic use of that function.
Comment 4 Fabio D'Urso 2013-07-01 16:30:30 UTC
I have tried it, but using Qt::escape within dolphin (i.e. the DolphinViewContainer::showItemInfo case) doesn't work, because the string gets truncated if it doesn't fit, and bad things happen if we truncate HTML strings. For example let's take "<img src=/dev/urandom>":
 &lt;img src=/dev/urandom&gt; -[truncate last two chars]-> &lt;img src=/dev/urandom&g
which results in "<img src=/dev/urandom&g" being shown.
This happens because QFontMetrics::elidedText (called in DolphinStatusBar::updateLabelText) doesn't know about HTML.

On the other hand, for the konqueror side, its heuristic approach to decide if a string is HTML seems to suggest that the solution is to always escape it.
The question is whether is konqueror wrong in expecting HTML from the Part's setStatusBarText text, or is dolphin wrong in not escaping.

The API doc doesn't help: http://api.kde.org/4.x-api/kdelibs-apidocs/kparts/html/classKParts_1_1Part.html#a809740a79edf0fa0bbe1dd982a6c4d35
Comment 5 Frank Reininghaus 2013-07-17 13:40:39 UTC
Thanks for the update, and sorry for the late reply. I just tried it in Konqueror, and for me it even displays the correct name of the file in the status bar if the file name is something like

<html><b>Hello<⁄b><⁄html> or <qt><b>Hello<⁄b><⁄qt>

Do you really see any problem in Konqueror with these (or similar) file names? If that is not the case, maybe we can just go ahead with the Qt::PlainText idea.
Comment 6 Fabio D'Urso 2013-07-25 13:48:59 UTC
Created attachment 81341 [details]
Konqueror screenshot

(In reply to comment #5)
> Do you really see any problem in Konqueror with these (or similar) file
> names? If that is not the case, maybe we can just go ahead with the
> Qt::PlainText idea.
Hi, sorry for the delay. Yes, I do see issues in konqueror (see attached screenshot).

I guess we might: 1) use Qt::PlainText internally (ie DolphinViewContainer::showItemInfo stays unchanged), 2) escape strings that are exported to the outside world (ie DolphinPart::slotRequestItemInfo).
Some quick tests show that this approach works for both dolphin and konqueror. Still, I'd like to be sure that Part's setStatusBarText is actually expected to carry HTML strings.
Comment 7 Frank Reininghaus 2013-07-25 21:17:28 UTC
Hm, strange - I still cannot reproduce the problem in Konqueror. Maybe I'm doing something wrong though, I don't know.

I think that the Dolphin part of your plan sounds good. I cannot say much about the DolphinPart/KParts part. Could you maybe submit a patch to https://git.reviewboard.kde.org/ and add the "dolphin" group and David Faure in the "People" field? I think he gets far too much bug mail to be able to read all of it in detail, but he always looks at incoming review requests.
Comment 8 Fabio D'Urso 2013-07-29 09:23:50 UTC
Git commit 4450f8449af91e491636728a4669e2a9e27b49fa by Fabio D'Urso.
Committed on 01/07/2013 at 00:02.
Pushed by fabiod into branch 'KDE/4.11'.

Don't let HTML-like filenames be interpreted as HTML strings

So that filenames that look like HTML don't get fancy-formatted when
we show info about them (i.e. on hover)

This patch fixes the same issue in two places:
 - dolphin, by setting Qt::PlainText on the status bar's label
 - konqueror, by escaping setStatusBarText strings emitted by
   DolphinPart
FIXED-IN: 4.11.0
REVIEW: 111746

M  +3    -1    dolphin/src/dolphinpart.cpp
M  +1    -0    dolphin/src/statusbar/dolphinstatusbar.cpp

http://commits.kde.org/kde-baseapps/4450f8449af91e491636728a4669e2a9e27b49fa
Comment 9 Fabio D'Urso 2013-08-05 00:16:16 UTC
Git commit 7eec927432e7c3e67e0a1633527e9b71f6c19fbd by Fabio D'Urso.
Committed on 01/08/2013 at 12:14.
Pushed by fabiod into branch 'KDE/4.11'.

DolphinPart: Use Qt::convertFromPlainText instead of Qt::escape for filenames

Unlike escape, convertFromPlainText preserves whitespace sequences
REVIEW: 111835

M  +1    -1    dolphin/src/dolphinpart.cpp

http://commits.kde.org/kde-baseapps/7eec927432e7c3e67e0a1633527e9b71f6c19fbd