870870 - (CVE-2016-2820) FHR accepts events from untrusted domains

Status: RESOLVED FIXED

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

Description

[Mark Goodwin [:mgoodwin]]

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

Comment 1

[Mark Goodwin [:mgoodwin]]

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

Comment 2

[Justin Dolske [:Dolske]]

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.

Comment 3

[Mark Goodwin [:mgoodwin]]

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

Comment 4

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 5

[Katie Parlante]

about:healthreport hasn't gone away, so I suspect this is still an issue. Moving needinfo to gfritzsche who will know better...

Comment 6

[Georg Fritzsche [:gfritzsche]]

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

Comment 7

[Mark Goodwin [:mgoodwin]]

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

Comment 8

[Liz Henry (:lizzard) (relman/hg->git project)]

Marking 46+ affected.

Comment 9

[Georg Fritzsche [:gfritzsche]]

Attachment: Check origin

Freddy, do you want to take a sanity look?

Comment 10

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8725347 [details] [diff] [review]
Check origin

Matt, can you review this?

Comment 11

[Frederik Braun [:freddy]]

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

Comment 12

[Matthew N. [:MattN]]

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

Comment 13

[Frederik Braun [:freddy]]

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

Comment 14

[Georg Fritzsche [:gfritzsche]]

Attachment: Check origin, v2

Good info, thanks Matt.

Comment 15

[Matthew N. [:MattN]]

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

Comment 16

[Georg Fritzsche [:gfritzsche]]

Attachment: Check origin, v2

Great, i failed to upload the actual updated patch.

Comment 17

[Georg Fritzsche [:gfritzsche]]

Attachment: Check origin, v2

Comment 18

[Matthew N. [:MattN]]

Comment on attachment 8726158 [details] [diff] [review]
Check origin, v2

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

Thanks

Comment 19

[Georg Fritzsche [:gfritzsche]]

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.

Comment 20

[Frederik Braun [:freddy]]

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)

Comment 21

[Georg Fritzsche [:gfritzsche]]

https://hg.mozilla.org/integration/fx-team/rev/99715b34b2da

Comment 22

[Georg Fritzsche [:gfritzsche]]

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.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/99715b34b2da

Comment 24

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0ab8ea0c78f0