Sanitise notification HTML
ClosedPublic

Authored by davidedmundson on Jan 29 2018, 10:44 PM.

Details

Summary

Qt labels support a HTML subset, using a completely internal parser in
QTextDocument.

The Notification spec support an even smaller subset of notification
elements.

It's important to strip out irrelevant tags that could potentially load
remote information without user interaction, such as img
src or even <b style="background:url...

But we want to maintain the basic rich text formatting of bold and
italics and links.

This parser iterates reads the XML, copying only permissable tags and
attributes.

A future obvious improvement would be to merge the original regular
expressions into this stream parser, but I'm trying to minimise
breakages to get this into 5.12.

Test Plan

Moved code into it's own class for easy unit testing
Tried a bunch of things, including what the old regexes were doing

Also ran notify send with a few options to make sure things worked

Diff Detail

Repository
R120 Plasma Workspace
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Restricted Application added a project: Plasma. · View Herald TranscriptJan 29 2018, 10:44 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
davidedmundson requested review of this revision.Jan 29 2018, 10:44 PM
fvogt requested changes to this revision.Jan 30 2018, 6:41 AM
fvogt added a subscriber: fvogt.

This is required for 5.8 as well.

This revision now requires changes to proceed.Jan 30 2018, 6:41 AM
fvogt added inline comments.Jan 30 2018, 12:07 PM
dataengines/notifications/notificationsanitizer.cpp
35

Missing )

58

Invert this and continue instead?

Fabian's requested changes

fvogt added a comment.Jan 30 2018, 3:02 PM

Looks good to me. I didn't try it out myself though, but the unit tests look almost complete to me:

Can you add a unit test for obviously broken HTML, like <img src="asdf" or << img?

aacid added a subscriber: aacid.Jan 30 2018, 10:44 PM

My hard drive crashed over the weekend so i haven't had much time for anything other than trying to salvage stuff from my non-backups, but meanwhile here a stuppid "make it const" comment.

dataengines/notifications/notificationsanitizer.cpp
50

make it const :D

Added const
Added two new unit test rows

One extra test

fvogt accepted this revision.Jan 31 2018, 2:24 PM
This revision is now accepted and ready to land.Jan 31 2018, 2:24 PM
This revision was automatically updated to reflect the committed changes.
broulik added a subscriber: broulik.Feb 2 2018, 4:15 PM

Thanks for taking care of this.

dataengines/notifications/notificationsanitizer.cpp
46

We need a QXmlStreamEntityResolver like KNotification has otherwise HTML entities like &Auml; (for Ä) will error out.

73

Don't write alt if it doesn't have one?

dataengines/notifications/notificationsanitizer.h
3

2018

dataengines/notifications/notificationsengine.cpp
285

Won't you end up with piles of <html> tags since _body is the body text of the notification it would group to.

<html>
<html>
old notification
</html>
new notification
</html>

Not that it really matters, though.

davidedmundson added inline comments.Feb 2 2018, 4:31 PM
dataengines/notifications/notificationsanitizer.cpp
46

They don't, because that's not what we're parsing.

We will parse &amp;Auml; which is handled

Sending Ä will get Ä
Sending &Auml; will show &Auml;

Old code is the same.

73

QmlStreamWriter skips it if the value is empty

dataengines/notifications/notificationsengine.cpp
285

Oh, I didn't think about that code. You're right.

I'll add a .mid()

broulik added inline comments.Feb 2 2018, 4:34 PM
dataengines/notifications/notificationsanitizer.cpp
46

I see. Nevermind then.

zx2c4 reopened this revision.Feb 4 2018, 11:32 PM
zx2c4 added a subscriber: zx2c4.

+ const QUrl url(src);
+ if (url.isLocalFile()) {
+ out.writeAttribute(QStringLiteral("src"), src);
+ } else {
+ //image denied for security reasons! Do not copy the image src here!
+ }

This probably isn't a good idea either, since a remote attacker can
specify any local path, which could have unintended consequences. It's
a nice way, for example, of expanding a remote memory access into a
remote file access (loading file into malloc'd buffers), causing
traffic on network-mapped file paths, or other mischief. Under no
circumstances should a remote user be allowed to supply an arbitrary
local file path.

I'd recommend entirely denying <img> tags, and instead provide
developers with some other API to show photos. I believe this already
exists, in fact.

If you absolutely must have <img> tags, then at least use an inline
data URI, though this of course has its own problems too.

This revision is now accepted and ready to land.Feb 4 2018, 11:32 PM

That would break very core functionality of existing clients and goes against the notification spec.

zx2c4 added a comment.Feb 4 2018, 11:42 PM

That would break very core functionality of existing clients and goes against the notification spec.

Then the spec itself is vulnerable and needs to change.

Switch people to data: URIs, or come up with some other kind of mechanism. Allowing remote users to load and render local paths is not okay. Full stop.

aacid added a comment.Feb 4 2018, 11:52 PM

That would break very core functionality of existing clients and goes against the notification spec.

Then the spec itself is vulnerable and needs to change.

Switch people to data: URIs, or come up with some other kind of mechanism. Allowing remote users to load and render local paths is not okay. Full stop.

There's nothing that allows remote users to load and render local paths in the spec, if that happens at all is because applications dont' sanitize what they send to the notification system, and that has to be fixed on *their side*. Nothing plasma can do. Full Stop.