Closed Bug 870870 (CVE-2016-2820) Opened 7 years ago Closed 4 years ago

FHR accepts events from untrusted domains

Categories

(Firefox Health Report Graveyard :: Client: Desktop, defect)

defect
Not set

Tracking

(firefox44 wontfix, firefox45 wontfix, firefox46 fixed, firefox47 fixed)

RESOLVED FIXED
Firefox 47
Tracking Status
firefox44 --- wontfix
firefox45 --- wontfix
firefox46 --- fixed
firefox47 --- fixed

People

(Reporter: mgoodwin, Assigned: gfritzsche)

Details

(Keywords: sec-moderate, Whiteboard: [csec-other] [csec-priv-escalation][post-critsmash-triage][adv-main46+])

Attachments

(2 files, 3 obsolete files)

Attached file PoC
Issue:
Firefox Health Report accepts DisableDataSubmission, EnableDataSubmission, RequestCurrentPrefs and RequestCurrentPayload events from any content document present in the remote-report iframe. This allows malicious content to change a users' sharing preferences.

Remediation:
An origin check should be performed in handleRemoteCommand. We could also check that the event is handling user input (where applicable).
STR:
1) Configure a profile for browser debugging. Using the browser debugger, set a breakpoint on abouthealth.js in handleRemoteCommand (say, line 95)
2) Visit about:healthreport
3) open web console
4) type the following into the web console, press enter: document.getElementById('remote-report').src='https://bug870870.bugzilla.mozilla.org/attachment.cgi?id=748043&t=jgghGsznjq'
5) Observe breakpoint has been hit
Keywords: sec-moderate
Whiteboard: [csec-other] [csec-priv-escalation]
Group: core-security → mozilla-services-security
Seems like it would be useful to have a simple way to limit docshell navigations to within a single site (or maybe a simple "can this docshell load page X?" callback). I suspect this is already possible with existing goop, but I don't know if it's "simple".

