Closed Bug 1565931 Opened 5 years ago Closed 5 years ago

Ensure consumers are aware readability.js is not itself a DOM sanitizer

Categories

(Toolkit :: Reader Mode, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
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?

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.

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).

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.

(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...

(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.

(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: nobody → fbraun
Group: core-security → core-security-release
Summary: Readability.js does not reliably remove scripts → Ensure consumers are aware readability.js is not itself a DOM sanitizer

Brave's iOS app is a fork of Firefox for iOS. Their Reader Mode code matches ours.

Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: