Sanitize parsed reader mode article when viewing

RESOLVED FIXED in Firefox 16

Status

()

Firefox for Android
Reader View
P1
normal
RESOLVED FIXED
5 years ago
9 months ago

People

(Reporter: bnicholson, Assigned: lucasr)

Tracking

unspecified
Firefox 18
ARM
Android
Points:
---

Firefox Tracking Flags

(firefox16 fixed, firefox17 fixed, firefox18 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

5 years ago
We sandbox reader mode content, so this should already protect us from content pages. As an added safety precaution, though, we can try sanitizing pages when they're viewed to prevent any scripts from running in the first place.

Comment 1

5 years ago
If you can stick the page in an <iframe>, you might be able to use iframe sandbox without 'allow-scripts' (and probably without 'allow-same-origin' too which would further help), which will disable plugins also. 

I'll paste dveditz's comment from bug 778582 comment 2 : 

"The only safe way is to use the same parser used by the browser itself. Look into nsIParserUtils, or ask the content folks what to do if that's not suitable (particularly hsivonen). Parsing by regexp is another red flag. There are zillions of ways to encode HTML to evade parsers, see all the XSS even on sites that are trying to be careful. The right sanitizer would have stripped out the event handlers."

I will echo that sanitizing HTML is very, very tricky.
(Assignee)

Updated

5 years ago
Assignee: nobody → lucasr.at.mozilla
(Assignee)

Comment 2

5 years ago
Created attachment 656974 [details] [diff] [review]
Sanitize parsed reader mode article when viewing
Attachment #656974 - Flags: review?
(Assignee)

Updated

5 years ago
Attachment #656974 - Flags: review? → review?(mark.finkle)
(Assignee)

Comment 3

5 years ago
Created attachment 657066 [details] [diff] [review]
Sanitize parsed reader mode article when viewing
Attachment #657066 - Flags: review?(mark.finkle)
(Assignee)

Updated

5 years ago
Attachment #656974 - Attachment is obsolete: true
Attachment #656974 - Flags: review?(mark.finkle)
Comment on attachment 657066 [details] [diff] [review]
Sanitize parsed reader mode article when viewing

Do we have any data on how this change affects performance and memory?
Comment on attachment 657066 [details] [diff] [review]
Sanitize parsed reader mode article when viewing

I'd still like to see some before/after speed and memory results
Attachment #657066 - Flags: review?(mark.finkle) → review+

Updated

5 years ago
Flags: sec-review?
(Assignee)

Comment 6

5 years ago
(In reply to Mark Finkle (:mfinkle) from comment #5)
> Comment on attachment 657066 [details] [diff] [review]
> Sanitize parsed reader mode article when viewing
> 
> I'd still like to see some before/after speed and memory results

On a more powerful device like the Galaxy Nexus, the parseFragment() call takes up to 4ms. On a less powerful device like the Galaxy S, it takes up to 5ms.

Still need to check memory usage, but given that the article content is a fairly simple HTML (it's usually just a DIV with some Ps inside) I wouldn't expect it to cause any relevant change.
Flags: sec-review? → sec-review?(dveditz)
(Assignee)

Comment 7

5 years ago
Used the memory reporter to track memory usage and couldn't see any jumps in memory usage when sanitizing the article content.

Pushed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/18155f074640
https://hg.mozilla.org/mozilla-central/rev/18155f074640
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 18
(Assignee)

Comment 9

5 years ago
Comment on attachment 657066 [details] [diff] [review]
Sanitize parsed reader mode article when viewing

[Approval Request Comment]
User impact if declined: Pages with javascript handlers might still be able to run javascript code inside reader. Although the about:reader is unprivileged, the code could still change the page content which is undesirable.
Testing completed (on m-c, etc.): Local testing, no performance regressions, no issues found. Landed in m-c.
Risk to taking this patch (and alternatives if risky): Very low, simply sanitizing the article content before showing it in reader.
String or UUID changes made by this patch: none.
Attachment #657066 - Flags: approval-mozilla-beta?
Attachment #657066 - Flags: approval-mozilla-aurora?
Comment on attachment 657066 [details] [diff] [review]
Sanitize parsed reader mode article when viewing

low risk and early enough in Beta still that we can take this sanitizing patch.
Attachment #657066 - Flags: approval-mozilla-beta?
Attachment #657066 - Flags: approval-mozilla-beta+
Attachment #657066 - Flags: approval-mozilla-aurora?
Attachment #657066 - Flags: approval-mozilla-aurora+
(Assignee)

Comment 11

5 years ago
Pushed to aurora:
https://hg.mozilla.org/releases/mozilla-aurora/rev/b9cd614bedaa

Pushed to beta:
https://hg.mozilla.org/releases/mozilla-beta/rev/d47f00b3ad36

Updated

5 years ago
status-firefox16: --- → fixed
status-firefox17: --- → fixed
status-firefox18: --- → fixed

Updated

4 years ago
Flags: sec-review?(dveditz)
You need to log in before you can comment on or make changes to this bug.