1432778 - (CVE-2018-5124) Chrome level XSS in LightWeight theme prompts

Status: VERIFIED FIXED

(WebExtensions :: Frontend, defect, P1)

Description

[Johann Hofmann [:johannh]]

Attachment: testcase.html

[Tracking Requested - why for this release]:
Chrome XSS

STR:

- Open testcase.html
- Click Install
- Note Chrome level alert dialog.

(Note that you likely don't even need the install button)

The code in question that injects this is

https://searchfox.org/mozilla-central/rev/e3cba77cee3ff1be38313abe9c804d13c51bd95b/browser/modules/ExtensionsUI.jsm#285

and the data comes from

https://searchfox.org/mozilla-central/rev/e3cba77cee3ff1be38313abe9c804d13c51bd95b/browser/base/content/browser-addons.js#648

It's supposed to be sanitized in

https://searchfox.org/mozilla-central/source/toolkit/mozapps/extensions/LightweightThemeManager.jsm#721

but that doesn't strip HTML tags, oh well.

I can work on a patch now and will unassign if I don't manage to upload it today.

Comment 1

[Johann Hofmann [:johannh]]

Attachment: patch

Asking mconley for review because his student is working on it and I know he's waking up in his timezone right now. Any other toolkit peer who would like to take this is welcome to.

Freddy, can you please give this another look, too?

Comment 2

[Johann Hofmann [:johannh]]

To clarify, I found this while looking at bug 1373642. We need to find a sensible way to deal with that bug until this one is resolved, because the problem is quite obvious from looking at that patch.

I did not contact any of the involved volunteers (ntim and Connor).

Comment 3

[Johann Hofmann [:johannh]]

The solution in that patch is intentionally simple, I was unrelatedly asking my intern Prathiksha to work on removing all innerHTML usage in these prompts the right way a few days ago and we'll now speed up that work (it's tracked in bug 1431242).

Comment 4

[Frederik Braun [:freddy]]

Comment on attachment 8945073 [details] [diff] [review]
patch

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

r+, but note that I'm not a Firefox/Toolkit peer, so this is mostly my "looks secure" stamp, with the following caveats:
This is fine for now, given that we want a simple patch that is easily uplifted (and this here seems to work for release and newer).

In the long run, we'd ideally get away from innerHTML or do the sanitization close to the source.

Comment 5

[:Gijs (he/him)]

Instead of this, can you use BrowserUtils.getLocalizedFragment ?

Manually HTML-escaping like this is... ugly, sometimes there are ways of bypassing it, etc. etc.

Comment 6

[:Gijs (he/him)]

(I just landed that in bug 1431428, but if necessary we can uplift that method to 59)

Comment 7

[:Gijs (he/him)]

Alternatively, creating a document fragment and assigning to .textContent and taking out innerHTML would have the same effect as the current sanitization and be safer. To get a docfrag you could probably use the hidden window ref from Services.wm/ww or whatever that is...

Comment 8

[Johann Hofmann [:johannh]]

Attachment: open_my_calculator.html

We need this in 58. There's no way this can't go to release. For reference, I've included a drive-by arbitrary code execution example (the good old calculator). This can be done by literally any website on the planet in release right now.

Comment 9

[Johann Hofmann [:johannh]]

(Example is built for OSX but can be trivially adapted to other cases)

Comment 10

[Johann Hofmann [:johannh]]

