Closed
Bug 785992
Opened 12 years ago
Closed 12 years ago
Sanitize parsed reader mode article when viewing
Categories
(Firefox for Android Graveyard :: Reader View, defect, P1)
Tracking
(firefox16 fixed, firefox17 fixed, firefox18 fixed)
RESOLVED
FIXED
Firefox 18
People
(Reporter: bnicholson, Assigned: lucasr)
Details
Attachments
(1 file, 1 obsolete file)
1.81 KB,
patch
|
mfinkle
:
review+
lsblakk
:
approval-mozilla-aurora+
lsblakk
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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•12 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•12 years ago
|
Assignee: nobody → lucasr.at.mozilla
Assignee | ||
Comment 2•12 years ago
|
||
Attachment #656974 -
Flags: review?
Assignee | ||
Updated•12 years ago
|
Attachment #656974 -
Flags: review? → review?(mark.finkle)
Assignee | ||
Comment 3•12 years ago
|
||
Attachment #657066 -
Flags: review?(mark.finkle)
Assignee | ||
Updated•12 years ago
|
Attachment #656974 -
Attachment is obsolete: true
Attachment #656974 -
Flags: review?(mark.finkle)
Comment 4•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
Flags: sec-review?
Assignee | ||
Comment 6•12 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.
Updated•12 years ago
|
Flags: sec-review? → sec-review?(dveditz)
Assignee | ||
Comment 7•12 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
Comment 8•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 18
Assignee | ||
Comment 9•12 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 10•12 years ago
|
||
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•12 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•12 years ago
|
Updated•11 years ago
|
Flags: sec-review?(dveditz)
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•