Use ParserUtils to sanitize entry content about brief

[tanriol]

OK. I've tested it with setting in headlines mode HEADLINES_LOAD_STEP_SIZE = 500
The computer was disconnected from the network and I was switching folders in Brief in order.

The results are inconsistent. On one hand, it seems that for some feeds using ParserUtils causes a slowdown of about 2.5x (the average time for 500 items goes from 150ms to 350ms in one case, from 600ms to 1400ms on average).

On the other hand, some strange effects were observed with JS disabled through docshell. The time to insert all entries for some feeds could increase dramatically (from 600ms to 6s-10s), likely depending on the previous selected feed (bad GC interactions?).

[ancestorak]

Are you measuring parsing alone or something else? What kind of feeds are you using?

I measured in full view (headlines view constructs content asynchronously which complicates measurement). Depending on the feed, with the sanitizer EntryView constructor takes from 40% to 70% longer. The whole refresh is slower by around 10%. Disabling scripts on the docshell had no apparent impact on performance and I didn't observe the weird effects you're describing.

One thing I forgot to consider is that disabling scripts also disables Flash. Videos and other sorts of Flash widgets are important for many feeds, such as music blogs I subscribe to myself.

Given that the privacy risk is minimal and other kinds of screw-ups have been purely theoretical so far, I'm starting to think we should keep things the way they are. The leaves the problem of resolving relative URLs but it can be tackled in other ways.

[tanriol]

Are you measuring parsing alone or something else? What kind of feeds are you using?

I measured the time for inserting items alone. The feeds are mainly text with several image-heavy posts and some posts with social buttons.

Given that the privacy risk is minimal and other kinds of screw-ups have been purely theoretical so far, I'm starting to think we should keep things the way they are.

Do you mean that you are not going to disable scripting at all? If so, I'm afraid that I will have to carry these patches with Digest and continue working with it in parallel. I do want relative URLs to work and do not want in-feed scripts to work in my browser. It would be very nice if I could have this with Brief, but if not... well, then the reader with these features will be called Digest.

P.S. By the way, could you please suggest such a feed with embedded Flash for testing?

[ancestorak]

Here's an example of a feed that embeds flash videos.

Relative URLs something I definitely still want to fix and there are other ways to do it than this.

Let's go over this again. In general, there are two kinds of things that can pose a security risk: giving untrusted parties permissions and exposing information to them.

As for the permissions, the matter is simple. Feeds are injected into a page with content privileges. I consulted with Mozilla developers and I made absolutely sure that a feed script can't do anything that a normal web page loaded by Firefox can't do.

As for information, there is one piece of it that Brief exposes. Because some views contain entries from different feeds, a feed can see what other feeds you subscribe. However, this information can already be obtained in other ways. It can be achieved using the classic CSS history leak that was partially plugged by browsers a few years ago, but is practically impossible to avoid completely and can still be exploited. All the user has to do is open a malicious webpage.

In summary: to execute an exploit, a malicious site would have to trick the user into subscribing to its feed and it would have to specifically target Brief with its relatively miniscule amount of users. All of this can be done much easier and in a way that works in every major browser.

It is like worrying about a poor lock on the door when there is giant hole in the wall next to it.

Am I missing something? Please explain your concerns in detail.

[tanriol]

Here's an example of a feed that embeds flash videos.
Hmm... using iframes... will look...

In general, there are two kinds of things that can pose a security risk: giving untrusted parties permissions and exposing information to them.

Well, there is one more risk - making it possible for an untrusted party to mess with our reader. For example, if a feed item contains an element with some ID and another one is introduced with the same ID, what will happen? Will Brief be able to display the second one or fail with an error? Can an item have ID and other properties setup in such a way that clicking its text will cause us to delete another item?

As for information, there is one piece of it that Brief exposes. Because some views contain entries from different feeds, a feed can see what other feeds you subscribe. However, this information can already be obtained in other ways. It can be achieved using the classic CSS history leak that was partially plugged by browsers a few years ago, but is practically impossible to avoid completely and can still be exploited. All the user has to do is open a malicious webpage.

And I'd prefer to leave it difficult to exploit. Timing attacks are pretty difficult. Looking at the elements around is much easier.

It is like worrying about a poor lock on the door when there is giant hole in the wall next to it.

I'd summaraze it as worrying about a very poor lock when there are banks and there are people who rob them... so the house should be safe as there's much more in a bank.

Furthermore, there is one more reason for disabling these scripts. I do not want to waste memory (not much, yes) and battery power of my laptop on them.

And probably that's not all... for example, what about a feed item (from tracking some forum, for example) with a spammy Flash ad with autostart, sound and volume set to maximum? Or just a video clip with autostart and loud music when I'm talking in Skype?

[ancestorak]

Well, there is one more risk - making it possible for an untrusted party to mess with our reader.

I thought we agreed there are just too many ways a feed item can mess up the reader.

<style> body { display: none !important }</style>
<style> * { font-size: 40em !important }</style>
<div height="900000"/>

To avoid it we would have to employ a whitelist sanitizer that bans all CSS and most HTML elements and attributes, except the several explicitly allowed.

