Ensure consumers are aware readability.js is not itself a DOM sanitizer
Categories
(Toolkit :: Reader Mode, defect)
Tracking
()
Tracking | Status | |
---|---|---|
firefox-esr60 | --- | unaffected |
firefox-esr68 | --- | unaffected |
firefox68 | --- | unaffected |
firefox69 | --- | unaffected |
firefox70 | --- | unaffected |
People
(Reporter: freddy, Assigned: freddy)
References
Details
(Keywords: sec-low, sec-want)
Attachments
(3 files)
This bug does not affect Reader Mode In Firefox
This bug is a problem with the readability library, as commonly used by various projects.
We do not have a problem in Firefox Reader Mode, because we use a sanitizer after using readability.
But anyhow, I found some trivial ways to XSS when using the Readability library with untrusted web documents and using innerHTML to insert the article into a document. (more in separate bug).
But I wonder what this means for users of Readability that don't.
Should Readability strongly encourage that people use a JS sanitizer like DOMPurify if working with untrusted documents or should it do a better job at stripping scripts (e.g., by using DOMPurify internally)?
What do you think, Gijs?
Assignee | ||
Comment 1•5 years ago
|
||
Assignee | ||
Comment 2•5 years ago
|
||
Assignee | ||
Comment 3•5 years ago
|
||
Assignee | ||
Comment 4•5 years ago
|
||
Hmpf, the poc does not work on Bugzilla, but a simple readerable HTML page with <img src=x onerror=alert(1)>
in the right place will suffice.
Comment 5•5 years ago
|
||
I added https://github.com/mozilla/readability/blob/master/README.md#security .
Do you think that's good enough?
As noted in that para, I don't really want to add a sanitizer into readability, nor do I think it'd be a good idea for DOMPurify to be a hard dependency (requiring Firefox to ship with it, which would be problematic for size reasons given the duplication with our own sanitizer, and perhaps for license reasons, too).
Assignee | ||
Comment 6•5 years ago
|
||
Yeah, I think that makes a lot of sense. I'll see if I can find offenders and file follow-ups as needed.
Just unsure about how harshly this violates the assumptions from non-Mozilla users of the library and what to do about them.
Comment 7•5 years ago
|
||
(In reply to Frederik Braun [:freddyb] (PTO until July 29th) from comment #6)
Yeah, I think that makes a lot of sense. I'll see if I can find offenders and file follow-ups as needed.
Out of interest, have you checked Firefox for iOS?
Just unsure about how harshly this violates the assumptions from non-Mozilla users of the library and what to do about them.
I don't know the answer to that. We don't publish on NPM, mostly because I've never been able to make time to sit down and figure out how that would work. So I'm not aware of getting a list of people who depend on it, or to see how many of them have figured this out for themselves or how it's used...
Assignee | ||
Comment 8•5 years ago
|
||
(In reply to :Gijs (he/him) from comment #7)
(In reply to Frederik Braun [:freddyb] (PTO until July 29th) from comment #6)
Yeah, I think that makes a lot of sense. I'll see if I can find offenders and file follow-ups as needed.
Out of interest, have you checked Firefox for iOS?
Funnily, I could not find the code that does the sanitizing, but fortunately my injections in the article body and the article title that successfully made it through Readability did not result in XSS problems on iOS.
Just unsure about how harshly this violates the assumptions from non-Mozilla users of the library and what to do about them.
I don't know the answer to that. We don't publish on NPM, mostly because I've never been able to make time to sit down and figure out how that would work. So I'm not aware of getting a list of people who depend on it, or to see how many of them have figured this out for themselves or how it's used...
There's a list at https://github.com/mozilla/readability/network/dependents but it does not seem to contain a lot of high-profile targets, except maybe Brave for iOS. I'll find out if there are security contacts and if so will let them know, but I don't intend to spend more than an hour on this.
Comment 9•5 years ago
|
||
(In reply to Frederik Braun [:freddyb] (PTO until July 29th) from comment #8)
(In reply to :Gijs (he/him) from comment #7)
(In reply to Frederik Braun [:freddyb] (PTO until July 29th) from comment #6)
Yeah, I think that makes a lot of sense. I'll see if I can find offenders and file follow-ups as needed.
Out of interest, have you checked Firefox for iOS?
Funnily, I could not find the code that does the sanitizing, but fortunately my injections in the article body and the article title that successfully made it through Readability did not result in XSS problems on iOS.
Just unsure about how harshly this violates the assumptions from non-Mozilla users of the library and what to do about them.
I don't know the answer to that. We don't publish on NPM, mostly because I've never been able to make time to sit down and figure out how that would work. So I'm not aware of getting a list of people who depend on it, or to see how many of them have figured this out for themselves or how it's used...
There's a list at https://github.com/mozilla/readability/network/dependents but it does not seem to contain a lot of high-profile targets, except maybe Brave for iOS. I'll find out if there are security contacts and if so will let them know, but I don't intend to spend more than an hour on this.
Sounds good to me. If you can close this when you're happy we're not 0-day'ing Brave, that'd be great. :-)
Assignee | ||
Comment 10•5 years ago
|
||
Brave's iOS app is a fork of Firefox for iOS. Their Reader Mode code matches ours.
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Description
•