1372527 - Investigate/Mitigate potential XSS risk in ExtensionsUI.jsm

Status: RESOLVED WORKSFORME

(WebExtensions :: General, enhancement)

Description

[Frederik Braun [:freddy]]

Having looked at the code for a while I'm not particularly worried about this, but I wanted to track this to err on the side of caution.
If this turns out exploitable, this is a sec-critical issue. Please help me investigate and discuss possible mitigations.

ExtensionsUI.jsm shows permission prompts involving add-on names and add-on metadata that is potentially harmful, because it does so using innerHTML assignments.

The source line in question is at <http://searchfox.org/mozilla-central/rev/1a054419976437d0778a2b89be1b00207a744e94/browser/modules/ExtensionsUI.jsm#381>:

>  showPermissionsPrompt(browser, strings, icon, histkey) {
>    function eventCallback(topic) {
>      let doc = this.browser.ownerDocument;
>      if (topic == "shown") {
>        doc.getElementById("addon-webext-permissions-notification")
>           .description.innerHTML = strings.header;


Understanding that strings.headercontains HTML, it's unfeasible to escape special characters completely. An alternative would be to use a sanitizer like nsTreeSanitizer or to re-architect the code that the HTML will be created within showPermissionsPrompt() and the parameter is just text that can be properly escaped (or assigned via textContent).

I have assigned you, Mark, because you touched the relevant lines last. If you are not a good fit, please help me find someone else so the bug is not dropped.

Comment 1

[Kris Maglione [:kmag]]

The add-on name in this string is sanitized here:

http://searchfox.org/mozilla-central/rev/1a054419976437d0778a2b89be1b00207a744e94/browser/modules/ExtensionsUI.jsm#338

so I'm not quite sure what the issue is.

Comment 2

[Frederik Braun [:freddy]]

As mentioned in comment 0, I'd rather check in with you fine folks before drawing an incorrect conclusion. Filing a security bug is, in my experience, a better forum than starting an email thread, even if it turns out not be closed as WORKSFORME.
Thank you for taking a look, Kris.

Comment 3

[Panos Astithas (he/him) [:past] (please ni?)]

Shouldn't we remove the s-s flag then?