1430974 - Stored XSS in chrome://browser/content/browser-media.js line 40

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[FLR]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID: 20180103231032

Steps to reproduce:

Like for BUG 1430511, setting the value of the app.support.baseURL preference to include HTML content will cause an XSS issue in line 40 of chrome://browser/content/browser-media.js.


Actual results:

Whenever a EME video player plugins sends a cdm-disabled message to Firefox, the HTML content generated at line line 40 of chrome://browser/content/browser-media.js is showned in a notification popup. Therefore, any malicious content in the HTML code be executed including JavaScript (executed has privileged js ?!?!). 

Moreover, since :
1- A cdm-disabled message is triggered whenever a user is trying to play DRM while CDM plugging is disabled ;
2- Disabling CDM can be performed by writing to the prefs.js (user_pref("media.gmp-widevinecdm.enabled", false);) 

The malicious content execution could be triggered simply visiting a Web site offfering DRM content.


Expected results:

The value of app.support.baseURL should be sanatized prior to be inserted in HTML content at line 40 of chrome://browser/content/browser-media.js.

Comment 1

[Frederik Braun [:freddy]]

Thank you for finding and reporting those bugs, FLR.
This one looks like a sec-moderate (similar to bug 1430511), as it's limited to local attackers.
But do keep looking, I'm sure you'll get there ;)

Comment 2

[:Gijs (he/him)]

Attachment: Patch v0.1

Comment 3

[Florian Quèze [:florian]]

Comment on attachment 8943192 [details] [diff] [review]
Patch v0.1

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

This (especially the getFormattedString partial reimplementation in JS) seems to add more complexity than this is worth.

::: browser/base/content/browser-media.js
@@ +35,5 @@
>      return true;
>    },
>    getLearnMoreLink(msgId) {
>      let text = gNavigatorBundle.getString("emeNotifications." + msgId + ".learnMoreLabel");
>      let baseURL = Services.urlFormatter.formatURLPref("app.support.baseURL");

I'm not sure I understand the threat correctly here, but could we just do something like:
try {
  if (!Services.io.newURI(baseURL).schemeIs("https"))
    throw;
} catch(e) {
  Cu.reportError("invalid app.support.baseURL");
  return "";
}

Should we check that the url is valid directly inside Services.urlFormatter? It seems we have plenty of callers, and some may also insert the url directly into the DOM without sanitization.

@@ +110,5 @@
> +    let message = gNavigatorBundle.getString(msgId);
> +    if (!labelParams.length) {
> +      message = [message];
> +    } else {
> +      message = message.split(/%(?:\$\d)?S/);

Splitting on eg. %$2S without recording the position (2) means we may be swapping the order of the variables if some localized strings don't display the variables in the same order as the en-US string.

Comment 4

[:Gijs (he/him)]

(In reply to Florian Quèze [:florian] from comment #3)
> Comment on attachment 8943192 [details] [diff] [review]
> Patch v0.1
> 
> Review of attachment 8943192 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This (especially the getFormattedString partial reimplementation in JS)
> seems to add more complexity than this is worth.

I disagree. The complexity will go away with fluent anyway, but we should kill the innerHTML stuff with fire.

> ::: browser/base/content/browser-media.js
> @@ +35,5 @@
> >      return true;
> >    },
> >    getLearnMoreLink(msgId) {
> >      let text = gNavigatorBundle.getString("emeNotifications." + msgId + ".learnMoreLabel");
> >      let baseURL = Services.urlFormatter.formatURLPref("app.support.baseURL");
> 
> I'm not sure I understand the threat correctly here, but could we just do
> something like:
> try {
>   if (!Services.io.newURI(baseURL).schemeIs("https"))
>     throw;
> } catch(e) {
>   Cu.reportError("invalid app.support.baseURL");
>   return "";
> }

We could, but I just really don't think we should be assigning to innerHTML, full stop. app.support.baseURL is only one of many issues.

> Should we check that the url is valid directly inside Services.urlFormatter?
> It seems we have plenty of callers, and some may also insert the url
> directly into the DOM without sanitization.

I guess we could. I'm nervous about breaking the entirety of the prefs when l10n or whatever mis-specifies the URL. Right now, the link would be broken but there'd be no other ill effects.

> @@ +110,5 @@
> > +    let message = gNavigatorBundle.getString(msgId);
> > +    if (!labelParams.length) {
> > +      message = [message];
> > +    } else {
> > +      message = message.split(/%(?:\$\d)?S/);
> 
> Splitting on eg. %$2S without recording the position (2) means we may be
> swapping the order of the variables if some localized strings don't display
> the variables in the same order as the en-US string.

That would be an issue if there was ever more than 1 replacement item, and there isn't.

Comment 5

[:Gijs (he/him)]

Attachment: Patch v0.2

Comment 6

[:Gijs (he/him)]

Comment on attachment 8943292 [details] [diff] [review]
Patch v0.2

>diff --git a/browser/base/content/browser-media.js b/browser/base/content/browser-media.js
>       case "cdm-insufficient-version":
>         notificationId = "drmContentCDMInsufficientVersion";
>-        params = [this._brandShortName];
>+        notificationMessage = this.getMessageWithBrandName(notificationId);
>         break;
> 
>       case "cdm-not-installed":
>         notificationId = "drmContentCDMInstalling";
>-        params = [this._brandShortName];
>+        notificationMessage = this.getMessageWithBrandName(notificationId);
>         break;

FWIW, I tried to look at removing this duplication but couldn't think of a reasonable solution. We could remove these lines and then add:

if (!notificationMessage) {
  notificationMessage = this.getMessageWithBrandName(notificationId);
}

but that would possibly surprise future consumers and actually adds lines, so I decided not to do that.

Comment 7

[Florian Quèze [:florian]]

Comment on attachment 8943292 [details] [diff] [review]
Patch v0.2

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

Thanks for updating the patch, looks much better now. :-)

Comment 8

[:Gijs (he/him)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ea40957614e1dddbe0a486b3ddac7dfa77709512

Comment 9

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/ea40957614e1dddbe0a486b3ddac7dfa77709512