Do you know the old joke about the difference between a mathematician and an engineer?

A mathematician and an engineer agreed to take part in a psychological test. They sat on one side of a room and waited not knowing what to expect. A door opened on the other side and a naked woman came in the room and stood on the far side. They were then instructed that every time they heard a beep they could move half the remaining distance to the woman. They heard a beep and the engineer jumped up and moved halfway across the room while the mathematician continued to sit, looking disgusted and bored. When the mathematician didn’t move after the second beep he was asked why. “Because I know I will never reach the woman.” The engineer was asked why he chose to move and replied, “Because I know that very soon I will be close enough for all practical purposes!”

I think you are looking at it more like a scientist, than an engineer. Probably because you are, in fact, a scientist :) In over 6 years since Brief was first released, there has been only one reported problem of this kind and I fixed it by disabling document.write(). We may discuss hypothetical problems all we want, but in the end we should prioritize problems that actually happen. Solving hypothetical problems that aren't known to actually occur (and if they show up, can be always be fixed then) at the cost of reducing functionality that people definitely do use is not a good engineering trade-off.

And probably that's not all... for example, what about a feed item (from tracking some forum, for example) with a spammy Flash ad with autostart, sound and volume set to maximum? Or just a video clip with autostart when I'm talking in Skype?

If a feed does that, then you can unsubscribe from it - just as you don't visit an annoying website. Do you think Firefox should disable Flash for all websites just because some of them abuse it? I mean, if you could protect from the annoying stuff without breaking the legitimate uses, it would be worth doing. But you can't.

I think the only argument that warrants merit is the privacy one. I still think the risk is minimal but I see I won't be able to convince you. How about creating an about:config pref, then?

[tanriol]

<style> body { display: none !important }</style>
<style> * { font-size: 40em !important }</style>
<div height="900000"/>

It may spoil itself (<div height="900000"/>). Style tags... IIRC, they are not allowed outside <head></head>, although I don't know whether they are really disrespected there.

But it's the place where I'm thinking as an engineer dealing with the (potentially) hostile environment. I'm unable to guarantee that nothing will get through, but I think that the current state - a common sandbox for all the feeds and may the winner eat everything - is not good enough. You said that a malicious site would have to target Brief... but at the moment a targeted attack is not required. A black hat does not need to target Brief as embedding a malicious script is the obvious first step. It does not need an APT, a common cracker is enough.

Do you think Firefox should disable Flash for all websites just because some of them abuse it?

Not because some abuse it, but because it is one of the least secure areas of Firefox.
I welcome the introduction of click-to-play. If I open a dozen pages with middle-click, I expect them to stay silent. If I'm reading an old news item and a new one is retreived... yes, I expect it to stay silent. Even if it contains a video I want to watch... not listen to at the moment it's downloaded.

How about creating an about:config pref, then?

Do you assume no one except me will want this feature and so configuration in GUI is not needed?

[ancestorak]

It may spoil itself (

). Style tags... IIRC, they are not allowed outside , although I don't know whether they are really disrespected there.

Although they don't validate, they are applied in practice. Besides, even entries styling themselves may be easily done in a manner that makes the entire page unusable.

Not because some abuse it, but because it is one of the least secure areas of Firefox. I welcome the introduction of click-to-play.

I understand this. However, what you're proposing isn't analogous to click-to-play, but to permanently disabling all Flash on all websites without an ability to turn it on. That's what I'm describing as a bad usability trade-off.

Do you assume no one except me will want this feature and so configuration in GUI is not needed?

Not necessarily nobody - but few. The switch requires technical knowledge to understand what it does and why. As you must have noticed, Brief's philosophy is to be simple and elegant, so I am very careful about adding stuff to the UI.

Ok, so it's settled - a hidden pref it is. Now we must decide on how to do it. Here's what we've found out so far.

ParserUtils.parseFragment()

• + As a bonus, fixes another bug of resolving relative URLs (not all of them, though).
• - Comes with a performance penalty.
• - Potentially less safe because there's always a risk of security bugs in the parser. It has happened before with a different sanitizer on which many extensions, including feed readers, relied. And that parser was actually safer because it was whitelist-based rather than blacklist-based.
• - Does more than disabling scripts so it may cripple feeds in unnecessary ways. It's quite liberal though, so it's not a big concern.
• - More complicated code-wise, especially if controlled by a pref.

nsIDocShell.allowJavascript

• Opposites of all the above
• - Random performance issues. It's really weird, can you still reproduce that? Do you know you should reload the content page after changing the flag? This is what I did in brief @@ function init():

getElement('feed-view').allowJavascript = false;
getElement('feed-view').reload();

[tanriol]

• As a bonus, fixes another bug of resolving relative URLs (not all of them, though).

Why not all? What are the URLs that are not resolved? It used to work partially before the link intercepting modification you've already merged. Now it should work fully.

[ancestorak]

Oh, ok then.

[tanriol]

This question no longer matters as WebExtensions have access to none of the APIs mentioned here and use an <iframe sandbox> instead as of 2.5 series.