(In reply to :Gijs (lower availability until Jan 27) from comment #7)
> Alternatively, creating a document fragment and assigning to .textContent
> and taking out innerHTML would have the same effect as the current
> sanitization and be safer. To get a docfrag you could probably use the
> hidden window ref from Services.wm/ww or whatever that is...

Yes, as I mentioned, we're working on this. Let's patch this up and solve the issue properly in a few days. There are existing bugs for this, I just don't want to complicate the fix.

Comment 11

[Mike Conley (:mconley) (:⚙️)]

Looking.

Comment 12

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8945073 [details] [diff] [review]
patch

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

Hey johannh,

Good (and disturbing) find.

I understand the urgency, but I'm only r-'ing for now because I want us to do due diligence. I want us to consult with smaug on whether or not nsIParserUtils is a better way of plugging this attack vector entirely than a regex search/replace.

::: toolkit/mozapps/extensions/LightweightThemeManager.jsm
@@ +736,5 @@
>      if (!val)
>        return null;
> +    // Escape &, <, and > characters in a string so that it may be
> +    // injected as part of raw markup.
> +    val = val.replace(/&/g, "&amp;")

Like Gijs, I'm a little worried about going for quick-and-dirty here. While we super-need-to-fix this, we need to be absolutely sure we're plugging this hole correctly and fully.

So, let's step back a second and see what our options are.

One thing I've found is that nsIParserUtils offers a function called convertToPlainText.

I've found one usage: https://searchfox.org/mozilla-central/rev/e3cba77cee3ff1be38313abe9c804d13c51bd95b/toolkit/components/feeds/FeedProcessor.js#610-614

Maybe even better is sanitize():

https://searchfox.org/mozilla-central/rev/e3cba77cee3ff1be38313abe9c804d13c51bd95b/parser/html/nsIParserUtils.idl#61-94

Tested here:

https://searchfox.org/mozilla-central/source/parser/xml/test/unit/test_sanitizer.js#16-19

I wonder if this would be more resilient? I've never used these functions before myself, but they seem suited to our purposes. Perhaps we should ask somebody who knows about them (smaug comes to mind).

Comment 13

[Johann Hofmann [:johannh]]

I think then at that point the easiest thing to do is to remove the boldness of the text from these prompts entirely, eliminating the need for innerHTML. We can restore it in bug 1369482. I'll make a patch.

Comment 14

[Johann Hofmann [:johannh]]

Looping in relman

Comment 15

[:Gijs (he/him)]

Is ESR affected? AFAICT this was introduced in bug 1336085 / bug 1316996 and so the answer is "no", but we should be sure before shipping a fix, and I can't quickly see if the lwt prompts were only switched to the extensionUI infrastructure "later". It'd also be helpful to know how long we've been shipping this.

Comment 16

[Sylvestre Ledru [:Sylvestre]]

AFAIK, this isn't a chemspill driver, right? Seems that has been found internally and nobody uses this issue.

Comment 17

[Johann Hofmann [:johannh]]

I can't reproduce on ESR. Is Thunderbird affected? AFAIK they support Themes, but I'm not sure about the mechanisms they use/don't use from our code. Are they using these prompts?

Comment 18

[Mike Conley (:mconley) (:⚙️)]

(In reply to Sylvestre Ledru [:sylvestre] from comment #16)
> AFAIK, this isn't a chemspill driver, right? Seems that has been found
> internally and nobody uses this issue.

Yes it was found internally, and hasn't (as far as I know) been observed being used out in the wild. But the ease and reliability with which this can be exploited, and the fact that the exploit results in chrome-level privileges, makes me think this is definitely worth chemspilling over. That's just my opinion, though.

Comment 19

[Johann Hofmann [:johannh]]

Second what Mike said, we don't know whether anyone is exploiting this in the wild, but it's a very bad exploit, that, once discovered, would render Firefox virtually too unsafe to use for the general population.

We should get this patch right, but at the same time move quickly here.

Comment 20

[Frederik Braun [:freddy]]

Re, chemspill: Finding and exploiting this is as easy as it gets. I agree with mconley.


(In reply to Mike Conley (:mconley) (:⚙️) from comment #12)
> …
> 
> So, let's step back a second and see what our options are.
> 
> One thing I've found is that nsIParserUtils offers a function called
> convertToPlainText.
> …
> Maybe even better is sanitize():
> …
> I wonder if this would be more resilient? I've never used these functions
> before myself, but they seem suited to our purposes. Perhaps we should ask
> somebody who knows about them (smaug comes to mind).


If we wanted to allow <b> and disallow <script>, then sanitize() would be a great solution.
But I think we want to disallow everything coming from the extension metadata, so convertToPlainText is the proper way.
This is less true, if we do the sanitize() very near to the innerHTML (as I suggested in comment 4), of course.

FWIW, in _this_ case, with the current way of using innerHTML and the localized string, Johann's patch is secure. 
If the goal is to have a quick & dirty patch that will be removed with the innerHTML usage very soon, I'm not sure it's worth bringing an HTML parser to the discussion :-) *shrugs*

Comment 21

[Sylvestre Ledru [:Sylvestre]]

I created a private slack channel and invited mconley & Johann with other stakeholders so that we can have a discussion about this.

Comment 22

[:Gijs (he/him)]

(In reply to Frederik Braun [:freddyb] from comment #20)
> FWIW, in _this_ case, with the current way of using innerHTML and the
> localized string, Johann's patch is secure. 
> If the goal is to have a quick & dirty patch that will be removed with the
> innerHTML usage very soon, I'm not sure it's worth bringing an HTML parser
> to the discussion :-) *shrugs*

Well, the other thing it does is it only updates .name. Have we audited whether there are other codepaths where the header or any other innerHTML'd things in ExtensionsUI.jsm use content from the web, like the domain name or other properties of the theme? Once we chemspill over this, or land on trunk + 59, we best be sure there aren't very similar things lying around. I'm already pretty worried about this over some of the more minor innerHTML cases that I fixed recently (but that weren't anywhere near this bad because they required local machine profile access to do anything anyway). A sensible attacker would audit the innerHTML eslint annotations more thoroughly than we have and go to town.

Comment 23

[Selena Deckelmann :selenamarie :selena]

From slack discussion bug 1350053 may have been where this was introduced.

Comment 24

[Mark Striemer [:mstriemer]]

Another vulnerability from bug 1397975 where `newEngine` is pulled from the manifest.json file and inserted into a notification without sanitization.

https://searchfox.org/mozilla-central/rev/e3cba77cee3ff1be38313abe9c804d13c51bd95b/browser/modules/ExtensionsUI.jsm#253

Comment 25

[Mark Striemer [:mstriemer]]

Attachment: 1369482-popup-xss.patch

Here's a patch that fixes both cases. It only escapes the theme name when it is being added to the popup.

Johann's patch fixes the theme case but it also causes escaped text in the add-on manager and maybe other places. I'm assuming this is why the `_sanitizeTheme()` helper doesn't escape HTML characters.

Comment 26

[Kris Maglione [:kmag]]

(In reply to Johann Hofmann [:johannh] from comment #2)
> To clarify, I found this while looking at bug 1373642. We need to find a
> sensible way to deal with that bug until this one is resolved, because the
> problem is quite obvious from looking at that patch.

That bug might be a good cover for this change. We could fix the bolding and the sanitization at the same time without attracting much attention. It would become pretty obvious once we chemspill, either way, but some cover is better than none. This is a massive sandbox escape.

Also, it's probably well past the time that we started working on applying CSP to chrome-privileged documents. Even if we can't get rid of inline scripts for our static XUL, we should be able to block them for dynamically-created elements at this point.

Comment 27

[Andrew Swan [:aswan]]

I agree with Kris about landing this in a separate bug to avoid attracting attention.  I nominate bug 1431242 though.

Comment 28

[Johann Hofmann [:johannh]]

Ok, let me quickly summarize what seem to be the most popular options right now:

- We fix this bug in a chemspill with Mark's patch and fix bug 1431242 afterwards and let it ride the trains (or give it a beta uplift).

- We fix bug 1431242 very soon on Nightly and, without a chemspill release, include Mark's patch in the next dot release (takes some time).

- We fix bug 1431242 and then (chemspill or dot release) uplift that one together with bug 1369482 (which is a pre-requisite) and optionally any other patches that prevent us from applying cleanly. We'd have to work with add-ons to find if there are any.

As dveditz mentioned on Slack, once we have revealed that there is a security issue around this, it's extremely important to update everyone as quickly as possible. To that end I can sympathize with a "let's not do any uplift at all" idea, but given the severity of this I don't think it's really an option to wait 2 full cycles (we're at the beginning of 60) before this goes to release. In an unrelated bug I've noticed yesterday how malicious sites seem to follow our changelog surprisingly closely.

Comment 29

[Mark Striemer [:mstriemer]]

Attachment: ExtensionsUI.txt

Here's what I looked at for the extensions popups

ExtensionsUI
    showPermissionsPrompt
        * Causes XSS
        * Uses strings from caller
        * All local calls are escaped
        * If the strings come from ExtensionsUI._buildStrings they get escaped
        * Call from browser-addons.js#657 is not escaped
    showDefaultSearchPrompt
        * Causes XSS
        * Uses strings from caller
        * Only one call in ExtensionsUI, does not escape search engine name
    showInstallNotification
        * Strings local to function, escaped

Extension
    formatPermissionStrings
        * This function creates strings with add-on names
        * This is escaped in ExtensionsUI._buildStrings
        * This is not escaped in fennec, but the string is sent to Java

Comment 30

[Kris Maglione [:kmag]]

(In reply to Mark Striemer [:mstriemer] from comment #29)
> Created attachment 8945199 [details]
> ExtensionsUI.txt
> 
> Here's what I looked at for the extensions popups
> 
> ExtensionsUI
>     showPermissionsPrompt
>         * Causes XSS
>         * Uses strings from caller

In other places where we support HTML values in prompts, we require the caller to pass a document fragment rather than a string if their values aren't plain text. We should probably do something similar here.

>         * All local calls are escaped
>         * If the strings come from ExtensionsUI._buildStrings they get
> escaped
>         * Call from browser-addons.js#657 is not escaped
>     showDefaultSearchPrompt
>         * Causes XSS
>         * Uses strings from caller

And here.

Comment 31

[Andrew Swan [:aswan]]

Comment on attachment 8945176 [details] [diff] [review]
1369482-popup-xss.patch

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

This looks right to me.  Another option is to just get rid of all the bold text in permission prompts which would let us replace all uses of .innerHTML with .textContent.  That would be a slightly longer patch but still not very big and it would be vastly easier to evaluate.  Bug 1431242 could then bring back bold text in a sane way...

Comment 32

[Johann Hofmann [:johannh]]

Attachment: Replace with .textContent

This is a patch that does what aswan talks about and removes all the innerHTML from ExtensionsUI (except one case where it's used for static image strings).

Just to have all the options on the table.

Please check this carefully in case I missed something.

Comment 33

[Johann Hofmann [:johannh]]

Also note that I have run a quick smoke test but no comprehensive testing of all webextension prompts on this.

Comment 34

[Kris Maglione [:kmag]]

Another possible plan of action is to land bug 1432966, and deal with these specific innerHTML issues discretely in the other cleanup bugs we already have, and let the cleanups ride the trains.

The advantage of bug 1432966 is that it doesn't point to any particular exploitable code (or even suggest that we know there are existing exploits), and takes care of this entire class of issues (including the ones we don't know about) in a single change. We should be able to land and uplift it without too much noise, and we can take it as a ride-along for a dot release without it looking like much more than a precaution.

Comment 35

[Johann Hofmann [:johannh]]

From a quick look that seems like a clever solution to me. Since we had some loose agreement that a chemspill release sounds necessary for this, we will probably lose the stealth aspect of it anyway, but it's still an advantage to get it on try and land it without drawing attention.

The final chemspill decision obviously isn't with me, so I'd leave that up to dveditz. We have a couple of alternative patches now, but the one from kmag looks most promising to me.

Comment 36

[Andrew Swan [:aswan]]

Comment on attachment 8945232 [details] [diff] [review]
Replace with .textContent

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

Looks great to me, thanks.  We can get rid of that last use of innerHTML pretty easily but since its not tainted, no need to do that here.

Comment 37

[Daniel Veditz [:dveditz]]

Nice that bug 1432966 doesn't point anywhere near the specific problem area and that it might fix other currently unknown vulnerabilities. For a chemspill it's hard to get my head around what that could affect or break. Is there any feature (perhaps hiding in the directories that disabled eslint) that relies on inserting script this way?

Comment 38

[Kris Maglione [:kmag]]

(In reply to Daniel Veditz [:dveditz] from comment #37)
> Nice that bug 1432966 doesn't point anywhere near the specific problem area
> and that it might fix other currently unknown vulnerabilities. For a
> chemspill it's hard to get my head around what that could affect or break.
> Is there any feature (perhaps hiding in the directories that disabled
> eslint) that relies on inserting script this way?

I hope not. For the most part, the only scripts this should affect are event listener attributes, since most of the APIs that create fragments this way disable script nodes.

It's possible that we have some code that relies on creating elements with event listeners this way, but I doubt it. And if we do, we should fix that code. I'm waiting for a try run to finish, which should hopefully tell us one way or the other.

Comment 39

[Frederik Braun [:freddy]]

(Created a tracking bug, in case we find more issues during the audit)

Comment 40

[Frederik Braun [:freddy]]

Err ..

Comment 41

[Mike Conley (:mconley) (:⚙️)]

Bug 1432966 has landed, and SoftVision has done a bunch of testing. There are a few more areas where we need to use unsafeSetInnerHTML for whitelisting (but _only_ for areas where we're certain that the input is coming from either hard-coded strings or prefs[1]).

johannh and I think we should start attaching patches for those fixes here. The areas in particular that need whitelisting:

EME notification bars
WebExtension install permission panels
WebRTC screen sharing permission panel
Fennec's about:config
GMP license info in about:addons

Comment 42

[Mike Conley (:mconley) (:⚙️)]

Attachment: WebRTC screen sharing patch

This fixes the "Learn more" link in the WebRTC screen sharing permission panel.

Comment 43

[Kris Maglione [:kmag]]

I think we should try to avoid resorting to unsafeSetInnerHTML as much as possible. For most of these, we should be able to switch to sanitizer-friendly HTML in place of XUL, or direct DOM creation. Aside from generally being safer, that also adds a bunch of red herrings to our chemspill diff rather than removing likely candidates for anyone looking for an exploit.

The WebRTC prompt, for instance, could be changed to something like:

            let learnMore = document.createElement("label");
            learnMore.className = "text-link";
            learnMore.setAttribute("href", baseURL + "screenshare-safety");
            learnMore.textContent = learnMoreText;

            if (type == "screen") {
              string = bundle.getFormattedString("getUserMedia.shareScreenWarning.message",
                                                 ["<>"]);
            } else {
              let brand =
                doc.getElementById("bundle_brand").getString("brandShortName");
              string = bundle.getFormattedString("getUserMedia.shareFirefoxWarning.message",
                                                 [brand, "<>"]);
            }

            let [pre, post] = string.split("<>");
            warning.textContent = pre;
            warning.appendChild(learnMore);
            warning.appendChild(document.createTextNode(post));

without too much trouble.

Comment 44

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Kris, Dan, Johan, Mike, if the plan is to not include the fix in ESR52.6.x dot release but instead land it for ESR52.7, do we need a separate bug to track this for ESR?

Comment 45

[Kris Maglione [:kmag]]

Attachment: Part 1 - Fix WebRTC innerHTML sanitization

MozReview-Commit-ID: JOkIXajO13O

Comment 46

[Kris Maglione [:kmag]]

Attachment: Part 2 - Fix ExtensionsUI innerHTML sanitization

MozReview-Commit-ID: B8Lzqx56fBi

Comment 47

[Kris Maglione [:kmag]]

Attachment: Part 3 - Fix Fennec about:config innerHTML sanitization

MozReview-Commit-ID: J9tmYCGfOcB

Comment 48

[Kris Maglione [:kmag]]

Attachment: Part 4 - Fix GMP license link innerHTML sanitization

MozReview-Commit-ID: E3Mu765xl1A

Comment 49

[Andrew Swan [:aswan]]

Comment on attachment 8945866 [details] [diff] [review]
Part 2 - Fix ExtensionsUI innerHTML sanitization

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

This patch seems fine, but I think that even safer would be to just let the names be in regular (not bold) text until we land patches to do proper DOM construction instead of injecting html strings.  Johann has an intern working on that project (bug 1431242) so those patches should land in 60.

Comment 50

[Kris Maglione [:kmag]]

(In reply to Andrew Swan [:aswan] from comment #49)
> Comment on attachment 8945866 [details] [diff] [review]
> Part 2 - Fix ExtensionsUI innerHTML sanitization
> 
> Review of attachment 8945866 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This patch seems fine, but I think that even safer would be to just let the
> names be in regular (not bold) text until we land patches to do proper DOM
> construction instead of injecting html strings.  Johann has an intern
> working on that project (bug 1431242) so those patches should land in 60.

If that's OK with you, it's OK with me. I'd rather not draw more attention to this file than we have to...

Comment 51

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8945867 [details] [diff] [review]
Part 3 - Fix Fennec about:config innerHTML sanitization

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

Tested this on top of the main fix patch, and this fixes the about:config issue on Fennec. Thanks!

::: mobile/android/chrome/content/config.js
@@ +596,5 @@
>  
>        this.li.setAttribute("contextmenu", "prefs-context-menu");
>  
>        // Create list item outline, bind to object actions
>        // eslint-disable-next-line no-unsanitized/property

We can probably get rid of this inline ESLint directive now.

Comment 52

[Andrew Swan [:aswan]]

(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #50)
> > This patch seems fine, but I think that even safer would be to just let the
> > names be in regular (not bold) text until we land patches to do proper DOM
> > construction instead of injecting html strings.  Johann has an intern
> > working on that project (bug 1431242) so those patches should land in 60.
> 
> If that's OK with you, it's OK with me. I'd rather not draw more attention
> to this file than we have to...

There was some banter about this on slack yesterday and I think andym was on board at that time but he can make the final decision.

Comment 53

[Andy McKay]

(In reply to Andrew Swan [:aswan] from comment #52)
Removing the bold from the permission dialog is fine.

Comment 54

[Kris Maglione [:kmag]]

I guess for the EME prompt bustage, we should also take bug 1430974 in the uplift?

Comment 55

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8945865 [details] [diff] [review]
Part 1 - Fix WebRTC innerHTML sanitization

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

This looks fine by inspection, but I'm waiting for a build of mozilla-release for Desktop to finish to run it through its paces.

Comment 56

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8945865 [details] [diff] [review]
Part 1 - Fix WebRTC innerHTML sanitization

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

Works as advertised, thanks kmag.

Comment 57

[Kris Maglione [:kmag]]

Attachment: Combined patch for release

This is the combined patches from bug 1432966, bug 1433414, bug 1430974, and this bug, rebased onto release.

I'll leave it to ryanvm to do a try run if he thinks it's necessary. I don't know the right steps to build with release config.

Comment 58

[Kris Maglione [:kmag]]

Attachment: Combined patch for beta

The same set of patches, applied to beta rather than release

Comment 59

[Ryan VanderMeulen [:RyanVM]]

(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #57)
> I'll leave it to ryanvm to do a try run if he thinks it's necessary. I don't
> know the right steps to build with release config.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a6f882645b6619961682183f2975cedf0345ad7a&filter-searchStr=signing

Comment 60

[:Gijs (he/him)]

(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #58)
> Created attachment 8945974 [details] [diff] [review]
> Combined patch for beta
> 
> The same set of patches, applied to beta rather than release

This wasn't the patch you were looking for.

Comment 61

[Kris Maglione [:kmag]]

(In reply to :Gijs from comment #60)
> This wasn't the patch you were looking for.

Oh, I guess I didn't flatten them properly. Thanks.

Comment 62

[:Gijs (he/him)]

For browser-media.js, I messed up the magical string notations in bug 1430974. In theory, you want:

 let [prefix, suffix] = mainMessage.split(/%(?:1\$)?S/).map(s => document.createTextNode(s));

instead of

 let [prefix, suffix] = mainMessage.split(/%(?:\$\d)?S/).map(s => document.createTextNode(s));

Though in practice, it makes no difference because none of our shipped locales use the indexed version of "%S" ( https://dxr.mozilla.org/l10n-central/search?q=emeNotifications.drmContentDisabled.message&redirect=false ) .

If you care, also apply the browser-media.js portion of bug 1431471 . It won't actually break with current strings if you don't, though.

Sorry for not responding to the ni earlier, I was traveling. :-(

Comment 63

[Kris Maglione [:kmag]]

Attachment: Combined patch for beta

Comment 64

[Kris Maglione [:kmag]]

(In reply to :Gijs from comment #62)
> For browser-media.js, I messed up the magical string notations in bug
> 1430974. In theory, you want:
> ...
> Though in practice, it makes no difference because none of our shipped
> locales use the indexed version of "%S"
> 
> If you care, also apply the browser-media.js portion of bug 1431471 . It
> won't actually break with current strings if you don't, though.

I guess it doesn't make much difference, but I may as well fix it any way. It's simple enough that I can just edit the diff.

> Sorry for not responding to the ni earlier, I was traveling. :-(

No worries. I remembered that after I needinfoed you, which is why I didn't wait.

Comment 65

[Kris Maglione [:kmag]]

Attachment: Combined patch for release

Comment 66

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-release/rev/c2db4a50dc5c93b44852d9a5201f7ec062ecc6cb

https://hg.mozilla.org/releases/mozilla-beta/rev/64737c752ac4af4766ad6f82720818521f3aca24

Comment 67

[Johann Hofmann [:johannh]]

(In reply to Ritu Kothari (:ritu) from comment #44)
> Hi Kris, Dan, Johan, Mike, if the plan is to not include the fix in
> ESR52.6.x dot release but instead land it for ESR52.7, do we need a separate
> bug to track this for ESR?

We probably do, maybe just to track whether this actually needs to go into ESR and what changes are required.

Updating the assignee in this bug to reflect that we used Kris' patch.

Comment 68

[CosminB]

Attachment: ScreenShotsBugBug1432778.zip

I was able to reproduce the issue from description on Firefox 58.0 (20180118215408) and Firefox 60.0a1 (20180124100321) under Win 7 64-bit and Ubuntu 12.04 64-bit.

Using the attached file from description I am unable to reproduce the issue on Firefox 60.0a1 (20180129100406), Firefox 59.0b5 (20180128191456) and Firefox 58.0.1 (20180128191252) under Wind 7 64-bit, Ubuntu 12.04 64-bit.

I also used an extension and observed that the text is not bolded in the pop-up.

After the install,in some cases, the text is bolded in the confirmation pop-up. 
Observed on: Firefox 58.0.1 and Firefox 59.0b5 under Win 7 64-bit and Ubuntu 12.04 64-bit.

Is this expected?

Comment 69

[Kris Maglione [:kmag]]

Yes, it's expected. We have a follow-up planned to fix the bolding.

Comment 70

[Daniel Veditz [:dveditz]]

(In reply to Ritu Kothari (:ritu) from comment #44)
> Hi Kris, Dan, Johan, Mike, if the plan is to not include the fix in
> ESR52.6.x dot release but instead land it for ESR52.7, do we need a separate
> bug to track this for ESR?

Normally I'd say "no, track it here" but I think the decision whether to fix ESR will be different enough that we'll want separate space to debate that.

Comment 71

[Daniel Veditz [:dveditz]]

Filed bug 1434086 for the ESR version

Comment 72

[CosminB]

Thanks Kris!

This issue was also verified on Firefox 60.0a1 (20180129220114), Firefox 59.0b5 (20180128191456) and Firefox 58.0.1 (20180128191252 under  Mac OS  X 10.13.2 

Based on comment 68 and comment 69, I will mark the bug, verified as fixed.