Last Comment Bug 785992 - Sanitize parsed reader mode article when viewing
: Sanitize parsed reader mode article when viewing
Status: RESOLVED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: Reader View (show other bugs)
: unspecified
: ARM Android
: P1 normal (vote)
: Firefox 18
Assigned To: Lucas Rocha (:lucasr)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-08-27 11:59 PDT by Brian Nicholson (:bnicholson) (PTO through August 1)
Modified: 2013-06-20 16:58 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed
fixed


Attachments
Sanitize parsed reader mode article when viewing (1.51 KB, patch)
2012-08-30 12:00 PDT, Lucas Rocha (:lucasr)
no flags Details | Diff | Splinter Review
Sanitize parsed reader mode article when viewing (1.81 KB, patch)
2012-08-30 15:24 PDT, Lucas Rocha (:lucasr)
mark.finkle: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Brian Nicholson (:bnicholson) (PTO through August 1) 2012-08-27 11:59:49 PDT
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 Ian Melven :imelven 2012-08-27 15:34:32 PDT
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.
Comment 2 Lucas Rocha (:lucasr) 2012-08-30 12:00:25 PDT
Created attachment 656974 [details] [diff] [review]
Sanitize parsed reader mode article when viewing
Comment 3 Lucas Rocha (:lucasr) 2012-08-30 15:24:28 PDT
Created attachment 657066 [details] [diff] [review]
Sanitize parsed reader mode article when viewing
Comment 4 Mark Finkle (:mfinkle) (use needinfo?) 2012-08-30 21:53:34 PDT
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 5 Mark Finkle (:mfinkle) (use needinfo?) 2012-08-31 12:24:57 PDT
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
Comment 6 Lucas Rocha (:lucasr) 2012-09-04 02:53:49 PDT
(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.
Comment 7 Lucas Rocha (:lucasr) 2012-09-06 11:40:37 PDT
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
Comment 8 Ryan VanderMeulen [:RyanVM] 2012-09-06 18:09:20 PDT
https://hg.mozilla.org/mozilla-central/rev/18155f074640
Comment 9 Lucas Rocha (:lucasr) 2012-09-07 01:26:54 PDT
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.
Comment 10 Lukas Blakk [:lsblakk] use ?needinfo 2012-09-07 15:07:27 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.