Bug 870870 (CVE-2016-2820)

FHR accepts events from untrusted domains

RESOLVED FIXED in Firefox 46

Status

Firefox Health Report
Client: Desktop
RESOLVED FIXED
5 years ago
11 months ago

People

(Reporter: mgoodwin, Assigned: gfritzsche)

Tracking

({sec-moderate})

Trunk
Firefox 47
sec-moderate
Points:
---

Firefox Tracking Flags

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

Details

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

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

5 years ago
Created attachment 748043 [details]
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).
(Reporter)

Comment 1

5 years ago
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
(Reporter)

Updated

5 years ago
Keywords: sec-moderate
Whiteboard: [csec-other] [csec-priv-escalation]

Updated

5 years ago
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.
(Reporter)

Comment 3

5 years ago
(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)

Comment 5

2 years ago
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)

Updated

2 years ago
Flags: needinfo?(kparlante)
(Assignee)

Comment 6

2 years ago
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)
(Assignee)

Updated

2 years ago
OS: Mac OS X → All
Hardware: x86 → All
Version: unspecified → Trunk
(Reporter)

Comment 7

2 years ago
(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.
status-firefox44: --- → wontfix
status-firefox45: --- → wontfix
status-firefox46: --- → affected
status-firefox47: --- → affected
(Assignee)

Updated

2 years ago
Assignee: nobody → gfritzsche
(Assignee)

Comment 9

2 years ago
Created attachment 8725347 [details] [diff] [review]
Check origin

Freddy, do you want to take a sanity look?
Attachment #8725347 - Flags: feedback?(fbraun)
(Assignee)

Comment 10

2 years ago
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+
Attachment #8725347 - Flags: feedback?(fbraun)
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 :)
(Assignee)

Comment 14

2 years ago
Created attachment 8725696 [details] [diff] [review]
Check origin, v2

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-
(Assignee)

Comment 16

2 years ago
Created attachment 8726154 [details] [diff] [review]
Check origin, v2

Great, i failed to upload the actual updated patch.
Attachment #8725696 - Attachment is obsolete: true
Attachment #8726154 - Flags: review?(MattN+bmo)
(Assignee)

Comment 17

2 years ago
Created attachment 8726158 [details] [diff] [review]
Check origin, v2
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+
(Assignee)

Comment 19

2 years ago
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)
(Assignee)

Updated

2 years ago
Attachment #8726158 - Flags: sec-approval?
(Assignee)

Comment 21

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/99715b34b2da
(Assignee)

Comment 22

2 years ago
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
Last Resolved: 2 years ago
status-firefox47: affected → fixed
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+
https://hg.mozilla.org/releases/mozilla-aurora/rev/0ab8ea0c78f0
status-firefox46: affected → fixed
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

Updated

11 months ago
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.