Closed Bug 1434086 Opened 4 years ago Closed 4 years ago

ESR version of sanitization patch from bug 1432778/1432966

Categories

(Firefox :: Security, defect, P1)

52 Branch
defect

Tracking

()

RESOLVED WONTFIX
Tracking Status
firefox-esr52 59+ wontfix
firefox58 --- unaffected
firefox59 --- unaffected
firefox60 --- unaffected

People

(Reporter: dveditz, Unassigned)

References

Details

(Keywords: csectype-priv-escalation, sec-vector)

+++ This bug was initially created as a clone of Bug #1432778 +++

Patches from bug 1432966 plus test breakage fixes landed to fix security bug 1432778 in post-quantum releases. The specific critical escalation path does not exist on the ESR branch but taking the general sanitization fix would protect us against unknown similar issues.

One downside is probable breakage of an unknown number of legacy add-ons, not an issue in the versions that got the original patch.

An alternate, more conservative, approach for ESR might be to finish the audit of that branch (already done? in progress?) and spot-fix any identified bugs. Wouldn't protect against unknown chrome escalation in add-ons, but also wouldn't break a bunch of them. In particular ESR might support a largish number of "Dark Matter" enterprise add-ons in deployments where the "require signing" pref is disabled.
Group: toolkit-core-security → firefox-core-security
Component: WebExtensions: Frontend → Security
Product: Toolkit → Firefox
Johann and I audited the ESR branch for potential XSS vulnerabilities, as discussed in the previous meeting.

Yesterday (Monday, Jan 29th) Johann and I went through all occurrences of outerHTML=, innerHTML=, insertAdjacentHTML(), document.write(), document.writeln() and made sure they are listed in the spreadsheet at <https://docs.google.com/spreadsheets/d/1PtTT_9Iz_yHFxN_oRMpkL0ZN1tisdBIn7L_YhtWvbOw/edit#gid=543963657>.
We've omitted uses in test cases and empty string assignments.

All identified source lines are considered secure.
(In reply to Frederik Braun [:freddyb] from comment #1)
> All identified source lines are considered secure.

If that's the case, then it sounds like we've got no spot-fixing to do for now.

It also sounds like what we need to decide here is whether or not landing the patches from bug 1432966 onto ESR is even feasible or desirable given the legacy add-on complication.
(In reply to Mike Conley (:mconley) (:⚙️) from comment #2)
> It also sounds like what we need to decide here is whether or not landing
> the patches from bug 1432966 onto ESR is even feasible or desirable given
> the legacy add-on complication.

We can add an exemption to skip sanitization for injections done by extensions without too much trouble. The problem with that, though, is that injections done by add-ons are the most likely ones to be dangerous...
(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #3)
> We can add an exemption to skip sanitization for injections done by
> extensions without too much trouble.

I think that's maybe worth exploring, yeah.

> The problem with that, though, is that
> injections done by add-ons are the most likely ones to be dangerous...

True, although I suspect it's strictly better than being vulnerable to something like this via drive-by. Like, the required conditions being that an add-on be installed, enabled, and either be openly hostile or insecurely written, is better than nothing, I suppose.
Hey lizzard, do you or anybody from relman have an opinion on how we proceed here? How much should we really be caring about third-party add-on breakage? And if the answer is "a lot", where does relman stand on the Secure-vs-Working trade-off we seem to be approaching here?
Flags: needinfo?(lhenry)
The bad addon scenario sounds more moderate-ish in severity? Fixing the critical issue and leaving a moderate one unfixed (especially when it's for compatibility reasons) wouldn't go against our stated policies for security fixes.
Maybe not even "moderate":
 * we've audited our use and not found anything
 * ESR is stable/bugfix-only branch: we don't need to worry about bugs introduced by new features
 * security bugs in add-ons are bugs in add-ons. Nice to protect against if we can, but not a Firefox bug.

Given comment 1 we could think about closing this "worksforme" or "wontfix".
Keywords: sec-highsec-vector
I'd also consider that we just publicly announced that the threat does not affect ESR (which is true, for all we know). I don't think uplifting this and consequently breaking some add-ons just to be on the safe side is the best way to proceed. ESR 60 is not that far away and we can still move quickly and ignore add-on breakage should we learn about any vulnerabilities we missed.
I concur with johannh. I've been swayed to the WONTFIX camp.
(In reply to Mike Conley (:mconley) (:⚙️) from comment #9)
> I concur with johannh. I've been swayed to the WONTFIX camp.

+1!
My guess is that we'd see a lot of complaint by enterprise-specific add-ons (as Dan mentions in comment 0) so if this is not a critical issue for ESR, I agree we can leave it alone for now and make sure it is completely fixed for 60.
Flags: needinfo?(lhenry)
Okay, thanks folks. I hope I'm not being too presumptuous here, but given comment 6 to comment 11, it sounds like this is a WONTFIX, so I'm going to mark it as such. Please re-open if there's other stuff to talk about here.
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → WONTFIX
Group: firefox-core-security
You need to log in before you can comment on or make changes to this bug.