Would also be good to consider if other iframes-loading-stuff have similar issues.
(In reply to Justin Dolske [:Dolske] from comment #2)
> Would also be good to consider if other iframes-loading-stuff have similar
> issues.

Yeah, freddyb and I were thinking of writing some guidelines around this kind of thing; it seems to be something people want to do more often.
Group: cloud-services-security → firefox-core-security
Katie can you follow up to see if this is still an issue? 
We are triaging some older sec-moderate bugs and think this may be obsolete.
Flags: needinfo?(kparlante)
about:healthreport hasn't gone away, so I suspect this is still an issue. Moving needinfo to gfritzsche who will know better...
Flags: needinfo?(gfritzsche)
Flags: needinfo?(kparlante)
This is still an issue.

I would like to understand what the attack vector here is, giving that we only load content from one origin here (i.e. what the pref "datareporting.healthreport.about.reportUrl" is set to, "https://fhr.cdn.mozilla.net/%LOCALE%/v4/").
Flags: needinfo?(gfritzsche) → needinfo?(mgoodwin)
OS: Mac OS X → All
Hardware: x86 → All
Version: unspecified → Trunk
(In reply to Georg Fritzsche [:gfritzsche] from comment #6)
> I would like to understand what the attack vector here is, giving that we
> only load content from one origin here (i.e. what the pref
> "datareporting.healthreport.about.reportUrl" is set to,
> "https://fhr.cdn.mozilla.net/%LOCALE%/v4/").

We're giving web content control over this part of the user's browser (we generally try to avoid that - though this is becoming less dangerous with the content signature work). Currently, compromise of the CDN or maybe a bug that allows navigation from within this iframe, etc. could all result in sharing preferences being changed.
Flags: needinfo?(mgoodwin)
Marking 46+ affected.
Assignee: nobody → gfritzsche
Attached patch Check origin (obsolete) — Splinter Review
Freddy, do you want to take a sanity look?
Attachment #8725347 - Flags: feedback?(fbraun)
Comment on attachment 8725347 [details] [diff] [review]
Check origin

Matt, can you review this?
Attachment #8725347 - Flags: review?(MattN+bmo)
Comment on attachment 8725347 [details] [diff] [review]
Check origin

Review of attachment 8725347 [details] [diff] [review]:
-----------------------------------------------------------------

You're doing a stricter check here (exact equality of the URLs) instead of just checking that the origin is the same (i.e., comparing the prePath attributes of the nsIURI objects). 
This is, of course, fine with me :)
Attachment #8725347 - Flags: feedback?(fbraun) → feedback+
Comment on attachment 8725347 [details] [diff] [review]
Check origin

Review of attachment 8725347 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/abouthealthreport/abouthealth.js
@@ +111,5 @@
>  
>    handleRemoteCommand: function (evt) {
> +    // Do an origin check to harden against the frame content being loaded from unexpected locations.
> +    let reportURI = this._getReportURI();
> +    let originURI = Services.io.newURI(evt.target.documentURI, null, null);

Quoting bz from another security bug:
> In general, document.documentURI should NOT be used for making security-related decisions.

Generally, principals are what you use for security checks. Please use evt.target.nodePrincipal.

@@ +112,5 @@
>    handleRemoteCommand: function (evt) {
> +    // Do an origin check to harden against the frame content being loaded from unexpected locations.
> +    let reportURI = this._getReportURI();
> +    let originURI = Services.io.newURI(evt.target.documentURI, null, null);
> +    if (!reportURI.equalsExceptRef(originURI)) {

You're including the path in this comparison which is somewhat stricter but may pose problems if we redirect on the server to a different path. Also note that you're naming the variable `originURI` but it's actually not just the origin.

I think what you want is:
> let allowedPrincipal = Services.scriptSecurityManager.getCodebasePrincipal(this._getReportURI());
> if (!allowedPrincipal.equals(evt.target.nodePrincipal)) {
Attachment #8725347 - Flags: review?(MattN+bmo)
Attachment #8725347 - Flags: review-
Attachment #8725347 - Flags: feedback?(fbraun)
Attachment #8725347 - Flags: feedback+
I'd trust bz's judgement more than mine :-)

I was (possibly incorrectly) assuming that looking at the document from chrome would protect us from DOM Clobbering attacks  so we'd see the "truth". I'm interested to find out more about this. If Georg still wants a second opinion, I suggest that someone with more content security chops could provide feedback here :)
Attached patch Check origin, v2 (obsolete) — Splinter Review
Good info, thanks Matt.
Attachment #8725347 - Attachment is obsolete: true
Attachment #8725696 - Flags: review?(MattN+bmo)
Comment on attachment 8725696 [details] [diff] [review]
Check origin, v2

Review of attachment 8725696 [details] [diff] [review]:
-----------------------------------------------------------------

This looks like the same patch as v1
Attachment #8725696 - Flags: review?(MattN+bmo) → review-
Attached patch Check origin, v2 (obsolete) — Splinter Review
Great, i failed to upload the actual updated patch.
Attachment #8725696 - Attachment is obsolete: true
Attachment #8726154 - Flags: review?(MattN+bmo)
Attached patch Check origin, v2Splinter Review
Attachment #8726154 - Attachment is obsolete: true
Attachment #8726154 - Flags: review?(MattN+bmo)
Attachment #8726158 - Flags: review?(MattN+bmo)
Comment on attachment 8726158 [details] [diff] [review]
Check origin, v2

Review of attachment 8726158 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks
Attachment #8726158 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8726158 [details] [diff] [review]
Check origin, v2

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
This issue alone does not allow an exploit - a bug that allows redirecting the content iframe client-side or hijacking the CDN would be required.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Yes, the comments make and calls used here make it very clear.

Which older supported branches are affected by this flaw?
All supported branches, the report goes back to 2013.

If not all supported branches, which bug introduced the flaw?
n/a

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
This applies fine, that message handling functionality hasn't really changed since 2013.

How likely is this patch to cause regressions; how much testing does it need?
It is unlikely to cause serious regressions, but should get flagged for QA confirming about:healthreport didn't break before it goes to release.
Attachment #8726158 - Flags: sec-approval?
FYI: A sec-moderate bug doesn't need sec-approval as per https://wiki.mozilla.org/Security/Bug_Approval_Process#Process.
(You'll still need branch approval if you want to land this anywhere else but central, which is orthogonal to sec-approval)
Attachment #8726158 - Flags: sec-approval?
Comment on attachment 8726158 [details] [diff] [review]
Check origin, v2

Approval Request Comment
[Feature/regressing bug #]: about:healthreport
[User impact if declined]: Potential to control data submission & read user data if the FHR CDN was compromised or in case of client bugs allowing redirecting about:healthreports content frame.
[Describe test coverage new/current, TreeHerder]: Manual testing.
[Risks and why]: Very low risk of affecting about:healthreport working, QA can mitigate.
[String/UUID change made/needed]: None.
Attachment #8726158 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/99715b34b2da
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Comment on attachment 8726158 [details] [diff] [review]
Check origin, v2

Tighten security for FHR, ok to uplift to aurora.
This should make the beta 1 build on Monday.
Attachment #8726158 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Group: firefox-core-security → core-security-release
Whiteboard: [csec-other] [csec-priv-escalation] → [csec-other] [csec-priv-escalation][post-critsmash-triage]
Whiteboard: [csec-other] [csec-priv-escalation][post-critsmash-triage] → [csec-other] [csec-priv-escalation][post-critsmash-triage][adv-main46+]
Alias: CVE-2016-2820
Group: core-security-release
Product: Firefox Health Report → Firefox Health Report Graveyard
You need to log in before you can comment on or make changes to this bug.